Re: [PATCH v2] nfsd: allow reaping files still under writeback

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2023-02-19 at 19:45 +0000, Chuck Lever III wrote:
> 
> > On Feb 15, 2023, at 6:53 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On most filesystems, there is no reason to delay reaping an nfsd_file
> > just because its underlying inode is still under writeback. nfsd just
> > relies on client activity or the local flusher threads to do writeback.
> > 
> > The main exception is NFS, which flushes all of its dirty data on last
> > close. Add a new EXPORT_OP_FLUSH_ON_CLOSE flag to allow filesystems to
> > signal that they do this, and only skip closing files under writeback on
> > such filesystems.
> > 
> > Also, remove a redundant NULL file pointer check in
> > nfsd_file_check_writeback, and clean up nfs's export op flag
> > definitions.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> I assume that I'm taking this via the nfsd tree? If so,
> I would like an Acked-by from the NFS client maintainers...
> 
> For the moment this is going to topic-filecache-cleanups,
> not to nfsd-next.
> 
> 

No need to rush on this one, IMO. It's not super-critical or anything.
The topic branch is probably fine.

> > ---
> > fs/nfs/export.c          |  9 ++++++---
> > fs/nfsd/filecache.c      | 11 ++++++++++-
> > include/linux/exportfs.h |  1 +
> > 3 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> > index 0a5ee1754d50..102a454e27c9 100644
> > --- a/fs/nfs/export.c
> > +++ b/fs/nfs/export.c
> > @@ -156,7 +156,10 @@ const struct export_operations nfs_export_ops = {
> > 	.fh_to_dentry = nfs_fh_to_dentry,
> > 	.get_parent = nfs_get_parent,
> > 	.fetch_iversion = nfs_fetch_iversion,
> > -	.flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK|
> > -		EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS|
> > -		EXPORT_OP_NOATOMIC_ATTR,
> > +	.flags = EXPORT_OP_NOWCC		|
> > +		 EXPORT_OP_NOSUBTREECHK 	|
> > +		 EXPORT_OP_CLOSE_BEFORE_UNLINK	|
> > +		 EXPORT_OP_REMOTE_FS		|
> > +		 EXPORT_OP_NOATOMIC_ATTR	|
> > +		 EXPORT_OP_FLUSH_ON_CLOSE,
> > };
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index e6617431df7c..98e1ab013ac0 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -302,8 +302,17 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
> > 	struct file *file = nf->nf_file;
> > 	struct address_space *mapping;
> > 
> > -	if (!file || !(file->f_mode & FMODE_WRITE))
> > +	/* File not open for write? */
> > +	if (!(file->f_mode & FMODE_WRITE))
> > 		return false;
> > +
> > +	/*
> > +	 * Some filesystems (e.g. NFS) flush all dirty data on close.
> > +	 * On others, there is no need to wait for writeback.
> > +	 */
> > +	if (!(file_inode(file)->i_sb->s_export_op->flags & EXPORT_OP_FLUSH_ON_CLOSE))
> > +		return false;
> > +
> > 	mapping = file->f_mapping;
> > 	return mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) ||
> > 		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index fe848901fcc3..218fc5c54e90 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -221,6 +221,7 @@ struct export_operations {
> > #define EXPORT_OP_NOATOMIC_ATTR		(0x10) /* Filesystem cannot supply
> > 						  atomic attribute updates
> > 						*/
> > +#define EXPORT_OP_FLUSH_ON_CLOSE	(0x20) /* fs flushes file data on close */
> > 	unsigned long	flags;
> > };
> > 
> > -- 
> > 2.39.1
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>