On Tue, Jan 3, 2023 at 11:14 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: > > > > > On Dec 18, 2022, at 7:55 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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. I have looked at it and logically it seems ok to me. I have tested it (sorta. i'm rarely able to finish). But I keep running into the other problem (nfsd4_state_shrinker_count soft lockup that's been already reported). I find it interesting that only my destination server hits the problem (but not the source server). I don't believe this patch has anything to do with this problem, but I found it interesting that ssc testing seems to trigger it 100%. > > 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 = ©->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, > > + ©->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, ©->c_fh, > > - ©->stateid); > > + filp = nfs42_ssc_open(copy->ss_nsui->nsui_vfsmount, > > + ©->c_fh, ©->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, > > - ©->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 > > >