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