On Mon, 2023-01-23 at 10:22 -0500, Olga Kornievskaia wrote: > On Sun, Jan 22, 2023 at 11:46 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: > > > > > > > > > On Jan 21, 2023, at 4:28 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote: > > > > > > > > > On 1/21/23 12:12 PM, Chuck Lever III wrote: > > > > > > > > > On Jan 21, 2023, at 3:05 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > > > > > > > On Sat, 2023-01-21 at 11:50 -0800, dai.ngo@xxxxxxxxxx wrote: > > > > > > On 1/21/23 10:56 AM, dai.ngo@xxxxxxxxxx wrote: > > > > > > > On 1/20/23 3:43 AM, Jeff Layton wrote: > > > > > > > > On Thu, 2023-01-19 at 10:38 -0800, dai.ngo@xxxxxxxxxx wrote: > > > > > > > > > On 1/19/23 2:56 AM, Jeff Layton wrote: > > > > > > > > > > On Wed, 2023-01-18 at 21:05 -0800, dai.ngo@xxxxxxxxxx wrote: > > > > > > > > > > > On 1/17/23 11:38 AM, Jeff Layton wrote: > > > > > > > > > > > > There are two different flavors of the nfsd4_copy struct. One is > > > > > > > > > > > > embedded in the compound and is used directly in synchronous > > > > > > > > > > > > copies. The > > > > > > > > > > > > other is dynamically allocated, refcounted and tracked in the client > > > > > > > > > > > > struture. For the embedded one, the cleanup just involves > > > > > > > > > > > > releasing any > > > > > > > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is > > > > > > > > > > > > a bit > > > > > > > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc. > > > > > > > > > > > > > > > > > > > > > > > > There is at least one potential refcount leak in this code now. > > > > > > > > > > > > If the > > > > > > > > > > > > kthread_create call fails, then both the src and dst nfsd_files > > > > > > > > > > > > in the > > > > > > > > > > > > original nfsd4_copy object are leaked. > > > > > > > > > > > > > > > > > > > > > > > > The cleanup in this codepath is also sort of weird. In the async > > > > > > > > > > > > copy > > > > > > > > > > > > case, we'll have up to four nfsd_file references (src and dst for > > > > > > > > > > > > both > > > > > > > > > > > > flavors of copy structure). They are both put at the end of > > > > > > > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the > > > > > > > > > > > > embedded > > > > > > > > > > > > one outlive that structure. > > > > > > > > > > > > > > > > > > > > > > > > Change it so that we always clean up the nfsd_file refs held by the > > > > > > > > > > > > embedded copy structure before nfsd4_copy returns. Rework > > > > > > > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate > > > > > > > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > > > > > > > > > > --- > > > > > > > > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++------------- > > > > > > > > > > > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > > > > > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644 > > > > > > > > > > > > --- a/fs/nfsd/nfs4proc.c > > > > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c > > > > > > > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct > > > > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp, > > > > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout); > > > > > > > > > > > > nfs42_ssc_close(filp); > > > > > > > > > > > > - nfsd_file_put(dst); > > > > > > > > > > > I think we still need this, in addition to release_copy_files called > > > > > > > > > > > from cleanup_async_copy. For async inter-copy, there are 2 reference > > > > > > > > > > > count added to the destination file, one from nfsd4_setup_inter_ssc > > > > > > > > > > > and the other one from dup_copy_fields. The above nfsd_file_put is > > > > > > > > > > > for > > > > > > > > > > > the count added by dup_copy_fields. > > > > > > > > > > > > > > > > > > > > > With this patch, the references held by the original copy structure > > > > > > > > > > are > > > > > > > > > > put by the call to release_copy_files at the end of nfsd4_copy. That > > > > > > > > > > means that the kthread task is only responsible for putting the > > > > > > > > > > references held by the (kmalloc'ed) async_copy structure. So, I think > > > > > > > > > > this gets the nfsd_file refcounting right. > > > > > > > > > Yes, I see. One refcount is decremented by release_copy_files at end > > > > > > > > > of nfsd4_copy and another is decremented by release_copy_files in > > > > > > > > > cleanup_async_copy. > > > > > > > > > > > > > > > > > > > > > fput(filp); > > > > > > > > > > > > spin_lock(&nn->nfsd_ssc_lock); > > > > > > > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp, > > > > > > > > > > > > ©->nf_dst); > > > > > > > > > > > > } > > > > > > > > > > > > -static void > > > > > > > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file > > > > > > > > > > > > *dst) > > > > > > > > > > > > -{ > > > > > > > > > > > > - nfsd_file_put(src); > > > > > > > > > > > > - nfsd_file_put(dst); > > > > > > > > > > > > -} > > > > > > > > > > > > - > > > > > > > > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb) > > > > > > > > > > > > { > > > > > > > > > > > > struct nfsd4_cb_offload *cbo = > > > > > > > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct > > > > > > > > > > > > nfsd4_copy *src, struct nfsd4_copy *dst) > > > > > > > > > > > > dst->ss_nsui = src->ss_nsui; > > > > > > > > > > > > } > > > > > > > > > > > > +static void release_copy_files(struct nfsd4_copy *copy) > > > > > > > > > > > > +{ > > > > > > > > > > > > + if (copy->nf_src) > > > > > > > > > > > > + nfsd_file_put(copy->nf_src); > > > > > > > > > > > > + if (copy->nf_dst) > > > > > > > > > > > > + nfsd_file_put(copy->nf_dst); > > > > > > > > > > > > +} > > > > > > > > > > > > + > > > > > > > > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy) > > > > > > > > > > > > { > > > > > > > > > > > > nfs4_free_copy_state(copy); > > > > > > > > > > > > - nfsd_file_put(copy->nf_dst); > > > > > > > > > > > > - if (!nfsd4_ssc_is_inter(copy)) > > > > > > > > > > > > - nfsd_file_put(copy->nf_src); > > > > > > > > > > > > + release_copy_files(copy); > > > > > > > > > > > > spin_lock(©->cp_clp->async_lock); > > > > > > > > > > > > list_del(©->copies); > > > > > > > > > > > > spin_unlock(©->cp_clp->async_lock); > > > > > > > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data) > > > > > > > > > > > > } else { > > > > > > > > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file, > > > > > > > > > > > > copy->nf_dst->nf_file, false); > > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst); > > > > > > > > > > > > } > > > > > > > > > > > > do_callback: > > > > > > > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct > > > > > > > > > > > > nfsd4_compound_state *cstate, > > > > > > > > > > > > } else { > > > > > > > > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file, > > > > > > > > > > > > copy->nf_dst->nf_file, true); > > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst); > > > > > > > > > > > > } > > > > > > > > > > > > out: > > > > > > > > > > > > + release_copy_files(copy); > > > > > > > > > > > > return status; > > > > > > > > > > > > out_err: > > > > > > > > > > > This is unrelated to the reference count issue. > > > > > > > > > > > > > > > > > > > > > > Here if this is an inter-copy then we need to decrement the reference > > > > > > > > > > > count of the nfsd4_ssc_umount_item so that the vfsmount can be > > > > > > > > > > > unmounted > > > > > > > > > > > later. > > > > > > > > > > > > > > > > > > > > > Oh, I think I see what you mean. Maybe something like the (untested) > > > > > > > > > > patch below on top of the original patch would fix that? > > > > > > > > > > > > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > > > > > > > > index c9057462b973..7475c593553c 100644 > > > > > > > > > > --- a/fs/nfsd/nfs4proc.c > > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c > > > > > > > > > > @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct > > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp, > > > > > > > > > > struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id); > > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout); > > > > > > > > > > - nfs42_ssc_close(filp); > > > > > > > > > > - fput(filp); > > > > > > > > > > + if (filp) { > > > > > > > > > > + nfs42_ssc_close(filp); > > > > > > > > > > + fput(filp); > > > > > > > > > > + } > > > > > > > > > > spin_lock(&nn->nfsd_ssc_lo > > > > > > > > > > list_del(&nsui->nsui_list); > > > > > > > > > > @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct > > > > > > > > > > nfsd4_compound_state *cstate, > > > > > > > > > > release_copy_files(copy); > > > > > > > > > > return status; > > > > > > > > > > out_err: > > > > > > > > > > - if (async_copy) > > > > > > > > > > + if (async_copy) { > > > > > > > > > > cleanup_async_copy(async_copy); > > > > > > > > > > + if (nfsd4_ssc_is_inter(async_copy)) > > > > > > > > > We don't need to call nfsd4_cleanup_inter_ssc since the thread > > > > > > > > > nfsd4_do_async_copy has not started yet so the file is not opened. > > > > > > > > > We just need to do refcount_dec(©->ss_nsui->nsui_refcnt), unless > > > > > > > > > you want to change nfsd4_cleanup_inter_ssc to detect this error > > > > > > > > > condition and only decrement the reference count. > > > > > > > > > > > > > > > > > Oh yeah, and this would break anyway since the nsui_list head is not > > > > > > > > being initialized. Dai, would you mind spinning up a patch for this > > > > > > > > since you're more familiar with the cleanup here? > > > > > > > Will do. My patch will only fix the unmount issue. Your patch does > > > > > > > the clean up potential nfsd_file refcount leaks in COPY codepath. > > > > > > Or do you want me to merge your patch and mine into one? > > > > > > > > > > > It probably is best to merge them, since backporters will probably want > > > > > both patches anyway. > > > > Unless these two changes are somehow interdependent, I'd like to keep > > > > them separate. They address two separate issues, yes? > > > > > > Yes. > > > > > > > > > > > And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait > > > > for nfsd-next. I'd rather not mix the two types of change. > > > > > > Ok. Can we do this: > > > > > > 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing > > > reference count. > > > > To make sure I haven't lost track of anything: > > > > The patch you refer to here is this one: > > > > https://lore.kernel.org/linux-nfs/20230117193831.75201-3-jlayton@xxxxxxxxxx/ > > > > Correct? > > > > (I was waiting for Jeff and Olga to come to consensus, and I think > > they have, so I can apply it to nfsd-fixes now). > > Sorry folks but I got a bit lost in the thread. I thought Dai pointed > out that we can't remove the put from the nfsd4_cleanup_inter_ssc() > because that's the put for the copied structure and not the original > file references which is what Jeff's patch is trying to address. > ...and then I replied: With this patch, the references held by the original copy structure are put by the call to release_copy_files at the end of nfsd4_copy. That means that the kthread task is only responsible for putting the references held by the (kmalloc'ed) async_copy structure. So, I think this gets the nfsd_file refcounting right. > > > > 2. My fix for the cleanup of allocated memory goes to nfsd-fixes. > > > > And this one hasn't been posted yet, right? Or did I miss it? > > > > > > > 3. I will do the optimization Jeff proposed about list_head and > > > nfsd4_compound in a separate patch that goes into nfsd-next. > > > > That should be fine. > > > > > > > -Dai > > > > > > > > Just make yourself the patch author and keep my S-o-b line. > > > > > > > > > > > I think we need a bit more cleanup in addition to your patch. When > > > > > > kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state > > > > > > fails, the async_copy is not initialized yet so calling cleanup_async_copy > > > > > > can be a problem. > > > > > > > > > > > Yeah. > > > > > > > > > > It may even be best to ensure that the list_head and such are fully > > > > > initialized for both allocated and embedded struct nfsd4_copy's. You > > > > > might shave off a few cpu cycles by not doing that, but it makes things > > > > > more fragile. > > > > > > > > > > Even better, we really ought to split a lot of the fields in nfsd4_copy > > > > > into a different structure (maybe nfsd4_async_copy). Trimming down > > > > > struct nfsd4_copy would cut down the size of nfsd4_compound as well > > > > > since it has a union that contains it. I was planning on doing that > > > > > eventually, but if you want to take that on, then that would be fine > > > > > too. > > > > > > > > > > -- > > > > > Jeff Layton <jlayton@xxxxxxxxxx> > > > > -- > > > > Chuck Lever > > > > -- > > Chuck Lever > > > > > > -- Jeff Layton <jlayton@xxxxxxxxxx>