[PATCH v1 00/13] server-side support for "inter" SSC copy

[PATCH v1 00/13] server-side support for "inter" SSC copy

[Olga Kornievskaia]

This patch series adds support for NFSv4.2 copy offload feature
allowing copy between two different NFS servers.

This feature is enabled by the kernel module parameter --
inter_copy_offload_enable -- and by default is disabled. There is
also a kernel compile configuration of NFSD_V4_2_INTER_SSC that
adds dependency on the NFS client side functions called from the
server.

These patches work on top of 4 existing async intra copy offload
patches. For the "inter" SSC, the implementation only supports
asynchronous inter copy.

On the source server, upon receiving a COPY_NOTIFY, it generate a
unique stateid that's kept in the global list. Upon receiving a READ
with a stateid, the code checks the normal list of open stateid and
now additionally, it'll check the copy state list as well before
deciding to either fail with BAD_STATEID or find one that matches.
The stored stateid is only valid to be used for the first time
with a choosen lease period (90s currently). When the source server
received an OFFLOAD_CANCEL, it will remove the stateid from the
global list. Otherwise, the copy stateid is removed upon the removal
of its "parent" stateid (open/lock/delegation stateid).

On the destination server, upon receiving a COPY request, the server
establishes the necessary clientid/session with the source server.
It calls into the NFS client code to establish the necessary
open stateid, filehandle, file description (without doing an NFS open).
Then the server calls into the copy_file_range() to preform the copy
where the source file will issue NFS READs and then do local file
system writes (this depends on the VFS ability to do cross device
copy_file_range().

Client-side patches are included for the ease of testing.

Anna Schumaker (1):
  fs: Don't copy beyond the end of the file

Olga Kornievskaia (12):
  VFS permit cross device vfs_copy_file_range
  NFS NFSD defining nl4_servers structure needed by both
  NFS inter ssc open
  NFS skip recovery of copy open on dest server
  NFSD fill-in netloc4 structure
  NFSD add ca_source_server<> to COPY
  NFSD return nfs4_stid in nfs4_preprocess_stateid_op
  NFSD add COPY_NOTIFY operation
  NFSD check stateids against copy stateids
  NFSD generalize nfsd4_compound_state flag names
  NFSD: allow inter server COPY to have a STALE source server fh
  NFSD add nfs4 inter ssc to nfsd4_copy

 Documentation/filesystems/vfs.txt |   4 +-
 fs/nfs/nfs4_fs.h                  |   8 +
 fs/nfs/nfs4file.c                 |  99 +++++++++
 fs/nfs/nfs4proc.c                 |   5 +-
 fs/nfs/nfs4state.c                |  14 ++
 fs/nfsd/Kconfig                   |  10 +
 fs/nfsd/nfs4proc.c                | 433 +++++++++++++++++++++++++++++++++++---
 fs/nfsd/nfs4state.c               | 128 +++++++++--
 fs/nfsd/nfs4xdr.c                 | 168 ++++++++++++++-
 fs/nfsd/nfsd.h                    |  31 ++-
 fs/nfsd/nfsfh.h                   |   5 +-
 fs/nfsd/nfssvc.c                  |   6 +
 fs/nfsd/state.h                   |  18 +-
 fs/nfsd/xdr4.h                    |  26 ++-
 fs/read_write.c                   |  16 +-
 include/linux/nfs4.h              |  25 +++
 16 files changed, 929 insertions(+), 67 deletions(-)

-- 
1.8.3.1

[PATCH v1 01/13] fs: Don't copy beyond the end of the file

[Olga Kornievskaia]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 fs/read_write.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/read_write.c b/fs/read_write.c
index 39b4a21..c60790f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (unlikely(ret))
 		return ret;
 
+	if (pos_in >= i_size_read(inode_in))
+		return -EINVAL;
+
 	if (!(file_in->f_mode & FMODE_READ) ||
 	    !(file_out->f_mode & FMODE_WRITE) ||
 	    (file_out->f_flags & O_APPEND))
-- 
1.8.3.1

[PATCH v1 02/13] VFS permit cross device vfs_copy_file_range

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Allow copy_file_range to copy between different superblocks but only
of the same file system types. This feature was of interest to CIFS
as well as NFS.

This feature is needed by NFSv4.2 to perform file copy operation on
the same server or file copy between different NFSv4.2 servers.

If a file system's fileoperations copy_file_range operation prohibits
cross-device copies, fall back to do_splice_direct. This would be
needed for the NFS (destination) server side implementation of the
file copy and currently for CIFS.

Besides NFS, there is only 1 implementor of the copy_file_range FS
operation -- CIFS. CIFS assumes incoming file descriptors are both
CIFS but it will check if they are coming from different servers and
return error code to fall back to do_splice_direct.

NFS will allow for copies between different NFS servers.

Adding to the vfs.txt documentation to explicitly warn about allowing
for different superblocks of the same file type to be passed into the
copy_file_range for the future users of copy_file_range method.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 Documentation/filesystems/vfs.txt |  4 +++-
 fs/read_write.c                   | 13 ++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index a6c6a8a..5e520de 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -958,7 +958,9 @@ otherwise noted.
 
   fallocate: called by the VFS to preallocate blocks or punch a hole.
 
-  copy_file_range: called by the copy_file_range(2) system call.
+  copy_file_range: called by copy_file_range(2) system call. This method
+		   works on two file descriptors that might reside on
+		   different superblocks of the same type of file system.
 
   clone_file_range: called by the ioctl(2) system call for FICLONERANGE and
 	FICLONE commands.
diff --git a/fs/read_write.c b/fs/read_write.c
index c60790f..474e740 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1578,10 +1578,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
-	/* this could be relaxed once a method supports cross-fs copies */
-	if (inode_in->i_sb != inode_out->i_sb)
-		return -EXDEV;
-
 	if (len == 0)
 		return 0;
 
@@ -1591,7 +1587,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	 * Try cloning first, this is supported by more file systems, and
 	 * more efficient if both clone and copy are supported (e.g. NFS).
 	 */
-	if (file_in->f_op->clone_file_range) {
+	if (inode_in->i_sb == inode_out->i_sb &&
+			file_in->f_op->clone_file_range) {
 		ret = file_in->f_op->clone_file_range(file_in, pos_in,
 				file_out, pos_out, len);
 		if (ret == 0) {
@@ -1600,10 +1597,12 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		}
 	}
 
-	if (file_out->f_op->copy_file_range) {
+	if (file_out->f_op->copy_file_range &&
+			(file_in->f_op->copy_file_range ==
+				file_out->f_op->copy_file_range)) {
 		ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out,
 						      pos_out, len, flags);
-		if (ret != -EOPNOTSUPP)
+		if (ret != -EOPNOTSUPP && ret != -EXDEV)
 			goto done;
 	}
 
-- 
1.8.3.1

[PATCH v1 03/13] NFS NFSD defining nl4_servers structure needed by both

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

These structures are needed by COPY_NOTIFY on the client and needed
by the nfsd as well

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/nfs4.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 1b06f0b..4d76f87 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 #include <linux/uidgid.h>
 #include <uapi/linux/nfs4.h>
+#include <linux/sunrpc/msg_prot.h>
 
 enum nfs4_acl_whotype {
 	NFS4_ACL_WHO_NAMED = 0,
@@ -672,4 +673,27 @@ struct nfs4_op_map {
 	} u;
 };
 
+struct nfs42_netaddr {
+	char		netid[RPCBIND_MAXNETIDLEN];
+	char		addr[RPCBIND_MAXUADDRLEN + 1];
+	u32		netid_len;
+	u32 addr_len;
+};
+
+enum netloc_type4 {
+	NL4_NAME		= 1,
+	NL4_URL			= 2,
+	NL4_NETADDR		= 3,
+};
+
+struct nl4_server {
+	enum netloc_type4	nl4_type;
+	union {
+		struct { /* NL4_NAME, NL4_URL */
+			int	nl4_str_sz;
+			char	nl4_str[NFS4_OPAQUE_LIMIT + 1];
+		};
+		struct nfs42_netaddr	nl4_addr; /* NL4_NETADDR */
+	} u;
+};
 #endif
-- 
1.8.3.1

[PATCH v1 04/13] NFS inter ssc open

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

NFSv4.2 inter server to server copy requires the destination server to
READ the data from the source server using the provided stateid and
file handle.

Given an NFSv4 stateid and filehandle from the COPY operaion, provide the
destination server with an NFS client function to create a struct file
suitable for the destiniation server to READ the data to be copied.

Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4_fs.h  |  7 ++++
 fs/nfs/nfs4file.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfs/nfs4proc.c |  5 ++-
 3 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 8d59c96..f229864 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -307,6 +307,13 @@ extern int nfs4_set_rw_stateid(nfs4_stateid *stateid,
 		const struct nfs_open_context *ctx,
 		const struct nfs_lock_context *l_ctx,
 		fmode_t fmode);
+extern int nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
+			     struct nfs_fattr *fattr, struct nfs4_label *label,
+			     struct inode *inode);
+extern int update_open_stateid(struct nfs4_state *state,
+				const nfs4_stateid *open_stateid,
+				const nfs4_stateid *deleg_stateid,
+				fmode_t fmode);
 
 #if defined(CONFIG_NFS_V4_1)
 extern int nfs41_sequence_done(struct rpc_task *, struct nfs4_sequence_res *);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 4288a6e..f82cb05 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -8,6 +8,7 @@
 #include <linux/file.h>
 #include <linux/falloc.h>
 #include <linux/nfs_fs.h>
+#include <linux/file.h>
 #include "delegation.h"
 #include "internal.h"
 #include "iostat.h"
@@ -242,6 +243,103 @@ static int nfs42_clone_file_range(struct file *src_file, loff_t src_off,
 out:
 	return ret;
 }
+
+static int read_name_gen = 1;
+#define SSC_READ_NAME_BODY "ssc_read_%d"
+
+struct file *
+nfs42_ssc_open(struct vfsmount *ss_mnt, struct nfs_fh *src_fh,
+		nfs4_stateid *stateid)
+{
+	struct nfs_fattr fattr;
+	struct file *filep, *res;
+	struct nfs_server *server;
+	struct inode *r_ino = NULL;
+	struct nfs_open_context *ctx;
+	struct nfs4_state_owner *sp;
+	char *read_name;
+	int len, status = 0;
+
+	server = NFS_SERVER(ss_mnt->mnt_root->d_inode);
+
+	nfs_fattr_init(&fattr);
+
+	status = nfs4_proc_getattr(server, src_fh, &fattr, NULL, NULL);
+	if (status < 0) {
+		res = ERR_PTR(status);
+		goto out;
+	}
+
+	res = ERR_PTR(-ENOMEM);
+	len = strlen(SSC_READ_NAME_BODY) + 16;
+	read_name = kzalloc(len, GFP_NOFS);
+	if (read_name == NULL)
+		goto out;
+	snprintf(read_name, len, SSC_READ_NAME_BODY, read_name_gen++);
+	dprintk("%s read_name %s\n", __func__, read_name);
+
+	r_ino = nfs_fhget(ss_mnt->mnt_root->d_inode->i_sb, src_fh, &fattr,
+			NULL);
+	if (IS_ERR(r_ino)) {
+		res = ERR_CAST(r_ino);
+		goto out;
+	}
+
+	filep = alloc_file_pseudo(r_ino, ss_mnt, read_name, FMODE_READ,
+				     r_ino->i_fop);
+	if (IS_ERR(filep)) {
+		res = ERR_CAST(filep);
+		goto out;
+	}
+	filep->f_mode |= FMODE_READ;
+
+	ctx = alloc_nfs_open_context(filep->f_path.dentry, filep->f_mode,
+					filep);
+	if (IS_ERR(ctx)) {
+		res = ERR_CAST(ctx);
+		goto out_filep;
+	}
+
+	res = ERR_PTR(-EINVAL);
+	sp = nfs4_get_state_owner(server, ctx->cred, GFP_KERNEL);
+	if (sp == NULL)
+		goto out_ctx;
+
+	ctx->state = nfs4_get_open_state(r_ino, sp);
+	if (ctx->state == NULL)
+		goto out_stateowner;
+
+	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
+	memcpy(&ctx->state->open_stateid.other, &stateid->other,
+	       NFS4_STATEID_OTHER_SIZE);
+	update_open_stateid(ctx->state, stateid, NULL, filep->f_mode);
+
+	nfs_file_set_open_context(filep, ctx);
+	put_nfs_open_context(ctx);
+
+	file_ra_state_init(&filep->f_ra, filep->f_mapping->host->i_mapping);
+	res = filep;
+out:
+	dprintk("<-- %s error %ld filep %p r_ino %p\n",
+		__func__, IS_ERR(res) ? PTR_ERR(res) : 0, res, r_ino);
+
+	return res;
+out_stateowner:
+	nfs4_put_state_owner(sp);
+out_ctx:
+	put_nfs_open_context(ctx);
+out_filep:
+	fput(filep);
+	goto out;
+}
+EXPORT_SYMBOL_GPL(nfs42_ssc_open);
+void nfs42_ssc_close(struct file *filep)
+{
+	struct nfs_open_context *ctx = nfs_file_open_context(filep);
+
+	ctx->state->flags = 0;
+}
+EXPORT_SYMBOL_GPL(nfs42_ssc_close);
 #endif /* CONFIG_NFS_V4_2 */
 
 const struct file_operations nfs4_file_operations = {
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index db84b4a..1876456 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -91,7 +91,6 @@
 static int _nfs4_recover_proc_open(struct nfs4_opendata *data);
 static int nfs4_do_fsinfo(struct nfs_server *, struct nfs_fh *, struct nfs_fsinfo *);
 static void nfs_fixup_referral_attributes(struct nfs_fattr *fattr);
-static int nfs4_proc_getattr(struct nfs_server *, struct nfs_fh *, struct nfs_fattr *, struct nfs4_label *label, struct inode *inode);
 static int _nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle, struct nfs_fattr *fattr, struct nfs4_label *label, struct inode *inode);
 static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 			    struct nfs_fattr *fattr, struct iattr *sattr,
@@ -1653,7 +1652,7 @@ static void nfs_state_clear_delegation(struct nfs4_state *state)
 	write_sequnlock(&state->seqlock);
 }
 
-static int update_open_stateid(struct nfs4_state *state,
+int update_open_stateid(struct nfs4_state *state,
 		const nfs4_stateid *open_stateid,
 		const nfs4_stateid *delegation,
 		fmode_t fmode)
@@ -3936,7 +3935,7 @@ static int _nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
 	return nfs4_call_sync(server->client, server, &msg, &args.seq_args, &res.seq_res, 0);
 }
 
-static int nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
+int nfs4_proc_getattr(struct nfs_server *server, struct nfs_fh *fhandle,
 				struct nfs_fattr *fattr, struct nfs4_label *label,
 				struct inode *inode)
 {
-- 
1.8.3.1

[PATCH v1 05/13] NFS skip recovery of copy open on dest server

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Mark the open created for the source file on the destination
server. Then if this open is going thru a recovery, then fail
the recovery as we don't need to be recoving a "fake" open.
We need to fail the ongoing READs and vfs_copy_file_range().

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4_fs.h   |  1 +
 fs/nfs/nfs4file.c  |  1 +
 fs/nfs/nfs4state.c | 14 ++++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index f229864..8b00c90 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -165,6 +165,7 @@ enum {
 	NFS_STATE_CHANGE_WAIT,		/* A state changing operation is outstanding */
 #ifdef CONFIG_NFS_V4_2
 	NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
+	NFS_SRV_SSC_COPY_STATE,		/* ssc state on the dst server */
 #endif /* CONFIG_NFS_V4_2 */
 };
 
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index f82cb05..7909cd4 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -309,6 +309,7 @@ struct file *
 	if (ctx->state == NULL)
 		goto out_stateowner;
 
+	set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
 	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
 	memcpy(&ctx->state->open_stateid.other, &stateid->other,
 	       NFS4_STATEID_OTHER_SIZE);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 62ae0fd..b0b82c6 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1606,6 +1606,9 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 {
 	struct nfs4_state *state;
 	int status = 0;
+#ifdef CONFIG_NFS_V4_2
+	bool found_ssc_copy_state = false;
+#endif /* CONFIG_NFS_V4_2 */
 
 	/* Note: we rely on the sp->so_states list being ordered 
 	 * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
@@ -1625,6 +1628,13 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 			continue;
 		if (state->state == 0)
 			continue;
+#ifdef CONFIG_NFS_V4_2
+		if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
+			nfs4_state_mark_recovery_failed(state, -EIO);
+			found_ssc_copy_state = true;
+			continue;
+		}
+#endif /* CONFIG_NFS_V4_2 */
 		refcount_inc(&state->count);
 		spin_unlock(&sp->so_lock);
 		status = __nfs4_reclaim_open_state(sp, state, ops);
@@ -1671,6 +1681,10 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
 	}
 	raw_write_seqcount_end(&sp->so_reclaim_seqcount);
 	spin_unlock(&sp->so_lock);
+#ifdef CONFIG_NFS_V4_2
+	if (found_ssc_copy_state)
+		return -EIO;
+#endif /* CONFIG_NFS_V4_2 */
 	return 0;
 out_err:
 	nfs4_put_open_state(state);
-- 
1.8.3.1

[PATCH v1 06/13] NFSD fill-in netloc4 structure

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

nfs.4 defines nfs42_netaddr structure that represents netloc4.

Populate needed fields from the sockaddr structure.

This will be used by flexfiles and 4.2 inter copy

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfsd.h | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 0668999..030fccb 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -18,7 +18,7 @@
 #include <linux/nfs4.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/msg_prot.h>
-
+#include <linux/sunrpc/addr.h>
 #include <uapi/linux/nfsd/debug.h>
 
 #include "stats.h"
@@ -366,6 +366,35 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
 
 extern const u32 nfsd_suppattrs[3][3];
 
+static inline u32 nfsd4_set_netaddr(struct sockaddr *addr, struct nfs42_netaddr *netaddr)
+{
+	struct sockaddr_in *sin = (struct sockaddr_in *)addr;
+	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
+	unsigned int port;
+	size_t ret;
+
+	switch (addr->sa_family) {
+	case AF_INET:
+		port = ntohs(sin->sin_port);
+		sprintf(netaddr->netid, "tcp");
+		netaddr->netid_len = 3;
+		break;
+	case AF_INET6:
+		port = ntohs(sin6->sin6_port);
+		sprintf(netaddr->netid, "tcp6");
+		netaddr->netid_len = 4;
+		break;
+	default:
+		return nfserrno(-EINVAL);
+	}
+	ret = rpc_ntop(addr, netaddr->addr, sizeof(netaddr->addr));
+	netaddr->addr_len = ret + snprintf(netaddr->addr + ret,
+				     RPCBIND_MAXUADDRLEN + 1,
+				     ".%u.%u", port >> 8, port & 0xff);
+
+	return 0;
+}
+
 static inline bool bmval_is_subset(const u32 *bm1, const u32 *bm2)
 {
 	return !((bm1[0] & ~bm2[0]) ||
-- 
1.8.3.1

[PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[Olga Kornievskaia]

Note: followed conventions and have struct nfsd4_compoundargs pointer as a
parameter even though it is unused.

Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/xdr4.h    |  1 +
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 3de42a7..9f6886f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -40,6 +40,7 @@
 #include <linux/utsname.h>
 #include <linux/pagemap.h>
 #include <linux/sunrpc/svcauth_gss.h>
+#include <linux/sunrpc/addr.h>
 
 #include "idmap.h"
 #include "acl.h"
@@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
 	DECODE_TAIL;
 }
 
+static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
+				      struct nl4_server *ns)
+{
+	DECODE_HEAD;
+	struct nfs42_netaddr *naddr;
+
+	READ_BUF(4);
+	ns->nl4_type = be32_to_cpup(p++);
+
+	/* currently support for 1 inter-server source server */
+	switch (ns->nl4_type) {
+	case NL4_NAME:
+	case NL4_URL:
+		READ_BUF(4);
+		ns->u.nl4_str_sz = be32_to_cpup(p++);
+		if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT)
+			goto xdr_error;
+
+		READ_BUF(ns->u.nl4_str_sz);
+		COPYMEM(ns->u.nl4_str,
+			ns->u.nl4_str_sz);
+		break;
+	case NL4_NETADDR:
+		naddr = &ns->u.nl4_addr;
+
+		READ_BUF(4);
+		naddr->netid_len = be32_to_cpup(p++);
+		if (naddr->netid_len > RPCBIND_MAXNETIDLEN)
+			goto xdr_error;
+
+		READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */
+		COPYMEM(naddr->netid, naddr->netid_len);
+
+		naddr->addr_len = be32_to_cpup(p++);
+		if (naddr->addr_len > RPCBIND_MAXUADDRLEN)
+			goto xdr_error;
+
+		READ_BUF(naddr->addr_len);
+		COPYMEM(naddr->addr, naddr->addr_len);
+		break;
+	default:
+		goto xdr_error;
+	}
+	DECODE_TAIL;
+}
+
 static __be32
 nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy)
 {
 	DECODE_HEAD;
-	unsigned int tmp;
+	struct nl4_server *ns;
+	int i, count;
 
 	status = nfsd4_decode_stateid(argp, &copy->cp_src_stateid);
 	if (status)
@@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
 	p = xdr_decode_hyper(p, &copy->cp_count);
 	p++; /* ca_consecutive: we always do consecutive copies */
 	copy->cp_synchronous = be32_to_cpup(p++);
-	tmp = be32_to_cpup(p); /* Source server list not supported */
+	count = be32_to_cpup(p++);
+
+	if (count == 0) /* intra-server copy */
+		goto intra;
 
+	/* decode all the supplied server addresses but use first */
+	copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);
+	if (copy->cp_src == NULL)
+		return nfserrno(-ENOMEM);
+
+	ns = copy->cp_src;
+	for (i = 0; i < count; i++) {
+		status = nfsd4_decode_nl4_server(argp, ns);
+		if (status)
+			return status;
+		ns++;
+	}
+intra:
 	DECODE_TAIL;
 }
 
@@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 	p = xdr_reserve_space(&resp->xdr, 4 + 4);
 	*p++ = xdr_one; /* cr_consecutive */
 	*p++ = cpu_to_be32(copy->cp_synchronous);
+
+	/* allocated in nfsd4_decode_copy */
+	kfree(copy->cp_src);
 	return 0;
 }
 
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index feeb6d4..b4d1140 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -521,6 +521,7 @@ struct nfsd4_copy {
 	u64		cp_src_pos;
 	u64		cp_dst_pos;
 	u64		cp_count;
+	struct nl4_server *cp_src;
 
 	/* both */
 	bool		cp_synchronous;
-- 
1.8.3.1

[PATCH v1 08/13] NFSD return nfs4_stid in nfs4_preprocess_stateid_op

[Olga Kornievskaia]

Needed for copy to add nfs4_cp_state to the nfs4_stid.

Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 17 ++++++++++-------
 fs/nfsd/nfs4state.c |  8 ++++++--
 fs/nfsd/state.h     |  3 ++-
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index edff074..29686df 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -781,7 +781,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	/* check stateid */
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					&read->rd_stateid, RD_STATE,
-					&read->rd_filp, &read->rd_tmp_file);
+					&read->rd_filp, &read->rd_tmp_file,
+					NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
 		goto out;
@@ -954,7 +955,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
 		status = nfs4_preprocess_stateid_op(rqstp, cstate,
 				&cstate->current_fh, &setattr->sa_stateid,
-				WR_STATE, NULL, NULL);
+				WR_STATE, NULL, NULL, NULL);
 		if (status) {
 			dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
 			return status;
@@ -1005,7 +1006,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	trace_nfsd_write_start(rqstp, &cstate->current_fh,
 			       write->wr_offset, cnt);
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
-						stateid, WR_STATE, &filp, NULL);
+					stateid, WR_STATE, &filp, NULL, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
 		return status;
@@ -1039,14 +1040,16 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	__be32 status;
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
-					    src_stateid, RD_STATE, src, NULL);
+					    src_stateid, RD_STATE, src, NULL,
+					    NULL);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process src stateid!\n", __func__);
 		goto out;
 	}
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
-					    dst_stateid, WR_STATE, dst, NULL);
+					    dst_stateid, WR_STATE, dst, NULL,
+					    NULL);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
 		goto out_put_src;
@@ -1350,7 +1353,7 @@ struct nfsd4_copy *
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    &fallocate->falloc_stateid,
-					    WR_STATE, &file, NULL);
+					    WR_STATE, &file, NULL, NULL);
 	if (status != nfs_ok) {
 		dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
 		return status;
@@ -1409,7 +1412,7 @@ struct nfsd4_copy *
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    &seek->seek_stateid,
-					    RD_STATE, &file, NULL);
+					    RD_STATE, &file, NULL, NULL);
 	if (status) {
 		dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
 		return status;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 07a57d0..e263fd0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5159,7 +5159,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 __be32
 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
-		stateid_t *stateid, int flags, struct file **filpp, bool *tmp_file)
+		stateid_t *stateid, int flags, struct file **filpp,
+		bool *tmp_file, struct nfs4_stid **cstid)
 {
 	struct inode *ino = d_inode(fhp->fh_dentry);
 	struct net *net = SVC_NET(rqstp);
@@ -5210,8 +5211,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	if (!status && filpp)
 		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
 out:
-	if (s)
+	if (s) {
+		if (!status && cstid)
+			*cstid = s;
 		nfs4_put_stid(s);
+	}
 	return status;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 6aacb32..304de3b 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -606,7 +606,8 @@ struct nfsd4_blocked_lock {
 
 extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
-		stateid_t *stateid, int flags, struct file **filp, bool *tmp_file);
+		stateid_t *stateid, int flags, struct file **filp,
+		bool *tmp_file, struct nfs4_stid **cstid);
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
-- 
1.8.3.1

[PATCH v1 09/13] NFSD add COPY_NOTIFY operation

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Introducing the COPY_NOTIFY operation.

Create a new unique stateid that will keep track of the copy
state and the upcoming READs that will use that stateid. Keep
it in the list associated with parent stateid.

Return single netaddr to advertise to the copy.

Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 69 ++++++++++++++++++++++++++++++++++---
 fs/nfsd/nfs4state.c | 70 +++++++++++++++++++++++++++++++++-----
 fs/nfsd/nfs4xdr.c   | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/state.h     | 15 +++++++++
 fs/nfsd/xdr4.h      | 13 +++++++
 5 files changed, 250 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 29686df..999c8cb 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -37,6 +37,7 @@
 #include <linux/falloc.h>
 #include <linux/slab.h>
 #include <linux/kthread.h>
+#include <linux/sunrpc/addr.h>
 
 #include "idmap.h"
 #include "cache.h"
@@ -1035,7 +1036,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 static __be32
 nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		  stateid_t *src_stateid, struct file **src,
-		  stateid_t *dst_stateid, struct file **dst)
+		  stateid_t *dst_stateid, struct file **dst,
+		  struct nfs4_stid **stid)
 {
 	__be32 status;
 
@@ -1049,7 +1051,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 
 	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
 					    dst_stateid, WR_STATE, dst, NULL,
-					    NULL);
+					    stid);
 	if (status) {
 		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
 		goto out_put_src;
@@ -1080,7 +1082,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
 	__be32 status;
 
 	status = nfsd4_verify_copy(rqstp, cstate, &clone->cl_src_stateid, &src,
-				   &clone->cl_dst_stateid, &dst);
+				   &clone->cl_dst_stateid, &dst, NULL);
 	if (status)
 		goto out;
 
@@ -1267,7 +1269,7 @@ static int nfsd4_do_async_copy(void *data)
 
 	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
 				   &copy->file_src, &copy->cp_dst_stateid,
-				   &copy->file_dst);
+				   &copy->file_dst, NULL);
 	if (status)
 		goto out;
 
@@ -1345,6 +1347,44 @@ struct nfsd4_copy *
 }
 
 static __be32
+nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+		  union nfsd4_op_u *u)
+{
+	struct nfsd4_copy_notify *cn = &u->copy_notify;
+	__be32 status;
+	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
+	struct nfs4_stid *stid;
+	struct nfs4_cpntf_state *cps;
+
+	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
+					&cn->cpn_src_stateid, RD_STATE, NULL,
+					NULL, &stid);
+	if (status)
+		return status;
+
+	cn->cpn_sec = nn->nfsd4_lease;
+	cn->cpn_nsec = 0;
+
+	status = nfserrno(-ENOMEM);
+	cps = nfs4_alloc_init_cpntf_state(nn, stid);
+	if (!cps)
+		return status;
+	memcpy(&cn->cpn_cnr_stateid, &cps->cp_stateid, sizeof(stateid_t));
+
+	/**
+	 * For now, only return one server address in cpn_src, the
+	 * address used by the client to connect to this server.
+	 */
+	cn->cpn_src.nl4_type = NL4_NETADDR;
+	status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr,
+				 &cn->cpn_src.u.nl4_addr);
+	if (status != 0)
+		nfs4_free_cpntf_state(cps);
+
+	return status;
+}
+
+static __be32
 nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		struct nfsd4_fallocate *fallocate, int flags)
 {
@@ -2296,6 +2336,21 @@ static inline u32 nfsd4_offload_status_rsize(struct svc_rqst *rqstp,
 		1 /* osr_complete<1> optional 0 for now */) * sizeof(__be32);
 }
 
+static inline u32 nfsd4_copy_notify_rsize(struct svc_rqst *rqstp,
+					struct nfsd4_op *op)
+{
+	return (op_encode_hdr_size +
+		3 /* cnr_lease_time */ +
+		1 /* We support one cnr_source_server */ +
+		1 /* cnr_stateid seq */ +
+		op_encode_stateid_maxsz /* cnr_stateid */ +
+		1 /* num cnr_source_server*/ +
+		1 /* nl4_type */ +
+		1 /* nl4 size */ +
+		XDR_QUADLEN(NFS4_OPAQUE_LIMIT) /*nl4_loc + nl4_loc_sz */)
+		* sizeof(__be32);
+}
+
 #ifdef CONFIG_NFSD_PNFS
 static inline u32 nfsd4_getdeviceinfo_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 {
@@ -2720,6 +2775,12 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
 		.op_name = "OP_OFFLOAD_CANCEL",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
+	[OP_COPY_NOTIFY] = {
+		.op_func = nfsd4_copy_notify,
+		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
+		.op_name = "OP_COPY_NOTIFY",
+		.op_rsize_bop = nfsd4_copy_notify_rsize,
+	},
 };
 
 /**
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e263fd0..7764a8b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -697,6 +697,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 	/* Will be incremented before return to client: */
 	refcount_set(&stid->sc_count, 1);
 	spin_lock_init(&stid->sc_lock);
+	INIT_LIST_HEAD(&stid->sc_cp_list);
 
 	/*
 	 * It shouldn't be a problem to reuse an opaque stateid value.
@@ -716,33 +717,85 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 /*
  * Create a unique stateid_t to represent each COPY.
  */
-int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
+int _nfs4_init_state(struct nfsd_net *nn, void *ptr, stateid_t *stid)
 {
 	int new_id;
 
 	idr_preload(GFP_KERNEL);
 	spin_lock(&nn->s2s_cp_lock);
-	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, copy, 0, 0, GFP_NOWAIT);
+	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, ptr, 0, 0, GFP_NOWAIT);
 	spin_unlock(&nn->s2s_cp_lock);
 	idr_preload_end();
 	if (new_id < 0)
 		return 0;
-	copy->cp_stateid.si_opaque.so_id = new_id;
-	copy->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
-	copy->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
+	stid->si_opaque.so_id = new_id;
+	stid->si_opaque.so_clid.cl_boot = nn->boot_time;
+	stid->si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
 	return 1;
 }
 
-void nfs4_free_cp_state(struct nfsd4_copy *copy)
+int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
+{
+	return _nfs4_init_state(nn, copy, &copy->cp_stateid);
+}
+
+struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
+						     struct nfs4_stid *p_stid)
+{
+	struct nfs4_cpntf_state *cps;
+
+	cps = kzalloc(sizeof(struct nfs4_cpntf_state), GFP_KERNEL);
+	if (!cps)
+		return NULL;
+	if (!_nfs4_init_state(nn, cps, &cps->cp_stateid))
+		goto out_free;
+	cps->cp_p_stid = p_stid;
+	cps->cp_active = false;
+	cps->cp_timeout = jiffies + (nn->nfsd4_lease * HZ);
+	INIT_LIST_HEAD(&cps->cp_list);
+	list_add(&cps->cp_list, &p_stid->sc_cp_list);
+
+	return cps;
+out_free:
+	kfree(cps);
+	return NULL;
+}
+
+void _nfs4_free_state(struct net *n, stateid_t *stid)
 {
 	struct nfsd_net *nn;
 
-	nn = net_generic(copy->cp_clp->net, nfsd_net_id);
+	nn = net_generic(n, nfsd_net_id);
 	spin_lock(&nn->s2s_cp_lock);
-	idr_remove(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
+	idr_remove(&nn->s2s_cp_stateids, stid->si_opaque.so_id);
 	spin_unlock(&nn->s2s_cp_lock);
 }
 
+void nfs4_free_cp_state(struct nfsd4_copy *copy)
+{
+	_nfs4_free_state(copy->cp_clp->net, &copy->cp_stateid);
+}
+
+void nfs4_free_cpntf_state(struct nfs4_cpntf_state *cps)
+{
+	_nfs4_free_state(cps->cp_p_stid->sc_client->net, &cps->cp_stateid);
+	kfree(cps);
+}
+
+static void nfs4_free_cpntf_statelist(struct nfs4_stid *stid)
+{
+	struct nfs4_cpntf_state *cps;
+
+	might_sleep();
+
+	while (!list_empty(&stid->sc_cp_list)) {
+		cps = list_first_entry(&stid->sc_cp_list,
+				       struct nfs4_cpntf_state, cp_list);
+		list_del(&cps->cp_list);
+		nfs4_free_cpntf_state(cps);
+	}
+}
+
 static struct nfs4_ol_stateid * nfs4_alloc_open_stateid(struct nfs4_client *clp)
 {
 	struct nfs4_stid *stid;
@@ -891,6 +944,7 @@ static void block_delegations(struct knfsd_fh *fh)
 	}
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
 	spin_unlock(&clp->cl_lock);
+	nfs4_free_cpntf_statelist(s);
 	s->sc_free(s);
 	if (fp)
 		put_nfs4_file(fp);
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 9f6886f..d4945a8 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1839,6 +1839,22 @@ static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
 }
 
 static __be32
+nfsd4_decode_copy_notify(struct nfsd4_compoundargs *argp,
+			 struct nfsd4_copy_notify *cn)
+{
+	int status;
+
+	status = nfsd4_decode_stateid(argp, &cn->cpn_src_stateid);
+	if (status)
+		return status;
+	status = nfsd4_decode_nl4_server(argp, &cn->cpn_dst);
+	if (status)
+		return status;
+
+	return status;
+}
+
+static __be32
 nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
 {
 	DECODE_HEAD;
@@ -1939,7 +1955,7 @@ static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
 	/* new operations for NFSv4.2 */
 	[OP_ALLOCATE]		= (nfsd4_dec)nfsd4_decode_fallocate,
 	[OP_COPY]		= (nfsd4_dec)nfsd4_decode_copy,
-	[OP_COPY_NOTIFY]	= (nfsd4_dec)nfsd4_decode_notsupp,
+	[OP_COPY_NOTIFY]	= (nfsd4_dec)nfsd4_decode_copy_notify,
 	[OP_DEALLOCATE]		= (nfsd4_dec)nfsd4_decode_fallocate,
 	[OP_IO_ADVISE]		= (nfsd4_dec)nfsd4_decode_notsupp,
 	[OP_LAYOUTERROR]	= (nfsd4_dec)nfsd4_decode_notsupp,
@@ -4324,6 +4340,45 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 }
 
 static __be32
+nfsd42_encode_nl4_server(struct nfsd4_compoundres *resp, struct nl4_server *ns)
+{
+	struct xdr_stream *xdr = &resp->xdr;
+	struct nfs42_netaddr *addr;
+	__be32 *p;
+
+	p = xdr_reserve_space(xdr, 4);
+	*p++ = cpu_to_be32(ns->nl4_type);
+
+	switch (ns->nl4_type) {
+	case NL4_NETADDR:
+		addr = &ns->u.nl4_addr;
+
+		/** netid_len, netid, uaddr_len, uaddr (port included
+		 * in RPCBIND_MAXUADDRLEN)
+		 */
+		p = xdr_reserve_space(xdr,
+			4 /* netid len */ +
+			(XDR_QUADLEN(addr->netid_len) * 4) +
+			4 /* uaddr len */ +
+			(XDR_QUADLEN(addr->addr_len) * 4));
+		if (!p)
+			return nfserr_resource;
+
+		*p++ = cpu_to_be32(addr->netid_len);
+		p = xdr_encode_opaque_fixed(p, addr->netid,
+					    addr->netid_len);
+		*p++ = cpu_to_be32(addr->addr_len);
+		p = xdr_encode_opaque_fixed(p, addr->addr,
+					addr->addr_len);
+		break;
+	default:
+		WARN_ON(ns->nl4_type != NL4_NETADDR);
+	}
+
+	return 0;
+}
+
+static __be32
 nfsd4_encode_copy(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  struct nfsd4_copy *copy)
 {
@@ -4360,6 +4415,44 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 }
 
 static __be32
+nfsd4_encode_copy_notify(struct nfsd4_compoundres *resp, __be32 nfserr,
+			 struct nfsd4_copy_notify *cn)
+{
+	struct xdr_stream *xdr = &resp->xdr;
+	__be32 *p;
+
+	if (nfserr)
+		return nfserr;
+
+	/* 8 sec, 4 nsec */
+	p = xdr_reserve_space(xdr, 12);
+	if (!p)
+		return nfserr_resource;
+
+	/* cnr_lease_time */
+	p = xdr_encode_hyper(p, cn->cpn_sec);
+	*p++ = cpu_to_be32(cn->cpn_nsec);
+
+	/* cnr_stateid */
+	nfserr = nfsd4_encode_stateid(xdr, &cn->cpn_cnr_stateid);
+	if (nfserr)
+		return nfserr;
+
+	/* cnr_src.nl_nsvr */
+	p = xdr_reserve_space(xdr, 4);
+	if (!p)
+		return nfserr_resource;
+
+	*p++ = cpu_to_be32(1);
+
+	nfserr = nfsd42_encode_nl4_server(resp, &cn->cpn_src);
+	if (nfserr)
+		return nfserr;
+
+	return nfserr;
+}
+
+static __be32
 nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
 		  struct nfsd4_seek *seek)
 {
@@ -4456,7 +4549,7 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
 	/* NFSv4.2 operations */
 	[OP_ALLOCATE]		= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_COPY]		= (nfsd4_enc)nfsd4_encode_copy,
-	[OP_COPY_NOTIFY]	= (nfsd4_enc)nfsd4_encode_noop,
+	[OP_COPY_NOTIFY]	= (nfsd4_enc)nfsd4_encode_copy_notify,
 	[OP_DEALLOCATE]		= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_IO_ADVISE]		= (nfsd4_enc)nfsd4_encode_noop,
 	[OP_LAYOUTERROR]	= (nfsd4_enc)nfsd4_encode_noop,
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 304de3b..6bfac6a 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -94,6 +94,7 @@ struct nfs4_stid {
 #define NFS4_REVOKED_DELEG_STID 16
 #define NFS4_CLOSED_DELEG_STID 32
 #define NFS4_LAYOUT_STID 64
+	struct list_head	sc_cp_list;
 	unsigned char		sc_type;
 	stateid_t		sc_stateid;
 	spinlock_t		sc_lock;
@@ -102,6 +103,17 @@ struct nfs4_stid {
 	void			(*sc_free)(struct nfs4_stid *);
 };
 
+/* Keep a list of stateids issued by the COPY_NOTIFY, associate it with the
+ * parent OPEN/LOCK/DELEG stateid.
+ */
+struct nfs4_cpntf_state {
+	stateid_t		cp_stateid;
+	struct list_head	cp_list;	/* per parent nfs4_stid */
+	struct nfs4_stid	*cp_p_stid;	/* pointer to parent */
+	bool			cp_active;	/* has the copy started */
+	unsigned long		cp_timeout;	/* copy timeout */
+};
+
 /*
  * Represents a delegation stateid. The nfs4_client holds references to these
  * and they are put when it is being destroyed or when the delegation is
@@ -615,6 +627,9 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 				  void (*sc_free)(struct nfs4_stid *));
 int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy);
 void nfs4_free_cp_state(struct nfsd4_copy *copy);
+struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
+			struct nfs4_stid *p_stid);
+void nfs4_free_cpntf_state(struct nfs4_cpntf_state *cps);
 void nfs4_unhash_stid(struct nfs4_stid *s);
 void nfs4_put_stid(struct nfs4_stid *s);
 void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b4d1140..4557f15 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -567,6 +567,18 @@ struct nfsd4_offload_status {
 	u32		status;
 };
 
+struct nfsd4_copy_notify {
+	/* request */
+	stateid_t		cpn_src_stateid;
+	struct nl4_server	cpn_dst;
+
+	/* response */
+	stateid_t		cpn_cnr_stateid;
+	u64			cpn_sec;
+	u32			cpn_nsec;
+	struct nl4_server	cpn_src;
+};
+
 struct nfsd4_op {
 	int					opnum;
 	const struct nfsd4_operation *		opdesc;
@@ -626,6 +638,7 @@ struct nfsd4_op {
 		struct nfsd4_clone		clone;
 		struct nfsd4_copy		copy;
 		struct nfsd4_offload_status	offload_status;
+		struct nfsd4_copy_notify	copy_notify;
 		struct nfsd4_seek		seek;
 	} u;
 	struct nfs4_replay *			replay;
-- 
1.8.3.1

[PATCH v1 10/13] NFSD check stateids against copy stateids

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Incoming stateid (used by a READ) could be a saved copy stateid.
On first use make it active and check that the copy has started
within the allowable lease time.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/nfs4state.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7764a8b..16359de 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5206,6 +5206,47 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 
 	return 0;
 }
+/*
+ * A READ from an inter server to server COPY will have a
+ * copy stateid. Return the parent nfs4_stid.
+ */
+static __be32 _find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
+		     struct nfs4_cpntf_state **cps)
+{
+	struct nfs4_cpntf_state *state = NULL;
+
+	if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
+		return nfserr_bad_stateid;
+	spin_lock(&nn->s2s_cp_lock);
+	state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
+	spin_unlock(&nn->s2s_cp_lock);
+	if (!state)
+		return nfserr_bad_stateid;
+	*cps = state;
+	return 0;
+}
+
+static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
+			       struct nfs4_stid **stid)
+{
+	__be32 status;
+	struct nfs4_cpntf_state *cps = NULL;
+
+	status = _find_cpntf_state(nn, st, &cps);
+	if (status)
+		return status;
+
+	/* Did the inter server to server copy start in time? */
+	if (cps->cp_active == false && !time_after(cps->cp_timeout, jiffies))
+		return nfserr_partner_no_auth;
+	else
+		cps->cp_active = true;
+
+	*stid = cps->cp_p_stid;
+	refcount_inc(&cps->cp_p_stid->sc_count);
+
+	return nfs_ok;
+}
 
 /*
  * Checks for stateid operations
@@ -5238,6 +5279,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	status = nfsd4_lookup_stateid(cstate, stateid,
 				NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
 				&s, nn);
+	if (status == nfserr_bad_stateid)
+		status = find_cpntf_state(nn, stateid, &s);
 	if (status)
 		return status;
 	status = nfsd4_stid_check_stateid_generation(stateid, s,
-- 
1.8.3.1

[PATCH v1 11/13] NFSD generalize nfsd4_compound_state flag names

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Allow for sid_flag field non-stateid use.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfsd/nfs4proc.c  | 8 ++++----
 fs/nfsd/nfs4state.c | 7 ++++---
 fs/nfsd/xdr4.h      | 6 +++---
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 999c8cb..43a83c7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -530,9 +530,9 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
 		return nfserr_restorefh;
 
 	fh_dup2(&cstate->current_fh, &cstate->save_fh);
-	if (HAS_STATE_ID(cstate, SAVED_STATE_ID_FLAG)) {
+	if (HAS_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG)) {
 		memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
-		SET_STATE_ID(cstate, CURRENT_STATE_ID_FLAG);
+		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
 	}
 	return nfs_ok;
 }
@@ -542,9 +542,9 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
 	     union nfsd4_op_u *u)
 {
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
-	if (HAS_STATE_ID(cstate, CURRENT_STATE_ID_FLAG)) {
+	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
 		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
-		SET_STATE_ID(cstate, SAVED_STATE_ID_FLAG);
+		SET_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG);
 	}
 	return nfs_ok;
 }
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 16359de..a39a581 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7424,7 +7424,8 @@ static int nfs4_state_create_net(struct net *net)
 static void
 get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
-	if (HAS_STATE_ID(cstate, CURRENT_STATE_ID_FLAG) && CURRENT_STATEID(stateid))
+	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
+	    CURRENT_STATEID(stateid))
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
 }
 
@@ -7433,14 +7434,14 @@ static int nfs4_state_create_net(struct net *net)
 {
 	if (cstate->minorversion) {
 		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-		SET_STATE_ID(cstate, CURRENT_STATE_ID_FLAG);
+		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
 	}
 }
 
 void
 clear_current_stateid(struct nfsd4_compound_state *cstate)
 {
-	CLEAR_STATE_ID(cstate, CURRENT_STATE_ID_FLAG);
+	CLEAR_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
 }
 
 /*
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4557f15..4a1e53d 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -46,9 +46,9 @@
 #define CURRENT_STATE_ID_FLAG (1<<0)
 #define SAVED_STATE_ID_FLAG (1<<1)
 
-#define SET_STATE_ID(c, f) ((c)->sid_flags |= (f))
-#define HAS_STATE_ID(c, f) ((c)->sid_flags & (f))
-#define CLEAR_STATE_ID(c, f) ((c)->sid_flags &= ~(f))
+#define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
+#define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
+#define CLEAR_CSTATE_FLAG(c, f) ((c)->sid_flags &= ~(f))
 
 struct nfsd4_compound_state {
 	struct svc_fh		current_fh;
-- 
1.8.3.1

[PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

The inter server to server COPY source server filehandle
is a foreign filehandle as the COPY is sent to the destination
server.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfsd/Kconfig    | 10 ++++++++++
 fs/nfsd/nfs4proc.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 fs/nfsd/nfsfh.h    |  5 ++++-
 fs/nfsd/xdr4.h     |  1 +
 4 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
index 20b1c17..37ff3d5 100644
--- a/fs/nfsd/Kconfig
+++ b/fs/nfsd/Kconfig
@@ -131,6 +131,16 @@ config NFSD_FLEXFILELAYOUT
 
 	  If unsure, say N.
 
+config NFSD_V4_2_INTER_SSC
+	bool "NFSv4.2 inter server to server COPY"
+	depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
+	help
+	  This option enables support for NFSv4.2 inter server to
+	  server copy where the destination server calls the NFSv4.2
+	  client to read the data to copy from the source server.
+
+	  If unsure, say N.
+
 config NFSD_V4_SECURITY_LABEL
 	bool "Provide Security Label support for NFSv4 server"
 	depends on NFSD_V4 && SECURITY
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 43a83c7..59e9d0c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -503,12 +503,21 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
 	    union nfsd4_op_u *u)
 {
 	struct nfsd4_putfh *putfh = &u->putfh;
+	__be32 ret;
 
 	fh_put(&cstate->current_fh);
 	cstate->current_fh.fh_handle.fh_size = putfh->pf_fhlen;
 	memcpy(&cstate->current_fh.fh_handle.fh_base, putfh->pf_fhval,
 	       putfh->pf_fhlen);
-	return fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
+	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
+#ifdef CONFIG_NFSD_V4_2_INTER_SSC
+	if (ret == nfserr_stale && HAS_CSTATE_FLAG(cstate, NO_VERIFY_FH)) {
+		CLEAR_CSTATE_FLAG(cstate, NO_VERIFY_FH);
+		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
+		ret = 0;
+	}
+#endif
+	return ret;
 }
 
 static __be32
@@ -1957,6 +1966,26 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
 		- rqstp->rq_auth_slack;
 }
 
+#ifdef CONFIG_NFSD_V4_2_INTER_SSC
+static bool _compound_contains_inter_copy(struct nfsd4_op *ops, int start,
+					  int end)
+{
+	bool found = false;
+	struct nfsd4_copy *copy;
+	int i;
+
+	for (i = start; i < end; i++) {
+		if (ops[i].opnum == OP_COPY) {
+			copy = (struct nfsd4_copy *)&ops[i].u;
+			if (copy->cp_src)
+				found = true;
+			break;
+		}
+	}
+	return found;
+}
+#endif
+
 /*
  * COMPOUND call.
  */
@@ -2019,13 +2048,23 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
 				op->status = nfsd4_open_omfg(rqstp, cstate, op);
 			goto encode_op;
 		}
+#ifdef CONFIG_NFSD_V4_2_INTER_SSC
+		if (op->opnum == OP_PUTFH &&
+			args->ops[resp->opcnt].opnum == OP_SAVEFH &&
+			args->ops[resp->opcnt+1].opnum == OP_PUTFH &&
+			_compound_contains_inter_copy(args->ops, resp->opcnt+2,
+						      args->opcnt))
+			SET_CSTATE_FLAG(cstate, NO_VERIFY_FH);
+#endif
 
-		if (!current_fh->fh_dentry) {
+		if (!current_fh->fh_dentry &&
+				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
 			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
 				op->status = nfserr_nofilehandle;
 				goto encode_op;
 			}
-		} else if (current_fh->fh_export->ex_fslocs.migrated &&
+		} else if (current_fh->fh_export &&
+			   current_fh->fh_export->ex_fslocs.migrated &&
 			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
 			op->status = nfserr_moved;
 			goto encode_op;
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 755e256..b9c7568 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -35,7 +35,7 @@ static inline ino_t u32_to_ino_t(__u32 uino)
 
 	bool			fh_locked;	/* inode locked by us */
 	bool			fh_want_write;	/* remount protection taken */
-
+	int			fh_flags;	/* FH flags */
 #ifdef CONFIG_NFSD_V3
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
@@ -56,6 +56,9 @@ static inline ino_t u32_to_ino_t(__u32 uino)
 #endif /* CONFIG_NFSD_V3 */
 
 } svc_fh;
+#define NFSD4_FH_FOREIGN (1<<0)
+#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
+#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
 
 enum nfsd_fsid {
 	FSID_DEV = 0,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4a1e53d..c98ef64 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -45,6 +45,7 @@
 
 #define CURRENT_STATE_ID_FLAG (1<<0)
 #define SAVED_STATE_ID_FLAG (1<<1)
+#define NO_VERIFY_FH (1<<2)
 
 #define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
 #define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
-- 
1.8.3.1

[PATCH v1 13/13] NFSD add nfs4 inter ssc to nfsd4_copy

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

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@netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 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"
+
+/**
+ * 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");
+		goto out_err;
+	}
+
+	naddr = &nss->u.nl4_addr;
+
+	tmp_addrlen = rpc_uaddr2sockaddr(SVC_NET(rqstp), naddr->addr,
+					 naddr->addr_len,
+					 (struct sockaddr *)&tmp_addr,
+					 sizeof(tmp_addr));
+	if (tmp_addrlen == 0)
+		goto out_err;
+
+	if (tmp_addr.ss_family == AF_INET6) {
+		startsep = "[";
+		endsep = "]";
+		match_netid = "tcp6";
+		match_netid_len = 4;
+	}
+
+	if (naddr->netid_len != match_netid_len ||
+		strncmp(naddr->netid, match_netid, naddr->netid_len))
+		goto out_err;
+
+	/* Construct the raw data for the vfs_kern_mount call */
+	len = RPC_MAX_ADDRBUFLEN + 1;
+	ipaddr = kzalloc(len, GFP_KERNEL);
+	if (!ipaddr)
+		goto out_err;
+
+	rpc_ntop((struct sockaddr *)&tmp_addr, ipaddr, len);
+
+	/* 2 for ipv6 endsep and startsep. 3 for ":/" and trailing '/0'*/
+	ipaddr2 = kzalloc(len + 5, GFP_KERNEL);
+	if (!ipaddr2)
+		goto out_free_ipaddr;
+
+	rpc_ntop((struct sockaddr *)&rqstp->rq_daddr, ipaddr2, len + 5);
+
+	raw_len = strlen(NFSD42_INTERSSC_MOUNTOPS) + strlen(ipaddr) +
+			strlen(ipaddr2);
+	raw_data = kzalloc(raw_len, GFP_KERNEL);
+	if (!raw_data)
+		goto out_free_ipaddr2;
+
+	snprintf(raw_data, raw_len, NFSD42_INTERSSC_MOUNTOPS, ipaddr,
+		 ipaddr2);
+
+	status = -ENODEV;
+	type = get_fs_type("nfs");
+	if (!type)
+		goto out_free_rawdata;
+
+	/* Set the server:<export> for the vfs_kerne_mount call */
+	memset(ipaddr2, 0, len + 5);
+	snprintf(ipaddr2, len + 5, "%s%s%s:/", startsep, ipaddr, endsep);
+
+	dprintk("%s  Raw mount data:  %s server:export %s\n", __func__,
+		raw_data, ipaddr2);
+
+	/* Use an 'internal' mount: MS_KERNMOUNT -> MNT_INTERNAL */
+	ss_mnt = vfs_kern_mount(type, MS_KERNMOUNT, ipaddr2, raw_data);
+	if (IS_ERR(ss_mnt)) {
+		status = PTR_ERR(ss_mnt);
+		goto out_free_rawdata;
+	}
+
+	kfree(raw_data);
+	kfree(ipaddr2);
+	kfree(ipaddr);
+
+	return ss_mnt;
+
+out_free_rawdata:
+	kfree(raw_data);
+out_free_ipaddr2:
+	kfree(ipaddr2);
+out_free_ipaddr:
+	kfree(ipaddr);
+out_err:
+	dprintk("--> %s ERROR %d\n", __func__, status);
+	return ERR_PTR(status);
+}
+
+static void
+nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
+{
+	nfs_sb_deactive(ss_mnt->mnt_sb);
+	mntput(ss_mnt);
+}
+
+/**
+ * nfsd4_setup_inter_ssc
+ *
+ * Verify COPY destination stateid.
+ * Connect to the source server with NFSv4.1.
+ * Create the source struct file for nfsd_copy_range.
+ * Called with COPY cstate:
+ *    SAVED_FH: source filehandle
+ *    CURRENT_FH: destination filehandle
+ *
+ * Returns errno (not nfserrxxx)
+ */
+static struct vfsmount *
+nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
+		      struct nfsd4_compound_state *cstate,
+		      struct nfsd4_copy *copy)
+{
+	struct svc_fh *s_fh = NULL;
+	stateid_t *s_stid = &copy->cp_src_stateid;
+	struct vfsmount *ss_mnt;
+	__be32 status;
+
+	/* Verify the destination stateid and set dst struct file*/
+	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
+					    &copy->cp_dst_stateid,
+					    WR_STATE, &copy->file_dst, NULL,
+					    NULL);
+	if (status) {
+		ss_mnt = ERR_PTR(be32_to_cpu(status));
+		goto out;
+	}
+
+	ss_mnt = nfsd4_interssc_connect(copy->cp_src, rqstp);
+	if (IS_ERR(ss_mnt))
+		goto out;
+
+	s_fh = &cstate->save_fh;
+
+	copy->c_fh.size = s_fh->fh_handle.fh_size;
+	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
+	copy->stateid.seqid = s_stid->si_generation;
+	memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
+	       sizeof(stateid_opaque_t));
+
+out:
+	return ss_mnt;
+}
+
+static void
+nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
+			struct file *dst)
+{
+	nfs42_ssc_close(src);
+	fput(src);
+	fput(dst);
+	mntput(ss_mnt);
+}
+
+#else /* CONFIG_NFSD_V4_2_INTER_SSC */
+
+static struct vfsmount *
+nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
+		      struct nfsd4_compound_state *cstate,
+		      struct nfsd4_copy *copy)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static void
+nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
+			struct file *dst)
+{
+}
+
+static void
+nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
+{
+}
+
+static struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
+				   struct nfs_fh *src_fh,
+				   nfs4_stateid *stateid)
+{
+	return NULL;
+}
+#endif /* CONFIG_NFSD_V4_2_INTER_SSC */
+
+static __be32
+nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
+		      struct nfsd4_compound_state *cstate,
+		      struct nfsd4_copy *copy)
+{
+	return nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
+				 &copy->file_src, &copy->cp_dst_stateid,
+				 &copy->file_dst, NULL);
+}
+
+static void
+nfsd4_cleanup_intra_ssc(struct file *src, struct file *dst)
+{
+	fput(src);
+	fput(dst);
+}
 
 static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
@@ -1217,12 +1440,16 @@ static __be32 nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
 		status = nfs_ok;
 	}
 
-	fput(copy->file_src);
-	fput(copy->file_dst);
+	if (copy->cp_src) /* Inter server SSC */
+		nfsd4_cleanup_inter_ssc(copy->ss_mnt, copy->file_src,
+					copy->file_dst);
+	else
+		nfsd4_cleanup_intra_ssc(copy->file_src, copy->file_dst);
+
 	return status;
 }
 
-static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
+static int dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
 {
 	dst->cp_src_pos = src->cp_src_pos;
 	dst->cp_dst_pos = src->cp_dst_pos;
@@ -1232,8 +1459,21 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
 	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
 	dst->cp_clp = src->cp_clp;
 	dst->file_dst = get_file(src->file_dst);
-	dst->file_src = get_file(src->file_src);
+	if (!src->cp_src) /* for inter, file_src doesnt exist yet */
+		dst->file_src = get_file(src->file_src);
 	memcpy(&dst->cp_stateid, &src->cp_stateid, sizeof(src->cp_stateid));
+	if (src->cp_src) {
+		dst->cp_src = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
+		if (!dst->cp_src)
+			return -ENOMEM;
+		memcpy(dst->cp_src, src->cp_src, sizeof(struct nl4_server));
+	}
+	memcpy(&dst->stateid, &src->stateid, sizeof(src->stateid));
+	memcpy(&dst->c_fh, &src->c_fh, sizeof(src->c_fh));
+	dst->ss_mnt = src->ss_mnt;
+
+	return 0;
+
 }
 
 static void cleanup_async_copy(struct nfsd4_copy *copy)
@@ -1244,6 +1484,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
 	spin_lock(&copy->cp_clp->async_lock);
 	list_del(&copy->copies);
 	spin_unlock(&copy->cp_clp->async_lock);
+	kfree(copy->cp_src);
 	nfs4_put_copy(copy);
 }
 
@@ -1252,7 +1493,18 @@ static int nfsd4_do_async_copy(void *data)
 	struct nfsd4_copy *copy = (struct nfsd4_copy *)data;
 	struct nfsd4_copy *cb_copy;
 
+	if (copy->cp_src) { /* Inter server SSC */
+		copy->file_src = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
+					      &copy->stateid);
+		if (IS_ERR(copy->file_src)) {
+			copy->nfserr = nfserr_offload_denied;
+			nfsd4_interssc_disconnect(copy->ss_mnt);
+			goto do_callback;
+		}
+	}
+
 	copy->nfserr = nfsd4_do_copy(copy, 0);
+do_callback:
 	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
 	if (!cb_copy)
 		goto out;
@@ -1276,11 +1528,19 @@ static int nfsd4_do_async_copy(void *data)
 	__be32 status;
 	struct nfsd4_copy *async_copy = NULL;
 
-	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
-				   &copy->file_src, &copy->cp_dst_stateid,
-				   &copy->file_dst, NULL);
-	if (status)
-		goto out;
+	if (copy->cp_src) { /* Inter server SSC */
+		if (!inter_copy_offload_enable || copy->cp_synchronous) {
+			status = nfserr_notsupp;
+			goto out;
+		}
+		copy->ss_mnt = nfsd4_setup_inter_ssc(rqstp, cstate, copy);
+		if (IS_ERR(copy->ss_mnt))
+			return nfserr_offload_denied;
+	} else {
+		status = nfsd4_setup_intra_ssc(rqstp, cstate, copy);
+		if (status)
+			return status;
+	}
 
 	copy->cp_clp = cstate->clp;
 	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
@@ -1291,15 +1551,15 @@ static int nfsd4_do_async_copy(void *data)
 		status = nfserrno(-ENOMEM);
 		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
 		if (!async_copy)
-			goto out;
-		if (!nfs4_init_cp_state(nn, copy)) {
-			kfree(async_copy);
-			goto out;
-		}
+			goto out_err;
+		if (!nfs4_init_cp_state(nn, copy))
+			goto out_err;
 		refcount_set(&async_copy->refcount, 1);
 		memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid,
 			sizeof(copy->cp_stateid));
-		dup_copy_fields(copy, async_copy);
+		status = dup_copy_fields(copy, async_copy);
+		if (status)
+			goto out_err;
 		async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
 				async_copy, "%s", "copy thread");
 		if (IS_ERR(async_copy->copy_task))
@@ -1310,13 +1570,17 @@ static int nfsd4_do_async_copy(void *data)
 		spin_unlock(&async_copy->cp_clp->async_lock);
 		wake_up_process(async_copy->copy_task);
 		status = nfs_ok;
-	} else
+	} else {
 		status = nfsd4_do_copy(copy, 1);
+	}
 out:
 	return status;
 out_err:
 	cleanup_async_copy(async_copy);
-	goto out;
+	status = nfserrno(-ENOMEM);
+	if (copy->cp_src)
+		nfsd4_interssc_disconnect(copy->ss_mnt);
+	goto out_err;
 }
 
 struct nfsd4_copy *
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 89cb484..9d254e7 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -30,6 +30,12 @@
 
 #define NFSDDBG_FACILITY	NFSDDBG_SVC
 
+bool inter_copy_offload_enable;
+EXPORT_SYMBOL_GPL(inter_copy_offload_enable);
+module_param(inter_copy_offload_enable, bool, 0644);
+MODULE_PARM_DESC(inter_copy_offload_enable,
+		 "Enable inter server to server copy offload. Default: false");
+
 extern struct svc_program	nfsd_program;
 static int			nfsd(void *vrqstp);
 
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index c98ef64..c7e3df1 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -546,7 +546,12 @@ struct nfsd4_copy {
 	struct task_struct	*copy_task;
 	refcount_t		refcount;
 	bool			stopped;
+
+	struct vfsmount		*ss_mnt;
+	struct nfs_fh		c_fh;
+	nfs4_stateid		stateid;
 };
+extern bool inter_copy_offload_enable;
 
 struct nfsd4_seek {
 	/* request */
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 4d76f87..e53a261 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -17,6 +17,7 @@
 #include <linux/uidgid.h>
 #include <uapi/linux/nfs4.h>
 #include <linux/sunrpc/msg_prot.h>
+#include <linux/nfs.h>
 
 enum nfs4_acl_whotype {
 	NFS4_ACL_WHO_NAMED = 0,
-- 
1.8.3.1

Re: [PATCH v1 00/13] server-side support for "inter" SSC copy

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:52AM -0400, Olga Kornievskaia wrote:
> This patch series adds support for NFSv4.2 copy offload feature
> allowing copy between two different NFS servers.

Apologies, it may take me another week or so to get to this....

This is the part that sounds trickiest!:

> On the destination server, upon receiving a COPY request, the server
> establishes the necessary clientid/session with the source server.
> It calls into the NFS client code to establish the necessary
> open stateid, filehandle, file description (without doing an NFS open).
> Then the server calls into the copy_file_range() to preform the copy
> where the source file will issue NFS READs and then do local file
> system writes (this depends on the VFS ability to do cross device
> copy_file_range().

--b.

Re: [PATCH v1 00/13] server-side support for "inter" SSC copy

[Olga Kornievskaia]

On Thu, Oct 25, 2018 at 12:08 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:52AM -0400, Olga Kornievskaia wrote:
> > This patch series adds support for NFSv4.2 copy offload feature
> > allowing copy between two different NFS servers.
>
> Apologies, it may take me another week or so to get to this....
>
> This is the part that sounds trickiest!:

I'll look for your comments next week. Please note that client side
patches that NFSD depends on have changed (due to the client side
review) so to run/compile the code, please grab all of the latest
client-side patches that I posted.

I'm assuming that async patches are going into 4.20, correct?

>
> > On the destination server, upon receiving a COPY request, the server
> > establishes the necessary clientid/session with the source server.
> > It calls into the NFS client code to establish the necessary
> > open stateid, filehandle, file description (without doing an NFS open).
> > Then the server calls into the copy_file_range() to preform the copy
> > where the source file will issue NFS READs and then do local file
> > system writes (this depends on the VFS ability to do cross device
> > copy_file_range().
>
> --b.

Re: [PATCH v1 00/13] server-side support for "inter" SSC copy

[J. Bruce Fields]

On Mon, Oct 29, 2018 at 01:54:37PM -0400, Olga Kornievskaia wrote:
> On Thu, Oct 25, 2018 at 12:08 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:52AM -0400, Olga Kornievskaia wrote:
> > > This patch series adds support for NFSv4.2 copy offload feature
> > > allowing copy between two different NFS servers.
> >
> > Apologies, it may take me another week or so to get to this....
> >
> > This is the part that sounds trickiest!:
> 
> I'll look for your comments next week. Please note that client side
> patches that NFSD depends on have changed (due to the client side
> review) so to run/compile the code, please grab all of the latest
> client-side patches that I posted.

OK, thanks.

> I'm assuming that async patches are going into 4.20, correct?

Yes.

--b.

Re: [PATCH v1 01/13] fs: Don't copy beyond the end of the file

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:53AM -0400, Olga Kornievskaia wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>

I have some idea we've had some discussion about this before, but if so
I've forgotten the conclusion.  Could we have more of a changelog?:

	- isn't there a race condition, or is there something preventing
	  the file size from changing here?
	- why are we doing this?  Does this change the behavior of
	  copy_file_range()?

--b.

> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  fs/read_write.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 39b4a21..c60790f 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	if (unlikely(ret))
>  		return ret;
>  
> +	if (pos_in >= i_size_read(inode_in))
> +		return -EINVAL;
> +
>  	if (!(file_in->f_mode & FMODE_READ) ||
>  	    !(file_out->f_mode & FMODE_WRITE) ||
>  	    (file_out->f_flags & O_APPEND))
> -- 
> 1.8.3.1

Re: [PATCH v1 01/13] fs: Don't copy beyond the end of the file

[Olga Kornievskaia]

On Wed, Oct 31, 2018 at 12:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:53AM -0400, Olga Kornievskaia wrote:
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>
> I have some idea we've had some discussion about this before, but if so
> I've forgotten the conclusion.  Could we have more of a changelog?:
>
>         - isn't there a race condition, or is there something preventing
>           the file size from changing here?

No there is nothing preventing the size from changing. Just like there
is nothing that prevents the file from changing if you are doing a
traditional copy either.

>         - why are we doing this?  Does this change the behavior of
>           copy_file_range()?

We are doing this because 1. NFS spec and 2. copy_file_range semantics
mandate that too. There is a whole different discussion under the
client-side patch for this where the plan now is that VFS themselves
are interested in making sure they are indeed enforcing the check
stated by the documentation of copy_file_range call which states
"copying a range beyond the end of the file" is EINVAL. I recall you
argued for a "short" read instead of a EINVAL but unless VFS community
is convinced to change it it'll be enforced (soon).

>
> --b.
>
> >
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> >  fs/read_write.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 39b4a21..c60790f 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >       if (unlikely(ret))
> >               return ret;
> >
> > +     if (pos_in >= i_size_read(inode_in))
> > +             return -EINVAL;
> > +
> >       if (!(file_in->f_mode & FMODE_READ) ||
> >           !(file_out->f_mode & FMODE_WRITE) ||
> >           (file_out->f_flags & O_APPEND))
> > --
> > 1.8.3.1

Re: [PATCH v1 01/13] fs: Don't copy beyond the end of the file

[J. Bruce Fields]

On Wed, Oct 31, 2018 at 01:07:11PM -0400, Olga Kornievskaia wrote:
> On Wed, Oct 31, 2018 at 12:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:53AM -0400, Olga Kornievskaia wrote:
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > I have some idea we've had some discussion about this before, but if so
> > I've forgotten the conclusion.  Could we have more of a changelog?:
> >
> >         - isn't there a race condition, or is there something preventing
> >           the file size from changing here?
> 
> No there is nothing preventing the size from changing. Just like there
> is nothing that prevents the file from changing if you are doing a
> traditional copy either.
> 
> >         - why are we doing this?  Does this change the behavior of
> >           copy_file_range()?
> 
> We are doing this because 1. NFS spec and 2. copy_file_range semantics
> mandate that too. There is a whole different discussion under the
> client-side patch for this where the plan now is that VFS themselves
> are interested in making sure they are indeed enforcing the check
> stated by the documentation of copy_file_range call which states
> "copying a range beyond the end of the file" is EINVAL. I recall you
> argued for a "short" read instead of a EINVAL but unless VFS community
> is convinced to change it it'll be enforced (soon).

OK.  Let's just make sure the reasoning's mentioned in the changelog,
whatever we do.

--b.

> > --b.
> >
> > >
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > >  fs/read_write.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index 39b4a21..c60790f 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> > >       if (unlikely(ret))
> > >               return ret;
> > >
> > > +     if (pos_in >= i_size_read(inode_in))
> > > +             return -EINVAL;
> > > +
> > >       if (!(file_in->f_mode & FMODE_READ) ||
> > >           !(file_out->f_mode & FMODE_WRITE) ||
> > >           (file_out->f_flags & O_APPEND))
> > > --
> > > 1.8.3.1

Re: [PATCH v1 01/13] fs: Don't copy beyond the end of the file

[Olga Kornievskaia]

On Wed, Oct 31, 2018 at 1:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Wed, Oct 31, 2018 at 01:07:11PM -0400, Olga Kornievskaia wrote:
> > On Wed, Oct 31, 2018 at 12:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Oct 19, 2018 at 11:28:53AM -0400, Olga Kornievskaia wrote:
> > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > >
> > > I have some idea we've had some discussion about this before, but if so
> > > I've forgotten the conclusion.  Could we have more of a changelog?:
> > >
> > >         - isn't there a race condition, or is there something preventing
> > >           the file size from changing here?
> >
> > No there is nothing preventing the size from changing. Just like there
> > is nothing that prevents the file from changing if you are doing a
> > traditional copy either.
> >
> > >         - why are we doing this?  Does this change the behavior of
> > >           copy_file_range()?
> >
> > We are doing this because 1. NFS spec and 2. copy_file_range semantics
> > mandate that too. There is a whole different discussion under the
> > client-side patch for this where the plan now is that VFS themselves
> > are interested in making sure they are indeed enforcing the check
> > stated by the documentation of copy_file_range call which states
> > "copying a range beyond the end of the file" is EINVAL. I recall you
> > argued for a "short" read instead of a EINVAL but unless VFS community
> > is convinced to change it it'll be enforced (soon).
>
> OK.  Let's just make sure the reasoning's mentioned in the changelog,
> whatever we do.

By the changelog, you mean the commit message?

>
> --b.
>
> > > --b.
> > >
> > > >
> > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > > ---
> > > >  fs/read_write.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > > index 39b4a21..c60790f 100644
> > > > --- a/fs/read_write.c
> > > > +++ b/fs/read_write.c
> > > > @@ -1570,6 +1570,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> > > >       if (unlikely(ret))
> > > >               return ret;
> > > >
> > > > +     if (pos_in >= i_size_read(inode_in))
> > > > +             return -EINVAL;
> > > > +
> > > >       if (!(file_in->f_mode & FMODE_READ) ||
> > > >           !(file_out->f_mode & FMODE_WRITE) ||
> > > >           (file_out->f_flags & O_APPEND))
> > > > --
> > > > 1.8.3.1

Re: [PATCH v1 01/13] fs: Don't copy beyond the end of the file

[J. Bruce Fields]

On Wed, Oct 31, 2018 at 02:01:45PM -0400, Olga Kornievskaia wrote:
> On Wed, Oct 31, 2018 at 1:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Wed, Oct 31, 2018 at 01:07:11PM -0400, Olga Kornievskaia wrote:
> > > On Wed, Oct 31, 2018 at 12:54 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > > >
> > > > On Fri, Oct 19, 2018 at 11:28:53AM -0400, Olga Kornievskaia wrote:
> > > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > >
> > > > I have some idea we've had some discussion about this before, but if so
> > > > I've forgotten the conclusion.  Could we have more of a changelog?:
> > > >
> > > >         - isn't there a race condition, or is there something preventing
> > > >           the file size from changing here?
> > >
> > > No there is nothing preventing the size from changing. Just like there
> > > is nothing that prevents the file from changing if you are doing a
> > > traditional copy either.
> > >
> > > >         - why are we doing this?  Does this change the behavior of
> > > >           copy_file_range()?
> > >
> > > We are doing this because 1. NFS spec and 2. copy_file_range semantics
> > > mandate that too. There is a whole different discussion under the
> > > client-side patch for this where the plan now is that VFS themselves
> > > are interested in making sure they are indeed enforcing the check
> > > stated by the documentation of copy_file_range call which states
> > > "copying a range beyond the end of the file" is EINVAL. I recall you
> > > argued for a "short" read instead of a EINVAL but unless VFS community
> > > is convinced to change it it'll be enforced (soon).
> >
> > OK.  Let's just make sure the reasoning's mentioned in the changelog,
> > whatever we do.
> 
> By the changelog, you mean the commit message?

Right.--b.

Re: [PATCH v1 04/13] NFS inter ssc open

[J. Bruce Fields]

Just to pick one thing that I don't understand yet:

On Fri, Oct 19, 2018 at 11:28:56AM -0400, Olga Kornievskaia wrote:
> +EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> +void nfs42_ssc_close(struct file *filep)
> +{
> +	struct nfs_open_context *ctx = nfs_file_open_context(filep);
> +
> +	ctx->state->flags = 0;

Why is this needed?

Also, given the name and the pairing with nfs42_ssc_open(), would it be
more logical for it to do the fput() as well?

--b.

Re: [PATCH v1 04/13] NFS inter ssc open

[Olga Kornievskaia]

On Wed, Oct 31, 2018 at 2:40 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> Just to pick one thing that I don't understand yet:
>
> On Fri, Oct 19, 2018 at 11:28:56AM -0400, Olga Kornievskaia wrote:
> > +EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> > +void nfs42_ssc_close(struct file *filep)
> > +{
> > +     struct nfs_open_context *ctx = nfs_file_open_context(filep);
> > +
> > +     ctx->state->flags = 0;
>
> Why is this needed?

This is needed so that CLOSE isn't going on the wire but closed internally.

> Also, given the name and the pairing with nfs42_ssc_open(), would it be
> more logical for it to do the fput() as well?

I'd like to keep that fput() in the nfsd to make it consistent with
the "intra". There are fput()s for intra but intra doesn't call into
nfs42_ssc_close().

Re: [PATCH v1 04/13] NFS inter ssc open

[J. Bruce Fields]

On Wed, Oct 31, 2018 at 02:54:51PM -0400, Olga Kornievskaia wrote:
> On Wed, Oct 31, 2018 at 2:40 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > Just to pick one thing that I don't understand yet:
> >
> > On Fri, Oct 19, 2018 at 11:28:56AM -0400, Olga Kornievskaia wrote:
> > > +EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> > > +void nfs42_ssc_close(struct file *filep)
> > > +{
> > > +     struct nfs_open_context *ctx = nfs_file_open_context(filep);
> > > +
> > > +     ctx->state->flags = 0;
> >
> > Why is this needed?
> 
> This is needed so that CLOSE isn't going on the wire but closed internally.
> 
> > Also, given the name and the pairing with nfs42_ssc_open(), would it be
> > more logical for it to do the fput() as well?
> 
> I'd like to keep that fput() in the nfsd to make it consistent with
> the "intra". There are fput()s for intra but intra doesn't call into
> nfs42_ssc_close().

OK, I think.

I'll need an ACK from Trond and/or Anna on this one in any case.

--b.

Re: [PATCH v1 05/13] NFS skip recovery of copy open on dest server

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Mark the open created for the source file on the destination
> server. Then if this open is going thru a recovery, then fail
> the recovery as we don't need to be recoving a "fake" open.
> We need to fail the ongoing READs and vfs_copy_file_range().

Nothing to do with this patch, really, but:

I'm not always a stickler about the 80-character-column rule, but, man,
nfs4_reclaim_open_state is deeply nested, could that be broken up a
little?  Also I notice there's a printk("AGLO: ...") in there that you
probably didn't mean to leave upstream?

--b.

> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4_fs.h   |  1 +
>  fs/nfs/nfs4file.c  |  1 +
>  fs/nfs/nfs4state.c | 14 ++++++++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index f229864..8b00c90 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -165,6 +165,7 @@ enum {
>  	NFS_STATE_CHANGE_WAIT,		/* A state changing operation is outstanding */
>  #ifdef CONFIG_NFS_V4_2
>  	NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
> +	NFS_SRV_SSC_COPY_STATE,		/* ssc state on the dst server */
>  #endif /* CONFIG_NFS_V4_2 */
>  };
>  
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index f82cb05..7909cd4 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -309,6 +309,7 @@ struct file *
>  	if (ctx->state == NULL)
>  		goto out_stateowner;
>  
> +	set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
>  	set_bit(NFS_OPEN_STATE, &ctx->state->flags);
>  	memcpy(&ctx->state->open_stateid.other, &stateid->other,
>  	       NFS4_STATEID_OTHER_SIZE);
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 62ae0fd..b0b82c6 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1606,6 +1606,9 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  {
>  	struct nfs4_state *state;
>  	int status = 0;
> +#ifdef CONFIG_NFS_V4_2
> +	bool found_ssc_copy_state = false;
> +#endif /* CONFIG_NFS_V4_2 */
>  
>  	/* Note: we rely on the sp->so_states list being ordered 
>  	 * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
> @@ -1625,6 +1628,13 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  			continue;
>  		if (state->state == 0)
>  			continue;
> +#ifdef CONFIG_NFS_V4_2
> +		if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
> +			nfs4_state_mark_recovery_failed(state, -EIO);
> +			found_ssc_copy_state = true;
> +			continue;
> +		}
> +#endif /* CONFIG_NFS_V4_2 */
>  		refcount_inc(&state->count);
>  		spin_unlock(&sp->so_lock);
>  		status = __nfs4_reclaim_open_state(sp, state, ops);
> @@ -1671,6 +1681,10 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
>  	}
>  	raw_write_seqcount_end(&sp->so_reclaim_seqcount);
>  	spin_unlock(&sp->so_lock);
> +#ifdef CONFIG_NFS_V4_2
> +	if (found_ssc_copy_state)
> +		return -EIO;
> +#endif /* CONFIG_NFS_V4_2 */
>  	return 0;
>  out_err:
>  	nfs4_put_open_state(state);
> -- 
> 1.8.3.1

Re: [PATCH v1 04/13] NFS inter ssc open

[Olga Kornievskaia]

On Thu, Nov 1, 2018 at 4:13 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Wed, Oct 31, 2018 at 02:54:51PM -0400, Olga Kornievskaia wrote:
> > On Wed, Oct 31, 2018 at 2:40 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > Just to pick one thing that I don't understand yet:
> > >
> > > On Fri, Oct 19, 2018 at 11:28:56AM -0400, Olga Kornievskaia wrote:
> > > > +EXPORT_SYMBOL_GPL(nfs42_ssc_open);
> > > > +void nfs42_ssc_close(struct file *filep)
> > > > +{
> > > > +     struct nfs_open_context *ctx = nfs_file_open_context(filep);
> > > > +
> > > > +     ctx->state->flags = 0;
> > >
> > > Why is this needed?
> >
> > This is needed so that CLOSE isn't going on the wire but closed internally.
> >
> > > Also, given the name and the pairing with nfs42_ssc_open(), would it be
> > > more logical for it to do the fput() as well?
> >
> > I'd like to keep that fput() in the nfsd to make it consistent with
> > the "intra". There are fput()s for intra but intra doesn't call into
> > nfs42_ssc_close().
>
> OK, I think.
>
> I'll need an ACK from Trond and/or Anna on this one in any case.

I included the client-side patches only so that you can compile and
run (or at least I tried as later I think I realized that you needed
one more patch. Thus I have asked if you are planning to run just
include all the client-side series). But I have submitted all of them
to be reviewed as client-side patches. Thus they have been reviewed by
Anna (don't know if Trond is looking of not) and some by others
(Jeff).

Re: [PATCH v1 06/13] NFSD fill-in netloc4 structure

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:58AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> nfs.4 defines nfs42_netaddr structure that represents netloc4.
> 
> Populate needed fields from the sockaddr structure.
> 
> This will be used by flexfiles and 4.2 inter copy
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfsd.h | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 0668999..030fccb 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -18,7 +18,7 @@
>  #include <linux/nfs4.h>
>  #include <linux/sunrpc/svc.h>
>  #include <linux/sunrpc/msg_prot.h>
> -
> +#include <linux/sunrpc/addr.h>
>  #include <uapi/linux/nfsd/debug.h>
>  
>  #include "stats.h"
> @@ -366,6 +366,35 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
>  
>  extern const u32 nfsd_suppattrs[3][3];
>  
> +static inline u32 nfsd4_set_netaddr(struct sockaddr *addr, struct nfs42_netaddr *netaddr)
> +{
> +	struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> +	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
> +	unsigned int port;
> +	size_t ret;
> +
> +	switch (addr->sa_family) {
> +	case AF_INET:
> +		port = ntohs(sin->sin_port);
> +		sprintf(netaddr->netid, "tcp");
> +		netaddr->netid_len = 3;
> +		break;
> +	case AF_INET6:
> +		port = ntohs(sin6->sin6_port);
> +		sprintf(netaddr->netid, "tcp6");
> +		netaddr->netid_len = 4;
> +		break;
> +	default:
> +		return nfserrno(-EINVAL);

Let's just write that nfserr_inval;

> +	}
> +	ret = rpc_ntop(addr, netaddr->addr, sizeof(netaddr->addr));
> +	netaddr->addr_len = ret + snprintf(netaddr->addr + ret,
> +				     RPCBIND_MAXUADDRLEN + 1,
> +				     ".%u.%u", port >> 8, port & 0xff);

Isn't the remaining space RPCBIND_MAXUADDRLEN + 1 - ret ?

But really I think we're depending on RPCBIND_MAXUADDRLEN being large
enough that there will never be overlow, in which case we may as well
just use sprintf.

And maybe add a WARN_ON() in case of overflow, if we want to be
paranoid about it.

--b.

> +
> +	return 0;
> +}
> +
>  static inline bool bmval_is_subset(const u32 *bm1, const u32 *bm2)
>  {
>  	return !((bm1[0] & ~bm2[0]) ||
> -- 
> 1.8.3.1

Re: [PATCH v1 05/13] NFS skip recovery of copy open on dest server

[Olga Kornievskaia]

On Thu, Nov 1, 2018 at 4:20 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Mark the open created for the source file on the destination
> > server. Then if this open is going thru a recovery, then fail
> > the recovery as we don't need to be recoving a "fake" open.
> > We need to fail the ongoing READs and vfs_copy_file_range().
>
> Nothing to do with this patch, really, but:

And because it not related to this patch, then I'm confused.

> I'm not always a stickler about the 80-character-column rule, but, man,
> nfs4_reclaim_open_state is deeply nested, could that be broken up a
> little?

really? it's nested? Anna reworked the reclaim state code and nesting
has gone done considerably. It's only 2 tabs in?

>  Also I notice there's a printk("AGLO: ...") in there that you
> probably didn't mean to leave upstream?

I don't believe I've ever had a printk() in the code. I did have a
leftover dprintk() and that has since been removed.

It looks like my attempt at providing you with the client side patches
might have created a problem where you are reviewing (now) an outdated
code. I never expected you to review the client side patches. However,
if you are so inclined then could you please just look at my latest
submission (v8) of the client patches as oppose to looking the version
submitted for the nfsd.

>
> --b.
>
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4_fs.h   |  1 +
> >  fs/nfs/nfs4file.c  |  1 +
> >  fs/nfs/nfs4state.c | 14 ++++++++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > index f229864..8b00c90 100644
> > --- a/fs/nfs/nfs4_fs.h
> > +++ b/fs/nfs/nfs4_fs.h
> > @@ -165,6 +165,7 @@ enum {
> >       NFS_STATE_CHANGE_WAIT,          /* A state changing operation is outstanding */
> >  #ifdef CONFIG_NFS_V4_2
> >       NFS_CLNT_DST_SSC_COPY_STATE,    /* dst server open state on client*/
> > +     NFS_SRV_SSC_COPY_STATE,         /* ssc state on the dst server */
> >  #endif /* CONFIG_NFS_V4_2 */
> >  };
> >
> > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> > index f82cb05..7909cd4 100644
> > --- a/fs/nfs/nfs4file.c
> > +++ b/fs/nfs/nfs4file.c
> > @@ -309,6 +309,7 @@ struct file *
> >       if (ctx->state == NULL)
> >               goto out_stateowner;
> >
> > +     set_bit(NFS_SRV_SSC_COPY_STATE, &ctx->state->flags);
> >       set_bit(NFS_OPEN_STATE, &ctx->state->flags);
> >       memcpy(&ctx->state->open_stateid.other, &stateid->other,
> >              NFS4_STATEID_OTHER_SIZE);
> > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> > index 62ae0fd..b0b82c6 100644
> > --- a/fs/nfs/nfs4state.c
> > +++ b/fs/nfs/nfs4state.c
> > @@ -1606,6 +1606,9 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >  {
> >       struct nfs4_state *state;
> >       int status = 0;
> > +#ifdef CONFIG_NFS_V4_2
> > +     bool found_ssc_copy_state = false;
> > +#endif /* CONFIG_NFS_V4_2 */
> >
> >       /* Note: we rely on the sp->so_states list being ordered
> >        * so that we always reclaim open(O_RDWR) and/or open(O_WRITE)
> > @@ -1625,6 +1628,13 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >                       continue;
> >               if (state->state == 0)
> >                       continue;
> > +#ifdef CONFIG_NFS_V4_2
> > +             if (test_bit(NFS_SRV_SSC_COPY_STATE, &state->flags)) {
> > +                     nfs4_state_mark_recovery_failed(state, -EIO);
> > +                     found_ssc_copy_state = true;
> > +                     continue;
> > +             }
> > +#endif /* CONFIG_NFS_V4_2 */
> >               refcount_inc(&state->count);
> >               spin_unlock(&sp->so_lock);
> >               status = __nfs4_reclaim_open_state(sp, state, ops);
> > @@ -1671,6 +1681,10 @@ static int nfs4_reclaim_open_state(struct nfs4_state_owner *sp, const struct nfs
> >       }
> >       raw_write_seqcount_end(&sp->so_reclaim_seqcount);
> >       spin_unlock(&sp->so_lock);
> > +#ifdef CONFIG_NFS_V4_2
> > +     if (found_ssc_copy_state)
> > +             return -EIO;
> > +#endif /* CONFIG_NFS_V4_2 */
> >       return 0;
> >  out_err:
> >       nfs4_put_open_state(state);
> > --
> > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> Note: followed conventions and have struct nfsd4_compoundargs pointer as a
> parameter even though it is unused.

It's used--see the definition of READ_BUF.

(I'm not a fan of those macros and they'll probably be replaced some
day.)

--b.

> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  fs/nfsd/xdr4.h    |  1 +
>  2 files changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 3de42a7..9f6886f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -40,6 +40,7 @@
>  #include <linux/utsname.h>
>  #include <linux/pagemap.h>
>  #include <linux/sunrpc/svcauth_gss.h>
> +#include <linux/sunrpc/addr.h>
>  
>  #include "idmap.h"
>  #include "acl.h"
> @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  	DECODE_TAIL;
>  }
>  
> +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
> +				      struct nl4_server *ns)
> +{
> +	DECODE_HEAD;
> +	struct nfs42_netaddr *naddr;
> +
> +	READ_BUF(4);
> +	ns->nl4_type = be32_to_cpup(p++);
> +
> +	/* currently support for 1 inter-server source server */
> +	switch (ns->nl4_type) {
> +	case NL4_NAME:
> +	case NL4_URL:
> +		READ_BUF(4);
> +		ns->u.nl4_str_sz = be32_to_cpup(p++);
> +		if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT)
> +			goto xdr_error;
> +
> +		READ_BUF(ns->u.nl4_str_sz);
> +		COPYMEM(ns->u.nl4_str,
> +			ns->u.nl4_str_sz);
> +		break;
> +	case NL4_NETADDR:
> +		naddr = &ns->u.nl4_addr;
> +
> +		READ_BUF(4);
> +		naddr->netid_len = be32_to_cpup(p++);
> +		if (naddr->netid_len > RPCBIND_MAXNETIDLEN)
> +			goto xdr_error;
> +
> +		READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */
> +		COPYMEM(naddr->netid, naddr->netid_len);
> +
> +		naddr->addr_len = be32_to_cpup(p++);
> +		if (naddr->addr_len > RPCBIND_MAXUADDRLEN)
> +			goto xdr_error;
> +
> +		READ_BUF(naddr->addr_len);
> +		COPYMEM(naddr->addr, naddr->addr_len);
> +		break;
> +	default:
> +		goto xdr_error;
> +	}
> +	DECODE_TAIL;
> +}
> +
>  static __be32
>  nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy)
>  {
>  	DECODE_HEAD;
> -	unsigned int tmp;
> +	struct nl4_server *ns;
> +	int i, count;
>  
>  	status = nfsd4_decode_stateid(argp, &copy->cp_src_stateid);
>  	if (status)
> @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  	p = xdr_decode_hyper(p, &copy->cp_count);
>  	p++; /* ca_consecutive: we always do consecutive copies */
>  	copy->cp_synchronous = be32_to_cpup(p++);
> -	tmp = be32_to_cpup(p); /* Source server list not supported */
> +	count = be32_to_cpup(p++);
> +
> +	if (count == 0) /* intra-server copy */
> +		goto intra;
>  
> +	/* decode all the supplied server addresses but use first */
> +	copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);
> +	if (copy->cp_src == NULL)
> +		return nfserrno(-ENOMEM);
> +
> +	ns = copy->cp_src;
> +	for (i = 0; i < count; i++) {
> +		status = nfsd4_decode_nl4_server(argp, ns);
> +		if (status)
> +			return status;
> +		ns++;
> +	}
> +intra:
>  	DECODE_TAIL;
>  }
>  
> @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
>  	p = xdr_reserve_space(&resp->xdr, 4 + 4);
>  	*p++ = xdr_one; /* cr_consecutive */
>  	*p++ = cpu_to_be32(copy->cp_synchronous);
> +
> +	/* allocated in nfsd4_decode_copy */
> +	kfree(copy->cp_src);
>  	return 0;
>  }
>  
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index feeb6d4..b4d1140 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -521,6 +521,7 @@ struct nfsd4_copy {
>  	u64		cp_src_pos;
>  	u64		cp_dst_pos;
>  	u64		cp_count;
> +	struct nl4_server *cp_src;
>  
>  	/* both */
>  	bool		cp_synchronous;
> -- 
> 1.8.3.1

Re: [PATCH v1 06/13] NFSD fill-in netloc4 structure

[Olga Kornievskaia]

On Thu, Nov 1, 2018 at 4:37 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:58AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > nfs.4 defines nfs42_netaddr structure that represents netloc4.
> >
> > Populate needed fields from the sockaddr structure.
> >
> > This will be used by flexfiles and 4.2 inter copy
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/nfsd.h | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 0668999..030fccb 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -18,7 +18,7 @@
> >  #include <linux/nfs4.h>
> >  #include <linux/sunrpc/svc.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > -
> > +#include <linux/sunrpc/addr.h>
> >  #include <uapi/linux/nfsd/debug.h>
> >
> >  #include "stats.h"
> > @@ -366,6 +366,35 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> >
> >  extern const u32 nfsd_suppattrs[3][3];
> >
> > +static inline u32 nfsd4_set_netaddr(struct sockaddr *addr, struct nfs42_netaddr *netaddr)
> > +{
> > +     struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> > +     struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
> > +     unsigned int port;
> > +     size_t ret;
> > +
> > +     switch (addr->sa_family) {
> > +     case AF_INET:
> > +             port = ntohs(sin->sin_port);
> > +             sprintf(netaddr->netid, "tcp");
> > +             netaddr->netid_len = 3;
> > +             break;
> > +     case AF_INET6:
> > +             port = ntohs(sin6->sin6_port);
> > +             sprintf(netaddr->netid, "tcp6");
> > +             netaddr->netid_len = 4;
> > +             break;
> > +     default:
> > +             return nfserrno(-EINVAL);
>
> Let's just write that nfserr_inval;

Ok will do.

>
> > +     }
> > +     ret = rpc_ntop(addr, netaddr->addr, sizeof(netaddr->addr));
> > +     netaddr->addr_len = ret + snprintf(netaddr->addr + ret,
> > +                                  RPCBIND_MAXUADDRLEN + 1,
> > +                                  ".%u.%u", port >> 8, port & 0xff);
>
> Isn't the remaining space RPCBIND_MAXUADDRLEN + 1 - ret ?

You are right.

> But really I think we're depending on RPCBIND_MAXUADDRLEN being large
> enough that there will never be overlow, in which case we may as well
> just use sprintf.
>
> And maybe add a WARN_ON() in case of overflow, if we want to be
> paranoid about it.

Are you proposing to first get the output from snprintf() and see if
there was an overflow because it returned more than the supplied size
and then print WARN_ON()? How do I detect an overflow with just a
sprintf()?

>
> --b.
>
> > +
> > +     return 0;
> > +}
> > +
> >  static inline bool bmval_is_subset(const u32 *bm1, const u32 *bm2)
> >  {
> >       return !((bm1[0] & ~bm2[0]) ||
> > --
> > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[Olga Kornievskaia]

On Thu, Nov 1, 2018 at 4:48 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > Note: followed conventions and have struct nfsd4_compoundargs pointer as a
> > parameter even though it is unused.
>
> It's used--see the definition of READ_BUF.

Sigh. It's a leftover comment from Andy. I'll remove it.

> (I'm not a fan of those macros and they'll probably be replaced some
> day.)

READ_BUF is used thru out so I'm assuming you are not against this
patch using it, correct?

>
> --b.
>
> >
> > Signed-off-by: Andy Adamson <andros@netapp.com>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  fs/nfsd/xdr4.h    |  1 +
> >  2 files changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 3de42a7..9f6886f 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/utsname.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/sunrpc/svcauth_gss.h>
> > +#include <linux/sunrpc/addr.h>
> >
> >  #include "idmap.h"
> >  #include "acl.h"
> > @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> >       DECODE_TAIL;
> >  }
> >
> > +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
> > +                                   struct nl4_server *ns)
> > +{
> > +     DECODE_HEAD;
> > +     struct nfs42_netaddr *naddr;
> > +
> > +     READ_BUF(4);
> > +     ns->nl4_type = be32_to_cpup(p++);
> > +
> > +     /* currently support for 1 inter-server source server */
> > +     switch (ns->nl4_type) {
> > +     case NL4_NAME:
> > +     case NL4_URL:
> > +             READ_BUF(4);
> > +             ns->u.nl4_str_sz = be32_to_cpup(p++);
> > +             if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT)
> > +                     goto xdr_error;
> > +
> > +             READ_BUF(ns->u.nl4_str_sz);
> > +             COPYMEM(ns->u.nl4_str,
> > +                     ns->u.nl4_str_sz);
> > +             break;
> > +     case NL4_NETADDR:
> > +             naddr = &ns->u.nl4_addr;
> > +
> > +             READ_BUF(4);
> > +             naddr->netid_len = be32_to_cpup(p++);
> > +             if (naddr->netid_len > RPCBIND_MAXNETIDLEN)
> > +                     goto xdr_error;
> > +
> > +             READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */
> > +             COPYMEM(naddr->netid, naddr->netid_len);
> > +
> > +             naddr->addr_len = be32_to_cpup(p++);
> > +             if (naddr->addr_len > RPCBIND_MAXUADDRLEN)
> > +                     goto xdr_error;
> > +
> > +             READ_BUF(naddr->addr_len);
> > +             COPYMEM(naddr->addr, naddr->addr_len);
> > +             break;
> > +     default:
> > +             goto xdr_error;
> > +     }
> > +     DECODE_TAIL;
> > +}
> > +
> >  static __be32
> >  nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy)
> >  {
> >       DECODE_HEAD;
> > -     unsigned int tmp;
> > +     struct nl4_server *ns;
> > +     int i, count;
> >
> >       status = nfsd4_decode_stateid(argp, &copy->cp_src_stateid);
> >       if (status)
> > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> >       p = xdr_decode_hyper(p, &copy->cp_count);
> >       p++; /* ca_consecutive: we always do consecutive copies */
> >       copy->cp_synchronous = be32_to_cpup(p++);
> > -     tmp = be32_to_cpup(p); /* Source server list not supported */
> > +     count = be32_to_cpup(p++);
> > +
> > +     if (count == 0) /* intra-server copy */
> > +             goto intra;
> >
> > +     /* decode all the supplied server addresses but use first */
> > +     copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);
> > +     if (copy->cp_src == NULL)
> > +             return nfserrno(-ENOMEM);
> > +
> > +     ns = copy->cp_src;
> > +     for (i = 0; i < count; i++) {
> > +             status = nfsd4_decode_nl4_server(argp, ns);
> > +             if (status)
> > +                     return status;
> > +             ns++;
> > +     }
> > +intra:
> >       DECODE_TAIL;
> >  }
> >
> > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> >       *p++ = xdr_one; /* cr_consecutive */
> >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > +
> > +     /* allocated in nfsd4_decode_copy */
> > +     kfree(copy->cp_src);
> >       return 0;
> >  }
> >
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index feeb6d4..b4d1140 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> >       u64             cp_src_pos;
> >       u64             cp_dst_pos;
> >       u64             cp_count;
> > +     struct nl4_server *cp_src;
> >
> >       /* both */
> >       bool            cp_synchronous;
> > --
> > 1.8.3.1

Re: [PATCH v1 05/13] NFS skip recovery of copy open on dest server

[J. Bruce Fields]

On Thu, Nov 01, 2018 at 04:38:43PM -0400, Olga Kornievskaia wrote:
> On Thu, Nov 1, 2018 at 4:20 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:57AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > Mark the open created for the source file on the destination
> > > server. Then if this open is going thru a recovery, then fail
> > > the recovery as we don't need to be recoving a "fake" open.
> > > We need to fail the ongoing READs and vfs_copy_file_range().
> >
> > Nothing to do with this patch, really, but:
> 
> And because it not related to this patch, then I'm confused.
> 
> > I'm not always a stickler about the 80-character-column rule, but, man,
> > nfs4_reclaim_open_state is deeply nested, could that be broken up a
> > little?
> 
> really? it's nested? Anna reworked the reclaim state code and nesting
> has gone done considerably. It's only 2 tabs in?
> 
> >  Also I notice there's a printk("AGLO: ...") in there that you
> > probably didn't mean to leave upstream?
> 
> I don't believe I've ever had a printk() in the code. I did have a
> leftover dprintk() and that has since been removed.

Oh, I see, this was all fixed recently upstream by Anna's cb7a8384dc02
"NFS: Split out the body of nfs4_reclaim_open_state()", ignore me.

--b.

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[J. Bruce Fields]

On Thu, Nov 01, 2018 at 05:00:46PM -0400, Olga Kornievskaia wrote:
> On Thu, Nov 1, 2018 at 4:48 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > > Note: followed conventions and have struct nfsd4_compoundargs pointer as a
> > > parameter even though it is unused.
> >
> > It's used--see the definition of READ_BUF.
> 
> Sigh. It's a leftover comment from Andy. I'll remove it.
> 
> > (I'm not a fan of those macros and they'll probably be replaced some
> > day.)
> 
> READ_BUF is used thru out so I'm assuming you are not against this
> patch using it, correct?

Correct, replacing those macros is a project for another day.

--b.

> 
> >
> > --b.
> >
> > >
> > > Signed-off-by: Andy Adamson <andros@netapp.com>
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  fs/nfsd/xdr4.h    |  1 +
> > >  2 files changed, 70 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index 3de42a7..9f6886f 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -40,6 +40,7 @@
> > >  #include <linux/utsname.h>
> > >  #include <linux/pagemap.h>
> > >  #include <linux/sunrpc/svcauth_gss.h>
> > > +#include <linux/sunrpc/addr.h>
> > >
> > >  #include "idmap.h"
> > >  #include "acl.h"
> > > @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> > >       DECODE_TAIL;
> > >  }
> > >
> > > +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp,
> > > +                                   struct nl4_server *ns)
> > > +{
> > > +     DECODE_HEAD;
> > > +     struct nfs42_netaddr *naddr;
> > > +
> > > +     READ_BUF(4);
> > > +     ns->nl4_type = be32_to_cpup(p++);
> > > +
> > > +     /* currently support for 1 inter-server source server */
> > > +     switch (ns->nl4_type) {
> > > +     case NL4_NAME:
> > > +     case NL4_URL:
> > > +             READ_BUF(4);
> > > +             ns->u.nl4_str_sz = be32_to_cpup(p++);
> > > +             if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT)
> > > +                     goto xdr_error;
> > > +
> > > +             READ_BUF(ns->u.nl4_str_sz);
> > > +             COPYMEM(ns->u.nl4_str,
> > > +                     ns->u.nl4_str_sz);
> > > +             break;
> > > +     case NL4_NETADDR:
> > > +             naddr = &ns->u.nl4_addr;
> > > +
> > > +             READ_BUF(4);
> > > +             naddr->netid_len = be32_to_cpup(p++);
> > > +             if (naddr->netid_len > RPCBIND_MAXNETIDLEN)
> > > +                     goto xdr_error;
> > > +
> > > +             READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */
> > > +             COPYMEM(naddr->netid, naddr->netid_len);
> > > +
> > > +             naddr->addr_len = be32_to_cpup(p++);
> > > +             if (naddr->addr_len > RPCBIND_MAXUADDRLEN)
> > > +                     goto xdr_error;
> > > +
> > > +             READ_BUF(naddr->addr_len);
> > > +             COPYMEM(naddr->addr, naddr->addr_len);
> > > +             break;
> > > +     default:
> > > +             goto xdr_error;
> > > +     }
> > > +     DECODE_TAIL;
> > > +}
> > > +
> > >  static __be32
> > >  nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy)
> > >  {
> > >       DECODE_HEAD;
> > > -     unsigned int tmp;
> > > +     struct nl4_server *ns;
> > > +     int i, count;
> > >
> > >       status = nfsd4_decode_stateid(argp, &copy->cp_src_stateid);
> > >       if (status)
> > > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> > >       p = xdr_decode_hyper(p, &copy->cp_count);
> > >       p++; /* ca_consecutive: we always do consecutive copies */
> > >       copy->cp_synchronous = be32_to_cpup(p++);
> > > -     tmp = be32_to_cpup(p); /* Source server list not supported */
> > > +     count = be32_to_cpup(p++);
> > > +
> > > +     if (count == 0) /* intra-server copy */
> > > +             goto intra;
> > >
> > > +     /* decode all the supplied server addresses but use first */
> > > +     copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);
> > > +     if (copy->cp_src == NULL)
> > > +             return nfserrno(-ENOMEM);
> > > +
> > > +     ns = copy->cp_src;
> > > +     for (i = 0; i < count; i++) {
> > > +             status = nfsd4_decode_nl4_server(argp, ns);
> > > +             if (status)
> > > +                     return status;
> > > +             ns++;
> > > +     }
> > > +intra:
> > >       DECODE_TAIL;
> > >  }
> > >
> > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> > >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> > >       *p++ = xdr_one; /* cr_consecutive */
> > >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > > +
> > > +     /* allocated in nfsd4_decode_copy */
> > > +     kfree(copy->cp_src);
> > >       return 0;
> > >  }
> > >
> > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > > index feeb6d4..b4d1140 100644
> > > --- a/fs/nfsd/xdr4.h
> > > +++ b/fs/nfsd/xdr4.h
> > > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> > >       u64             cp_src_pos;
> > >       u64             cp_dst_pos;
> > >       u64             cp_count;
> > > +     struct nl4_server *cp_src;
> > >
> > >       /* both */
> > >       bool            cp_synchronous;
> > > --
> > > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
>  	p = xdr_decode_hyper(p, &copy->cp_count);
>  	p++; /* ca_consecutive: we always do consecutive copies */
>  	copy->cp_synchronous = be32_to_cpup(p++);
> -	tmp = be32_to_cpup(p); /* Source server list not supported */
> +	count = be32_to_cpup(p++);
> +
> +	if (count == 0) /* intra-server copy */
> +		goto intra;
>  
> +	/* decode all the supplied server addresses but use first */
> +	copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);

The client could pass an arbitrarily large count here.  I don't know if
the ability to force the server to attempt a large kmalloc() is really
useful to an attacker, but I'd definitely rather not allow it.

Possibly more serious: if that multiplication overflows, then in theory
it might be possible to make the kmalloc() succeed and allocate too
little memory, after which the following loop could overwrite memory
past the end of the allocation.

As long as we're only using the first address, maybe it's simplest just
not to bother allocating memory for the rest.  Just copy the first one,
and for the rest call nfsd4_decode_nl4_server() with a dummy struct
nl4_server.

--b.

> +	if (copy->cp_src == NULL)
> +		return nfserrno(-ENOMEM);
> +
> +	ns = copy->cp_src;
> +	for (i = 0; i < count; i++) {
> +		status = nfsd4_decode_nl4_server(argp, ns);
> +		if (status)
> +			return status;
> +		ns++;
> +	}
> +intra:
>  	DECODE_TAIL;
>  }
>  
> @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
>  	p = xdr_reserve_space(&resp->xdr, 4 + 4);
>  	*p++ = xdr_one; /* cr_consecutive */
>  	*p++ = cpu_to_be32(copy->cp_synchronous);
> +
> +	/* allocated in nfsd4_decode_copy */
> +	kfree(copy->cp_src);
>  	return 0;
>  }
>  
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index feeb6d4..b4d1140 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -521,6 +521,7 @@ struct nfsd4_copy {
>  	u64		cp_src_pos;
>  	u64		cp_dst_pos;
>  	u64		cp_count;
> +	struct nl4_server *cp_src;
>  
>  	/* both */
>  	bool		cp_synchronous;
> -- 
> 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
>  	p = xdr_reserve_space(&resp->xdr, 4 + 4);
>  	*p++ = xdr_one; /* cr_consecutive */
>  	*p++ = cpu_to_be32(copy->cp_synchronous);
> +
> +	/* allocated in nfsd4_decode_copy */
> +	kfree(copy->cp_src);

This can result in a leak--for example, if we decode the compound
succesfully, but processing fails before we could to this op, then we'll
never call this encoder, so we'll allocate without freeing.

I think simplest would be to replace this:

> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index feeb6d4..b4d1140 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -521,6 +521,7 @@ struct nfsd4_copy {
>  	u64		cp_src_pos;
>  	u64		cp_dst_pos;
>  	u64		cp_count;
> +	struct nl4_server *cp_src;

by just a

	struct nl4_server cp_src;

since it sounds like you really only need one of them, not a whole array
(at least for now).

--b.

>  
>  	/* both */
>  	bool		cp_synchronous;
> -- 
> 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[Olga Kornievskaia]

On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> >       *p++ = xdr_one; /* cr_consecutive */
> >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > +
> > +     /* allocated in nfsd4_decode_copy */
> > +     kfree(copy->cp_src);
>
> This can result in a leak--for example, if we decode the compound
> succesfully, but processing fails before we could to this op, then we'll
> never call this encoder, so we'll allocate without freeing.
>
> I think simplest would be to replace this:
>
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index feeb6d4..b4d1140 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> >       u64             cp_src_pos;
> >       u64             cp_dst_pos;
> >       u64             cp_count;
> > +     struct nl4_server *cp_src;
>
> by just a
>
>         struct nl4_server cp_src;
>
> since it sounds like you really only need one of them, not a whole array
> (at least for now).

So this is problematic as the presence of this memory is what is used
to distinguish "inter" from "intra".

Can things really fail between the xdr and calling of the operation?

What gets freed in the encoder is the "copy" of the what was decoded
in the decoder. But really freeing in the encoder is the wrong place.
Encoder doesn't need to free. I already free the "copy" of the
copy->cp_src in the cleanup_async_copy(). However, what is missing is
freeing the original copy->cp_src which needs to be freed in the
dup_copy_fields().

To clarify:
copy->cp_src gets allocated in the decoder
during the process of the copy:
1. it gets copied to the kthread and the original copy->cp_src needs
to be freed. Or during any error it will be freed.
2. cleanup_async_copy frees the copy of the copy->cp_src.
(need to remove the kfree from the encoder).

>
> --b.
>
> >
> >       /* both */
> >       bool            cp_synchronous;
> > --
> > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[Olga Kornievskaia]

On Fri, Nov 2, 2018 at 10:04 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> >       p = xdr_decode_hyper(p, &copy->cp_count);
> >       p++; /* ca_consecutive: we always do consecutive copies */
> >       copy->cp_synchronous = be32_to_cpup(p++);
> > -     tmp = be32_to_cpup(p); /* Source server list not supported */
> > +     count = be32_to_cpup(p++);
> > +
> > +     if (count == 0) /* intra-server copy */
> > +             goto intra;
> >
> > +     /* decode all the supplied server addresses but use first */
> > +     copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL);
>
> The client could pass an arbitrarily large count here.  I don't know if
> the ability to force the server to attempt a large kmalloc() is really
> useful to an attacker, but I'd definitely rather not allow it.
>
> Possibly more serious: if that multiplication overflows, then in theory
> it might be possible to make the kmalloc() succeed and allocate too
> little memory, after which the following loop could overwrite memory
> past the end of the allocation.
>
> As long as we're only using the first address, maybe it's simplest just
> not to bother allocating memory for the rest.  Just copy the first one,
> and for the rest call nfsd4_decode_nl4_server() with a dummy struct
> nl4_server.

Ok will try.

>
> --b.
>
> > +     if (copy->cp_src == NULL)
> > +             return nfserrno(-ENOMEM);
> > +
> > +     ns = copy->cp_src;
> > +     for (i = 0; i < count; i++) {
> > +             status = nfsd4_decode_nl4_server(argp, ns);
> > +             if (status)
> > +                     return status;
> > +             ns++;
> > +     }
> > +intra:
> >       DECODE_TAIL;
> >  }
> >
> > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> >       *p++ = xdr_one; /* cr_consecutive */
> >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > +
> > +     /* allocated in nfsd4_decode_copy */
> > +     kfree(copy->cp_src);
> >       return 0;
> >  }
> >
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index feeb6d4..b4d1140 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> >       u64             cp_src_pos;
> >       u64             cp_dst_pos;
> >       u64             cp_count;
> > +     struct nl4_server *cp_src;
> >
> >       /* both */
> >       bool            cp_synchronous;
> > --
> > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[J. Bruce Fields]

On Fri, Nov 02, 2018 at 12:35:26PM -0400, Olga Kornievskaia wrote:
> On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> > >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> > >       *p++ = xdr_one; /* cr_consecutive */
> > >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > > +
> > > +     /* allocated in nfsd4_decode_copy */
> > > +     kfree(copy->cp_src);
> >
> > This can result in a leak--for example, if we decode the compound
> > succesfully, but processing fails before we could to this op, then we'll
> > never call this encoder, so we'll allocate without freeing.
> >
> > I think simplest would be to replace this:
> >
> > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > > index feeb6d4..b4d1140 100644
> > > --- a/fs/nfsd/xdr4.h
> > > +++ b/fs/nfsd/xdr4.h
> > > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> > >       u64             cp_src_pos;
> > >       u64             cp_dst_pos;
> > >       u64             cp_count;
> > > +     struct nl4_server *cp_src;
> >
> > by just a
> >
> >         struct nl4_server cp_src;
> >
> > since it sounds like you really only need one of them, not a whole array
> > (at least for now).
> 
> So this is problematic as the presence of this memory is what is used
> to distinguish "inter" from "intra".

It would be easy enough to add a new bit for that.

> Can things really fail between the xdr and calling of the operation?

Yes.  Consider a PUTFH+SAVEFH+COPY compound.  We decode the whole thing
before starting any processing.  Then we call nfsd4_putfh() and
fh_verify fails (maybe the filehandle is stale or something).  Then
neither nfsd4_copy() nor nfsd4_encode_copy() will be called.

If you absolutely have to do this, you can look at SAVEMEM.

--b.

> What gets freed in the encoder is the "copy" of the what was decoded
> in the decoder. But really freeing in the encoder is the wrong place.
> Encoder doesn't need to free. I already free the "copy" of the
> copy->cp_src in the cleanup_async_copy(). However, what is missing is
> freeing the original copy->cp_src which needs to be freed in the
> dup_copy_fields().
> 
> To clarify:
> copy->cp_src gets allocated in the decoder
> during the process of the copy:
> 1. it gets copied to the kthread and the original copy->cp_src needs
> to be freed. Or during any error it will be freed.
> 2. cleanup_async_copy frees the copy of the copy->cp_src.
> (need to remove the kfree from the encoder).
> 
> >
> > --b.
> >
> > >
> > >       /* both */
> > >       bool            cp_synchronous;
> > > --
> > > 1.8.3.1

Re: [PATCH v1 07/13] NFSD add ca_source_server<> to COPY

[Olga Kornievskaia]

On Fri, Nov 2, 2018 at 12:49 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Nov 02, 2018 at 12:35:26PM -0400, Olga Kornievskaia wrote:
> > On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote:
> > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp,
> > > >       p = xdr_reserve_space(&resp->xdr, 4 + 4);
> > > >       *p++ = xdr_one; /* cr_consecutive */
> > > >       *p++ = cpu_to_be32(copy->cp_synchronous);
> > > > +
> > > > +     /* allocated in nfsd4_decode_copy */
> > > > +     kfree(copy->cp_src);
> > >
> > > This can result in a leak--for example, if we decode the compound
> > > succesfully, but processing fails before we could to this op, then we'll
> > > never call this encoder, so we'll allocate without freeing.
> > >
> > > I think simplest would be to replace this:
> > >
> > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > > > index feeb6d4..b4d1140 100644
> > > > --- a/fs/nfsd/xdr4.h
> > > > +++ b/fs/nfsd/xdr4.h
> > > > @@ -521,6 +521,7 @@ struct nfsd4_copy {
> > > >       u64             cp_src_pos;
> > > >       u64             cp_dst_pos;
> > > >       u64             cp_count;
> > > > +     struct nl4_server *cp_src;
> > >
> > > by just a
> > >
> > >         struct nl4_server cp_src;
> > >
> > > since it sounds like you really only need one of them, not a whole array
> > > (at least for now).
> >
> > So this is problematic as the presence of this memory is what is used
> > to distinguish "inter" from "intra".
>
> It would be easy enough to add a new bit for that.
>
> > Can things really fail between the xdr and calling of the operation?
>
> Yes.  Consider a PUTFH+SAVEFH+COPY compound.  We decode the whole thing
> before starting any processing.  Then we call nfsd4_putfh() and
> fh_verify fails (maybe the filehandle is stale or something).  Then
> neither nfsd4_copy() nor nfsd4_encode_copy() will be called.
>
> If you absolutely have to do this, you can look at SAVEMEM.

Using a new bit is good enough for me.

>
> --b.
>
> > What gets freed in the encoder is the "copy" of the what was decoded
> > in the decoder. But really freeing in the encoder is the wrong place.
> > Encoder doesn't need to free. I already free the "copy" of the
> > copy->cp_src in the cleanup_async_copy(). However, what is missing is
> > freeing the original copy->cp_src which needs to be freed in the
> > dup_copy_fields().
> >
> > To clarify:
> > copy->cp_src gets allocated in the decoder
> > during the process of the copy:
> > 1. it gets copied to the kthread and the original copy->cp_src needs
> > to be freed. Or during any error it will be freed.
> > 2. cleanup_async_copy frees the copy of the copy->cp_src.
> > (need to remove the kfree from the encoder).
> >
> > >
> > > --b.
> > >
> > > >
> > > >       /* both */
> > > >       bool            cp_synchronous;
> > > > --
> > > > 1.8.3.1

Re: [PATCH v1 08/13] NFSD return nfs4_stid in nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:29:00AM -0400, Olga Kornievskaia wrote:
> Needed for copy to add nfs4_cp_state to the nfs4_stid.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4proc.c  | 17 ++++++++++-------
>  fs/nfsd/nfs4state.c |  8 ++++++--
>  fs/nfsd/state.h     |  3 ++-
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index edff074..29686df 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -781,7 +781,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)

By the way, I wonder why git-diff is doing such a bad job of guessing
the right function?  Not a big deal, the patches still apply fine, it
just makes reading them in email a little harder.

--b.

>  	/* check stateid */
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					&read->rd_stateid, RD_STATE,
> -					&read->rd_filp, &read->rd_tmp_file);
> +					&read->rd_filp, &read->rd_tmp_file,
> +					NULL);
>  	if (status) {
>  		dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
>  		goto out;
> @@ -954,7 +955,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
>  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
>  		status = nfs4_preprocess_stateid_op(rqstp, cstate,
>  				&cstate->current_fh, &setattr->sa_stateid,
> -				WR_STATE, NULL, NULL);
> +				WR_STATE, NULL, NULL, NULL);
>  		if (status) {
>  			dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
>  			return status;
> @@ -1005,7 +1006,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
>  	trace_nfsd_write_start(rqstp, &cstate->current_fh,
>  			       write->wr_offset, cnt);
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> -						stateid, WR_STATE, &filp, NULL);
> +					stateid, WR_STATE, &filp, NULL, NULL);
>  	if (status) {
>  		dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
>  		return status;
> @@ -1039,14 +1040,16 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
>  	__be32 status;
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
> -					    src_stateid, RD_STATE, src, NULL);
> +					    src_stateid, RD_STATE, src, NULL,
> +					    NULL);
>  	if (status) {
>  		dprintk("NFSD: %s: couldn't process src stateid!\n", __func__);
>  		goto out;
>  	}
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> -					    dst_stateid, WR_STATE, dst, NULL);
> +					    dst_stateid, WR_STATE, dst, NULL,
> +					    NULL);
>  	if (status) {
>  		dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
>  		goto out_put_src;
> @@ -1350,7 +1353,7 @@ struct nfsd4_copy *
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    &fallocate->falloc_stateid,
> -					    WR_STATE, &file, NULL);
> +					    WR_STATE, &file, NULL, NULL);
>  	if (status != nfs_ok) {
>  		dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
>  		return status;
> @@ -1409,7 +1412,7 @@ struct nfsd4_copy *
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    &seek->seek_stateid,
> -					    RD_STATE, &file, NULL);
> +					    RD_STATE, &file, NULL, NULL);
>  	if (status) {
>  		dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
>  		return status;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 07a57d0..e263fd0 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5159,7 +5159,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  __be32
>  nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> -		stateid_t *stateid, int flags, struct file **filpp, bool *tmp_file)
> +		stateid_t *stateid, int flags, struct file **filpp,
> +		bool *tmp_file, struct nfs4_stid **cstid)
>  {
>  	struct inode *ino = d_inode(fhp->fh_dentry);
>  	struct net *net = SVC_NET(rqstp);
> @@ -5210,8 +5211,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	if (!status && filpp)
>  		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
>  out:
> -	if (s)
> +	if (s) {
> +		if (!status && cstid)
> +			*cstid = s;
>  		nfs4_put_stid(s);
> +	}
>  	return status;
>  }
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 6aacb32..304de3b 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -606,7 +606,8 @@ struct nfsd4_blocked_lock {
>  
>  extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> -		stateid_t *stateid, int flags, struct file **filp, bool *tmp_file);
> +		stateid_t *stateid, int flags, struct file **filp,
> +		bool *tmp_file, struct nfs4_stid **cstid);
>  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		     stateid_t *stateid, unsigned char typemask,
>  		     struct nfs4_stid **s, struct nfsd_net *nn);
> -- 
> 1.8.3.1

Re: [PATCH v1 08/13] NFSD return nfs4_stid in nfs4_preprocess_stateid_op

[Olga Kornievskaia]

On Fri, Nov 2, 2018 at 3:06 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:29:00AM -0400, Olga Kornievskaia wrote:
> > Needed for copy to add nfs4_cp_state to the nfs4_stid.
> >
> > Signed-off-by: Andy Adamson <andros@netapp.com>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/nfs4proc.c  | 17 ++++++++++-------
> >  fs/nfsd/nfs4state.c |  8 ++++++--
> >  fs/nfsd/state.h     |  3 ++-
> >  3 files changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index edff074..29686df 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -781,7 +781,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
>
> By the way, I wonder why git-diff is doing such a bad job of guessing
> the right function?  Not a big deal, the patches still apply fine, it
> just makes reading them in email a little harder.

Could it be because I worked against Trond's git repo (as that's my
default location)? This is against whatever is in 4.19-rc6. This is
also git from RHEL7.4 (1.8.3.1 version), don't know if that matters.
In the next version, I do the patches against your git repo. Hopefully
it'll be better.

>
> --b.
>
> >       /* check stateid */
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> >                                       &read->rd_stateid, RD_STATE,
> > -                                     &read->rd_filp, &read->rd_tmp_file);
> > +                                     &read->rd_filp, &read->rd_tmp_file,
> > +                                     NULL);
> >       if (status) {
> >               dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
> >               goto out;
> > @@ -954,7 +955,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> >       if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> >               status = nfs4_preprocess_stateid_op(rqstp, cstate,
> >                               &cstate->current_fh, &setattr->sa_stateid,
> > -                             WR_STATE, NULL, NULL);
> > +                             WR_STATE, NULL, NULL, NULL);
> >               if (status) {
> >                       dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
> >                       return status;
> > @@ -1005,7 +1006,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> >       trace_nfsd_write_start(rqstp, &cstate->current_fh,
> >                              write->wr_offset, cnt);
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > -                                             stateid, WR_STATE, &filp, NULL);
> > +                                     stateid, WR_STATE, &filp, NULL, NULL);
> >       if (status) {
> >               dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
> >               return status;
> > @@ -1039,14 +1040,16 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> >       __be32 status;
> >
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
> > -                                         src_stateid, RD_STATE, src, NULL);
> > +                                         src_stateid, RD_STATE, src, NULL,
> > +                                         NULL);
> >       if (status) {
> >               dprintk("NFSD: %s: couldn't process src stateid!\n", __func__);
> >               goto out;
> >       }
> >
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > -                                         dst_stateid, WR_STATE, dst, NULL);
> > +                                         dst_stateid, WR_STATE, dst, NULL,
> > +                                         NULL);
> >       if (status) {
> >               dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
> >               goto out_put_src;
> > @@ -1350,7 +1353,7 @@ struct nfsd4_copy *
> >
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> >                                           &fallocate->falloc_stateid,
> > -                                         WR_STATE, &file, NULL);
> > +                                         WR_STATE, &file, NULL, NULL);
> >       if (status != nfs_ok) {
> >               dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
> >               return status;
> > @@ -1409,7 +1412,7 @@ struct nfsd4_copy *
> >
> >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> >                                           &seek->seek_stateid,
> > -                                         RD_STATE, &file, NULL);
> > +                                         RD_STATE, &file, NULL, NULL);
> >       if (status) {
> >               dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
> >               return status;
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 07a57d0..e263fd0 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5159,7 +5159,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> >  __be32
> >  nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> >               struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> > -             stateid_t *stateid, int flags, struct file **filpp, bool *tmp_file)
> > +             stateid_t *stateid, int flags, struct file **filpp,
> > +             bool *tmp_file, struct nfs4_stid **cstid)
> >  {
> >       struct inode *ino = d_inode(fhp->fh_dentry);
> >       struct net *net = SVC_NET(rqstp);
> > @@ -5210,8 +5211,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> >       if (!status && filpp)
> >               status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
> >  out:
> > -     if (s)
> > +     if (s) {
> > +             if (!status && cstid)
> > +                     *cstid = s;
> >               nfs4_put_stid(s);
> > +     }
> >       return status;
> >  }
> >
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 6aacb32..304de3b 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -606,7 +606,8 @@ struct nfsd4_blocked_lock {
> >
> >  extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> >               struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> > -             stateid_t *stateid, int flags, struct file **filp, bool *tmp_file);
> > +             stateid_t *stateid, int flags, struct file **filp,
> > +             bool *tmp_file, struct nfs4_stid **cstid);
> >  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> >                    stateid_t *stateid, unsigned char typemask,
> >                    struct nfs4_stid **s, struct nfsd_net *nn);
> > --
> > 1.8.3.1

Re: [PATCH v1 08/13] NFSD return nfs4_stid in nfs4_preprocess_stateid_op

[J. Bruce Fields]

On Fri, Nov 02, 2018 at 03:25:18PM -0400, Olga Kornievskaia wrote:
> On Fri, Nov 2, 2018 at 3:06 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:29:00AM -0400, Olga Kornievskaia wrote:
> > > Needed for copy to add nfs4_cp_state to the nfs4_stid.
> > >
> > > Signed-off-by: Andy Adamson <andros@netapp.com>
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  fs/nfsd/nfs4proc.c  | 17 ++++++++++-------
> > >  fs/nfsd/nfs4state.c |  8 ++++++--
> > >  fs/nfsd/state.h     |  3 ++-
> > >  3 files changed, 18 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index edff074..29686df 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -781,7 +781,8 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> >
> > By the way, I wonder why git-diff is doing such a bad job of guessing
> > the right function?  Not a big deal, the patches still apply fine, it
> > just makes reading them in email a little harder.
> 
> Could it be because I worked against Trond's git repo (as that's my
> default location)? This is against whatever is in 4.19-rc6. This is
> also git from RHEL7.4 (1.8.3.1 version), don't know if that matters.
> In the next version, I do the patches against your git repo. Hopefully
> it'll be better.

I dunno.  I just spent more time than it's worth trying to figure out
what's going on.

I think it's ignoring functions written like

	static int
	somefunction(int arg1, int arg2,...)

and instead only catching functions like

	static int somefunction(int arg1, int arg2,...)

I'm using git version 2.17.2, your patches seem to be generated with
1.8.3.1, so that may be the difference, though I looked at the git logs
for git and didn't see an obvious commit saying "here we fixed the regex
that identifies C functions".  There are also configuration options (see
man gitattributes, under the "Defining a custom hunk-header" heading for
too many details), but I can't imagine why you'd have those set
differently.

Whatever, it's not important.

--b.

> 
> >
> > --b.
> >
> > >       /* check stateid */
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > >                                       &read->rd_stateid, RD_STATE,
> > > -                                     &read->rd_filp, &read->rd_tmp_file);
> > > +                                     &read->rd_filp, &read->rd_tmp_file,
> > > +                                     NULL);
> > >       if (status) {
> > >               dprintk("NFSD: nfsd4_read: couldn't process stateid!\n");
> > >               goto out;
> > > @@ -954,7 +955,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> > >       if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > >               status = nfs4_preprocess_stateid_op(rqstp, cstate,
> > >                               &cstate->current_fh, &setattr->sa_stateid,
> > > -                             WR_STATE, NULL, NULL);
> > > +                             WR_STATE, NULL, NULL, NULL);
> > >               if (status) {
> > >                       dprintk("NFSD: nfsd4_setattr: couldn't process stateid!\n");
> > >                       return status;
> > > @@ -1005,7 +1006,7 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> > >       trace_nfsd_write_start(rqstp, &cstate->current_fh,
> > >                              write->wr_offset, cnt);
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > > -                                             stateid, WR_STATE, &filp, NULL);
> > > +                                     stateid, WR_STATE, &filp, NULL, NULL);
> > >       if (status) {
> > >               dprintk("NFSD: nfsd4_write: couldn't process stateid!\n");
> > >               return status;
> > > @@ -1039,14 +1040,16 @@ static __be32 nfsd4_do_lookupp(struct svc_rqst *rqstp, struct svc_fh *fh)
> > >       __be32 status;
> > >
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
> > > -                                         src_stateid, RD_STATE, src, NULL);
> > > +                                         src_stateid, RD_STATE, src, NULL,
> > > +                                         NULL);
> > >       if (status) {
> > >               dprintk("NFSD: %s: couldn't process src stateid!\n", __func__);
> > >               goto out;
> > >       }
> > >
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > > -                                         dst_stateid, WR_STATE, dst, NULL);
> > > +                                         dst_stateid, WR_STATE, dst, NULL,
> > > +                                         NULL);
> > >       if (status) {
> > >               dprintk("NFSD: %s: couldn't process dst stateid!\n", __func__);
> > >               goto out_put_src;
> > > @@ -1350,7 +1353,7 @@ struct nfsd4_copy *
> > >
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > >                                           &fallocate->falloc_stateid,
> > > -                                         WR_STATE, &file, NULL);
> > > +                                         WR_STATE, &file, NULL, NULL);
> > >       if (status != nfs_ok) {
> > >               dprintk("NFSD: nfsd4_fallocate: couldn't process stateid!\n");
> > >               return status;
> > > @@ -1409,7 +1412,7 @@ struct nfsd4_copy *
> > >
> > >       status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > >                                           &seek->seek_stateid,
> > > -                                         RD_STATE, &file, NULL);
> > > +                                         RD_STATE, &file, NULL, NULL);
> > >       if (status) {
> > >               dprintk("NFSD: nfsd4_seek: couldn't process stateid!\n");
> > >               return status;
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 07a57d0..e263fd0 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -5159,7 +5159,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> > >  __be32
> > >  nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> > >               struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> > > -             stateid_t *stateid, int flags, struct file **filpp, bool *tmp_file)
> > > +             stateid_t *stateid, int flags, struct file **filpp,
> > > +             bool *tmp_file, struct nfs4_stid **cstid)
> > >  {
> > >       struct inode *ino = d_inode(fhp->fh_dentry);
> > >       struct net *net = SVC_NET(rqstp);
> > > @@ -5210,8 +5211,11 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> > >       if (!status && filpp)
> > >               status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
> > >  out:
> > > -     if (s)
> > > +     if (s) {
> > > +             if (!status && cstid)
> > > +                     *cstid = s;
> > >               nfs4_put_stid(s);
> > > +     }
> > >       return status;
> > >  }
> > >
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index 6aacb32..304de3b 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -606,7 +606,8 @@ struct nfsd4_blocked_lock {
> > >
> > >  extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> > >               struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
> > > -             stateid_t *stateid, int flags, struct file **filp, bool *tmp_file);
> > > +             stateid_t *stateid, int flags, struct file **filp,
> > > +             bool *tmp_file, struct nfs4_stid **cstid);
> > >  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> > >                    stateid_t *stateid, unsigned char typemask,
> > >                    struct nfs4_stid **s, struct nfsd_net *nn);
> > > --
> > > 1.8.3.1

Re: [PATCH v1 09/13] NFSD add COPY_NOTIFY operation

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:29:01AM -0400, Olga Kornievskaia wrote:
> @@ -1345,6 +1347,44 @@ struct nfsd4_copy *
>  }
>  
>  static __be32
> +nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> +		  union nfsd4_op_u *u)
> +{
> +	struct nfsd4_copy_notify *cn = &u->copy_notify;
> +	__be32 status;
> +	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> +	struct nfs4_stid *stid;
> +	struct nfs4_cpntf_state *cps;
> +
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> +					&cn->cpn_src_stateid, RD_STATE, NULL,
> +					NULL, &stid);
> +	if (status)
> +		return status;
> +
> +	cn->cpn_sec = nn->nfsd4_lease;
> +	cn->cpn_nsec = 0;
> +
> +	status = nfserrno(-ENOMEM);
> +	cps = nfs4_alloc_init_cpntf_state(nn, stid);
> +	if (!cps)
> +		return status;
> +	memcpy(&cn->cpn_cnr_stateid, &cps->cp_stateid, sizeof(stateid_t));
> +
> +	/**
> +	 * For now, only return one server address in cpn_src, the
> +	 * address used by the client to connect to this server.
> +	 */
> +	cn->cpn_src.nl4_type = NL4_NETADDR;
> +	status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr,
> +				 &cn->cpn_src.u.nl4_addr);
> +	if (status != 0)
> +		nfs4_free_cpntf_state(cps);

It looks like this would free cps while it was still on the parent's
sc_cp_list.

Is an error actually possible here? If not, just remove this check or
replace it by WARN_ON_ONCE(status).

> +
> +	return status;
> +}
...
> @@ -2720,6 +2775,12 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
>  		.op_name = "OP_OFFLOAD_CANCEL",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> +	[OP_COPY_NOTIFY] = {
> +		.op_func = nfsd4_copy_notify,
> +		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,

CACHEME is actually only used for 4.0, let's drop it from
nfs4.1/4.2-only operations.

> +		.op_name = "OP_COPY_NOTIFY",
> +		.op_rsize_bop = nfsd4_copy_notify_rsize,
> +	},
>  };
>  
>  /**
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e263fd0..7764a8b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -697,6 +697,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>  	/* Will be incremented before return to client: */
>  	refcount_set(&stid->sc_count, 1);
>  	spin_lock_init(&stid->sc_lock);
> +	INIT_LIST_HEAD(&stid->sc_cp_list);
>  
>  	/*
>  	 * It shouldn't be a problem to reuse an opaque stateid value.
> @@ -716,33 +717,85 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>  /*
>   * Create a unique stateid_t to represent each COPY.
>   */
> -int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> +int _nfs4_init_state(struct nfsd_net *nn, void *ptr, stateid_t *stid)

This is still copy-specific code, so the names might be more
helpful if they left that in.  Maybe:

	_nfs4_init_state -> nfs4_init_cp_state
	nfs4_init_cp_state -> nfs4_init_copy_state
	nfs4_init_cp_state -> nfs4_init_cpnotify_state

?  Unless you can think of something better.

>  {
>  	int new_id;
>  
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(&nn->s2s_cp_lock);
> -	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, copy, 0, 0, GFP_NOWAIT);
> +	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, ptr, 0, 0, GFP_NOWAIT);
>  	spin_unlock(&nn->s2s_cp_lock);
>  	idr_preload_end();
>  	if (new_id < 0)
>  		return 0;
> -	copy->cp_stateid.si_opaque.so_id = new_id;
> -	copy->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
> -	copy->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> +	stid->si_opaque.so_id = new_id;
> +	stid->si_opaque.so_clid.cl_boot = nn->boot_time;
> +	stid->si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
>  	return 1;
>  }
>  
> -void nfs4_free_cp_state(struct nfsd4_copy *copy)
> +int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> +{
> +	return _nfs4_init_state(nn, copy, &copy->cp_stateid);
> +}
> +
> +struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
> +						     struct nfs4_stid *p_stid)
> +{
> +	struct nfs4_cpntf_state *cps;
> +
> +	cps = kzalloc(sizeof(struct nfs4_cpntf_state), GFP_KERNEL);
> +	if (!cps)
> +		return NULL;
> +	if (!_nfs4_init_state(nn, cps, &cps->cp_stateid))
> +		goto out_free;
> +	cps->cp_p_stid = p_stid;
> +	cps->cp_active = false;
> +	cps->cp_timeout = jiffies + (nn->nfsd4_lease * HZ);
> +	INIT_LIST_HEAD(&cps->cp_list);
> +	list_add(&cps->cp_list, &p_stid->sc_cp_list);

What prevents concurrent nfs4_alloc_init_cpntf_state()s from running and
corrupting this list?

> +
> +	return cps;
> +out_free:
> +	kfree(cps);
> +	return NULL;
> +}
> +
> +void _nfs4_free_state(struct net *n, stateid_t *stid)

Ditto on the naming.

>  {
>  	struct nfsd_net *nn;
>  
> -	nn = net_generic(copy->cp_clp->net, nfsd_net_id);
> +	nn = net_generic(n, nfsd_net_id);
>  	spin_lock(&nn->s2s_cp_lock);
> -	idr_remove(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
> +	idr_remove(&nn->s2s_cp_stateids, stid->si_opaque.so_id);
>  	spin_unlock(&nn->s2s_cp_lock);
>  }
>  
> +void nfs4_free_cp_state(struct nfsd4_copy *copy)
> +{
> +	_nfs4_free_state(copy->cp_clp->net, &copy->cp_stateid);
> +}
> +
> +void nfs4_free_cpntf_state(struct nfs4_cpntf_state *cps)
> +{
> +	_nfs4_free_state(cps->cp_p_stid->sc_client->net, &cps->cp_stateid);
> +	kfree(cps);
> +}
> +
> +static void nfs4_free_cpntf_statelist(struct nfs4_stid *stid)
> +{
> +	struct nfs4_cpntf_state *cps;
> +
> +	might_sleep();
> +
> +	while (!list_empty(&stid->sc_cp_list)) {
> +		cps = list_first_entry(&stid->sc_cp_list,
> +				       struct nfs4_cpntf_state, cp_list);
> +		list_del(&cps->cp_list);
> +		nfs4_free_cpntf_state(cps);
> +	}

Same question on concurrent modifications of this lock--do we need some
more locking?

--b.

Re: [PATCH v1 10/13] NFSD check stateids against copy stateids

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:29:02AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Incoming stateid (used by a READ) could be a saved copy stateid.
> On first use make it active and check that the copy has started
> within the allowable lease time.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/nfs4state.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7764a8b..16359de 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5206,6 +5206,47 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  
>  	return 0;
>  }
> +/*
> + * A READ from an inter server to server COPY will have a
> + * copy stateid. Return the parent nfs4_stid.
> + */
> +static __be32 _find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> +		     struct nfs4_cpntf_state **cps)
> +{
> +	struct nfs4_cpntf_state *state = NULL;
> +
> +	if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
> +		return nfserr_bad_stateid;
> +	spin_lock(&nn->s2s_cp_lock);
> +	state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
> +	spin_unlock(&nn->s2s_cp_lock);
> +	if (!state)
> +		return nfserr_bad_stateid;
> +	*cps = state;
> +	return 0;
> +}
> +
> +static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> +			       struct nfs4_stid **stid)
> +{
> +	__be32 status;
> +	struct nfs4_cpntf_state *cps = NULL;
> +
> +	status = _find_cpntf_state(nn, st, &cps);
> +	if (status)
> +		return status;
> +
> +	/* Did the inter server to server copy start in time? */
> +	if (cps->cp_active == false && !time_after(cps->cp_timeout, jiffies))

Is there any other time limit, or can the destination server keeping
using this stateid indefinitely as long as it manages to get its first
READ in before cp_timeout?

> +		return nfserr_partner_no_auth;
> +	else
> +		cps->cp_active = true;
> +
> +	*stid = cps->cp_p_stid;
> +	refcount_inc(&cps->cp_p_stid->sc_count);

Does the caller hold some lock?  If not, what's prevnting cps from being
freed before we get around to incrementing sc_count here?

I would have expected the refcount_inc to happen before we drop
s2s_cp_lock, but, like I say, maybe I'm missing some other locking.

--b.

> +
> +	return nfs_ok;
> +}
>  
>  /*
>   * Checks for stateid operations
> @@ -5238,6 +5279,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	status = nfsd4_lookup_stateid(cstate, stateid,
>  				NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
>  				&s, nn);
> +	if (status == nfserr_bad_stateid)
> +		status = find_cpntf_state(nn, stateid, &s);
>  	if (status)
>  		return status;
>  	status = nfsd4_stid_check_stateid_generation(stateid, s,
> -- 
> 1.8.3.1

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> The inter server to server COPY source server filehandle
> is a foreign filehandle as the COPY is sent to the destination
> server.

Compounds can do a lot of different strange things, and I'm not
convinced this code handles every case correctly.  Examples: I think
that

	PUTFH
	TEST_STATEID
	SAVEFH
	COPY

will incorrectly return nfserr_stale if the PUTHF gets a foreign
filehandle, even though that filehandle is only used as the source of
the COPY.  And:

	PUTFH
	SAVEFH
	RENAME
	COPY

will pass an unverified source filehandle to rename.

I can think of a couple ways to get this right for certain:

	- delay all filehandle verification till the time the filehandle
	  isused.  That would make checking this simple, but it would
	  change our behavior so, for example PUTFH+READ with a bad
	  filehandle will return the error on the READ where it used to
	  return it on the PUTFH.  I don't know if that's a problem.

	- somewhere at the start of nfsd4_proc_compound, do one pass
	  through the compound checking where the filehandles will be
	  used and marking those ops that can skip checking.  E.g.:

		nfsd4_op *current, *saved

		foreach op in compound:
			- if op is putfh:
				current := op
			- if op is savefh:
				saved := current
			- if op is restorefh:
				current := saved
			- etc.
			- if op is copy:
				mark_no_verify(saved)

	  Or something like that.

--b.

> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfsd/Kconfig    | 10 ++++++++++
>  fs/nfsd/nfs4proc.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  fs/nfsd/nfsfh.h    |  5 ++++-
>  fs/nfsd/xdr4.h     |  1 +
>  4 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index 20b1c17..37ff3d5 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -131,6 +131,16 @@ config NFSD_FLEXFILELAYOUT
>  
>  	  If unsure, say N.
>  
> +config NFSD_V4_2_INTER_SSC
> +	bool "NFSv4.2 inter server to server COPY"
> +	depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
> +	help
> +	  This option enables support for NFSv4.2 inter server to
> +	  server copy where the destination server calls the NFSv4.2
> +	  client to read the data to copy from the source server.
> +
> +	  If unsure, say N.
> +
>  config NFSD_V4_SECURITY_LABEL
>  	bool "Provide Security Label support for NFSv4 server"
>  	depends on NFSD_V4 && SECURITY
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 43a83c7..59e9d0c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -503,12 +503,21 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
>  	    union nfsd4_op_u *u)
>  {
>  	struct nfsd4_putfh *putfh = &u->putfh;
> +	__be32 ret;
>  
>  	fh_put(&cstate->current_fh);
>  	cstate->current_fh.fh_handle.fh_size = putfh->pf_fhlen;
>  	memcpy(&cstate->current_fh.fh_handle.fh_base, putfh->pf_fhval,
>  	       putfh->pf_fhlen);
> -	return fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> +	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> +	if (ret == nfserr_stale && HAS_CSTATE_FLAG(cstate, NO_VERIFY_FH)) {
> +		CLEAR_CSTATE_FLAG(cstate, NO_VERIFY_FH);
> +		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
> +		ret = 0;
> +	}
> +#endif
> +	return ret;
>  }
>  
>  static __be32
> @@ -1957,6 +1966,26 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>  		- rqstp->rq_auth_slack;
>  }
>  
> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> +static bool _compound_contains_inter_copy(struct nfsd4_op *ops, int start,
> +					  int end)
> +{
> +	bool found = false;
> +	struct nfsd4_copy *copy;
> +	int i;
> +
> +	for (i = start; i < end; i++) {
> +		if (ops[i].opnum == OP_COPY) {
> +			copy = (struct nfsd4_copy *)&ops[i].u;
> +			if (copy->cp_src)
> +				found = true;
> +			break;
> +		}
> +	}
> +	return found;
> +}
> +#endif
> +
>  /*
>   * COMPOUND call.
>   */
> @@ -2019,13 +2048,23 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>  				op->status = nfsd4_open_omfg(rqstp, cstate, op);
>  			goto encode_op;
>  		}
> +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> +		if (op->opnum == OP_PUTFH &&
> +			args->ops[resp->opcnt].opnum == OP_SAVEFH &&
> +			args->ops[resp->opcnt+1].opnum == OP_PUTFH &&
> +			_compound_contains_inter_copy(args->ops, resp->opcnt+2,
> +						      args->opcnt))
> +			SET_CSTATE_FLAG(cstate, NO_VERIFY_FH);
> +#endif
>  
> -		if (!current_fh->fh_dentry) {
> +		if (!current_fh->fh_dentry &&
> +				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
>  			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
>  				op->status = nfserr_nofilehandle;
>  				goto encode_op;
>  			}
> -		} else if (current_fh->fh_export->ex_fslocs.migrated &&
> +		} else if (current_fh->fh_export &&
> +			   current_fh->fh_export->ex_fslocs.migrated &&
>  			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
>  			op->status = nfserr_moved;
>  			goto encode_op;
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 755e256..b9c7568 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -35,7 +35,7 @@ static inline ino_t u32_to_ino_t(__u32 uino)
>  
>  	bool			fh_locked;	/* inode locked by us */
>  	bool			fh_want_write;	/* remount protection taken */
> -
> +	int			fh_flags;	/* FH flags */
>  #ifdef CONFIG_NFSD_V3
>  	bool			fh_post_saved;	/* post-op attrs saved */
>  	bool			fh_pre_saved;	/* pre-op attrs saved */
> @@ -56,6 +56,9 @@ static inline ino_t u32_to_ino_t(__u32 uino)
>  #endif /* CONFIG_NFSD_V3 */
>  
>  } svc_fh;
> +#define NFSD4_FH_FOREIGN (1<<0)
> +#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
> +#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
>  
>  enum nfsd_fsid {
>  	FSID_DEV = 0,
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 4a1e53d..c98ef64 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -45,6 +45,7 @@
>  
>  #define CURRENT_STATE_ID_FLAG (1<<0)
>  #define SAVED_STATE_ID_FLAG (1<<1)
> +#define NO_VERIFY_FH (1<<2)
>  
>  #define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
>  #define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
> -- 
> 1.8.3.1

Re: [PATCH v1 13/13] NFSD add nfs4 inter ssc to nfsd4_copy

[J. Bruce Fields]

On Fri, Oct 19, 2018 at 11:29:05AM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> 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@netapp.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  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.

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.)

> +
> +/**
> + * 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?
That would make this WARN() triggerable by a client--that's bad.

> +		goto out_err;
> +	}
> +
> +	naddr = &nss->u.nl4_addr;
> +
> +	tmp_addrlen = rpc_uaddr2sockaddr(SVC_NET(rqstp), naddr->addr,
> +					 naddr->addr_len,
> +					 (struct sockaddr *)&tmp_addr,
> +					 sizeof(tmp_addr));
> +	if (tmp_addrlen == 0)
> +		goto out_err;
> +
> +	if (tmp_addr.ss_family == AF_INET6) {
> +		startsep = "[";
> +		endsep = "]";
> +		match_netid = "tcp6";
> +		match_netid_len = 4;
> +	}
> +
> +	if (naddr->netid_len != match_netid_len ||
> +		strncmp(naddr->netid, match_netid, naddr->netid_len))

Just strcmp(naddr->netid, match_netid) would do the job.

> +		goto out_err;
> +
> +	/* Construct the raw data for the vfs_kern_mount call */
> +	len = RPC_MAX_ADDRBUFLEN + 1;
> +	ipaddr = kzalloc(len, GFP_KERNEL);
> +	if (!ipaddr)
> +		goto out_err;
> +
> +	rpc_ntop((struct sockaddr *)&tmp_addr, ipaddr, len);
> +
> +	/* 2 for ipv6 endsep and startsep. 3 for ":/" and trailing '/0'*/
> +	ipaddr2 = kzalloc(len + 5, GFP_KERNEL);
> +	if (!ipaddr2)
> +		goto out_free_ipaddr;
> +
> +	rpc_ntop((struct sockaddr *)&rqstp->rq_daddr, ipaddr2, len + 5);

Replace the above by two calls to a function that does kmalloc+rpcntop?
(Though actually I don't think we need ipaddr.)

> +
> +	raw_len = strlen(NFSD42_INTERSSC_MOUNTOPS) + strlen(ipaddr) +
> +			strlen(ipaddr2);
> +	raw_data = kzalloc(raw_len, GFP_KERNEL);
> +	if (!raw_data)
> +		goto out_free_ipaddr2;
> +
> +	snprintf(raw_data, raw_len, NFSD42_INTERSSC_MOUNTOPS, ipaddr,
> +		 ipaddr2);
> +
> +	status = -ENODEV;
> +	type = get_fs_type("nfs");
> +	if (!type)
> +		goto out_free_rawdata;

I believe you also need a put_filesystem after this.  (e.g. see
kernel/trace/trace.c:trace_automount().)

> +
> +	/* Set the server:<export> for the vfs_kerne_mount call */
> +	memset(ipaddr2, 0, len + 5);
> +	snprintf(ipaddr2, len + 5, "%s%s%s:/", startsep, ipaddr, endsep);
> +
> +	dprintk("%s  Raw mount data:  %s server:export %s\n", __func__,
> +		raw_data, ipaddr2);
> +
> +	/* Use an 'internal' mount: MS_KERNMOUNT -> MNT_INTERNAL */
> +	ss_mnt = vfs_kern_mount(type, MS_KERNMOUNT, ipaddr2, raw_data);
> +	if (IS_ERR(ss_mnt)) {
> +		status = PTR_ERR(ss_mnt);
> +		goto out_free_rawdata;
> +	}
> +

Let's combine the successful and failure cases, so the below should be
something like:

	out_free_rawdata:
		kfree(raw_data);
	out_free_ipaddr2:
		kfree(ipaddr2);
	out_free_ipaddr:
		kfree(ipaddr);
	out_err:
		if (IS_ERR(ret))
			dprintk("--> %s ERROR %d\n", __func__, status);
		return ret;


> +	kfree(raw_data);
> +	kfree(ipaddr2);
> +	kfree(ipaddr);
> +
> +	return ss_mnt;
> +
> +out_free_rawdata:
> +	kfree(raw_data);
> +out_free_ipaddr2:
> +	kfree(ipaddr2);
> +out_free_ipaddr:
> +	kfree(ipaddr);
> +out_err:
> +	dprintk("--> %s ERROR %d\n", __func__, status);
> +	return ERR_PTR(status);
> +}
> +
> +static void
> +nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> +{
> +	nfs_sb_deactive(ss_mnt->mnt_sb);
> +	mntput(ss_mnt);
> +}
> +
> +/**
> + * nfsd4_setup_inter_ssc
> + *
> + * Verify COPY destination stateid.
> + * Connect to the source server with NFSv4.1.
> + * Create the source struct file for nfsd_copy_range.
> + * Called with COPY cstate:
> + *    SAVED_FH: source filehandle
> + *    CURRENT_FH: destination filehandle
> + *
> + * Returns errno (not nfserrxxx)
> + */
> +static struct vfsmount *
> +nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
> +		      struct nfsd4_compound_state *cstate,
> +		      struct nfsd4_copy *copy)
> +{
> +	struct svc_fh *s_fh = NULL;
> +	stateid_t *s_stid = &copy->cp_src_stateid;
> +	struct vfsmount *ss_mnt;
> +	__be32 status;
> +
> +	/* Verify the destination stateid and set dst struct file*/
> +	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> +					    &copy->cp_dst_stateid,
> +					    WR_STATE, &copy->file_dst, NULL,
> +					    NULL);
> +	if (status) {
> +		ss_mnt = ERR_PTR(be32_to_cpu(status));

That looks wrong.  I don't think IS_ERR() is going to be true for that
value.

If we need to return either an nfserr or a pointer, best is probably to
have the function return __be32 and have the pointer returned in an
argument.

(Thought I notice the only caller ignores the error value, I wonder if
that's right.)

> +		goto out;
> +	}
> +
> +	ss_mnt = nfsd4_interssc_connect(copy->cp_src, rqstp);
> +	if (IS_ERR(ss_mnt))
> +		goto out;

So this function can return -ERRNO, or nfserr_*, or a pointer?  That
won't work.

> +	s_fh = &cstate->save_fh;
> +
> +	copy->c_fh.size = s_fh->fh_handle.fh_size;
> +	memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
> +	copy->stateid.seqid = s_stid->si_generation;
> +	memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
> +	       sizeof(stateid_opaque_t));
> +
> +out:
> +	return ss_mnt;
> +}
> +
> +static void
> +nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
> +			struct file *dst)
> +{
> +	nfs42_ssc_close(src);
> +	fput(src);
> +	fput(dst);
> +	mntput(ss_mnt);
> +}
> +
> +#else /* CONFIG_NFSD_V4_2_INTER_SSC */
> +
> +static struct vfsmount *
> +nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
> +		      struct nfsd4_compound_state *cstate,
> +		      struct nfsd4_copy *copy)
> +{
> +	return ERR_PTR(-EINVAL);

I wonder if that's really the right error for the
server-to-server-copy-unsupported case.

> +}
> +
> +static void
> +nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
> +			struct file *dst)
> +{
> +}
> +
> +static void
> +nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> +{
> +}
> +
> +static struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> +				   struct nfs_fh *src_fh,
> +				   nfs4_stateid *stateid)
> +{
> +	return NULL;
> +}
> +#endif /* CONFIG_NFSD_V4_2_INTER_SSC */
> +
> +static __be32
> +nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> +		      struct nfsd4_compound_state *cstate,
> +		      struct nfsd4_copy *copy)
> +{
> +	return nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> +				 &copy->file_src, &copy->cp_dst_stateid,
> +				 &copy->file_dst, NULL);
> +}
> +
> +static void
> +nfsd4_cleanup_intra_ssc(struct file *src, struct file *dst)
> +{
> +	fput(src);
> +	fput(dst);
> +}
>  
>  static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>  {
> @@ -1217,12 +1440,16 @@ static __be32 nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
>  		status = nfs_ok;
>  	}
>  
> -	fput(copy->file_src);
> -	fput(copy->file_dst);
> +	if (copy->cp_src) /* Inter server SSC */
> +		nfsd4_cleanup_inter_ssc(copy->ss_mnt, copy->file_src,
> +					copy->file_dst);
> +	else
> +		nfsd4_cleanup_intra_ssc(copy->file_src, copy->file_dst);
> +
>  	return status;
>  }
>  
> -static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> +static int dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>  {
>  	dst->cp_src_pos = src->cp_src_pos;
>  	dst->cp_dst_pos = src->cp_dst_pos;
> @@ -1232,8 +1459,21 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>  	memcpy(&dst->fh, &src->fh, sizeof(src->fh));
>  	dst->cp_clp = src->cp_clp;
>  	dst->file_dst = get_file(src->file_dst);
> -	dst->file_src = get_file(src->file_src);
> +	if (!src->cp_src) /* for inter, file_src doesnt exist yet */
> +		dst->file_src = get_file(src->file_src);
>  	memcpy(&dst->cp_stateid, &src->cp_stateid, sizeof(src->cp_stateid));
> +	if (src->cp_src) {
> +		dst->cp_src = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
> +		if (!dst->cp_src)
> +			return -ENOMEM;
> +		memcpy(dst->cp_src, src->cp_src, sizeof(struct nl4_server));
> +	}
> +	memcpy(&dst->stateid, &src->stateid, sizeof(src->stateid));
> +	memcpy(&dst->c_fh, &src->c_fh, sizeof(src->c_fh));
> +	dst->ss_mnt = src->ss_mnt;
> +
> +	return 0;
> +
>  }
>  
>  static void cleanup_async_copy(struct nfsd4_copy *copy)
> @@ -1244,6 +1484,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
>  	spin_lock(&copy->cp_clp->async_lock);
>  	list_del(&copy->copies);
>  	spin_unlock(&copy->cp_clp->async_lock);
> +	kfree(copy->cp_src);
>  	nfs4_put_copy(copy);
>  }
>  
> @@ -1252,7 +1493,18 @@ static int nfsd4_do_async_copy(void *data)
>  	struct nfsd4_copy *copy = (struct nfsd4_copy *)data;
>  	struct nfsd4_copy *cb_copy;
>  
> +	if (copy->cp_src) { /* Inter server SSC */
> +		copy->file_src = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
> +					      &copy->stateid);
> +		if (IS_ERR(copy->file_src)) {
> +			copy->nfserr = nfserr_offload_denied;
> +			nfsd4_interssc_disconnect(copy->ss_mnt);
> +			goto do_callback;
> +		}
> +	}
> +
>  	copy->nfserr = nfsd4_do_copy(copy, 0);
> +do_callback:
>  	cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>  	if (!cb_copy)
>  		goto out;
> @@ -1276,11 +1528,19 @@ static int nfsd4_do_async_copy(void *data)
>  	__be32 status;
>  	struct nfsd4_copy *async_copy = NULL;
>  
> -	status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> -				   &copy->file_src, &copy->cp_dst_stateid,
> -				   &copy->file_dst, NULL);
> -	if (status)
> -		goto out;
> +	if (copy->cp_src) { /* Inter server SSC */
> +		if (!inter_copy_offload_enable || copy->cp_synchronous) {
> +			status = nfserr_notsupp;
> +			goto out;
> +		}
> +		copy->ss_mnt = nfsd4_setup_inter_ssc(rqstp, cstate, copy);
> +		if (IS_ERR(copy->ss_mnt))
> +			return nfserr_offload_denied;

We should check that this is the right error to return in all those
failure cases.

That's all I have for now.

--b.

> +	} else {
> +		status = nfsd4_setup_intra_ssc(rqstp, cstate, copy);
> +		if (status)
> +			return status;
> +	}
>  
>  	copy->cp_clp = cstate->clp;
>  	memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> @@ -1291,15 +1551,15 @@ static int nfsd4_do_async_copy(void *data)
>  		status = nfserrno(-ENOMEM);
>  		async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
>  		if (!async_copy)
> -			goto out;
> -		if (!nfs4_init_cp_state(nn, copy)) {
> -			kfree(async_copy);
> -			goto out;
> -		}
> +			goto out_err;
> +		if (!nfs4_init_cp_state(nn, copy))
> +			goto out_err;
>  		refcount_set(&async_copy->refcount, 1);
>  		memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid,
>  			sizeof(copy->cp_stateid));
> -		dup_copy_fields(copy, async_copy);
> +		status = dup_copy_fields(copy, async_copy);
> +		if (status)
> +			goto out_err;
>  		async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
>  				async_copy, "%s", "copy thread");
>  		if (IS_ERR(async_copy->copy_task))
> @@ -1310,13 +1570,17 @@ static int nfsd4_do_async_copy(void *data)
>  		spin_unlock(&async_copy->cp_clp->async_lock);
>  		wake_up_process(async_copy->copy_task);
>  		status = nfs_ok;
> -	} else
> +	} else {
>  		status = nfsd4_do_copy(copy, 1);
> +	}
>  out:
>  	return status;
>  out_err:
>  	cleanup_async_copy(async_copy);
> -	goto out;
> +	status = nfserrno(-ENOMEM);
> +	if (copy->cp_src)
> +		nfsd4_interssc_disconnect(copy->ss_mnt);
> +	goto out_err;
>  }
>  
>  struct nfsd4_copy *
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 89cb484..9d254e7 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -30,6 +30,12 @@
>  
>  #define NFSDDBG_FACILITY	NFSDDBG_SVC
>  
> +bool inter_copy_offload_enable;
> +EXPORT_SYMBOL_GPL(inter_copy_offload_enable);
> +module_param(inter_copy_offload_enable, bool, 0644);
> +MODULE_PARM_DESC(inter_copy_offload_enable,
> +		 "Enable inter server to server copy offload. Default: false");
> +
>  extern struct svc_program	nfsd_program;
>  static int			nfsd(void *vrqstp);
>  
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index c98ef64..c7e3df1 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -546,7 +546,12 @@ struct nfsd4_copy {
>  	struct task_struct	*copy_task;
>  	refcount_t		refcount;
>  	bool			stopped;
> +
> +	struct vfsmount		*ss_mnt;
> +	struct nfs_fh		c_fh;
> +	nfs4_stateid		stateid;
>  };
> +extern bool inter_copy_offload_enable;
>  
>  struct nfsd4_seek {
>  	/* request */
> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> index 4d76f87..e53a261 100644
> --- a/include/linux/nfs4.h
> +++ b/include/linux/nfs4.h
> @@ -17,6 +17,7 @@
>  #include <linux/uidgid.h>
>  #include <uapi/linux/nfs4.h>
>  #include <linux/sunrpc/msg_prot.h>
> +#include <linux/nfs.h>
>  
>  enum nfs4_acl_whotype {
>  	NFS4_ACL_WHO_NAMED = 0,
> -- 
> 1.8.3.1

Re: [PATCH v1 09/13] NFSD add COPY_NOTIFY operation

[Olga Kornievskaia]

On Mon, Nov 5, 2018 at 12:50 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:29:01AM -0400, Olga Kornievskaia wrote:
> > @@ -1345,6 +1347,44 @@ struct nfsd4_copy *
> >  }
> >
> >  static __be32
> > +nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > +               union nfsd4_op_u *u)
> > +{
> > +     struct nfsd4_copy_notify *cn = &u->copy_notify;
> > +     __be32 status;
> > +     struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > +     struct nfs4_stid *stid;
> > +     struct nfs4_cpntf_state *cps;
> > +
> > +     status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > +                                     &cn->cpn_src_stateid, RD_STATE, NULL,
> > +                                     NULL, &stid);
> > +     if (status)
> > +             return status;
> > +
> > +     cn->cpn_sec = nn->nfsd4_lease;
> > +     cn->cpn_nsec = 0;
> > +
> > +     status = nfserrno(-ENOMEM);
> > +     cps = nfs4_alloc_init_cpntf_state(nn, stid);
> > +     if (!cps)
> > +             return status;
> > +     memcpy(&cn->cpn_cnr_stateid, &cps->cp_stateid, sizeof(stateid_t));
> > +
> > +     /**
> > +      * For now, only return one server address in cpn_src, the
> > +      * address used by the client to connect to this server.
> > +      */
> > +     cn->cpn_src.nl4_type = NL4_NETADDR;
> > +     status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr,
> > +                              &cn->cpn_src.u.nl4_addr);
> > +     if (status != 0)
> > +             nfs4_free_cpntf_state(cps);
>
> It looks like this would free cps while it was still on the parent's
> sc_cp_list.
>
> Is an error actually possible here? If not, just remove this check or
> replace it by WARN_ON_ONCE(status).

Yes i think it can fail (socket type is not a IPv4/v6. don't know how
real it is that it's something else)  so I need to change it so that
it's freed and removed from the list.

> > +
> > +     return status;
> > +}
> ...
> > @@ -2720,6 +2775,12 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
> >               .op_name = "OP_OFFLOAD_CANCEL",
> >               .op_rsize_bop = nfsd4_only_status_rsize,
> >       },
> > +     [OP_COPY_NOTIFY] = {
> > +             .op_func = nfsd4_copy_notify,
> > +             .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
>
> CACHEME is actually only used for 4.0, let's drop it from
> nfs4.1/4.2-only operations.

Ah well COPY And CLONE had the CACHEME and thus I had it. I'll remove
it. We provably need to remove it from 4.2 ops: COPY, CLONE, ALLOCATE,
DEALLOCATE...

> > +             .op_name = "OP_COPY_NOTIFY",
> > +             .op_rsize_bop = nfsd4_copy_notify_rsize,
> > +     },
> >  };
> >
> >  /**
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e263fd0..7764a8b 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -697,6 +697,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
> >       /* Will be incremented before return to client: */
> >       refcount_set(&stid->sc_count, 1);
> >       spin_lock_init(&stid->sc_lock);
> > +     INIT_LIST_HEAD(&stid->sc_cp_list);
> >
> >       /*
> >        * It shouldn't be a problem to reuse an opaque stateid value.
> > @@ -716,33 +717,85 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
> >  /*
> >   * Create a unique stateid_t to represent each COPY.
> >   */
> > -int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> > +int _nfs4_init_state(struct nfsd_net *nn, void *ptr, stateid_t *stid)
>
> This is still copy-specific code, so the names might be more
> helpful if they left that in.  Maybe:
>
>         _nfs4_init_state -> nfs4_init_cp_state
>         nfs4_init_cp_state -> nfs4_init_copy_state
>         nfs4_init_cp_state -> nfs4_init_cpnotify_state
>
> ?  Unless you can think of something better.

I will change the names.

>
> >  {
> >       int new_id;
> >
> >       idr_preload(GFP_KERNEL);
> >       spin_lock(&nn->s2s_cp_lock);
> > -     new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, copy, 0, 0, GFP_NOWAIT);
> > +     new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, ptr, 0, 0, GFP_NOWAIT);
> >       spin_unlock(&nn->s2s_cp_lock);
> >       idr_preload_end();
> >       if (new_id < 0)
> >               return 0;
> > -     copy->cp_stateid.si_opaque.so_id = new_id;
> > -     copy->cp_stateid.si_opaque.so_clid.cl_boot = nn->boot_time;
> > -     copy->cp_stateid.si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> > +     stid->si_opaque.so_id = new_id;
> > +     stid->si_opaque.so_clid.cl_boot = nn->boot_time;
> > +     stid->si_opaque.so_clid.cl_id = nn->s2s_cp_cl_id;
> >       return 1;
> >  }
> >
> > -void nfs4_free_cp_state(struct nfsd4_copy *copy)
> > +int nfs4_init_cp_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
> > +{
> > +     return _nfs4_init_state(nn, copy, &copy->cp_stateid);
> > +}
> > +
> > +struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
> > +                                                  struct nfs4_stid *p_stid)
> > +{
> > +     struct nfs4_cpntf_state *cps;
> > +
> > +     cps = kzalloc(sizeof(struct nfs4_cpntf_state), GFP_KERNEL);
> > +     if (!cps)
> > +             return NULL;
> > +     if (!_nfs4_init_state(nn, cps, &cps->cp_stateid))
> > +             goto out_free;
> > +     cps->cp_p_stid = p_stid;
> > +     cps->cp_active = false;
> > +     cps->cp_timeout = jiffies + (nn->nfsd4_lease * HZ);
> > +     INIT_LIST_HEAD(&cps->cp_list);
> > +     list_add(&cps->cp_list, &p_stid->sc_cp_list);
>
> What prevents concurrent nfs4_alloc_init_cpntf_state()s from running and
> corrupting this list?

Nothing. I'll introduce a new lock.

>
> > +
> > +     return cps;
> > +out_free:
> > +     kfree(cps);
> > +     return NULL;
> > +}
> > +
> > +void _nfs4_free_state(struct net *n, stateid_t *stid)
>
> Ditto on the naming.
>
> >  {
> >       struct nfsd_net *nn;
> >
> > -     nn = net_generic(copy->cp_clp->net, nfsd_net_id);
> > +     nn = net_generic(n, nfsd_net_id);
> >       spin_lock(&nn->s2s_cp_lock);
> > -     idr_remove(&nn->s2s_cp_stateids, copy->cp_stateid.si_opaque.so_id);
> > +     idr_remove(&nn->s2s_cp_stateids, stid->si_opaque.so_id);
> >       spin_unlock(&nn->s2s_cp_lock);
> >  }
> >
> > +void nfs4_free_cp_state(struct nfsd4_copy *copy)
> > +{
> > +     _nfs4_free_state(copy->cp_clp->net, &copy->cp_stateid);
> > +}
> > +
> > +void nfs4_free_cpntf_state(struct nfs4_cpntf_state *cps)
> > +{
> > +     _nfs4_free_state(cps->cp_p_stid->sc_client->net, &cps->cp_stateid);
> > +     kfree(cps);
> > +}
> > +
> > +static void nfs4_free_cpntf_statelist(struct nfs4_stid *stid)
> > +{
> > +     struct nfs4_cpntf_state *cps;
> > +
> > +     might_sleep();
> > +
> > +     while (!list_empty(&stid->sc_cp_list)) {
> > +             cps = list_first_entry(&stid->sc_cp_list,
> > +                                    struct nfs4_cpntf_state, cp_list);
> > +             list_del(&cps->cp_list);
> > +             nfs4_free_cpntf_state(cps);
> > +     }
>
> Same question on concurrent modifications of this lock--do we need some
> more locking?
>
> --b.

Re: [PATCH v1 10/13] NFSD check stateids against copy stateids

[Olga Kornievskaia]

On Mon, Nov 5, 2018 at 4:33 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:29:02AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Incoming stateid (used by a READ) could be a saved copy stateid.
> > On first use make it active and check that the copy has started
> > within the allowable lease time.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/nfs4state.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 7764a8b..16359de 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5206,6 +5206,47 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> >
> >       return 0;
> >  }
> > +/*
> > + * A READ from an inter server to server COPY will have a
> > + * copy stateid. Return the parent nfs4_stid.
> > + */
> > +static __be32 _find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> > +                  struct nfs4_cpntf_state **cps)
> > +{
> > +     struct nfs4_cpntf_state *state = NULL;
> > +
> > +     if (st->si_opaque.so_clid.cl_id != nn->s2s_cp_cl_id)
> > +             return nfserr_bad_stateid;
> > +     spin_lock(&nn->s2s_cp_lock);
> > +     state = idr_find(&nn->s2s_cp_stateids, st->si_opaque.so_id);
> > +     spin_unlock(&nn->s2s_cp_lock);
> > +     if (!state)
> > +             return nfserr_bad_stateid;
> > +     *cps = state;
> > +     return 0;
> > +}
> > +
> > +static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> > +                            struct nfs4_stid **stid)
> > +{
> > +     __be32 status;
> > +     struct nfs4_cpntf_state *cps = NULL;
> > +
> > +     status = _find_cpntf_state(nn, st, &cps);
> > +     if (status)
> > +             return status;
> > +
> > +     /* Did the inter server to server copy start in time? */
> > +     if (cps->cp_active == false && !time_after(cps->cp_timeout, jiffies))
>
> Is there any other time limit, or can the destination server keeping
> using this stateid indefinitely as long as it manages to get its first
> READ in before cp_timeout?

I don't believe there is anything in the spec that prohibits the
stateid from being used once it was started within a timeout period (a
copy might take a long time). Then it's invalidated either by
OFFLOAD_CANCEL or the parent stateid going away.

>
> > +             return nfserr_partner_no_auth;
> > +     else
> > +             cps->cp_active = true;
> > +
> > +     *stid = cps->cp_p_stid;
> > +     refcount_inc(&cps->cp_p_stid->sc_count);
>
> Does the caller hold some lock?  If not, what's prevnting cps from being
> freed before we get around to incrementing sc_count here?
>
> I would have expected the refcount_inc to happen before we drop
> s2s_cp_lock, but, like I say, maybe I'm missing some other locking.

No you are not missing it, the lock is taken only to find it in the
list. I'll move the refcount_inc into the lock section.

>
> --b.
>
> > +
> > +     return nfs_ok;
> > +}
> >
> >  /*
> >   * Checks for stateid operations
> > @@ -5238,6 +5279,8 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
> >       status = nfsd4_lookup_stateid(cstate, stateid,
> >                               NFS4_DELEG_STID|NFS4_OPEN_STID|NFS4_LOCK_STID,
> >                               &s, nn);
> > +     if (status == nfserr_bad_stateid)
> > +             status = find_cpntf_state(nn, stateid, &s);
> >       if (status)
> >               return status;
> >       status = nfsd4_stid_check_stateid_generation(stateid, s,
> > --
> > 1.8.3.1

Re: [PATCH v1 09/13] NFSD add COPY_NOTIFY operation

[J. Bruce Fields]

On Thu, Nov 08, 2018 at 01:29:26PM -0500, Olga Kornievskaia wrote:
> On Mon, Nov 5, 2018 at 12:50 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:29:01AM -0400, Olga Kornievskaia wrote:
> > > @@ -2720,6 +2775,12 @@ static inline u32 nfsd4_seek_rsize(struct svc_rqst *rqstp, struct nfsd4_op *op)
> > >               .op_name = "OP_OFFLOAD_CANCEL",
> > >               .op_rsize_bop = nfsd4_only_status_rsize,
> > >       },
> > > +     [OP_COPY_NOTIFY] = {
> > > +             .op_func = nfsd4_copy_notify,
> > > +             .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
> >
> > CACHEME is actually only used for 4.0, let's drop it from
> > nfs4.1/4.2-only operations.
> 
> Ah well COPY And CLONE had the CACHEME and thus I had it. I'll remove
> it. We provably need to remove it from 4.2 ops: COPY, CLONE, ALLOCATE,
> DEALLOCATE...

OK, that could be a separate patch.

--b.

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > The inter server to server COPY source server filehandle
> > is a foreign filehandle as the COPY is sent to the destination
> > server.
>
> Compounds can do a lot of different strange things, and I'm not
> convinced this code handles every case correctly.  Examples: I think
> that
>
>         PUTFH
>         TEST_STATEID
>         SAVEFH
>         COPY
>
> will incorrectly return nfserr_stale if the PUTHF gets a foreign
> filehandle, even though that filehandle is only used as the source of
> the COPY.  And:
>
>         PUTFH
>         SAVEFH
>         RENAME
>         COPY
>
> will pass an unverified source filehandle to rename.
>
> I can think of a couple ways to get this right for certain:
>
>         - delay all filehandle verification till the time the filehandle
>           isused.  That would make checking this simple, but it would
>           change our behavior so, for example PUTFH+READ with a bad
>           filehandle will return the error on the READ where it used to
>           return it on the PUTFH.  I don't know if that's a problem.
>
>         - somewhere at the start of nfsd4_proc_compound, do one pass
>           through the compound checking where the filehandles will be
>           used and marking those ops that can skip checking.  E.g.:
>
>                 nfsd4_op *current, *saved
>
>                 foreach op in compound:
>                         - if op is putfh:
>                                 current := op
>                         - if op is savefh:
>                                 saved := current
>                         - if op is restorefh:
>                                 current := saved
>                         - etc.
>                         - if op is copy:
>                                 mark_no_verify(saved)
>
>           Or something like that.

Do you have a preference over the 2 proposed methods? I'm not sure if
there is anything wrong with returning ERR_STALE  on READ instead of
the PUTFH but for historical reasons it seems wrong to change it. Thus
I'd say doing it the 2nd way is better. But then 2nd approach adds an
overhead of going thru operations twice for any compound. Is that
acceptable?

I have to ask: for simplicify can't we just support COPY compound if
and only if it's in a specific order and then only allow it?

>
> --b.
>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/Kconfig    | 10 ++++++++++
> >  fs/nfsd/nfs4proc.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> >  fs/nfsd/nfsfh.h    |  5 ++++-
> >  fs/nfsd/xdr4.h     |  1 +
> >  4 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > index 20b1c17..37ff3d5 100644
> > --- a/fs/nfsd/Kconfig
> > +++ b/fs/nfsd/Kconfig
> > @@ -131,6 +131,16 @@ config NFSD_FLEXFILELAYOUT
> >
> >         If unsure, say N.
> >
> > +config NFSD_V4_2_INTER_SSC
> > +     bool "NFSv4.2 inter server to server COPY"
> > +     depends on NFSD_V4 && NFS_V4_1 && NFS_V4_2
> > +     help
> > +       This option enables support for NFSv4.2 inter server to
> > +       server copy where the destination server calls the NFSv4.2
> > +       client to read the data to copy from the source server.
> > +
> > +       If unsure, say N.
> > +
> >  config NFSD_V4_SECURITY_LABEL
> >       bool "Provide Security Label support for NFSv4 server"
> >       depends on NFSD_V4 && SECURITY
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 43a83c7..59e9d0c 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -503,12 +503,21 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
> >           union nfsd4_op_u *u)
> >  {
> >       struct nfsd4_putfh *putfh = &u->putfh;
> > +     __be32 ret;
> >
> >       fh_put(&cstate->current_fh);
> >       cstate->current_fh.fh_handle.fh_size = putfh->pf_fhlen;
> >       memcpy(&cstate->current_fh.fh_handle.fh_base, putfh->pf_fhval,
> >              putfh->pf_fhlen);
> > -     return fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> > +     ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> > +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > +     if (ret == nfserr_stale && HAS_CSTATE_FLAG(cstate, NO_VERIFY_FH)) {
> > +             CLEAR_CSTATE_FLAG(cstate, NO_VERIFY_FH);
> > +             SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
> > +             ret = 0;
> > +     }
> > +#endif
> > +     return ret;
> >  }
> >
> >  static __be32
> > @@ -1957,6 +1966,26 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
> >               - rqstp->rq_auth_slack;
> >  }
> >
> > +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > +static bool _compound_contains_inter_copy(struct nfsd4_op *ops, int start,
> > +                                       int end)
> > +{
> > +     bool found = false;
> > +     struct nfsd4_copy *copy;
> > +     int i;
> > +
> > +     for (i = start; i < end; i++) {
> > +             if (ops[i].opnum == OP_COPY) {
> > +                     copy = (struct nfsd4_copy *)&ops[i].u;
> > +                     if (copy->cp_src)
> > +                             found = true;
> > +                     break;
> > +             }
> > +     }
> > +     return found;
> > +}
> > +#endif
> > +
> >  /*
> >   * COMPOUND call.
> >   */
> > @@ -2019,13 +2048,23 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
> >                               op->status = nfsd4_open_omfg(rqstp, cstate, op);
> >                       goto encode_op;
> >               }
> > +#ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > +             if (op->opnum == OP_PUTFH &&
> > +                     args->ops[resp->opcnt].opnum == OP_SAVEFH &&
> > +                     args->ops[resp->opcnt+1].opnum == OP_PUTFH &&
> > +                     _compound_contains_inter_copy(args->ops, resp->opcnt+2,
> > +                                                   args->opcnt))
> > +                     SET_CSTATE_FLAG(cstate, NO_VERIFY_FH);
> > +#endif
> >
> > -             if (!current_fh->fh_dentry) {
> > +             if (!current_fh->fh_dentry &&
> > +                             !HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
> >                       if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
> >                               op->status = nfserr_nofilehandle;
> >                               goto encode_op;
> >                       }
> > -             } else if (current_fh->fh_export->ex_fslocs.migrated &&
> > +             } else if (current_fh->fh_export &&
> > +                        current_fh->fh_export->ex_fslocs.migrated &&
> >                         !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
> >                       op->status = nfserr_moved;
> >                       goto encode_op;
> > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > index 755e256..b9c7568 100644
> > --- a/fs/nfsd/nfsfh.h
> > +++ b/fs/nfsd/nfsfh.h
> > @@ -35,7 +35,7 @@ static inline ino_t u32_to_ino_t(__u32 uino)
> >
> >       bool                    fh_locked;      /* inode locked by us */
> >       bool                    fh_want_write;  /* remount protection taken */
> > -
> > +     int                     fh_flags;       /* FH flags */
> >  #ifdef CONFIG_NFSD_V3
> >       bool                    fh_post_saved;  /* post-op attrs saved */
> >       bool                    fh_pre_saved;   /* pre-op attrs saved */
> > @@ -56,6 +56,9 @@ static inline ino_t u32_to_ino_t(__u32 uino)
> >  #endif /* CONFIG_NFSD_V3 */
> >
> >  } svc_fh;
> > +#define NFSD4_FH_FOREIGN (1<<0)
> > +#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
> > +#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
> >
> >  enum nfsd_fsid {
> >       FSID_DEV = 0,
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 4a1e53d..c98ef64 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -45,6 +45,7 @@
> >
> >  #define CURRENT_STATE_ID_FLAG (1<<0)
> >  #define SAVED_STATE_ID_FLAG (1<<1)
> > +#define NO_VERIFY_FH (1<<2)
> >
> >  #define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
> >  #define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
> > --
> > 1.8.3.1

Re: [PATCH v1 13/13] NFSD add nfs4 inter ssc to nfsd4_copy

[Olga Kornievskaia]

On Wed, Nov 7, 2018 at 4:49 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:29:05AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > 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@netapp.com>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  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.

> 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.

>
> > +
> > +/**
> > + * 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.

> That would make this WARN() triggerable by a client--that's bad.

Why? Would you rather it silently failed?

> > +             goto out_err;
> > +     }
> > +
> > +     naddr = &nss->u.nl4_addr;
> > +
> > +     tmp_addrlen = rpc_uaddr2sockaddr(SVC_NET(rqstp), naddr->addr,
> > +                                      naddr->addr_len,
> > +                                      (struct sockaddr *)&tmp_addr,
> > +                                      sizeof(tmp_addr));
> > +     if (tmp_addrlen == 0)
> > +             goto out_err;
> > +
> > +     if (tmp_addr.ss_family == AF_INET6) {
> > +             startsep = "[";
> > +             endsep = "]";
> > +             match_netid = "tcp6";
> > +             match_netid_len = 4;
> > +     }
> > +
> > +     if (naddr->netid_len != match_netid_len ||
> > +             strncmp(naddr->netid, match_netid, naddr->netid_len))
>
> Just strcmp(naddr->netid, match_netid) would do the job.

Will change.

> > +             goto out_err;
> > +
> > +     /* Construct the raw data for the vfs_kern_mount call */
> > +     len = RPC_MAX_ADDRBUFLEN + 1;
> > +     ipaddr = kzalloc(len, GFP_KERNEL);
> > +     if (!ipaddr)
> > +             goto out_err;
> > +
> > +     rpc_ntop((struct sockaddr *)&tmp_addr, ipaddr, len);
> > +
> > +     /* 2 for ipv6 endsep and startsep. 3 for ":/" and trailing '/0'*/
> > +     ipaddr2 = kzalloc(len + 5, GFP_KERNEL);
> > +     if (!ipaddr2)
> > +             goto out_free_ipaddr;
> > +
> > +     rpc_ntop((struct sockaddr *)&rqstp->rq_daddr, ipaddr2, len + 5);
>
> Replace the above by two calls to a function that does kmalloc+rpcntop?
> (Though actually I don't think we need ipaddr.)

Will do.

> > +
> > +     raw_len = strlen(NFSD42_INTERSSC_MOUNTOPS) + strlen(ipaddr) +
> > +                     strlen(ipaddr2);
> > +     raw_data = kzalloc(raw_len, GFP_KERNEL);
> > +     if (!raw_data)
> > +             goto out_free_ipaddr2;
> > +
> > +     snprintf(raw_data, raw_len, NFSD42_INTERSSC_MOUNTOPS, ipaddr,
> > +              ipaddr2);
> > +
> > +     status = -ENODEV;
> > +     type = get_fs_type("nfs");
> > +     if (!type)
> > +             goto out_free_rawdata;
>
> I believe you also need a put_filesystem after this.  (e.g. see
> kernel/trace/trace.c:trace_automount().)

Got it. Thanks.

>
> > +
> > +     /* Set the server:<export> for the vfs_kerne_mount call */
> > +     memset(ipaddr2, 0, len + 5);
> > +     snprintf(ipaddr2, len + 5, "%s%s%s:/", startsep, ipaddr, endsep);
> > +
> > +     dprintk("%s  Raw mount data:  %s server:export %s\n", __func__,
> > +             raw_data, ipaddr2);
> > +
> > +     /* Use an 'internal' mount: MS_KERNMOUNT -> MNT_INTERNAL */
> > +     ss_mnt = vfs_kern_mount(type, MS_KERNMOUNT, ipaddr2, raw_data);
> > +     if (IS_ERR(ss_mnt)) {
> > +             status = PTR_ERR(ss_mnt);
> > +             goto out_free_rawdata;
> > +     }
> > +
>
> Let's combine the successful and failure cases, so the below should be
> something like:
>
>         out_free_rawdata:
>                 kfree(raw_data);
>         out_free_ipaddr2:
>                 kfree(ipaddr2);
>         out_free_ipaddr:
>                 kfree(ipaddr);
>         out_err:
>                 if (IS_ERR(ret))
>                         dprintk("--> %s ERROR %d\n", __func__, status);
>                 return ret;

Ok will do.

>
>
> > +     kfree(raw_data);
> > +     kfree(ipaddr2);
> > +     kfree(ipaddr);
> > +
> > +     return ss_mnt;
> > +
> > +out_free_rawdata:
> > +     kfree(raw_data);
> > +out_free_ipaddr2:
> > +     kfree(ipaddr2);
> > +out_free_ipaddr:
> > +     kfree(ipaddr);
> > +out_err:
> > +     dprintk("--> %s ERROR %d\n", __func__, status);
> > +     return ERR_PTR(status);
> > +}
> > +
> > +static void
> > +nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> > +{
> > +     nfs_sb_deactive(ss_mnt->mnt_sb);
> > +     mntput(ss_mnt);
> > +}
> > +
> > +/**
> > + * nfsd4_setup_inter_ssc
> > + *
> > + * Verify COPY destination stateid.
> > + * Connect to the source server with NFSv4.1.
> > + * Create the source struct file for nfsd_copy_range.
> > + * Called with COPY cstate:
> > + *    SAVED_FH: source filehandle
> > + *    CURRENT_FH: destination filehandle
> > + *
> > + * Returns errno (not nfserrxxx)
> > + */
> > +static struct vfsmount *
> > +nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
> > +                   struct nfsd4_compound_state *cstate,
> > +                   struct nfsd4_copy *copy)
> > +{
> > +     struct svc_fh *s_fh = NULL;
> > +     stateid_t *s_stid = &copy->cp_src_stateid;
> > +     struct vfsmount *ss_mnt;
> > +     __be32 status;
> > +
> > +     /* Verify the destination stateid and set dst struct file*/
> > +     status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> > +                                         &copy->cp_dst_stateid,
> > +                                         WR_STATE, &copy->file_dst, NULL,
> > +                                         NULL);
> > +     if (status) {
> > +             ss_mnt = ERR_PTR(be32_to_cpu(status));
>
> That looks wrong.  I don't think IS_ERR() is going to be true for that
> value.
>
> If we need to return either an nfserr or a pointer, best is probably to
> have the function return __be32 and have the pointer returned in an
> argument.
>
> (Thought I notice the only caller ignores the error value, I wonder if
> that's right.)
>
> > +             goto out;
> > +     }
> > +
> > +     ss_mnt = nfsd4_interssc_connect(copy->cp_src, rqstp);
> > +     if (IS_ERR(ss_mnt))
> > +             goto out;
>
> So this function can return -ERRNO, or nfserr_*, or a pointer?  That
> won't work.

Ok I'll change the function to return the __be32 always. And return
pointer will be one of the args.

>
> > +     s_fh = &cstate->save_fh;
> > +
> > +     copy->c_fh.size = s_fh->fh_handle.fh_size;
> > +     memcpy(copy->c_fh.data, &s_fh->fh_handle.fh_base, copy->c_fh.size);
> > +     copy->stateid.seqid = s_stid->si_generation;
> > +     memcpy(copy->stateid.other, (void *)&s_stid->si_opaque,
> > +            sizeof(stateid_opaque_t));
> > +
> > +out:
> > +     return ss_mnt;
> > +}
> > +
> > +static void
> > +nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
> > +                     struct file *dst)
> > +{
> > +     nfs42_ssc_close(src);
> > +     fput(src);
> > +     fput(dst);
> > +     mntput(ss_mnt);
> > +}
> > +
> > +#else /* CONFIG_NFSD_V4_2_INTER_SSC */
> > +
> > +static struct vfsmount *
> > +nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
> > +                   struct nfsd4_compound_state *cstate,
> > +                   struct nfsd4_copy *copy)
> > +{
> > +     return ERR_PTR(-EINVAL);
>
> I wonder if that's really the right error for the
> server-to-server-copy-unsupported case.

Should be not_supported because COPY itself is not supported. If COPY
was supported but failed for whatever reason we couldn't mount then
the error should be OFFLOAD_DENIED.

>
> > +}
> > +
> > +static void
> > +nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *src,
> > +                     struct file *dst)
> > +{
> > +}
> > +
> > +static void
> > +nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
> > +{
> > +}
> > +
> > +static struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
> > +                                struct nfs_fh *src_fh,
> > +                                nfs4_stateid *stateid)
> > +{
> > +     return NULL;
> > +}
> > +#endif /* CONFIG_NFSD_V4_2_INTER_SSC */
> > +
> > +static __be32
> > +nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > +                   struct nfsd4_compound_state *cstate,
> > +                   struct nfsd4_copy *copy)
> > +{
> > +     return nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> > +                              &copy->file_src, &copy->cp_dst_stateid,
> > +                              &copy->file_dst, NULL);
> > +}
> > +
> > +static void
> > +nfsd4_cleanup_intra_ssc(struct file *src, struct file *dst)
> > +{
> > +     fput(src);
> > +     fput(dst);
> > +}
> >
> >  static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> >  {
> > @@ -1217,12 +1440,16 @@ static __be32 nfsd4_do_copy(struct nfsd4_copy *copy, bool sync)
> >               status = nfs_ok;
> >       }
> >
> > -     fput(copy->file_src);
> > -     fput(copy->file_dst);
> > +     if (copy->cp_src) /* Inter server SSC */
> > +             nfsd4_cleanup_inter_ssc(copy->ss_mnt, copy->file_src,
> > +                                     copy->file_dst);
> > +     else
> > +             nfsd4_cleanup_intra_ssc(copy->file_src, copy->file_dst);
> > +
> >       return status;
> >  }
> >
> > -static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > +static int dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> >  {
> >       dst->cp_src_pos = src->cp_src_pos;
> >       dst->cp_dst_pos = src->cp_dst_pos;
> > @@ -1232,8 +1459,21 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> >       memcpy(&dst->fh, &src->fh, sizeof(src->fh));
> >       dst->cp_clp = src->cp_clp;
> >       dst->file_dst = get_file(src->file_dst);
> > -     dst->file_src = get_file(src->file_src);
> > +     if (!src->cp_src) /* for inter, file_src doesnt exist yet */
> > +             dst->file_src = get_file(src->file_src);
> >       memcpy(&dst->cp_stateid, &src->cp_stateid, sizeof(src->cp_stateid));
> > +     if (src->cp_src) {
> > +             dst->cp_src = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
> > +             if (!dst->cp_src)
> > +                     return -ENOMEM;
> > +             memcpy(dst->cp_src, src->cp_src, sizeof(struct nl4_server));
> > +     }
> > +     memcpy(&dst->stateid, &src->stateid, sizeof(src->stateid));
> > +     memcpy(&dst->c_fh, &src->c_fh, sizeof(src->c_fh));
> > +     dst->ss_mnt = src->ss_mnt;
> > +
> > +     return 0;
> > +
> >  }
> >
> >  static void cleanup_async_copy(struct nfsd4_copy *copy)
> > @@ -1244,6 +1484,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
> >       spin_lock(&copy->cp_clp->async_lock);
> >       list_del(&copy->copies);
> >       spin_unlock(&copy->cp_clp->async_lock);
> > +     kfree(copy->cp_src);
> >       nfs4_put_copy(copy);
> >  }
> >
> > @@ -1252,7 +1493,18 @@ static int nfsd4_do_async_copy(void *data)
> >       struct nfsd4_copy *copy = (struct nfsd4_copy *)data;
> >       struct nfsd4_copy *cb_copy;
> >
> > +     if (copy->cp_src) { /* Inter server SSC */
> > +             copy->file_src = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
> > +                                           &copy->stateid);
> > +             if (IS_ERR(copy->file_src)) {
> > +                     copy->nfserr = nfserr_offload_denied;
> > +                     nfsd4_interssc_disconnect(copy->ss_mnt);
> > +                     goto do_callback;
> > +             }
> > +     }
> > +
> >       copy->nfserr = nfsd4_do_copy(copy, 0);
> > +do_callback:
> >       cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >       if (!cb_copy)
> >               goto out;
> > @@ -1276,11 +1528,19 @@ static int nfsd4_do_async_copy(void *data)
> >       __be32 status;
> >       struct nfsd4_copy *async_copy = NULL;
> >
> > -     status = nfsd4_verify_copy(rqstp, cstate, &copy->cp_src_stateid,
> > -                                &copy->file_src, &copy->cp_dst_stateid,
> > -                                &copy->file_dst, NULL);
> > -     if (status)
> > -             goto out;
> > +     if (copy->cp_src) { /* Inter server SSC */
> > +             if (!inter_copy_offload_enable || copy->cp_synchronous) {
> > +                     status = nfserr_notsupp;
> > +                     goto out;
> > +             }
> > +             copy->ss_mnt = nfsd4_setup_inter_ssc(rqstp, cstate, copy);
> > +             if (IS_ERR(copy->ss_mnt))
> > +                     return nfserr_offload_denied;
>
> We should check that this is the right error to return in all those
> failure cases.

Well once I change nfsd4_setup_inter_ssc() to return an error itself
it'll just return status. But I'll double check the error returns.

> That's all I have for now.

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.

>
> --b.
>
> > +     } else {
> > +             status = nfsd4_setup_intra_ssc(rqstp, cstate, copy);
> > +             if (status)
> > +                     return status;
> > +     }
> >
> >       copy->cp_clp = cstate->clp;
> >       memcpy(&copy->fh, &cstate->current_fh.fh_handle,
> > @@ -1291,15 +1551,15 @@ static int nfsd4_do_async_copy(void *data)
> >               status = nfserrno(-ENOMEM);
> >               async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >               if (!async_copy)
> > -                     goto out;
> > -             if (!nfs4_init_cp_state(nn, copy)) {
> > -                     kfree(async_copy);
> > -                     goto out;
> > -             }
> > +                     goto out_err;
> > +             if (!nfs4_init_cp_state(nn, copy))
> > +                     goto out_err;
> >               refcount_set(&async_copy->refcount, 1);
> >               memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid,
> >                       sizeof(copy->cp_stateid));
> > -             dup_copy_fields(copy, async_copy);
> > +             status = dup_copy_fields(copy, async_copy);
> > +             if (status)
> > +                     goto out_err;
> >               async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
> >                               async_copy, "%s", "copy thread");
> >               if (IS_ERR(async_copy->copy_task))
> > @@ -1310,13 +1570,17 @@ static int nfsd4_do_async_copy(void *data)
> >               spin_unlock(&async_copy->cp_clp->async_lock);
> >               wake_up_process(async_copy->copy_task);
> >               status = nfs_ok;
> > -     } else
> > +     } else {
> >               status = nfsd4_do_copy(copy, 1);
> > +     }
> >  out:
> >       return status;
> >  out_err:
> >       cleanup_async_copy(async_copy);
> > -     goto out;
> > +     status = nfserrno(-ENOMEM);
> > +     if (copy->cp_src)
> > +             nfsd4_interssc_disconnect(copy->ss_mnt);
> > +     goto out_err;
> >  }
> >
> >  struct nfsd4_copy *
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 89cb484..9d254e7 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -30,6 +30,12 @@
> >
> >  #define NFSDDBG_FACILITY     NFSDDBG_SVC
> >
> > +bool inter_copy_offload_enable;
> > +EXPORT_SYMBOL_GPL(inter_copy_offload_enable);
> > +module_param(inter_copy_offload_enable, bool, 0644);
> > +MODULE_PARM_DESC(inter_copy_offload_enable,
> > +              "Enable inter server to server copy offload. Default: false");
> > +
> >  extern struct svc_program    nfsd_program;
> >  static int                   nfsd(void *vrqstp);
> >
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index c98ef64..c7e3df1 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -546,7 +546,12 @@ struct nfsd4_copy {
> >       struct task_struct      *copy_task;
> >       refcount_t              refcount;
> >       bool                    stopped;
> > +
> > +     struct vfsmount         *ss_mnt;
> > +     struct nfs_fh           c_fh;
> > +     nfs4_stateid            stateid;
> >  };
> > +extern bool inter_copy_offload_enable;
> >
> >  struct nfsd4_seek {
> >       /* request */
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 4d76f87..e53a261 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -17,6 +17,7 @@
> >  #include <linux/uidgid.h>
> >  #include <uapi/linux/nfs4.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > +#include <linux/nfs.h>
> >
> >  enum nfs4_acl_whotype {
> >       NFS4_ACL_WHO_NAMED = 0,
> > --
> > 1.8.3.1

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

On Thu, Nov 08, 2018 at 01:51:58PM -0500, Olga Kornievskaia wrote:
> On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > The inter server to server COPY source server filehandle
> > > is a foreign filehandle as the COPY is sent to the destination
> > > server.
> >
> > Compounds can do a lot of different strange things, and I'm not
> > convinced this code handles every case correctly.  Examples: I think
> > that
> >
> >         PUTFH
> >         TEST_STATEID
> >         SAVEFH
> >         COPY
> >
> > will incorrectly return nfserr_stale if the PUTHF gets a foreign
> > filehandle, even though that filehandle is only used as the source of
> > the COPY.  And:
> >
> >         PUTFH
> >         SAVEFH
> >         RENAME
> >         COPY
> >
> > will pass an unverified source filehandle to rename.
> >
> > I can think of a couple ways to get this right for certain:
> >
> >         - delay all filehandle verification till the time the filehandle
> >           isused.  That would make checking this simple, but it would
> >           change our behavior so, for example PUTFH+READ with a bad
> >           filehandle will return the error on the READ where it used to
> >           return it on the PUTFH.  I don't know if that's a problem.
> >
> >         - somewhere at the start of nfsd4_proc_compound, do one pass
> >           through the compound checking where the filehandles will be
> >           used and marking those ops that can skip checking.  E.g.:
> >
> >                 nfsd4_op *current, *saved
> >
> >                 foreach op in compound:
> >                         - if op is putfh:
> >                                 current := op
> >                         - if op is savefh:
> >                                 saved := current
> >                         - if op is restorefh:
> >                                 current := saved
> >                         - etc.
> >                         - if op is copy:
> >                                 mark_no_verify(saved)
> >
> >           Or something like that.
> 
> Do you have a preference over the 2 proposed methods? I'm not sure if
> there is anything wrong with returning ERR_STALE  on READ instead of
> the PUTFH but for historical reasons it seems wrong to change it. Thus
> I'd say doing it the 2nd way is better. But then 2nd approach adds an
> overhead of going thru operations twice for any compound. Is that
> acceptable?

I think so.  Most compounds are pretty short and I don't think it'll be
a big deal.

> I have to ask: for simplicify can't we just support COPY compound if
> and only if it's in a specific order and then only allow it?

We could probably narrow the possibilities down to a few, but I'm a
little afraid of overlooking some possible creative client behavior.

I don't think it's that hard to follow the spec here, and it may be
simpler than verifying an argument about which cases matter.

--b.

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[J. Bruce Fields]

On Thu, Nov 08, 2018 at 02:25:02PM -0500, J. Bruce Fields wrote:
> On Thu, Nov 08, 2018 at 01:51:58PM -0500, Olga Kornievskaia wrote:
> > On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > >
> > > > The inter server to server COPY source server filehandle
> > > > is a foreign filehandle as the COPY is sent to the destination
> > > > server.
> > >
> > > Compounds can do a lot of different strange things, and I'm not
> > > convinced this code handles every case correctly.  Examples: I think
> > > that
> > >
> > >         PUTFH
> > >         TEST_STATEID
> > >         SAVEFH
> > >         COPY
> > >
> > > will incorrectly return nfserr_stale if the PUTHF gets a foreign
> > > filehandle, even though that filehandle is only used as the source of
> > > the COPY.  And:
> > >
> > >         PUTFH
> > >         SAVEFH
> > >         RENAME
> > >         COPY
> > >
> > > will pass an unverified source filehandle to rename.
> > >
> > > I can think of a couple ways to get this right for certain:
> > >
> > >         - delay all filehandle verification till the time the filehandle
> > >           isused.  That would make checking this simple, but it would
> > >           change our behavior so, for example PUTFH+READ with a bad
> > >           filehandle will return the error on the READ where it used to
> > >           return it on the PUTFH.  I don't know if that's a problem.
> > >
> > >         - somewhere at the start of nfsd4_proc_compound, do one pass
> > >           through the compound checking where the filehandles will be
> > >           used and marking those ops that can skip checking.  E.g.:
> > >
> > >                 nfsd4_op *current, *saved
> > >
> > >                 foreach op in compound:
> > >                         - if op is putfh:
> > >                                 current := op
> > >                         - if op is savefh:
> > >                                 saved := current
> > >                         - if op is restorefh:
> > >                                 current := saved
> > >                         - etc.
> > >                         - if op is copy:
> > >                                 mark_no_verify(saved)
> > >
> > >           Or something like that.
> > 
> > Do you have a preference over the 2 proposed methods? I'm not sure if
> > there is anything wrong with returning ERR_STALE  on READ instead of
> > the PUTFH but for historical reasons it seems wrong to change it. Thus
> > I'd say doing it the 2nd way is better. But then 2nd approach adds an
> > overhead of going thru operations twice for any compound. Is that
> > acceptable?
> 
> I think so.  Most compounds are pretty short and I don't think it'll be
> a big deal.

So, yes, could you try the second approach?

I've been working on the 1st approach--I have a patch here and I may
experiment with it some more.  But I'm starting to think that the
separate scan will work better.

--b.

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

On Thu, Nov 8, 2018 at 2:28 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Nov 08, 2018 at 02:25:02PM -0500, J. Bruce Fields wrote:
> > On Thu, Nov 08, 2018 at 01:51:58PM -0500, Olga Kornievskaia wrote:
> > > On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > > >
> > > > On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > > >
> > > > > The inter server to server COPY source server filehandle
> > > > > is a foreign filehandle as the COPY is sent to the destination
> > > > > server.
> > > >
> > > > Compounds can do a lot of different strange things, and I'm not
> > > > convinced this code handles every case correctly.  Examples: I think
> > > > that
> > > >
> > > >         PUTFH
> > > >         TEST_STATEID
> > > >         SAVEFH
> > > >         COPY
> > > >
> > > > will incorrectly return nfserr_stale if the PUTHF gets a foreign
> > > > filehandle, even though that filehandle is only used as the source of
> > > > the COPY.  And:
> > > >
> > > >         PUTFH
> > > >         SAVEFH
> > > >         RENAME
> > > >         COPY
> > > >
> > > > will pass an unverified source filehandle to rename.
> > > >
> > > > I can think of a couple ways to get this right for certain:
> > > >
> > > >         - delay all filehandle verification till the time the filehandle
> > > >           isused.  That would make checking this simple, but it would
> > > >           change our behavior so, for example PUTFH+READ with a bad
> > > >           filehandle will return the error on the READ where it used to
> > > >           return it on the PUTFH.  I don't know if that's a problem.
> > > >
> > > >         - somewhere at the start of nfsd4_proc_compound, do one pass
> > > >           through the compound checking where the filehandles will be
> > > >           used and marking those ops that can skip checking.  E.g.:
> > > >
> > > >                 nfsd4_op *current, *saved
> > > >
> > > >                 foreach op in compound:
> > > >                         - if op is putfh:
> > > >                                 current := op
> > > >                         - if op is savefh:
> > > >                                 saved := current
> > > >                         - if op is restorefh:
> > > >                                 current := saved
> > > >                         - etc.
> > > >                         - if op is copy:
> > > >                                 mark_no_verify(saved)
> > > >
> > > >           Or something like that.
> > >
> > > Do you have a preference over the 2 proposed methods? I'm not sure if
> > > there is anything wrong with returning ERR_STALE  on READ instead of
> > > the PUTFH but for historical reasons it seems wrong to change it. Thus
> > > I'd say doing it the 2nd way is better. But then 2nd approach adds an
> > > overhead of going thru operations twice for any compound. Is that
> > > acceptable?
> >
> > I think so.  Most compounds are pretty short and I don't think it'll be
> > a big deal.
>
> So, yes, could you try the second approach?
>
> I've been working on the 1st approach--I have a patch here and I may
> experiment with it some more.  But I'm starting to think that the
> separate scan will work better.
>
> --b.

Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

[Olga Kornievskaia]

On Thu, Nov 8, 2018 at 2:28 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Nov 08, 2018 at 02:25:02PM -0500, J. Bruce Fields wrote:
> > On Thu, Nov 08, 2018 at 01:51:58PM -0500, Olga Kornievskaia wrote:
> > > On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > > >
> > > > On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > > >
> > > > > The inter server to server COPY source server filehandle
> > > > > is a foreign filehandle as the COPY is sent to the destination
> > > > > server.
> > > >
> > > > Compounds can do a lot of different strange things, and I'm not
> > > > convinced this code handles every case correctly.  Examples: I think
> > > > that
> > > >
> > > >         PUTFH
> > > >         TEST_STATEID
> > > >         SAVEFH
> > > >         COPY
> > > >
> > > > will incorrectly return nfserr_stale if the PUTHF gets a foreign
> > > > filehandle, even though that filehandle is only used as the source of
> > > > the COPY.  And:
> > > >
> > > >         PUTFH
> > > >         SAVEFH
> > > >         RENAME
> > > >         COPY
> > > >
> > > > will pass an unverified source filehandle to rename.
> > > >
> > > > I can think of a couple ways to get this right for certain:
> > > >
> > > >         - delay all filehandle verification till the time the filehandle
> > > >           isused.  That would make checking this simple, but it would
> > > >           change our behavior so, for example PUTFH+READ with a bad
> > > >           filehandle will return the error on the READ where it used to
> > > >           return it on the PUTFH.  I don't know if that's a problem.
> > > >
> > > >         - somewhere at the start of nfsd4_proc_compound, do one pass
> > > >           through the compound checking where the filehandles will be
> > > >           used and marking those ops that can skip checking.  E.g.:
> > > >
> > > >                 nfsd4_op *current, *saved
> > > >
> > > >                 foreach op in compound:
> > > >                         - if op is putfh:
> > > >                                 current := op
> > > >                         - if op is savefh:
> > > >                                 saved := current
> > > >                         - if op is restorefh:
> > > >                                 current := saved
> > > >                         - etc.
> > > >                         - if op is copy:
> > > >                                 mark_no_verify(saved)
> > > >
> > > >           Or something like that.
> > >
> > > Do you have a preference over the 2 proposed methods? I'm not sure if
> > > there is anything wrong with returning ERR_STALE  on READ instead of
> > > the PUTFH but for historical reasons it seems wrong to change it. Thus
> > > I'd say doing it the 2nd way is better. But then 2nd approach adds an
> > > overhead of going thru operations twice for any compound. Is that
> > > acceptable?
> >
> > I think so.  Most compounds are pretty short and I don't think it'll be
> > a big deal.
>
> So, yes, could you try the second approach?

Yes of course.

> I've been working on the 1st approach--I have a patch here and I may
> experiment with it some more.  But I'm starting to think that the
> separate scan will work better.
>
> --b.

Re: [PATCH v1 13/13] NFSD add nfs4 inter ssc to nfsd4_copy

[J. Bruce Fields]

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@fieldses.org> wrote:
> >
> > On Fri, Oct 19, 2018 at 11:29:05AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > 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@netapp.com>
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > ---
> > >  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.