> On Jan 1, 2023, at 1:09 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: > > > >> On Dec 14, 2022, at 12:37 AM, Ian Kent <raven@xxxxxxxxxx> wrote: >> >> On 14/12/22 08:39, Jeff Layton wrote: >>> On Wed, 2022-12-14 at 07:14 +0800, Ian Kent wrote: >>>> On 14/12/22 04:02, Jeff Layton wrote: >>>>> On Tue, 2022-12-13 at 19:00 +0000, Chuck Lever III wrote: >>>>>>> On Dec 13, 2022, at 1:08 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >>>>>>> >>>>>>> If v4 READDIR operation hits a mountpoint and gets back an error, >>>>>>> then it will include that entry in the reply and set RDATTR_ERROR for it >>>>>>> to the error. >>>>>>> >>>>>>> That's fine for "normal" exported filesystems, but on the v4root, we >>>>>>> need to be more careful to only expose the existence of dentries that >>>>>>> lead to exports. >>>>>>> >>>>>>> If the mountd upcall times out while checking to see whether a >>>>>>> mountpoint on the v4root is exported, then we have no recourse other >>>>>>> than to fail the whole operation. >>>>>> Thank you for chasing this down! >>>>>> >>>>>> Failing the whole READDIR when mountd times out might be a bad idea. >>>>>> If the mountd upcall times out every time, the client can't make >>>>>> any progress and will continue to emit the failing READDIR request. >>>>>> >>>>>> Would it be better to skip the unresolvable entry instead and let >>>>>> the READDIR succeed without that entry? >>>>>> >>>>> Mounting doesn't usually require working READDIR. In that situation, a >>>>> readdir() might hang (until the client kills), but a lookup of other >>>>> dentries that aren't perpetually stalled should be ok in this situation. >>>>> >>>>> If mountd is that hosed then I think it's unlikely that any progress >>>>> will be possible anyway. >>>> The READDIR shouldn't trigger a mount yes, but if it's a valid automount >>>> >>>> point (basically a valid dentry in this case I think) it should be listed. >>>> >>>> It certainly shouldn't hold up the READDIR, passing into it is when a >>>> >>>> mount should occur. >>>> >>>> >>>> That's usually the behavior we want for automounts, we don't want mount >>>> >>>> storms on directories full of automount points. >>>> >>> >>> We only want to display it if it's a valid _exported_ mountpoint. >>> >>> The idea here is to only reveal the parts of the namespace that are >>> exported in the nfsv4 pseudoroot. The "normal" contents are not shown -- >>> only exported mountpoints and ancestor directories of those mountpoints. >>> >>> We don't want mountd triggering automounts, in general. If the >>> underlying filesystem was exported, then it should also already be >>> mounted, since nfsd doesn't currently trigger automounts in >>> follow_down(). >> >> Umm ... must they already be mounted? >> >> >> Can't it be a valid mount point either not yet mounted or timed out >> >> and umounted. In that case shouldn't it be listed, I know that's >> >> not the that good an outcome because its stat info will change when >> >> it gets walked into but it's usually the only sane choice. >> >> >>> >>> There is also a separate patchset by Richard Weinberger to allow nfsd to >>> trigger automounts if the parent filesystem is exported with -o >>> crossmnt. That should be ok with this patch, since the automount will be >>> triggered before the upcall to mountd. That should ensure that it's >>> already mounted by the time we get to upcalling for its export. >> >> Yep, saw that, ;) > > I'm not sure if there is consensus on this patch. > > It's been pushed to nfsd's for-rc branch for wider testing, but if > there's a strong objection I can pull it out before the next -rc PR. Also, do we agree that it should get a "Cc: stable" tag? -- Chuck Lever
