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

[Mike Snitzer <snitzer@kernel.org>]

On Wed, Dec 18, 2024 at 04:05:23PM -0500, Chuck Lever wrote:
> On 12/18/24 3:55 PM, Anna Schumaker wrote:
> > 
> > 
> > On 12/18/24 12:31 PM, Mike Snitzer wrote:
> > > On Fri, Nov 22, 2024 at 09:31:23PM -0500, Mike Snitzer wrote:
> > > > 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
> > > > 
> > > 
> > > Hi all,
> > > 
> > > These LOCALIO changes didn't land for 6.13 merge, please advise on if
> > > we might get these changes staged for 6.14 now-ish.
> > 
> > Works for me.
> > 
> > > 
> > > Trond and/or Anna, do you feel comfortable picking this series up
> > > (nfsd cachnges too) or would you like to see any changes before that
> > > is possible?
> > 
> > I'll go through the patches once more, but they should be fine. I will need Acked-by's for the NFSD bits from Chuck or Jeff.
> 
> Looks like Jeff gave his Reviewed-by for the series already.
> 
> I had asked for some minor changes, haven't seen them go by,

Only thing I was aware of, and addressed in v2, was this:
https://lore.kernel.org/all/ZzNm0hekxOyAUhib@tissot.1015granger.net/

I added a module parameters section to the localio.rst and mentioned
that in v2's 0th header:
https://lore.kernel.org/r/all/20241114035952.13889-1-snitzer@kernel.org/

with:

"- updated Documentation/filesystems/nfs/localio.rst to reflect the
  percpu_ref change from nfsd_serv to nfsd_net. Also discuss O_DIRECT
  relative to LOCALIO and document the nfs module param (Chuck, I do
  think we need it, otherwise O_DIRECT regressions are possible)."

> but, for the NFSD-related hunks:
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>

Thanks!