MIME-Version: 1.0
References: <20230215115354.14907-1-jlayton@kernel.org> <ACB5CC34-59DD-42DD-95D4-F4B7CA93552A@oracle.com>
In-Reply-To: <ACB5CC34-59DD-42DD-95D4-F4B7CA93552A@oracle.com>
From:   Anna Schumaker <anna@kernel.org>
Date:   Wed, 22 Feb 2023 17:29:38 -0500
Message-ID: <CAFX2Jf=k4nfwv1WuUOa42PZAi0RkjvXDdRUCv6Fp-aSt=AiE6A@mail.gmail.com>
Subject: Re: [PATCH v2] nfsd: allow reaping files still under writeback
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Chuck,

On Sun, Feb 19, 2023 at 2:46 PM Chuck Lever III <chuck.lever@oracle.com> 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...

The NFS changes look fine to me.

Acked-by: Anna Schumaker <Anna.Schumaker@Netapp.com>

>
> For the moment this is going to topic-filecache-cleanups,
> not to nfsd-next.
>
>
> > ---
> > 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
>
>
>