On Wed, 26 Aug 2015 15:49:23 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
