On Wed, Jan 18, 2023 at 12:26 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Wed, 2023-01-18 at 17:11 +0000, Chuck Lever III wrote: > > > > > 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? > > > > I can, unless Olga, Dai or someone else would rather do it. Not sure how > soon I can get to it though. I'm not going to volunteer as I don't believe in the suggested change. I think there is a performance advantage to having this structure preallocated and ready for use. > -- > Jeff Layton <jlayton@xxxxxxxxxx>
