Re: [PATCH v2 1/1] NFSD: enhance inter-server copy cleanup

From: Chuck Lever III <hidden>
Date: 2023-01-03 16:08:50

On Dec 18, 2022, at 7:55 PM, Dai Ngo [off-list ref] wrote:

Currently nfsd4_setup_inter_ssc returns the vfsmount of the source
server's export when the mount completes. After the copy is done
nfsd4_cleanup_inter_ssc is called with the vfsmount of the source
server and it searches nfsd_ssc_mount_list for a matching entry
to do the clean up.

The problems with this approach are (1) the need to search the
nfsd_ssc_mount_list and (2) the code has to handle the case where
the matching entry is not found which looks ugly.

The enhancement is instead of nfsd4_setup_inter_ssc returning the
vfsmount, it returns the nfsd4_ssc_umount_item which has the
vfsmount embedded in it. When nfsd4_cleanup_inter_ssc is called
it's passed with the nfsd4_ssc_umount_item directly to do the
clean up so no searching is needed and there is no need to handle
the 'not found' case.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
V2: fix compile error when CONFIG_NFSD_V4_2_INTER_SSC not defined.
   Reported by kernel test robot.

Hello Dai - I've looked at this, nothing to comment on so far. I
plan to go over it again sometime this week.

I'd like to hear from others before applying it.

quoted hunk ↗ jump to hunk

fs/nfsd/nfs4proc.c      | 94 +++++++++++++++++++------------------------------
fs/nfsd/xdr4.h          |  2 +-
include/linux/nfs_ssc.h |  2 +-
3 files changed, 38 insertions(+), 60 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index b79ee65ae016..6515b00520bc 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c

@@ -1295,15 +1295,15 @@ extern void nfs_sb_deactive(struct super_block *sb);
 * setup a work entry in the ssc delayed unmount list.
 */

static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,
-		struct nfsd4_ssc_umount_item **retwork, struct vfsmount **ss_mnt)
+		struct nfsd4_ssc_umount_item **nsui)
{
	struct nfsd4_ssc_umount_item *ni = NULL;
	struct nfsd4_ssc_umount_item *work = NULL;
	struct nfsd4_ssc_umount_item *tmp;
	DEFINE_WAIT(wait);
+	__be32 status = 0;

-	*ss_mnt = NULL;
-	*retwork = NULL;
+	*nsui = NULL;
	work = kzalloc(sizeof(*work), GFP_KERNEL);
try_again:
	spin_lock(&nn->nfsd_ssc_lock);

@@ -1326,12 +1326,12 @@ static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,

			finish_wait(&nn->nfsd_ssc_waitq, &wait);
			goto try_again;
		}
-		*ss_mnt = ni->nsui_vfsmount;
+		*nsui = ni;
		refcount_inc(&ni->nsui_refcnt);
		spin_unlock(&nn->nfsd_ssc_lock);
		kfree(work);

-		/* return vfsmount in ss_mnt */
+		/* return vfsmount in (*nsui)->nsui_vfsmount */
		return 0;
	}
	if (work) {

@@ -1339,10 +1339,11 @@ static __be32 nfsd4_ssc_setup_dul(struct nfsd_net *nn, char *ipaddr,

		refcount_set(&work->nsui_refcnt, 2);
		work->nsui_busy = true;
		list_add_tail(&work->nsui_list, &nn->nfsd_ssc_mount_list);
-		*retwork = work;
-	}
+		*nsui = work;
+	} else
+		status = nfserr_resource;
	spin_unlock(&nn->nfsd_ssc_lock);
-	return 0;
+	return status;
}

static void nfsd4_ssc_update_dul_work(struct nfsd_net *nn,

@@ -1371,7 +1372,7 @@ static void nfsd4_ssc_cancel_dul_work(struct nfsd_net *nn,
 */

static __be32
nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,
-		       struct vfsmount **mount)
+			struct nfsd4_ssc_umount_item **nsui)
{
	struct file_system_type *type;
	struct vfsmount *ss_mnt;

@@ -1382,7 +1383,6 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,

	char *ipaddr, *dev_name, *raw_data;
	int len, raw_len;
	__be32 status = nfserr_inval;
-	struct nfsd4_ssc_umount_item *work = NULL;
	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);

	naddr = &nss->u.nl4_addr;

@@ -1390,6 +1390,7 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,

					 naddr->addr_len,
					 (struct sockaddr *)&tmp_addr,
					 sizeof(tmp_addr));
+	*nsui = NULL;
	if (tmp_addrlen == 0)
		goto out_err;

@@ -1432,10 +1433,10 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,

		goto out_free_rawdata;
	snprintf(dev_name, len + 5, "%s%s%s:/", startsep, ipaddr, endsep);

-	status = nfsd4_ssc_setup_dul(nn, ipaddr, &work, &ss_mnt);
+	status = nfsd4_ssc_setup_dul(nn, ipaddr, nsui);
	if (status)
		goto out_free_devname;
-	if (ss_mnt)
+	if ((*nsui)->nsui_vfsmount)
		goto out_done;

	/* Use an 'internal' mount: SB_KERNMOUNT -> MNT_INTERNAL */

@@ -1443,15 +1444,12 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,

	module_put(type->owner);
	if (IS_ERR(ss_mnt)) {
		status = nfserr_nodev;
-		if (work)
-			nfsd4_ssc_cancel_dul_work(nn, work);
+		nfsd4_ssc_cancel_dul_work(nn, *nsui);
		goto out_free_devname;
	}
-	if (work)
-		nfsd4_ssc_update_dul_work(nn, work, ss_mnt);
+	nfsd4_ssc_update_dul_work(nn, *nsui, ss_mnt);
out_done:
	status = 0;
-	*mount = ss_mnt;

out_free_devname:
	kfree(dev_name);

@@ -1474,8 +1472,7 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,
 */

static __be32
nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
-		      struct nfsd4_compound_state *cstate,
-		      struct nfsd4_copy *copy, struct vfsmount **mount)
+		struct nfsd4_compound_state *cstate, struct nfsd4_copy *copy)
{
	struct svc_fh *s_fh = NULL;
	stateid_t *s_stid = &copy->cp_src_stateid;

@@ -1488,7 +1485,8 @@ nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,

	if (status)
		goto out;

-	status = nfsd4_interssc_connect(copy->cp_src, rqstp, mount);
+	status = nfsd4_interssc_connect(copy->cp_src, rqstp,
+				&copy->ss_nsui);
	if (status)
		goto out;

@@ -1506,61 +1504,42 @@ nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,

}

static void
-nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *filp,
+nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *ni, struct file *filp,
			struct nfsd_file *dst)
{
-	bool found = false;
	long timeout;
-	struct nfsd4_ssc_umount_item *tmp;
-	struct nfsd4_ssc_umount_item *ni = NULL;
	struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);

	nfs42_ssc_close(filp);
	nfsd_file_put(dst);
	fput(filp);

-	if (!nn) {
-		mntput(ss_mnt);
-		return;
-	}
	spin_lock(&nn->nfsd_ssc_lock);
	timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
-	list_for_each_entry_safe(ni, tmp, &nn->nfsd_ssc_mount_list, nsui_list) {
-		if (ni->nsui_vfsmount->mnt_sb == ss_mnt->mnt_sb) {
-			list_del(&ni->nsui_list);
-			/*
-			 * vfsmount can be shared by multiple exports,
-			 * decrement refcnt. If the count drops to 1 it
-			 * will be unmounted when nsui_expire expires.
-			 */
-			refcount_dec(&ni->nsui_refcnt);
-			ni->nsui_expire = jiffies + timeout;
-			list_add_tail(&ni->nsui_list, &nn->nfsd_ssc_mount_list);
-			found = true;
-			break;
-		}
-	}
+	list_del(&ni->nsui_list);
+	/*
+	 * vfsmount can be shared by multiple exports,
+	 * decrement refcnt. If the count drops to 1 it
+	 * will be unmounted when nsui_expire expires.
+	 */
+	refcount_dec(&ni->nsui_refcnt);
+	ni->nsui_expire = jiffies + timeout;
+	list_add_tail(&ni->nsui_list, &nn->nfsd_ssc_mount_list);
	spin_unlock(&nn->nfsd_ssc_lock);
-	if (!found) {
-		mntput(ss_mnt);
-		return;
-	}
}

#else /* CONFIG_NFSD_V4_2_INTER_SSC */

static __be32
nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
-		      struct nfsd4_compound_state *cstate,
-		      struct nfsd4_copy *copy,
-		      struct vfsmount **mount)
+			struct nfsd4_compound_state *cstate,
+			struct nfsd4_copy *copy)
{
-	*mount = NULL;
	return nfserr_inval;
}

static void
-nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *filp,
+nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *ni, struct file *filp,
			struct nfsd_file *dst)
{
}

@@ -1700,7 +1679,7 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)

	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;
+	dst->ss_nsui = src->ss_nsui;
}

static void cleanup_async_copy(struct nfsd4_copy *copy)

@@ -1749,8 +1728,8 @@ static int nfsd4_do_async_copy(void *data)

	if (nfsd4_ssc_is_inter(copy)) {
		struct file *filp;

-		filp = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
-				      &copy->stateid);
+		filp = nfs42_ssc_open(copy->ss_nsui->nsui_vfsmount,
+				&copy->c_fh, &copy->stateid);
		if (IS_ERR(filp)) {
			switch (PTR_ERR(filp)) {
			case -EBADF:

@@ -1764,7 +1743,7 @@ static int nfsd4_do_async_copy(void *data)

		}
		nfserr = nfsd4_do_copy(copy, filp, copy->nf_dst->nf_file,
				       false);
-		nfsd4_cleanup_inter_ssc(copy->ss_mnt, filp, copy->nf_dst);
+		nfsd4_cleanup_inter_ssc(copy->ss_nsui, filp, copy->nf_dst);
	} else {
		nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
				       copy->nf_dst->nf_file, false);

@@ -1790,8 +1769,7 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,

			status = nfserr_notsupp;
			goto out;
		}
-		status = nfsd4_setup_inter_ssc(rqstp, cstate, copy,
-				&copy->ss_mnt);
+		status = nfsd4_setup_inter_ssc(rqstp, cstate, copy);
		if (status)
			return nfserr_offload_denied;
	} else {

diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 0eb00105d845..36c3340c1d54 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h

@@ -571,7 +571,7 @@ struct nfsd4_copy {

	struct task_struct	*copy_task;
	refcount_t		refcount;

-	struct vfsmount		*ss_mnt;
+	struct nfsd4_ssc_umount_item *ss_nsui;
	struct nfs_fh		c_fh;
	nfs4_stateid		stateid;
};

diff --git a/include/linux/nfs_ssc.h b/include/linux/nfs_ssc.h
index 75843c00f326..22265b1ff080 100644
--- a/include/linux/nfs_ssc.h
+++ b/include/linux/nfs_ssc.h

@@ -53,6 +53,7 @@ static inline void nfs42_ssc_close(struct file *filep)

	if (nfs_ssc_client_tbl.ssc_nfs4_ops)
		(*nfs_ssc_client_tbl.ssc_nfs4_ops->sco_close)(filep);
}
+#endif

struct nfsd4_ssc_umount_item {
	struct list_head nsui_list;

@@ -66,7 +67,6 @@ struct nfsd4_ssc_umount_item {

	struct vfsmount *nsui_vfsmount;
	char nsui_ipaddr[RPC_MAX_ADDRBUFLEN + 1];
};
-#endif

/*
 * NFS_FS
-- 
2.9.5

--
Chuck Lever

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help