Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <d214d283-a9dc-092e-2be5-ad847ecaf871@themaw.net>
Date:   Mon, 2 Jan 2023 14:41:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH] nfsd: fix handling of readdir in v4root vs. mount upcall
 timeout
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Steve Dickson <steved@redhat.com>,
        JianHong Yin <yin-jianhong@163.com>,
        Richard Weinberger <richard@nod.at>
References: <20221213180826.216690-1-jlayton@kernel.org>
 <0918676C-124C-417F-B8DE-DA1946EE91CC@oracle.com>
 <988799bd54c391259cfeff002660a4002adb96d2.camel@kernel.org>
 <81f891ef-b498-24b0-12e3-4ddda8062dc0@themaw.net>
 <0d6deecbe0dff95ebbe061914ddb00ca04d1f3c1.camel@kernel.org>
 <b2593a91-0957-5203-b556-f93bdd2dc0dd@themaw.net>
 <4fbce57518a3870e82411b31e026ffdd8ea1c98d.camel@kernel.org>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <4fbce57518a3870e82411b31e026ffdd8ea1c98d.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/1/23 05:16, Jeff Layton wrote:
> On Wed, 2022-12-14 at 13:37 +0800, Ian Kent 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.
>>
> Yes, it does need to already be mounted.
>
> The proposed kernel patches from Richard only trigger an automount if
> the parent mount is exported with -o crossmnt. I think this is necessary
> to avoid nfs client activity triggering automounts of filesystems that
> are not exported.

I'll be interested to see how this goes.


Over the years I've had a lot of difficulty with automount unwanted

mounting ...


Still nfsd exports are a bit like invisible dentry trees to the local

system aren't they ... so this situation is very different to what

I've worked on ...


Ian

>
>>> 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.