> On Jan 18, 2023, at 12:06 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote: >> >>> On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote: >>> >>> On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: >>>> >>>> On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote: >>>>> On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <jlayton@xxxxxxxxxx> 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. >>>>> >>>>> I don't believe that's true. If kthread_create thread fails we call >>>>> cleanup_async_copy() that does a put on the file descriptors. >>>>> >>>> >>>> You mean this? >>>> >>>> out_err: >>>> if (async_copy) >>>> cleanup_async_copy(async_copy); >>>> >>>> That puts the references that were taken in dup_copy_fields, but the >>>> original (embedded) nfsd4_copy also holds references and those are not >>>> being put in this codepath. >>> >>> Can you please point out where do we take a reference on the original copy? >>> >>>>>> 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). >>>>> >>>>> That's not true. There is a careful distinction between intra -- which >>>>> had 2 valid file pointers and does a get on both as they both point to >>>>> something that's opened on this server--- but inter -- only does a get >>>>> on the dst file descriptor, the src doesn't exit. And yes I realize >>>>> the code checks for nfs_src being null which it should be but it makes >>>>> the code less clear and at some point somebody might want to decide to >>>>> really do a put on it. >>>>> >>>> >>>> This is part of the problem here. We have a nfsd4_copy structure, and >>>> depending on what has been done to it, you need to call different >>>> methods to clean it up. That seems like a real antipattern to me. >>> >>> But they call different methods because different things need to be >>> done there and it makes it clear what needs to be for what type of >>> copy. >> >> In cases like this, it makes sense to consider using types to >> ensure the code can't do the wrong thing. So you might want to >> have a struct nfs4_copy_A for the inter code to use, and a struct >> nfs4_copy_B for the intra code to use. Sharing the same struct >> for both use cases is probably what's confusing to human readers. >> >> I've never been a stickler for removing every last ounce of code >> duplication. Here, it might help to have a little duplication >> just to make it easier to reason about the reference counting in >> the two use cases. >> >> That's my view from the mountain top, worth every penny you paid >> for it. >> > > +1 > > The nfsd4_copy structure has a lot of fields in it that only matter for > the async copy case. ISTM that nfsd4_copy (the function) should > dynamically allocate a struct nfsd4_async_copy that contains a > nfsd4_copy and whatever other fields are needed. > > Then, we could trim down struct nfsd4_copy to just the info needed. Yeah, some of those fields are actually quite large, like filehandles. > For instance, the nf_src and nf_dst fields really don't need to be in > nfsd4_copy. For the synchronous copy case, we can just keep those > pointers on the stack, and for the async case they would be inside the > larger structure. > > That would allow us to trim down the footprint of the compound union > too. That seems sensible. Do you feel like redriving this clean-up series with the changes you describe above? -- Chuck Lever
