Re: [for-6.13 PATCH v3 00/14] nfs/nfsd: improvements for LOCALIO

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Nov 22, 2024 at 12:26:39PM -0500, Jeff Layton wrote:
> On Fri, 2024-11-15 at 20:40 -0500, Mike Snitzer wrote:
> > Hi,
> > 
> > All available here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next
> > 
> > Changes since v2:
> > - switched from rcu_assign_pointer to RCU_INIT_POINTER when setting to
> >   NULL.
> > - removed some unnecessary #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > - revised the NFS v3 probe patch to use a new nfsv3.ko modparam
> >   'nfs3_localio_probe_throttle' to control if NFSv3 will probe for
> >   LOCALIO. Avoids use of NFS_CS_LOCAL_IO and will probe every
> >   'nfs3_localio_probe_throttle' IO requests (defaults to 0, disabled).
> > - added "Module Parameters" section to localio.rst
> > 
> > All review appreciated, thanks.
> > Mike
> > 
> > Mike Snitzer (14):
> >   nfs/localio: add direct IO enablement with sync and async IO support
> >   nfsd: add nfsd_file_{get,put} to 'nfs_to' nfsd_localio_operations
> >   nfs_common: rename functions that invalidate LOCALIO nfs_clients
> >   nfs_common: move localio_lock to new lock member of nfs_uuid_t
> >   nfs: cache all open LOCALIO nfsd_file(s) in client
> >   nfsd: update percpu_ref to manage references on nfsd_net
> >   nfsd: rename nfsd_serv_ prefixed methods and variables with nfsd_net_
> >   nfsd: nfsd_file_acquire_local no longer returns GC'd nfsd_file
> >   nfs_common: rename nfslocalio nfs_uuid_lock to nfs_uuids_lock
> >   nfs_common: track all open nfsd_files per LOCALIO nfs_client
> >   nfs_common: add nfs_localio trace events
> >   nfs/localio: remove redundant code and simplify LOCALIO enablement
> >   nfs: probe for LOCALIO when v4 client reconnects to server
> >   nfs: probe for LOCALIO when v3 client reconnects to server
> > 
> >  Documentation/filesystems/nfs/localio.rst |  98 +++++----
> >  fs/nfs/client.c                           |   6 +-
> >  fs/nfs/direct.c                           |   1 +
> >  fs/nfs/flexfilelayout/flexfilelayout.c    |  25 +--
> >  fs/nfs/flexfilelayout/flexfilelayout.h    |   1 +
> >  fs/nfs/inode.c                            |   3 +
> >  fs/nfs/internal.h                         |   9 +-
> >  fs/nfs/localio.c                          | 232 +++++++++++++++-----
> >  fs/nfs/nfs3proc.c                         |  46 +++-
> >  fs/nfs/nfs4state.c                        |   1 +
> >  fs/nfs/nfstrace.h                         |  32 ---
> >  fs/nfs/pagelist.c                         |   5 +-
> >  fs/nfs/write.c                            |   3 +-
> >  fs/nfs_common/Makefile                    |   3 +-
> >  fs/nfs_common/localio_trace.c             |  10 +
> >  fs/nfs_common/localio_trace.h             |  56 +++++
> >  fs/nfs_common/nfslocalio.c                | 250 +++++++++++++++++-----
> >  fs/nfsd/filecache.c                       |  20 +-
> >  fs/nfsd/localio.c                         |   9 +-
> >  fs/nfsd/netns.h                           |  12 +-
> >  fs/nfsd/nfsctl.c                          |   6 +-
> >  fs/nfsd/nfssvc.c                          |  40 ++--
> >  include/linux/nfs_fs.h                    |  22 +-
> >  include/linux/nfs_fs_sb.h                 |   3 +-
> >  include/linux/nfs_xdr.h                   |   1 +
> >  include/linux/nfslocalio.h                |  48 +++--
> >  26 files changed, 674 insertions(+), 268 deletions(-)
> >  create mode 100644 fs/nfs_common/localio_trace.c
> >  create mode 100644 fs/nfs_common/localio_trace.h
> > 
> 
> I went through the set and it looks mostly sane to me. The one concern
> I have is that you have the client set up to start caching nfsd files
> before there is a mechanism to call it and ask them to return them. You
> might see some weird behavior there on a bisect, but it looks like it
> all gets resolved in the end.

Yeah, couldn't see a better way to atomically pivot to the new disable
functionality without it needing to be a large muddled patch.

Shouldn't be bad even if someone did bisect, its only the server being
restarted during LOCALIO that could see issues (unlikely thing for
someone to be testing for specifically with a bisect).

> How do you intend for this to go in? Since most of this is client side,
> will this be going in via Trond/Anna's tree?

Yes, likely easiest to have it go through Trond/Anna's tree.  Trond
did have it in his testing tree, maybe your Reviewed-by helps it all
land.

> You can add:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks,
Mike