Re: find_fh_dentry returned a DISCONNECTED directory

[ebiederm@xmission.com (Eric W. Biederman)]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Fri, Feb 14, 2014 at 07:49:35AM -0800, Eric W. Biederman wrote:
>> 
>> I believe we want both groups of dentries on the s_anon list so that if
>> they remain disconnected when the filesystem is unmounted we can
>> find them and deal with them.
>
> Note it's IS_ROOT(), not DCACHE_DISCONNECTED, that determines whether a
> hashed dentry is on s_anon or not.  (See
> 7632e465feb182cadc3c9aa1282a057201818a8c for more detailed
> discussion.)

Interesting.  It remains the case that d_obtain_alias that sets
DCACHE_DISCONNECTED is the only way to get on the s_anon list.

>From the rest of the conversation below I think what is needed is
d_obtain_alias_root.   A function like d_obtain_alias but that does not
set DCACHE_DISCONNECTED, that places IS_ROOT dentries on the s_anon list
and that is usually expected to not find an alias.

>> I can see distinguishing between dentries that are supposed to be
>> disconnected for a short time, and dentries that are supposed to be
>> disconnected indefinitely but we currently (as of 3.14-rc1) don't have
>> that distinction.
>
> I believe we do: DCACHE_DISCONNECTED is for the former, and the latter
> should be IS_ROOT(), !DCACHE_DISCONNECTED.
>
> DCACHE_DISCONNECTED was intended to be used only for dentries created
> while performing lookup-by-filehandle which have not yet been confirmed
> to be linked back to filesystem root by a chain of ->d_parent
> pointers.

Given the current usages that was not at all clear from reading through
the code.   So I suggest d_obtain_alias_root to sort the last of it out.

>> But a blanket statement that the long term disconnected dentries are
>> doing it wrong seems off base.
>> 
>> If those dentires can tolerate not being on the s_anon list
>> d_alloc_pseudo or d_make_root looks like it will serve just as well
>
> The difference is that d_alloc_pseudo and d_make_root unconditionally
> create new dentries, whereas d_materialise_unique lets us reuse an
> existing dentry.

Aliasing differences aside when dentries are allocated my point is that
d_materialise_uniuqe does not care afterwards if DCACHE_DISCONNECTED is
set or not, and d_materialise_unique will perform the connection if that
is possible later on.

> (In the NFS client case, I believe this happens for example when you
> mount export A from a server, then export B from the same server, and
> then one day you look up A/foo and find out that it's the same directory
> as B's root.  You don't want to duplicate the inode or give it multiple
> dentries, so you instead reuse the existing IS_ROOT dentry for B to
> represent A/foo.
>
> In the btrfs case I guess it's a similar situation but with two
> subvolumes instead of two exports?)

That is what it sounds like.  And d_materialise_unique will connect them
in either case.

So we just need d_obtain_alias_root to allocate these weird oddball
dentries and we should be good.

I suspect the code would be much clearer if we were to do a mass
s/DCACHE_DISCONNECTED/DCACHE_CONNECTING/ to make it clear that the
flag is not intended to cover the general case of when dentries are
floating around without parents.

>> from
>> the perspective of d_materialise_unique, but that leaves me with the
>> queasy feeling that we will leak dentries and inodes when we unmount the
>> filesystems in question, if those dentries have never been attached.
>
> In the NFS server (lookup-by-filehandle) case, I believe dentries in the
> process of being reconnected are all children of some IS_ROOT dentry
> which is on the s_anon list, so everyone's accounted for.

My concern there is for all of the other cases.  Because so far today
only DCACHE_DISCONNECTED dentries land on the s_anon list and that means
if we don't use d_obtain_alias and we can have dentry leaks and unmount.

> Thanks for looking at this as I've found myself easily confused by it
> all....  (And judging by some of the code in the tree I'm not alone.)

I am in the final stages of putting together some patches so that
filesystems don't need to like to the vfs to preserve vfs invariants
about mountpoints.  In particular I am implemeting automatic detaching
of mountpionts on unlink and d_invalidate.  Once that is done and
everyone is dropping directory dentries instead of sticking to the
silly 2.2 era logic that present resides in d_invalidate some of these
other dcache uses should become a little less mysterious.

Eric