Re: Do we really need d_weak_revalidate???

[NeilBrown]

On Mon, Aug 21 2017, Ian Kent wrote:

> On 21/08/17 14:23, NeilBrown wrote:
>> On Fri, Aug 18 2017, Ian Kent wrote:
>> 
>>> On 18/08/17 13:24, NeilBrown wrote:
>>>> On Thu, Aug 17 2017, Ian Kent wrote:
>>>>
>>>>> On 16/08/17 19:34, Jeff Layton wrote:
>>>>>> On Wed, 2017-08-16 at 12:43 +1000, NeilBrown wrote:
>>>>>>> On Mon, Aug 14 2017, Jeff Layton wrote:
>>>>>>>
>>>>>>>> On Mon, 2017-08-14 at 09:36 +1000, NeilBrown wrote:
>>>>>>>>> On Fri, Aug 11 2017, Jeff Layton wrote:
>>>>>>>>>
>>>>>>>>>> On Fri, 2017-08-11 at 05:55 +0000, Trond Myklebust wrote:
>>>>>>>>>>> On Fri, 2017-08-11 at 14:31 +1000, NeilBrown wrote:
>>>>>>>>>>>> Funny story.  4.5 years ago we discarded the FS_REVAL_DOT superblock
>>>>>>>>>>>> flag and introduced the d_weak_revalidate dentry operation instead.
>>>>>>>>>>>> We duly removed the flag from NFS superblocks and NFSv4 superblocks,
>>>>>>>>>>>> and added the new dentry operation to NFS dentries .... but not to
>>>>>>>>>>>> NFSv4
>>>>>>>>>>>> dentries.
>>>>>>>>>>>>
>>>>>>>>>>>> And nobody noticed.
>>>>>>>>>>>>
>>>>>>>>>>>> Until today.
>>>>>>>>>>>>
>>>>>>>>>>>> A customer reports a situation where mount(....,MS_REMOUNT,..) on an
>>>>>>>>>>>> NFS
>>>>>>>>>>>> filesystem hangs because the network has been deconfigured.  This
>>>>>>>>>>>> makes
>>>>>>>>>>>> perfect sense and I suggested a code change to fix the problem.
>>>>>>>>>>>> However when a colleague was trying to reproduce the problem to
>>>>>>>>>>>> validate
>>>>>>>>>>>> the fix, he couldn't.  Then nor could I.
>>>>>>>>>>>>
>>>>>>>>>>>> The problem is trivially reproducible with NFSv3, and not at all with
>>>>>>>>>>>> NFSv4.  The reason is the missing d_weak_revalidate.
>>>>>>>>>>>>
>>>>>>>>>>>> We could simply add d_weak_revalidate for NFSv4, but given that it
>>>>>>>>>>>> has been missing for 4.5 years, and the only time anyone noticed was
>>>>>>>>>>>> when the ommission resulted in a better user experience, I do wonder
>>>>>>>>>>>> if
>>>>>>>>>>>> we need to.  Can we just discard d_weak_revalidate?  What purpose
>>>>>>>>>>>> does
>>>>>>>>>>>> it serve?  I couldn't find one.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> NeilBrown
>>>>>>>>>>>>
>>>>>>>>>>>> For reference, see
>>>>>>>>>>>> Commit: ecf3d1f1aa74 ("vfs: kill FS_REVAL_DOT by adding a
>>>>>>>>>>>> d_weak_revalidate dentry op")
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> To reproduce the problem at home, on a system that uses systemd:
>>>>>>>>>>>> 1/ place (or find) a filesystem image in a file on an NFS filesystem.
>>>>>>>>>>>> 2/ mount the nfs filesystem with "noac" - choose v3 or v4
>>>>>>>>>>>> 3/ loop-mount the filesystem image read-only somewhere
>>>>>>>>>>>> 4/ reboot
>>>>>>>>>>>>
>>>>>>>>>>>> If you choose v4, the reboot will succeed, possibly after a 90second
>>>>>>>>>>>> timeout.
>>>>>>>>>>>> If you choose v3, the reboot will hang indefinitely in systemd-
>>>>>>>>>>>> shutdown while
>>>>>>>>>>>> remounting the nfs filesystem read-only.
>>>>>>>>>>>>
>>>>>>>>>>>> If you don't use "noac" it can still hang, but only if something
>>>>>>>>>>>> slows
>>>>>>>>>>>> down the reboot enough that attributes have timed out by the time
>>>>>>>>>>>> that
>>>>>>>>>>>> systemd-shutdown runs.  This happens for our customer.
>>>>>>>>>>>>
>>>>>>>>>>>> If the loop-mounted filesystem is not read-only, you get other
>>>>>>>>>>>> problems.
>>>>>>>>>>>>
>>>>>>>>>>>> We really want systemd to figure out that the loop-mount needs to be
>>>>>>>>>>>> unmounted first.  I have ideas concerning that, but it is messy.  But
>>>>>>>>>>>> that isn't the only bug here.
>>>>>>>>>>>
>>>>>>>>>>> The main purpose of d_weak_revalidate() was to catch the issues that
>>>>>>>>>>> arise when someone changes the contents of the current working
>>>>>>>>>>> directory or its parent on the server. Since '.' and '..' are treated
>>>>>>>>>>> specially in the lookup code, they would not be revalidated without
>>>>>>>>>>> special treatment. That leads to issues when looking up files as
>>>>>>>>>>> ./<filename> or ../<filename>, since the client won't detect that its
>>>>>>>>>>> dcache is stale until it tries to use the cached dentry+inode.
>>>>>>>>>>>
>>>>>>>>>>> The one thing that has changed since its introduction is, I believe,
>>>>>>>>>>> the ESTALE handling in the VFS layer. That might fix a lot of the
>>>>>>>>>>> dcache lookup bugs that were previously handled by d_weak_revalidate().
>>>>>>>>>>> I haven't done an audit to figure out if it actually can handle all of
>>>>>>>>>>> them.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It may also be related to 8033426e6bdb2690d302872ac1e1fadaec1a5581:
>>>>>>>>>>
>>>>>>>>>>     vfs: allow umount to handle mountpoints without revalidating them
>>>>>>>>>
>>>>>>>>> You say in the comment for that commit:
>>>>>>>>>
>>>>>>>>>      but there
>>>>>>>>>     are cases where we do want to revalidate the root of the fs.
>>>>>>>>>
>>>>>>>>> Do you happen to remember what those cases are?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not exactly, but I _think_ I might have been assuming that we needed to
>>>>>>>> ensure that the inode attrs on the root were up to date after the
>>>>>>>> pathwalk.
>>>>>>>>
>>>>>>>> I think that was probably wrong. d_revalidate is really intended to
>>>>>>>> ensure that the dentry in question still points to the same inode. In
>>>>>>>> the case of the root of the mount though, we don't really care about the
>>>>>>>> dentry on the server at all. We're attaching the root of the mount to an
>>>>>>>> inode and don't care of the dentry name changes. If we do need to ensure
>>>>>>>> the inode attrs are updated, we'll just revalidate them at that point.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Possibly the fact that we no longer try to revalidate during unmount
>>>>>>>>>> means that this is no longer necessary?
>>>>>>>>>>
>>>>>>>>>> The original patch that added d_weak_revalidate had a reproducer in the
>>>>>>>>>> patch description. Have you verified that that problem is still not
>>>>>>>>>> reproducible when you remove d_weak_revalidate?
>>>>>>>>>
>>>>>>>>> I did try the reproducer and it works as expected both with and without
>>>>>>>>> d_weak_revalidate.
>>>>>>>>> On reflection, the problem it displayed was caused by d_revalidate()
>>>>>>>>> being called when the dentry name was irrelevant.  We remove that
>>>>>>>>> (fixing the problem) and introduce d_weak_revalidate because we thought
>>>>>>>>> that minimum functionality was still useful.  I'm currently not
>>>>>>>>> convinced that even that is needed.
>>>>>>>>>
>>>>>>>>> If we discarded d_weak_revalidate(), we could get rid of the special
>>>>>>>>> handling of umount....
>>>>>>>>
>>>>>>>> I like idea. I say go for it and we can see what (if anything) breaks?
>>>>>>>
>>>>>>> Getting rid of d_weak_revalidate is easy enough - hardly any users.
>>>>>>>
>>>>>>> Getting rid of filename_mountpoint() isn't so easy unfortunately.
>>>>>>> autofs4 uses kern_path_mountpoint() - presumably to avoid getting stuck
>>>>>>> in autofs4_d_manage()?  It would be a shame to keep this infrastructure
>>>>>>> around just so that one part of autofs4 can talk to another part of
>>>>>>> autofs4.
>>>>>
>>>>> When this was first implemented autofs didn't use kern_path_mountpoint()
>>>>> (it didn't exist) it used a path lookup on the parent and a separate
>>>>> lookup for the last component.
>>>>
>>>> This was before commit 4e44b6852e03 ("Get rid of path_lookup in
>>>> autofs4").  This used kern_path().
>>>
>>> I have to plead not guilty of this one.
>>>
>>> IIRC it broke the requirement of "lookup parent then lookup last component"
>>> rather it walked the whole path then followed up to find the mount point
>>> struct path.
>>>
>>> Like it says in the description of ac8387199656 the caller might not yet
>>> "own" the autofs mount which causes a mount to be triggered during the
>>> walk that can't be satisfied because of the deadlock that occurs.
>> 
>> A mount isn't triggered by kern_path(pathname, 0, &path).
>> That '0' would need to include one of
>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>> 
>> to trigger an automount (otherwise you just get -EISDIR).
>
> It's perfectly sensible to think that but there is a case where a
> a mount is triggered when using kern_path().
>
> The EISDIR return occurs for positive dentrys, negative dentrys
> will still trigger an automount (which is autofs specific,
> indirect mount map using nobrowse option, the install default).

Ahh  -  light dawns.  Thanks :-)

NeilBrown


>
>> 
>> That is why I assumed that ->d_managed was the problem.
>> 
>>>
>>>>
>>>> I'm more interested in commit ac8387199656 ("autofs4 - fix device ioctl
>>>> mount lookup")  which replaced the use of kern_path() with
>>>> kern_path_mountpoint().
>>>
>>> Probably should have had a Fixes: 4e44b6852e03 ...
>> 
>> Still a bit confused as to exactly what was fixed...
>
> Hopefully also considering the negative dentry case will clear that up.
>
> Ian

Attachment: signature.asc
Description: PGP signature