svn commit: r255797 - head/sys/kern

[Konstantin Belousov]

On Sun, Sep 22, 2013 at 09:49:34PM +0100, Attilio Rao wrote:
> On Sun, Sep 22, 2013 at 9:34 PM, Konstantin Belousov
> <kostikbel at gmail.com> wrote:
> > On Sun, Sep 22, 2013 at 11:19:16PM +0300, Konstantin Belousov wrote:
> >> On Sun, Sep 22, 2013 at 01:14:21PM -0700, Matthew Fleming wrote:
> >> > On Sun, Sep 22, 2013 at 12:23 PM, Konstantin Belousov <kib at freebsd.org>wrote:
> >> >> > > Author: kib
> >> > > Date: Sun Sep 22 19:23:48 2013
> >> > > New Revision: 255797
> >> > > URL: http://svnweb.freebsd.org/changeset/base/255797
> >> > >
> >> > > Log:
> >> > >   Increase the chance of the buffer write from the bufdaemon helper
> >> > >   context to succeed.  If the locked vnode which owns the buffer to be
> >> > >   written is shared locked, try the non-blocking upgrade of the lock to
> >> > >   exclusive.
> >> > >
> >> > >   PR:   kern/178997
> >> > >   Reported and tested by:       Klaus Weber <
> >> > > fbsd-bugs-2013-1 at unix-admin.de>
> >> > >   Sponsored by: The FreeBSD Foundation
> >> > >   MFC after:    1 week
> >> > >   Approved by:  re (marius)
> >> > >
> >> > > Modified:
> >> > >   head/sys/kern/vfs_bio.c
> >> > >
> >> > > Modified: head/sys/kern/vfs_bio.c
> >> > >
> >> > > ==============================================================================
> >> > > --- head/sys/kern/vfs_bio.c     Sun Sep 22 19:15:24 2013        (r255796)
> >> > > +++ head/sys/kern/vfs_bio.c     Sun Sep 22 19:23:48 2013        (r255797)
> >> > > @@ -2624,6 +2624,8 @@ flushbufqueues(struct vnode *lvp, int ta
> >> > >         int hasdeps;
> >> > >         int flushed;
> >> > >         int queue;
> >> > > +       int error;
> >> > > +       bool unlock;
> >> > >
> >> > >         flushed = 0;
> >> > >         queue = QUEUE_DIRTY;
> >> > > @@ -2699,7 +2701,16 @@ flushbufqueues(struct vnode *lvp, int ta
> >> > >                         BUF_UNLOCK(bp);
> >> > >                         continue;
> >> > >                 }
> >> > > -               if (vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT | LK_CANRECURSE)
> >> > > == 0) {
> >> > > +               if (lvp == NULL) {
> >> > > +                       unlock = true;
> >> > > +                       error = vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT);
> >> > > +               } else {
> >> > > +                       ASSERT_VOP_LOCKED(vp, "getbuf");
> >> > > +                       unlock = false;
> >> > > +                       error = VOP_ISLOCKED(vp) == LK_EXCLUSIVE ? 0 :
> >> > > +                           vn_lock(vp, LK_UPGRADE | LK_NOWAIT);
> >> > >
> >> >
> >> > I don't think this is quite right.
> >> >
> >> > When the lock is held shared, and VOP_LOCK is implemented by lockmgr(9),
> >> > (i.e. all in-tree filesystems?), LK_UPGRADE may drop the lock, and not
> >> > reacquire it.  This would happen when the vnode is locked shared, the
> >> > upgrade fails (2 shared owners), then lockmgr(9) will try to lock EX, which
> >> > will also fail (still one shared owner).  The caller's lock is no longer
> >> > held.
> >> >
> >> > Doesn't that scenario (LK_UPGRADE failing) cause problems both for the
> >> > caller (unexpected unlock) and for flushbufqueues(), which expects the
> >> > vnode lock to be held since lvp is non-NULL?
> >>
> >> Does it ? If the lock is dropped, the code is indeed in trouble.
> >> Please note that LK_NOWAIT is specified for upgrade, and I believe
> >> that this causes lockmgr to return with EBUSY without dropping
> >> the lock.
> >
> > Yes, you are right, I reverted the patch.  Thank you for noting this.
> >
> > I am bitten by unreasonable behaviour of non-blocking upgrade once more.
> > It has a history.
> >
> > Some time ago I proposed the following patch, which was turned down.
> > That time, I was able to work-around the case. For the bufdaemon helper,
> > I do not see any way to avoid this, except of sometimes locking the
> > reader vnode exclusive in anticipation of the too high dirty buffer
> > mark.
> 
> If you are speaking about me, you are mistaken, I never turned out this patch.
> What I said is completely different: I said that LK_UPGRADE is a
> completely wrong semantic because it can hide wrong things like the
> one you hit today.
> I wanted to see it removed and replaced by explicit LK_RELEASE +
> LK_EXCLUSIVE operations.
> Note that this would have avoided this patch.
> 
> I'm completely in favour of LK_TRYUPGRADE.

Excellent, I will expedite the LK_TRYUPGRADE into tree and then reinstantiate
the bufdaemon helper patch.

Thank you.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20130922/87f6dd2d/attachment-0001.sig>