Date: Mon, 25 Aug 2014 02:57:00 +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: <20140825005659.GA14344@dft-labs.eu> In-Reply-To: <20140824164236.GX2737@kib.kiev.ua> References: <20140824115729.GC2045@dft-labs.eu> <20140824162331.GW2737@kib.kiev.ua> <20140824164236.GX2737@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Aug 24, 2014 at 07:42:36PM +0300, Konstantin Belousov wrote:
> On Sun, Aug 24, 2014 at 07:23:31PM +0300, Konstantin Belousov wrote:
> > On Sun, Aug 24, 2014 at 01:57:29PM +0200, Mateusz Guzik wrote:
> > > Writer side is:
> > > 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));
> > >
> > > Reader side is:
> > > if (atomic_load_acq_int(&(fp->f_flag)) & FRDAHEAD)
> > > return (fp->f_seqcount << IO_SEQSHIFT);
> > >
> > > We can easily get the following despite load_acq:
> > > CPU0 CPU1
> > > load_acq fp->f_flag
> > > fp->f_seqcount = ...
> > > store_rel fp->f_flag
> > > read fp->f_seqcount
> > >
> > > So the barrier does not seem to serve any purpose.
> > It does.
> >
> > Consider initial situation, when the flag is not set yet. There, we
> > do not want to allow the reader to interpret automatically calculated
> > f_seqcount as the user-supplied constant. Without barriers, we might
> > read the flag as set, while user-provided value for f_seqcount is still
> > not visible to processor doing read.
> That said, I think now that there is a real bug.
>
> If we did not read the FRDAHEAD in sequential_heuristic(), we may
> override user-supplied value for f_seqcount. I do not see other
> solution than start to use locking.
Right.
How about abusing vnode lock for this purpose? All callers of
sequential_heuristic have the vnode at least shared locked.
FRDAHEAD setting code locks it shared. We can change that to exclusive,
which will close the race and should not be problematic given that it is
rather rare.
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 7abdca0..643920b 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -762,18 +762,18 @@ 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);
+ 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));
+ VOP_UNLOCK(vp, 0);
} else {
do {
new = old = fp->f_flag;
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>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140825005659.GA14344>