atomic_load_acq_int in sequential_heuristic

[Konstantin Belousov]

On Mon, Aug 25, 2014 at 11:10:56AM +0200, Mateusz Guzik wrote:
> On Mon, Aug 25, 2014 at 11:35:39AM +0300, Konstantin Belousov wrote:
> > > +			atomic_set_int(&fp->f_flag, FHASLOCK);
> > You misspelled FRDAHEAD as FHASLOCK, below as well.
> > Was this tested ?
> > 
> 
> Oops, damn copy-pasto. Sorry.
> 
> > > +			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 ?
> > 
> 
> Sure.
> 
> So this time I tested it properly and found out it is impossible to
> disable the hint. The test is:
> 
> -1 is truncated and then read from intptr_t which yields a big positive
> number instead. Other users in the function use int tmp to work around
> this issue.
Could you provide me with the test case which demonstrates the problem ?

The fcntl(2) prototype in sys/fcntl.h is variadic, so int arg argument
is not promoted.  On the other hand, syscalls.master declares arg as long.
Did you tried to pass -1L as third argument to disable ?

> 
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 7abdca0..774f647 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -760,7 +760,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
>  			error = EBADF;
>  			break;
>  		}
> -		if (arg >= 0) {
> +		tmp = arg;
> +		if (tmp >= 0) {
>  			vp = fp->f_vnode;
>  			error = vn_lock(vp, LK_SHARED);
>  			if (error != 0) {
> @@ -769,7 +770,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
>  			}
>  			bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
>  			VOP_UNLOCK(vp, 0);
> -			fp->f_seqcount = (arg + bsize - 1) / bsize;
> +			fp->f_seqcount = (tmp + bsize - 1) / bsize;
>  			do {
>  				new = old = fp->f_flag;
>  				new |= FRDAHEAD;
> 
> Then the patch in question:
> 
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 774f647..4efadb0 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;
>  
> @@ -760,27 +759,25 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
>  			error = EBADF;
>  			break;
>  		}
> +		vp = fp->f_vnode;
> +		/*
> +		 * Exclusive lock synchronizes against
> +		 * sequential_heuristic().
I would also add a sentence that we care about f_seqcount update in
seq_heur().

Another place to add the locking annotation is the struct file in
sys/file.h.  Now f_seqcount is 'protected' by the vnode lock.
I am not sure how to express the locking model shortly.

> +		 */
> +		error = vn_lock(vp, LK_EXCLUSIVE);
> +		if (error != 0) {
> +			fdrop(fp, td);
> +			break;
> +		}
>  		tmp = arg;
>  		if (tmp >= 0) {
> -			vp = fp->f_vnode;
> -			error = vn_lock(vp, LK_SHARED);
> -			if (error != 0) {
> -				fdrop(fp, td);
> -				break;
> -			}
>  			bsize = fp->f_vnode->v_mount->mnt_stat.f_iosize;
> -			VOP_UNLOCK(vp, 0);
>  			fp->f_seqcount = (tmp + 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, FRDAHEAD);
>  		} 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, FRDAHEAD);
>  		}
> +		VOP_UNLOCK(vp, 0);
>  		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/94e287c3/attachment.sig>