> On Sep 3, 2024, at 12:09 PM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > > On Tue, Sep 03, 2024 at 03:59:31PM +0000, Chuck Lever III wrote: >> >> >>> On Sep 3, 2024, at 11:29 AM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: >>> >>> I had to double check but I did add a comment that speaks directly to >>> this "nuance" above the code you quoted: >>> >>> /* >>> * uuid->net must not be NULL, otherwise NFS may not have ref >>> * on NFSD and therefore cannot safely make 'nfs_to' calls. >>> */ >>> >>> So yeah, this code needs to stay like this. The __must_hold(rcu) just >>> ensures the RCU is held on entry and exit.. the bouncing of RCU >>> (dropping and retaking) isn't of immediate concern is it? While I >>> agree it isn't ideal, it is what it is given: >>> 1) NFS caller of NFSD symbol is only safe if it has RCU amd verified >>> uuid->net valid >>> 2) nfsd_file_do_acquire() can allocate. >> >> OK, understood, but the annotation is still wrong. The lock >> is dropped here so I think you need __releases and __acquires >> in that case. However... > > Sure, that seems like more precise context with which to train > lockdep. > >> Let's wait for Neil's comments, but I think this needs to be >> properly addressed before merging. The comments are not going >> to be enough IMO. > > I obviously have no issues with Neil confirming/expanding what I > shared about the need for checking uuid->net with RCU held to ensure > it safe to call this nfs_to method. Without it we cannot make the > call, which happens to then take other references (nfsd_serv and > nfsd_file) that we can then lean on for the duration of the NFS client > issuing IO and then dropping the references/interlock when completing > the IO. > > The NFS client maintainers need to give a good review anyway, so > plenty of time for Neil to weigh in. I forgot to mention before: I don't see any other issues at this point. Acked-by: Chuck Lever <chuck.lever@xxxxxxxxxx <mailto:chuck.lever@xxxxxxxxxx>> -- Chuck Lever
