Date: Mon, 25 Aug 2014 15:04:33 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-hackers@freebsd.org Subject: Re: atomic_load_acq_int in sequential_heuristic Message-ID: <20140825130433.GD14344@dft-labs.eu> In-Reply-To: <20140825111000.GC2737@kib.kiev.ua> References: <20140824115729.GC2045@dft-labs.eu> <20140824162331.GW2737@kib.kiev.ua> <20140824164236.GX2737@kib.kiev.ua> <20140825005659.GA14344@dft-labs.eu> <20140825073404.GZ2737@kib.kiev.ua> <20140825081526.GB14344@dft-labs.eu> <20140825083539.GB2737@kib.kiev.ua> <20140825091056.GC14344@dft-labs.eu> <20140825111000.GC2737@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Aug 25, 2014 at 02:10:01PM +0300, Konstantin Belousov wrote:
> 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 ?
>
Nothing special:
https://people.freebsd.org/~mjg/patches/F_READAHEAD.c
> 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 ?
>
Yes, -1L deals with the problem. I would still argue that using 'tmp'
like the rest of the function would not hurt as a cheap solution.
> >
> > 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().
>
/*
* Exclusive lock synchronizes against f_seqcount reads and writes in
* sequential_heuristic().
*/
> 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.
>
/*
* (a) f_vnode lock required (shared allows both reads and writes)
*/
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140825130433.GD14344>