On 1/22/23 8:45 AM, Chuck Lever III 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).
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?
I will post this patch soon.
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.
Thanks,
-Dai
-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
