Re: [PATCH] nfsd: fix handling of readdir in v4root vs. mount upcall timeout

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2023-01-01 at 18:18 +0000, Chuck Lever III wrote:
> 
> > On Jan 1, 2023, at 1:09 PM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> > 
> > 
> > 
> > > On Dec 14, 2022, at 12:37 AM, Ian Kent <raven@themaw.net> 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@kernel.org> 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?
> 

Yes, I think so. This potentially exposes some info to clients that they
really shouldn't have.
-- 
Jeff Layton <jlayton@kernel.org>