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