atomic_load_acq_int in sequential_heuristic

[Konstantin Belousov]

On Mon, Aug 25, 2014 at 10:15:26AM +0200, Mateusz Guzik wrote:
> On Mon, Aug 25, 2014 at 10:34:04AM +0300, Konstantin Belousov wrote:
> [..]
> > Two notes.  First, please add a comment explaining which other part
> > of the code is locked against in F_READAHEAD switch case.  Second,
> > should the vnode lock cover the FRDAHEAD reset case too, at least
> > for consistency ?
> 
> I'll start with a side thing:
> 
> diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
> index 98823f3..2c3df2b 100644
> --- a/sys/kern/vfs_vnops.c
> +++ b/sys/kern/vfs_vnops.c
> @@ -365,7 +365,7 @@ vn_open_vnode(struct vnode *vp, int fmode, struct ucred *cred,
>  				fp->f_ops= &badfileops;
>  			return (error);
>  		}
> -		fp->f_flag |= FHASLOCK;
> +		atomic_set_int(&fp->f_flag, FHASLOCK);
>  	}
>  	if (fmode & FWRITE) {
>  		VOP_ADD_WRITECOUNT(vp, 1);
Ok.

> 
> And now for the patch in question:
> 
> Yes, _rel is not necessary. In fact, the loop is not necessary either. It
> would be useful if more than one flag was altered.
> 
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 7abdca0..433b809 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -476,7 +476,6 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
>  	struct vnode *vp;
>  	cap_rights_t rights;
>  	int error, flg, tmp;
> -	u_int old, new;
>  	uint64_t bsize;
>  	off_t foffset;
>  
> @@ -762,23 +761,21 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
>  		}
>  		if (arg >= 0) {
>  			vp = fp->f_vnode;
> -			error = vn_lock(vp, LK_SHARED);
> +			/*
> +			 * Exclusive lock synchronizes against
> +			 * sequential_heuristic().
> +			 */
> +			error = vn_lock(vp, LK_EXCLUSIVE);
>  			if (error != 0) {
>  				fdrop(fp, td);
>  				break;
>  			}
>  			bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
> -			VOP_UNLOCK(vp, 0);
>  			fp->f_seqcount = (arg + bsize - 1) / bsize;
> -			do {
> -				new = old = fp->f_flag;
> -				new |= FRDAHEAD;
> -			} while (!atomic_cmpset_rel_int(&fp->f_flag, old, new));
> +			atomic_set_int(&fp->f_flag, FHASLOCK);
You misspelled FRDAHEAD as FHASLOCK, below as well.
Was this tested ?

> +			VOP_UNLOCK(vp, 0);
>  		} else {
> -			do {
> -				new = old = fp->f_flag;
> -				new &= ~FRDAHEAD;
> -			} while (!atomic_cmpset_rel_int(&fp->f_flag, old, new));
> +			atomic_clear_int(&fp->f_flag, FHASLOCK);
So what about extending the vnode lock to cover the flag reset ?

>  		}
>  		fdrop(fp, td);
>  		break;
> diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
> index f1d19ac..98823f3 100644
> --- a/sys/kern/vfs_vnops.c
> +++ b/sys/kern/vfs_vnops.c
> @@ -438,7 +438,8 @@ static int
>  sequential_heuristic(struct uio *uio, struct file *fp)
>  {
>  
> -	if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD)
> +	ASSERT_VOP_LOCKED(fp->f_vnode, __func__);
> +	if (fp->f_flag & FRDAHEAD)
>  		return (fp->f_seqcount << IO_SEQSHIFT);
>  
>  	/*
> 
> -- 
> Mateusz Guzik <mjguzik gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140825/a7120f1e/attachment.sig>