Re: [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE

[Benjamin Coddington <bcodding@redhat.com>]

On Wed, 14 Oct 2015, Jeff Layton wrote:

> On Wed, 14 Oct 2015 14:23:37 -0400
> Benjamin Coddington <bcodding@redhat.com> wrote:
>
> > NFS unlock procedures will wait for IO to complete before sending an unlock.
> > In the case that this wait is interrupted, an unlock may never be sent if
> > the unlock is part of cleaning up locks during a close.  This lost lock can
> > then prevent other clients from locking the file.
> >
> > Fix this by deferring an unlock that should wait for IO during FL_CLOSE by
> > copying it to a list on the nfs_lock_context, which can then be used to
> > release the lock when the IO has completed.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> >  fs/nfs/file.c          |   36 +++++++++++++++++++++++++++++++++++-
> >  fs/nfs/inode.c         |    1 +
> >  fs/nfs/pagelist.c      |   23 ++++++++++++++++++++---
> >  include/linux/nfs_fs.h |    7 +++++++
> >  4 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > index d16c50f..460311a 100644
> > --- a/fs/nfs/file.c
> > +++ b/fs/nfs/file.c
> > @@ -738,6 +738,36 @@ out_noconflict:
> >  }
> >
> >  static int
> > +defer_unlk(struct nfs_lock_context *l_ctx, int cmd, struct file_lock *fl)
> > +{
> > +	struct inode *inode = d_inode(l_ctx->open_context->dentry);
> > +	struct nfs_io_counter *c = &l_ctx->io_count;
> > +	struct nfs_deferred_unlock *dunlk;
> > +	int status = 0;
> > +
> > +	if (atomic_read(&c->io_count) == 0)
> > +		return 0;
> > +
> > +	/* free in nfs_iocounter_dec */
> > +	dunlk = kmalloc(sizeof(*dunlk), GFP_NOFS);
> > +	if (dunlk == NULL)
> > +		return -ENOMEM;
> > +
>
> This is a little ugly...

> You're probably going to calling this from something like
> locks_remove_posix, and if this allocation fails then the unlock will
> just never happen.
>
> Is there any way to avoid this allocation?

Yes!  As you go on to suggest..

> The "cmd" field in nfs_deferred_unlock is more or less redundant. We're
> always calling this with that set to F_UNLCK. We also know that this
> will be called on the whole file range. Maybe we can simply add a flag
> to the lock context to indicate whether we should send a whole-file
> unlock on it when the io_count goes to zero.

That simplifies things quite a bit..  I'm going to resubmit this with that
approach.  Thanks!

> Also, on a somewhat related note...we aren't currently setting FL_CLOSE
> in locks_remove_flock and we probably should be.

I'll add that as well.

Ben