by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline

On Thu, 22 Oct 2015, Christoph Hellwig wrote:

> On Wed, Oct 21, 2015 at 02:48:45PM -0700, Trond Myklebust wrote:
> > > > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > > +{
> > > > + int res = 0;
> > > > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > > + case FL_POSIX:
> > > > + res = posix_lock_inode_wait(inode, fl);
> > > > + break;
> > > > + case FL_FLOCK:
> > > > + res = flock_lock_inode_wait(inode, fl);
> > > > + break;
> > > > + default:
> > > > + BUG();
> > > > + }
> > > > + return res;
> > > > +}
>
> This is a) not a good name for a global function, and b) probably
> shouldn't be inline.
>
> Given how similar the functions are I'd rather have a
> file_lock_inode_wait that handles both cases right in fs/locks.c

Makes sense to me.. I'll do it.

2015-12-07 16:05:46

by Benjamin Coddington

[permalink] [raw]

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

On Wed, 14 Oct 2015, Jeff Layton wrote:

> On Wed, 14 Oct 2015 14:23:37 -0400
> Benjamin Coddington <[email protected]> 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 <[email protected]>
> > ---
> > 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