On Thu, Nov 08, 2018 at 02:16:04PM -0500, Olga Kornievskaia wrote:
> On Wed, Nov 7, 2018 at 4:49 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:29:05AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > >
> > > Given a universal address, mount the source server from the destination
> > > server. Use an internal mount. Call the NFS client nfs42_ssc_open to
> > > obtain the NFS struct file suitable for nfsd_copy_range.
> > >
> > > Ability to do "inter" server-to-server depends on the an nfsd kernel
> > > parameter "inter_copy_offload_enabled".
> > >
> > > Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
> > > Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/nfs4proc.c | 298 ++++++++++++++++++++++++++++++++++++++++++++++++---
> > > fs/nfsd/nfssvc.c | 6 ++
> > > fs/nfsd/xdr4.h | 5 +
> > > include/linux/nfs4.h | 1 +
> > > 4 files changed, 293 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 59e9d0c..6dcd80c 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -1153,6 +1153,229 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
> > > while ((copy = nfsd4_get_copy(clp)) != NULL)
> > > nfsd4_stop_copy(copy);
> > > }
> > > +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > > +
> > > +extern struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> > > + struct nfs_fh *src_fh,
> > > + nfs4_stateid *stateid);
> > > +extern void nfs42_ssc_close(struct file *filep);
> > > +
> > > +extern void nfs_sb_deactive(struct super_block *sb);
> > > +
> > > +#define NFSD42_INTERSSC_MOUNTOPS "minorversion=2,vers=4,addr=%s,clientaddr=%s"
> >
> > The nfs man page says "clientaddr=" has no effect on 4.2 mounts.
>
> I only have nfs man page from RHEL7.5 and I don't see that.
>From nfs-utils/utils/mount/nfs.man:
NFS protocol versions 4.1 and 4.2 use the client-established TCP
connection for callback requests, so do not require the server
to connect to the client. This option is therefore only affect
NFS version 4.0 mounts.
(Maybe I should send a patch for that "is therefore" typo.)
> > Also, what's the "addr=" option for, isn't the server address already
> > given in the mount string? (Honest question, I may be wrong here.)
>
> I believe going thru the kernel vfs_kern_mount() we need to specify
> "addr=" otherwise it doesn't know which server to mount.
Yeah, now that I think of it I guess the kernel hasn't traditionally
done DNS resolution so of course there'd have to be something like this.
OK.
> > > +
> > > +/**
> > > + * Support one copy source server for now.
> > > + */
> > > +static struct vfsmount *
> > > +nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp)
> > > +{
> > > + struct file_system_type *type;
> > > + struct vfsmount *ss_mnt;
> > > + struct nfs42_netaddr *naddr;
> > > + struct sockaddr_storage tmp_addr;
> > > + size_t tmp_addrlen, match_netid_len = 3;
> > > + char *startsep = "", *endsep = "", *match_netid = "tcp";
> > > + char *ipaddr, *ipaddr2, *raw_data;
> > > + int len, raw_len, status = -EINVAL;
> > > +
> > > + /* Currently support only NL4_NETADDR source server */
> > > + if (nss->nl4_type != NL4_NETADDR) {
> > > + WARN(nss->nl4_type != NL4_NETADDR,
> > > + "nfsd4_copy src server not NL4_NETADDR\n");
> >
> > Won't nfsd4_decode_nl4_server actually let through NL4_NAME and NL4_URL?
>
> Yes. I think the logic would be not to limit the xdr functionality
> from not parsing it as if the support in the main code the xdr code
> doesn't change.
I think it would be simplest just to return the right error from
nfsd4_decode_nl4_server() in the NL4_NAME/NL4_URL cases.
> > That would make this WARN() triggerable by a client--that's bad.
>
> Why? Would you rather it silently failed?
Returning an error would be fine.
But it should never be possible for an ordinary user or somebody on the
network to trigger a WARN() or a BUG(). Those should be reserved for
things that we assume never happen (so they indicate that our
assumptions are wrong, hence we have a possible kernel bug).
> Thank you for the reviews. I'm working on the next version. But in
> addition to this, I need the VFS piece with this patch series now
> because server piece needs the generic cross filesystem
> copy_file_range() support via do_splice because the server reads out
> of NFS and writes into the local file system.
OK. In addition to mailing the patches it might also be useful if you
could point me to a git branch somewhere just to make sure I've got all
the right prerequisites.
--b.
