Re: [PATCH v6 06/18] nfs/nfsd: add "localio" support

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Jun 21, 2024 at 04:08:20PM +1000, NeilBrown wrote:
> On Thu, 20 Jun 2024, Mike Snitzer wrote:
> > From: Weston Andros Adamson <dros@primarydata.com>
> > 
> > Add client support for bypassing NFS for localhost reads, writes, and
> > commits. This is only useful when the client and the server are
> > running on the same host.
> > 
> > nfs_local_probe() is stubbed out, later commits will enable client and
> > server handshake via a Linux-only LOCALIO auxiliary RPC protocol.
> > 
> > This has dynamic binding with the nfsd module (via nfs_localio module
> > which is part of nfs_common). Localio will only work if nfsd is
> > already loaded.
> > 
> > The "localio_enabled" nfs kernel module parameter can be used to
> > disable and enable the ability to use localio support.
> > 
> > Tracepoints were added for nfs_local_open_fh, nfs_local_enable and
> > nfs_local_disable.
> > 
> > Also, pass the stored cl_nfssvc_net from the client to the server as
> > first argument to nfsd_open_local_fh() to ensure the proper network
> > namespace is used for localio.
> > 
> > Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> > Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs/Makefile           |   1 +
> >  fs/nfs/client.c           |   3 +
> >  fs/nfs/inode.c            |   4 +
> >  fs/nfs/internal.h         |  51 +++
> >  fs/nfs/localio.c          | 722 ++++++++++++++++++++++++++++++++++++++
> >  fs/nfs/nfstrace.h         |  61 ++++
> >  fs/nfs/pagelist.c         |   3 +
> >  fs/nfs/write.c            |   3 +
> >  fs/nfsd/Makefile          |   1 +
> >  fs/nfsd/filecache.c       |   2 +-
> >  fs/nfsd/localio.c         | 244 +++++++++++++
> >  fs/nfsd/nfssvc.c          |   1 +
> >  fs/nfsd/trace.h           |   3 +-
> >  fs/nfsd/vfs.h             |   9 +
> >  include/linux/nfs.h       |   2 +
> >  include/linux/nfs_fs.h    |   2 +
> >  include/linux/nfs_fs_sb.h |   1 +
> >  include/linux/nfs_xdr.h   |   1 +
> >  18 files changed, 1112 insertions(+), 2 deletions(-)
> >  create mode 100644 fs/nfs/localio.c
> >  create mode 100644 fs/nfsd/localio.c
> > 

<snip>

> > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > new file mode 100644
> > index 000000000000..38d0832442b2
> > --- /dev/null
> > +++ b/fs/nfs/localio.c

<snip>

> > +static void
> > +nfs_local_iter_init(struct iov_iter *i, struct nfs_local_kiocb *iocb, int dir)
> > +{
> > +	struct nfs_pgio_header *hdr = iocb->hdr;
> > +
> > +	if (hdr->args.pgbase != 0) {
> > +		iov_iter_bvec(i, dir, iocb->bvec,
> > +				hdr->page_array.npages,
> > +				hdr->args.count + hdr->args.pgbase);
> > +		iov_iter_advance(i, hdr->args.pgbase);
> > +	} else
> > +		iov_iter_bvec(i, dir, iocb->bvec,
> > +				hdr->page_array.npages, hdr->args.count);
> 
> Both branches of this if() do exactly the same thing.  iov_iter_advance
> is a no-op if the size arg is zero.

iov_iter_advance doesn't look to be a no-op if the size arg is zero.
 
> Is it really worth increasing the code size to sometimes avoid a
> function call?
>
> At least we should for the iov_iter_bvec() inconditionally, then maybe
> call _advance().

For v7, I've fixed it so we do what you suggest.

> > +/*
> > + * Complete the I/O from iocb->kiocb.ki_complete()
> > + *
> > + * Note that this function can be called from a bottom half context,
> > + * hence we need to queue the fput() etc to a workqueue
> 
> fput() is not a good excuse for a workqueue - the work is always
> deferred either to a workqueue or to a process return-from-syscall
> context.
> However the ->rpc_call_done() and vfs_fsync_range() calls are excellent
> justification for a workqueue.
> So I think the comment should be improved, but the code looks sensible.

OK.

> > +static void
> > +nfs_get_vfs_attr(struct file *filp, struct nfs_fattr *fattr)
> > +{
> > +	struct kstat stat;
> > +
> > +	if (fattr != NULL && vfs_getattr(&filp->f_path, &stat,
> > +					 STATX_INO |
> > +					 STATX_ATIME |
> > +					 STATX_MTIME |
> > +					 STATX_CTIME |
> > +					 STATX_SIZE |
> > +					 STATX_BLOCKS,
> > +					 AT_STATX_SYNC_AS_STAT) == 0) {
> > +		fattr->valid = NFS_ATTR_FATTR_FILEID |
> > +			NFS_ATTR_FATTR_CHANGE |
> > +			NFS_ATTR_FATTR_SIZE |
> > +			NFS_ATTR_FATTR_ATIME |
> > +			NFS_ATTR_FATTR_MTIME |
> > +			NFS_ATTR_FATTR_CTIME |
> > +			NFS_ATTR_FATTR_SPACE_USED;
> > +		fattr->fileid = stat.ino;
> > +		fattr->size = stat.size;
> > +		fattr->atime = stat.atime;
> > +		fattr->mtime = stat.mtime;
> > +		fattr->ctime = stat.ctime;
> > +		fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> 
> This looks wrong for NFSv4.  I think we should use
> nfsd4_change_attribute().
> Maybe it isn't important, but if it isn't I'd like to see an explanation
> why.
> 
> > +		fattr->du.nfs3.used = stat.blocks << 9;
> > +	}
> > +}

Not following, this is client code so it doesn't have access to
nfsd4_change_attribute().

Pending clarification, and further review on my part, leaving this
item to one side (so won't be addressed in v7).

Thanks,
Mike