Re: [PATCH v3 06/20] locks/nfsd: create a new notifier chain for lease attempts

[Jeff Layton <jlayton@poochiereds.net>]

On Wed, 26 Aug 2015 15:49:23 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Aug 20, 2015 at 07:17:06AM -0400, Jeff Layton wrote:
> > With the new file caching infrastructure in nfsd, we can end up holding
> > files open for an indefinite period of time, even when they are still
> > idle. This may prevent the kernel from handing out leases on the file,
> > which we don't really want to block.
> > 
> > Fix this by running a blocking notifier call chain whenever on any
> > lease attempt. nfsd can then purge the cache for that inode before
> > returning.
> 
> Could this be expensive?  There's potentially a lease attempt on every
> NFSv4 open.
> 

That's the reason for the FL_LEASE check. nfsd never sets an FL_LEASE
-- only FL_DELEG and FL_LAYOUT. So nfsd will call into the notifier
callback but it'll never do anything.

We'll take a rwsem (which is icky, granted), call into the notifier and
then return immediately once we see it's not FL_LEASE. We could (in
principle) only call the notifier for FL_LEASE calls, but that seems a
little like a layering violation. Maybe it's worthwhile to consider
though.

> Could we use the cache to decide whether a delegation's a good idea?
> (So, skip requesting the delegation if the cache gives evidence of
> recent activity on the file that would have conflicted with the
> delegation we're about to grant.)  That might avoid unnecessary cache
> purges too.
> 

That's not a bad idea either. I'd have to think about that...

> --b.
> 
> > 
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  fs/locks.c          | 15 +++++++++++++++
> >  fs/nfsd/filecache.c | 27 +++++++++++++++++++++++++++
> >  include/linux/fs.h  |  1 +
> >  3 files changed, 43 insertions(+)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index d3d558ba4da7..c81b96159e5c 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -167,6 +167,13 @@ DEFINE_STATIC_LGLOCK(file_lock_lglock);
> >  static DEFINE_PER_CPU(struct hlist_head, file_lock_list);
> >  
> >  /*
> > + * Some subsystems would like to be notified if someone attempts to set a
> > + * lease on a file. This notifier chain will be called whenever this occurs.
> > + */
> > +BLOCKING_NOTIFIER_HEAD(lease_notifier_chain);
> > +EXPORT_SYMBOL_GPL(lease_notifier_chain);
> > +
> > +/*
> >   * The blocked_hash is used to find POSIX lock loops for deadlock detection.
> >   * It is protected by blocked_lock_lock.
> >   *
> > @@ -1795,10 +1802,18 @@ EXPORT_SYMBOL(generic_setlease);
> >   *
> >   * The "priv" pointer is passed directly to the lm_setup function as-is. It
> >   * may be NULL if the lm_setup operation doesn't require it.
> > + *
> > + * Kernel subsystems can also register to be notified on any attempt to set
> > + * a new lease with the lease_notifier_chain. This is used by (e.g.) nfsd
> > + * to close files that it may have cached when there is an attempt to set a
> > + * conflicting lease.
> >   */
> >  int
> >  vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv)
> >  {
> > +	if (arg != F_UNLCK)
> > +		blocking_notifier_call_chain(&lease_notifier_chain, arg, *lease);
> > +
> >  	if (filp->f_op->setlease)
> >  		return filp->f_op->setlease(filp, arg, lease, priv);
> >  	else
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 669e62f6f4f6..77041967d8ff 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -158,6 +158,22 @@ static struct shrinker	nfsd_file_shrinker = {
> >  	.seeks = 1,
> >  };
> >  
> > +static int
> > +nfsd_file_lease_notifier_call(struct notifier_block *nb, unsigned long arg,
> > +			    void *data)
> > +{
> > +	struct file_lock *fl = data;
> > +
> > +	/* Don't close files if we're the one trying to set the lease */
> > +	if (fl->fl_type == FL_LEASE)
> > +		nfsd_file_close_inode(file_inode(fl->fl_file));
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block nfsd_file_lease_notifier = {
> > +	.notifier_call = nfsd_file_lease_notifier_call,
> > +};
> > +
> >  int
> >  nfsd_file_cache_init(void)
> >  {
> > @@ -186,12 +202,21 @@ nfsd_file_cache_init(void)
> >  		goto out_lru;
> >  	}
> >  
> > +	ret = blocking_notifier_chain_register(&lease_notifier_chain,
> > +				&nfsd_file_lease_notifier);
> > +	if (ret) {
> > +		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
> > +		goto out_shrinker;
> > +	}
> > +
> >  	for (i = 0; i < NFSD_FILE_HASH_SIZE; i++) {
> >  		INIT_HLIST_HEAD(&nfsd_file_hashtbl[i].nfb_head);
> >  		spin_lock_init(&nfsd_file_hashtbl[i].nfb_lock);
> >  	}
> >  out:
> >  	return ret;
> > +out_shrinker:
> > +	unregister_shrinker(&nfsd_file_shrinker);
> >  out_lru:
> >  	list_lru_destroy(&nfsd_file_lru);
> >  out_err:
> > @@ -207,6 +232,8 @@ nfsd_file_cache_shutdown(void)
> >  	struct nfsd_file	*nf;
> >  	LIST_HEAD(dispose);
> >  
> > +	blocking_notifier_chain_unregister(&lease_notifier_chain,
> > +				&nfsd_file_lease_notifier);
> >  	unregister_shrinker(&nfsd_file_shrinker);
> >  	for (i = 0; i < NFSD_FILE_HASH_SIZE; i++) {
> >  		spin_lock(&nfsd_file_hashtbl[i].nfb_lock);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9a9d314f7b27..01bb82eae684 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1041,6 +1041,7 @@ extern int fcntl_setlease(unsigned int fd, struct file *filp, long arg);
> >  extern int fcntl_getlease(struct file *filp);
> >  
> >  /* fs/locks.c */
> > +extern struct blocking_notifier_head	lease_notifier_chain;
> >  void locks_free_lock_context(struct file_lock_context *ctx);
> >  void locks_free_lock(struct file_lock *fl);
> >  extern void locks_init_lock(struct file_lock *);
> > -- 
> > 2.4.3


-- 
Jeff Layton <jlayton@poochiereds.net>