On Wed, Aug 26, 2015 at 06:53:31PM -0400, Jeff Layton wrote:
> On Wed, 26 Aug 2015 16:00:32 -0400
> "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
>
> > On Thu, Aug 20, 2015 at 07:17:14AM -0400, Jeff Layton wrote:
> > > It's not uncommon for some workloads to do a bunch of I/O to a file and
> > > delete it just afterward. If knfsd has a cached open file however, then
> > > the file may still be open when the dentry is unlinked. If the
> > > underlying filesystem is nfs, then that could trigger it to do a
> > > sillyrename.
> >
> > Possibly worth noting that situation doesn't currently occur upstream.
> >
> > (And, another justification worth noting: space used by a file should be
> > deallocated on last unlink or close. People do sometimes notice if it's
> > not, especially if the file is large.)
> >
>
> Good points.
>
> > > On a REMOVE or RENAME scan the nfsd_file cache for open files that
> > > correspond to the inode, and proactively unhash and put their
> > > references. This should prevent any delete-on-last-close activity from
> > > occurring, solely due to knfsd's open file cache.
> >
> > Is there anything here to prevent a new cache entry being added after
> > nfsd_file_close_inode and before the file is actually removed?
> >
>
> No, nothing -- it's strictly best effort.
Unfortunately I think this is something we really want to guarantee.
--b.
> What might make sense is to consider looking at the dentry associated
> with the struct file when putting the last reference to the nfsd_file.
> If it's unhashed, then we could unhash the nfsd_file and put the hash
> reference for it.
>
> That won't prevent silly renames in the case of NFS being reexported,
> of course, but it should ensure that we don't leave the thing open
> indefinitely in the case of such a race.
>
> I'll have to think about that one as well...
>
> > --b.
> >
> > >
> > > Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> > > ---
> > > fs/nfsd/filecache.c | 25 +++++++++++++++++++++++++
> > > fs/nfsd/filecache.h | 1 +
> > > fs/nfsd/trace.h | 17 +++++++++++++++++
> > > fs/nfsd/vfs.c | 17 +++++++++++++++--
> > > 4 files changed, 58 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index e48b536762aa..4bd683f03b6e 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -283,6 +283,31 @@ nfsd_file_find_locked(struct inode *inode, unsigned int may_flags,
> > > return NULL;
> > > }
> > >
> > > +/**
> > > + * nfsd_file_close_inode - attempt to forcibly close a nfsd_file
> > > + * @inode: inode of the file to attempt to remove
> > > + *
> > > + * Walk the whole hash bucket, looking for any files that correspond to "inode".
> > > + * If any do, then unhash them and put the hashtable reference to them.
> > > + */
> > > +void
> > > +nfsd_file_close_inode(struct inode *inode)
> > > +{
> > > + struct nfsd_file *nf;
> > > + struct hlist_node *tmp;
> > > + unsigned int hashval = (unsigned int)hash_ptr(inode, NFSD_FILE_HASH_BITS);
> > > + LIST_HEAD(dispose);
> > > +
> > > + spin_lock(&nfsd_file_hashtbl[hashval].nfb_lock);
> > > + hlist_for_each_entry_safe(nf, tmp, &nfsd_file_hashtbl[hashval].nfb_head, nf_node) {
> > > + if (inode == nf->nf_inode)
> > > + nfsd_file_unhash_and_release_locked(nf, &dispose);
> > > + }
> > > + spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
> > > + trace_nfsd_file_close_inode(hashval, inode, !list_empty(&dispose));
> > > + nfsd_file_dispose_list(&dispose);
> > > +}
> > > +
> > > __be32
> > > nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > unsigned int may_flags, struct nfsd_file **pnf)
> > > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> > > index debd558ef786..191cdb25aa66 100644
> > > --- a/fs/nfsd/filecache.h
> > > +++ b/fs/nfsd/filecache.h
> > > @@ -26,6 +26,7 @@ int nfsd_file_cache_init(void);
> > > void nfsd_file_cache_shutdown(void);
> > > void nfsd_file_put(struct nfsd_file *nf);
> > > struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
> > > +void nfsd_file_close_inode(struct inode *inode);
> > > __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > unsigned int may_flags, struct nfsd_file **nfp);
> > > #endif /* _FS_NFSD_FILECACHE_H */
> > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > > index 2dac872d31e8..95af3b9c7b66 100644
> > > --- a/fs/nfsd/trace.h
> > > +++ b/fs/nfsd/trace.h
> > > @@ -139,6 +139,23 @@ TRACE_EVENT(nfsd_file_acquire,
> > > show_nf_may(__entry->nf_may), __entry->nf_file,
> > > be32_to_cpu(__entry->status))
> > > );
> > > +
> > > +TRACE_EVENT(nfsd_file_close_inode,
> > > + TP_PROTO(unsigned int hash, struct inode *inode, int found),
> > > + TP_ARGS(hash, inode, found),
> > > + TP_STRUCT__entry(
> > > + __field(unsigned int, hash)
> > > + __field(struct inode *, inode)
> > > + __field(int, found)
> > > + ),
> > > + TP_fast_assign(
> > > + __entry->hash = hash;
> > > + __entry->inode = inode;
> > > + __entry->found = found;
> > > + ),
> > > + TP_printk("hash=0x%x inode=0x%p found=%d", __entry->hash,
> > > + __entry->inode, __entry->found)
> > > +);
> > > #endif /* _NFSD_TRACE_H */
> > >
> > > #undef TRACE_INCLUDE_PATH
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index 6cfd96adcc71..98d3b9d96480 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -1583,6 +1583,15 @@ out_nfserr:
> > > goto out_unlock;
> > > }
> > >
> > > +static void
> > > +nfsd_close_cached_files(struct dentry *dentry)
> > > +{
> > > + struct inode *inode = d_inode(dentry);
> > > +
> > > + if (inode && S_ISREG(inode->i_mode))
> > > + nfsd_file_close_inode(inode);
> > > +}
> > > +
> > > /*
> > > * Rename a file
> > > * N.B. After this call _both_ ffhp and tfhp need an fh_put
> > > @@ -1652,6 +1661,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
> > > if (ffhp->fh_export->ex_path.dentry != tfhp->fh_export->ex_path.dentry)
> > > goto out_dput_new;
> > >
> > > + nfsd_close_cached_files(ndentry);
> > > host_err = vfs_rename(fdir, odentry, tdir, ndentry, NULL, 0);
> > > if (!host_err) {
> > > host_err = commit_metadata(tfhp);
> > > @@ -1721,10 +1731,13 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> > > if (!type)
> > > type = d_inode(rdentry)->i_mode & S_IFMT;
> > >
> > > - if (type != S_IFDIR)
> > > + if (type != S_IFDIR) {
> > > + nfsd_close_cached_files(rdentry);
> > > host_err = vfs_unlink(dirp, rdentry, NULL);
> > > - else
> > > + } else {
> > > host_err = vfs_rmdir(dirp, rdentry);
> > > + }
> > > +
> > > if (!host_err)
> > > host_err = commit_metadata(fhp);
> > > dput(rdentry);
> > > --
> > > 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
