Re: Do we really need d_weak_revalidate???

[NeilBrown]

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.

Do you know if the fact that filename_mountpoint() skips ->d_manage is
important for sys_umount ??

Thanks,
NeilBrown


> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature