find_fh_dentry returned a DISCONNECTED directory

find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

Yesterday you passed on a report of this printk from nfsdfh.c firing:

	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
	                                dentry);

I think the dentry probably comes from the FILEID_ROOT case of:

	if (fileid_type == FILEID_ROOT)
		dentry = dget(exp->ex_path.dentry);
	else {
		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
				data_left, fileid_type,
				nfsd_acceptable, exp);
        }

In that case the dentry was found using ordinary filesystem lookups, so
doesn't go through the same DISCONNECTED-clearing logic as in the case
of lookups by filehandle.

Probably they have an export root that's not a filesystem root, and the
lookups happened in the right order?

I suspect that's fine, and that the printk is just stupid, but maybe we
should clear DISCONNECTED when possible on normal lookups.  The
following is my attempt, though I'm not sure if d_alloc is the right
place to do this.  In any case it might help confirm this is what's
happening.

So if you pass along this patch to the person who was seeing that printk
I'd be interested in the results.

--b.


commit 49fc5637ca951d2add808a695768512f8b5bd7ad
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Thu Feb 13 14:04:43 2014 -0500

    dcache: clear DCACHE_DISCONNECTED on child when cleared on parent
    
    Currently DCACHE_DISCONNECTED is only cleared by the filehandle lookup
    code.  That's OK, it's only the filehandle code that really cares about
    this flag.  But it might be kinder to also clear DCACHE_DISCONNECTED on
    regular lookups when we notice it's appropriate.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/dcache.c b/fs/dcache.c
index 265e0ce..c4a9478 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1534,6 +1534,16 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	return dentry;
 }
 
+static void d_inherit_connected(struct dentry *dentry)
+{
+	/*
+	 * If the parent is connected, then the child is too.  (But if
+	 * DISCONNECTED is set, that doesn't tell us anything certain.)
+	 */
+	if (!(dentry->d_parent->d_flags & DCACHE_DISCONNECTED))
+		dentry->d_flags &= ~DCACHE_DISCONNECTED;
+}
+
 /**
  * d_alloc	-	allocate a dcache entry
  * @parent: parent of entry to allocate
@@ -1556,6 +1566,7 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
 	 */
 	__dget_dlock(parent);
 	dentry->d_parent = parent;
+	d_inherit_connected(dentry);
 	list_add(&dentry->d_u.d_child, &parent->d_subdirs);
 	spin_unlock(&parent->d_lock);
 

Re: find_fh_dentry returned a DISCONNECTED directory

[Eric W. Biederman]

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

> Yesterday you passed on a report of this printk from nfsdfh.c firing:
>
> 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
> 	                                dentry);
>
> I think the dentry probably comes from the FILEID_ROOT case of:
>
> 	if (fileid_type == FILEID_ROOT)
> 		dentry = dget(exp->ex_path.dentry);
> 	else {
> 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> 				data_left, fileid_type,
> 				nfsd_acceptable, exp);
>         }
>
> In that case the dentry was found using ordinary filesystem lookups, so
> doesn't go through the same DISCONNECTED-clearing logic as in the case
> of lookups by filehandle.
>
> Probably they have an export root that's not a filesystem root, and the
> lookups happened in the right order?
>
> I suspect that's fine, and that the printk is just stupid, but maybe we
> should clear DISCONNECTED when possible on normal lookups.  The
> following is my attempt, though I'm not sure if d_alloc is the right
> place to do this.  In any case it might help confirm this is what's
> happening.
>
> So if you pass along this patch to the person who was seeing that printk
> I'd be interested in the results.

I have been reading through the dentry code for other reasons and your
patch definitely won't change anything. __d_alloc sets d_flags = 0.
Therefore d_alloc always returns with d_flags == 0.


Eric

> --b.
>
>
> commit 49fc5637ca951d2add808a695768512f8b5bd7ad
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Thu Feb 13 14:04:43 2014 -0500
>
>     dcache: clear DCACHE_DISCONNECTED on child when cleared on parent
>     
>     Currently DCACHE_DISCONNECTED is only cleared by the filehandle lookup
>     code.  That's OK, it's only the filehandle code that really cares about
>     this flag.  But it might be kinder to also clear DCACHE_DISCONNECTED on
>     regular lookups when we notice it's appropriate.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 265e0ce..c4a9478 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1534,6 +1534,16 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
>  	return dentry;
>  }
>  
> +static void d_inherit_connected(struct dentry *dentry)
> +{
> +	/*
> +	 * If the parent is connected, then the child is too.  (But if
> +	 * DISCONNECTED is set, that doesn't tell us anything certain.)
> +	 */
> +	if (!(dentry->d_parent->d_flags & DCACHE_DISCONNECTED))
> +		dentry->d_flags &= ~DCACHE_DISCONNECTED;
> +}
> +
>  /**
>   * d_alloc	-	allocate a dcache entry
>   * @parent: parent of entry to allocate
> @@ -1556,6 +1566,7 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
>  	 */
>  	__dget_dlock(parent);
>  	dentry->d_parent = parent;
> +	d_inherit_connected(dentry);
>  	list_add(&dentry->d_u.d_child, &parent->d_subdirs);
>  	spin_unlock(&parent->d_lock);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > Yesterday you passed on a report of this printk from nfsdfh.c firing:
> >
> > 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
> > 	                                dentry);
> >
> > I think the dentry probably comes from the FILEID_ROOT case of:
> >
> > 	if (fileid_type == FILEID_ROOT)
> > 		dentry = dget(exp->ex_path.dentry);
> > 	else {
> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> > 				data_left, fileid_type,
> > 				nfsd_acceptable, exp);
> >         }
> >
> > In that case the dentry was found using ordinary filesystem lookups, so
> > doesn't go through the same DISCONNECTED-clearing logic as in the case
> > of lookups by filehandle.
> >
> > Probably they have an export root that's not a filesystem root, and the
> > lookups happened in the right order?
> >
> > I suspect that's fine, and that the printk is just stupid, but maybe we
> > should clear DISCONNECTED when possible on normal lookups.  The
> > following is my attempt, though I'm not sure if d_alloc is the right
> > place to do this.  In any case it might help confirm this is what's
> > happening.
> >
> > So if you pass along this patch to the person who was seeing that printk
> > I'd be interested in the results.
> 
> I have been reading through the dentry code for other reasons and your
> patch definitely won't change anything. __d_alloc sets d_flags = 0.
> Therefore d_alloc always returns with d_flags == 0.

You're right, of course.  I wasn't thinking straight.

So the only dentries with DISCONNECTED set are those created with
d_obtain_alias, which is normally only used when you're looking up by
filehandle.

Except btrfs has a weird use in get_default_root().  So maybe they were
running into the dentry that created?

So btrfs should probably be using something else, I'm not sure what.

--b.

Re: find_fh_dentry returned a DISCONNECTED directory

[Eric W. Biederman]

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

> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>> > Yesterday you passed on a report of this printk from nfsdfh.c firing:
>> >
>> > 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
>> > 	                                dentry);
>> >
>> > I think the dentry probably comes from the FILEID_ROOT case of:
>> >
>> > 	if (fileid_type == FILEID_ROOT)
>> > 		dentry = dget(exp->ex_path.dentry);
>> > 	else {
>> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
>> > 				data_left, fileid_type,
>> > 				nfsd_acceptable, exp);
>> >         }
>> >
>> > In that case the dentry was found using ordinary filesystem lookups, so
>> > doesn't go through the same DISCONNECTED-clearing logic as in the case
>> > of lookups by filehandle.
>> >
>> > Probably they have an export root that's not a filesystem root, and the
>> > lookups happened in the right order?
>> >
>> > I suspect that's fine, and that the printk is just stupid, but maybe we
>> > should clear DISCONNECTED when possible on normal lookups.  The
>> > following is my attempt, though I'm not sure if d_alloc is the right
>> > place to do this.  In any case it might help confirm this is what's
>> > happening.
>> >
>> > So if you pass along this patch to the person who was seeing that printk
>> > I'd be interested in the results.
>> 
>> I have been reading through the dentry code for other reasons and your
>> patch definitely won't change anything. __d_alloc sets d_flags = 0.
>> Therefore d_alloc always returns with d_flags == 0.
>
> You're right, of course.  I wasn't thinking straight.
>
> So the only dentries with DISCONNECTED set are those created with
> d_obtain_alias, which is normally only used when you're looking up by
> filehandle.
>
> Except btrfs has a weird use in get_default_root().  So maybe they were
> running into the dentry that created?
>
> So btrfs should probably be using something else, I'm not sure what.

The nfs client also has the case where it uses DISCONNECTED dentries for
directories that are not root on the server.  Which seems very similiar
to the btrfs case.

Usually I would expect some of these things to pass through
d_materialise_unique and become connected.  But I don't know much about
the nfsd usage and how things connected.

Eric

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> 
> >> > Yesterday you passed on a report of this printk from nfsdfh.c firing:
> >> >
> >> > 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
> >> > 	                                dentry);
> >> >
> >> > I think the dentry probably comes from the FILEID_ROOT case of:
> >> >
> >> > 	if (fileid_type == FILEID_ROOT)
> >> > 		dentry = dget(exp->ex_path.dentry);
> >> > 	else {
> >> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> >> > 				data_left, fileid_type,
> >> > 				nfsd_acceptable, exp);
> >> >         }
> >> >
> >> > In that case the dentry was found using ordinary filesystem lookups, so
> >> > doesn't go through the same DISCONNECTED-clearing logic as in the case
> >> > of lookups by filehandle.
> >> >
> >> > Probably they have an export root that's not a filesystem root, and the
> >> > lookups happened in the right order?
> >> >
> >> > I suspect that's fine, and that the printk is just stupid, but maybe we
> >> > should clear DISCONNECTED when possible on normal lookups.  The
> >> > following is my attempt, though I'm not sure if d_alloc is the right
> >> > place to do this.  In any case it might help confirm this is what's
> >> > happening.
> >> >
> >> > So if you pass along this patch to the person who was seeing that printk
> >> > I'd be interested in the results.
> >> 
> >> I have been reading through the dentry code for other reasons and your
> >> patch definitely won't change anything. __d_alloc sets d_flags = 0.
> >> Therefore d_alloc always returns with d_flags == 0.
> >
> > You're right, of course.  I wasn't thinking straight.
> >
> > So the only dentries with DISCONNECTED set are those created with
> > d_obtain_alias, which is normally only used when you're looking up by
> > filehandle.
> >
> > Except btrfs has a weird use in get_default_root().  So maybe they were
> > running into the dentry that created?
> >
> > So btrfs should probably be using something else, I'm not sure what.
> 
> The nfs client also has the case where it uses DISCONNECTED dentries for
> directories that are not root on the server.  Which seems very similiar
> to the btrfs case.

I don't think there's any reason for those to be flagged DISCONNECTED
either.

--b.

> 
> Usually I would expect some of these things to pass through
> d_materialise_unique and become connected.  But I don't know much about
> the nfsd usage and how things connected.
> 
> Eric
> 

Re: find_fh_dentry returned a DISCONNECTED directory

[Eric W. Biederman]

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

> On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>> > On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
>> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> >> 
>> >> > Yesterday you passed on a report of this printk from nfsdfh.c firing:
>> >> >
>> >> > 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
>> >> > 	                                dentry);
>> >> >
>> >> > I think the dentry probably comes from the FILEID_ROOT case of:
>> >> >
>> >> > 	if (fileid_type == FILEID_ROOT)
>> >> > 		dentry = dget(exp->ex_path.dentry);
>> >> > 	else {
>> >> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
>> >> > 				data_left, fileid_type,
>> >> > 				nfsd_acceptable, exp);
>> >> >         }
>> >> >
>> >> > In that case the dentry was found using ordinary filesystem lookups, so
>> >> > doesn't go through the same DISCONNECTED-clearing logic as in the case
>> >> > of lookups by filehandle.
>> >> >
>> >> > Probably they have an export root that's not a filesystem root, and the
>> >> > lookups happened in the right order?
>> >> >
>> >> > I suspect that's fine, and that the printk is just stupid, but maybe we
>> >> > should clear DISCONNECTED when possible on normal lookups.  The
>> >> > following is my attempt, though I'm not sure if d_alloc is the right
>> >> > place to do this.  In any case it might help confirm this is what's
>> >> > happening.
>> >> >
>> >> > So if you pass along this patch to the person who was seeing that printk
>> >> > I'd be interested in the results.
>> >> 
>> >> I have been reading through the dentry code for other reasons and your
>> >> patch definitely won't change anything. __d_alloc sets d_flags = 0.
>> >> Therefore d_alloc always returns with d_flags == 0.
>> >
>> > You're right, of course.  I wasn't thinking straight.
>> >
>> > So the only dentries with DISCONNECTED set are those created with
>> > d_obtain_alias, which is normally only used when you're looking up by
>> > filehandle.
>> >
>> > Except btrfs has a weird use in get_default_root().  So maybe they were
>> > running into the dentry that created?
>> >
>> > So btrfs should probably be using something else, I'm not sure what.
>> 
>> The nfs client also has the case where it uses DISCONNECTED dentries for
>> directories that are not root on the server.  Which seems very similiar
>> to the btrfs case.
>
> I don't think there's any reason for those to be flagged DISCONNECTED
> either.

The only practical difference between the two cases is how quickly it is
desirable to connect the entries.

The disconnected dentries processed by exportfs are dentries that we
want to connect immediately, and it is an error/problem to have the
disconnected after processing.

The dentries that are the roots of file systems we want to connect them
if we get the chance with d_materialise_unique but we don't care if they
go long periods without being connected.

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.

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.

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

Eric

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 07:49:35AM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> 
> >> > On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
> >> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> >> 
> >> >> > Yesterday you passed on a report of this printk from nfsdfh.c firing:
> >> >> >
> >> >> > 	printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
> >> >> > 	                                dentry);
> >> >> >
> >> >> > I think the dentry probably comes from the FILEID_ROOT case of:
> >> >> >
> >> >> > 	if (fileid_type == FILEID_ROOT)
> >> >> > 		dentry = dget(exp->ex_path.dentry);
> >> >> > 	else {
> >> >> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> >> >> > 				data_left, fileid_type,
> >> >> > 				nfsd_acceptable, exp);
> >> >> >         }
> >> >> >
> >> >> > In that case the dentry was found using ordinary filesystem lookups, so
> >> >> > doesn't go through the same DISCONNECTED-clearing logic as in the case
> >> >> > of lookups by filehandle.
> >> >> >
> >> >> > Probably they have an export root that's not a filesystem root, and the
> >> >> > lookups happened in the right order?
> >> >> >
> >> >> > I suspect that's fine, and that the printk is just stupid, but maybe we
> >> >> > should clear DISCONNECTED when possible on normal lookups.  The
> >> >> > following is my attempt, though I'm not sure if d_alloc is the right
> >> >> > place to do this.  In any case it might help confirm this is what's
> >> >> > happening.
> >> >> >
> >> >> > So if you pass along this patch to the person who was seeing that printk
> >> >> > I'd be interested in the results.
> >> >> 
> >> >> I have been reading through the dentry code for other reasons and your
> >> >> patch definitely won't change anything. __d_alloc sets d_flags = 0.
> >> >> Therefore d_alloc always returns with d_flags == 0.
> >> >
> >> > You're right, of course.  I wasn't thinking straight.
> >> >
> >> > So the only dentries with DISCONNECTED set are those created with
> >> > d_obtain_alias, which is normally only used when you're looking up by
> >> > filehandle.
> >> >
> >> > Except btrfs has a weird use in get_default_root().  So maybe they were
> >> > running into the dentry that created?
> >> >
> >> > So btrfs should probably be using something else, I'm not sure what.
> >> 
> >> The nfs client also has the case where it uses DISCONNECTED dentries for
> >> directories that are not root on the server.  Which seems very similiar
> >> to the btrfs case.
> >
> > I don't think there's any reason for those to be flagged DISCONNECTED
> > either.
> 
> The only practical difference between the two cases is how quickly it is
> desirable to connect the entries.
> 
> The disconnected dentries processed by exportfs are dentries that we
> want to connect immediately, and it is an error/problem to have the
> disconnected after processing.
> 
> The dentries that are the roots of file systems we want to connect them
> if we get the chance with d_materialise_unique but we don't care if they
> go long periods without being connected.
> 
> 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.)

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

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

(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?)

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


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

--b.

Re: find_fh_dentry returned a DISCONNECTED directory

[Josef Bacik]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 02/14/2014 11:14 AM, J. Bruce Fields wrote:
> On Fri, Feb 14, 2014 at 07:49:35AM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>>> On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman
>>> wrote:
>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>>> 
>>>>> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman
>>>>> wrote:
>>>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>>>>> 
>>>>>>> Yesterday you passed on a report of this printk from
>>>>>>> nfsdfh.c firing:
>>>>>>> 
>>>>>>> printk("nfsd: find_fh_dentry returned a DISCONNECTED
>>>>>>> directory: %pd2\n", dentry);
>>>>>>> 
>>>>>>> I think the dentry probably comes from the FILEID_ROOT
>>>>>>> case of:
>>>>>>> 
>>>>>>> if (fileid_type == FILEID_ROOT) dentry =
>>>>>>> dget(exp->ex_path.dentry); else { dentry =
>>>>>>> exportfs_decode_fh(exp->ex_path.mnt, fid, data_left,
>>>>>>> fileid_type, nfsd_acceptable, exp); }
>>>>>>> 
>>>>>>> In that case the dentry was found using ordinary
>>>>>>> filesystem lookups, so doesn't go through the same
>>>>>>> DISCONNECTED-clearing logic as in the case of lookups
>>>>>>> by filehandle.
>>>>>>> 
>>>>>>> Probably they have an export root that's not a
>>>>>>> filesystem root, and the lookups happened in the right
>>>>>>> order?
>>>>>>> 
>>>>>>> I suspect that's fine, and that the printk is just
>>>>>>> stupid, but maybe we should clear DISCONNECTED when
>>>>>>> possible on normal lookups.  The following is my
>>>>>>> attempt, though I'm not sure if d_alloc is the right 
>>>>>>> place to do this.  In any case it might help confirm
>>>>>>> this is what's happening.
>>>>>>> 
>>>>>>> So if you pass along this patch to the person who was
>>>>>>> seeing that printk I'd be interested in the results.
>>>>>> 
>>>>>> I have been reading through the dentry code for other
>>>>>> reasons and your patch definitely won't change anything.
>>>>>> __d_alloc sets d_flags = 0. Therefore d_alloc always
>>>>>> returns with d_flags == 0.
>>>>> 
>>>>> You're right, of course.  I wasn't thinking straight.
>>>>> 
>>>>> So the only dentries with DISCONNECTED set are those
>>>>> created with d_obtain_alias, which is normally only used
>>>>> when you're looking up by filehandle.
>>>>> 
>>>>> Except btrfs has a weird use in get_default_root().  So
>>>>> maybe they were running into the dentry that created?
>>>>> 
>>>>> So btrfs should probably be using something else, I'm not
>>>>> sure what.
>>>> 
>>>> The nfs client also has the case where it uses DISCONNECTED
>>>> dentries for directories that are not root on the server.
>>>> Which seems very similiar to the btrfs case.
>>> 
>>> I don't think there's any reason for those to be flagged
>>> DISCONNECTED either.
>> 
>> The only practical difference between the two cases is how
>> quickly it is desirable to connect the entries.
>> 
>> The disconnected dentries processed by exportfs are dentries that
>> we want to connect immediately, and it is an error/problem to
>> have the disconnected after processing.
>> 
>> The dentries that are the roots of file systems we want to
>> connect them if we get the chance with d_materialise_unique but
>> we don't care if they go long periods without being connected.
>> 
>> 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.)
> 
>> 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.
> 
>> 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.
> 
> (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?)
> 
>> 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.
> 
> 
> 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.)
> 

Ok using d_make_root makes us leak inodes on umount (I don't know why,
I'm not drunk enough yet.)  I haven't tried it yet but I'm almost 100%
positive if I just clear out DISCONNECTED we will BUG_ON() in
d_splice_alias() when we go to walk into the subvol we've mounted with
the default subvol option.  So what I think I need to do is use use
d_materialise_unique in our lookup function instead of
d_splice_alias() and keep using d_obtain_alias() in get_default_root()
and just clear DISOCONNECTED on success.  Does this sound crazy to
anybody else?  Thanks,

Josef
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS/kaBAAoJEANb+wAKly3BXp8P/RiWEPYsJXwmD9N2JamQZawa
p5Fh507ZX3PRaOAZ6zgWB2uN4xRQtqFpsIjoZl6c2NoxnR4L89d4x/ExBF39WE2t
PmK3Ot/KJ6GXBVHSyexWJMix8R8u40eKHJcyywqj35HhHML47Ll1fy6k2vmRno5q
FVP+E0kLiBOH0W2ae8o7kPImLp2Ue7moDvExlqKTI3jeRPQI5u+/lH4uj8oVQXx4
/HNoZ7+htzi0Eb+mA+ve0k2/efiUlPloerE0oFUaCrrOF0HMLAVoXxhiDR7eDM96
nZ1LHZ422DOBRCBQ6kpDo7iAebQyRKx6rdkdr/n5/5Bs0iTevg3bWy8WEbMP+NvW
OR+mQ4r3X8J7t91Dp15OJuiEPhyH6lb2McPcxq/ozRDcGR0enZ1iYGl/GNw02eaw
1/JS4+nwokLCcEvTiW16n8sLGv+iU4fXawisjdKs76KQGO+rzdOd2msYjnOF8ZgT
YTO4k689qlWJu6TtY8iC0fRStIksTAMesMmCoCBl+2zkGsHMS9tMQt3kzIF91UP4
WTuZOBxdqByKmQiWy0INKhYXOSVxAUs27JuaHPIBUz65fDDu80IW+2Ih1sro73y/
+t9+7vSueviUub1azUocEdYxuc8mDQ7totpzrJBYHCF7C1T2DZV5nEpGUHD/d19R
3QwsV2oPiu230jh+nJR2
=VIUq
-----END PGP SIGNATURE-----

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 11:38:25AM -0500, Josef Bacik wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 02/14/2014 11:14 AM, J. Bruce Fields wrote:
> > On Fri, Feb 14, 2014 at 07:49:35AM -0800, Eric W. Biederman wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> 
> >>> On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman
> >>> wrote:
> >>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >>>> 
> >>>>> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman
> >>>>> wrote:
> >>>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >>>>>> 
> >>>>>>> Yesterday you passed on a report of this printk from
> >>>>>>> nfsdfh.c firing:
> >>>>>>> 
> >>>>>>> printk("nfsd: find_fh_dentry returned a DISCONNECTED
> >>>>>>> directory: %pd2\n", dentry);
> >>>>>>> 
> >>>>>>> I think the dentry probably comes from the FILEID_ROOT
> >>>>>>> case of:
> >>>>>>> 
> >>>>>>> if (fileid_type == FILEID_ROOT) dentry =
> >>>>>>> dget(exp->ex_path.dentry); else { dentry =
> >>>>>>> exportfs_decode_fh(exp->ex_path.mnt, fid, data_left,
> >>>>>>> fileid_type, nfsd_acceptable, exp); }
> >>>>>>> 
> >>>>>>> In that case the dentry was found using ordinary
> >>>>>>> filesystem lookups, so doesn't go through the same
> >>>>>>> DISCONNECTED-clearing logic as in the case of lookups
> >>>>>>> by filehandle.
> >>>>>>> 
> >>>>>>> Probably they have an export root that's not a
> >>>>>>> filesystem root, and the lookups happened in the right
> >>>>>>> order?
> >>>>>>> 
> >>>>>>> I suspect that's fine, and that the printk is just
> >>>>>>> stupid, but maybe we should clear DISCONNECTED when
> >>>>>>> possible on normal lookups.  The following is my
> >>>>>>> attempt, though I'm not sure if d_alloc is the right 
> >>>>>>> place to do this.  In any case it might help confirm
> >>>>>>> this is what's happening.
> >>>>>>> 
> >>>>>>> So if you pass along this patch to the person who was
> >>>>>>> seeing that printk I'd be interested in the results.
> >>>>>> 
> >>>>>> I have been reading through the dentry code for other
> >>>>>> reasons and your patch definitely won't change anything.
> >>>>>> __d_alloc sets d_flags = 0. Therefore d_alloc always
> >>>>>> returns with d_flags == 0.
> >>>>> 
> >>>>> You're right, of course.  I wasn't thinking straight.
> >>>>> 
> >>>>> So the only dentries with DISCONNECTED set are those
> >>>>> created with d_obtain_alias, which is normally only used
> >>>>> when you're looking up by filehandle.
> >>>>> 
> >>>>> Except btrfs has a weird use in get_default_root().  So
> >>>>> maybe they were running into the dentry that created?
> >>>>> 
> >>>>> So btrfs should probably be using something else, I'm not
> >>>>> sure what.
> >>>> 
> >>>> The nfs client also has the case where it uses DISCONNECTED
> >>>> dentries for directories that are not root on the server.
> >>>> Which seems very similiar to the btrfs case.
> >>> 
> >>> I don't think there's any reason for those to be flagged
> >>> DISCONNECTED either.
> >> 
> >> The only practical difference between the two cases is how
> >> quickly it is desirable to connect the entries.
> >> 
> >> The disconnected dentries processed by exportfs are dentries that
> >> we want to connect immediately, and it is an error/problem to
> >> have the disconnected after processing.
> >> 
> >> The dentries that are the roots of file systems we want to
> >> connect them if we get the chance with d_materialise_unique but
> >> we don't care if they go long periods without being connected.
> >> 
> >> 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.)
> > 
> >> 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.
> > 
> >> 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.
> > 
> > (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?)
> > 
> >> 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.
> > 
> > 
> > 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.)
> > 
> 
> Ok using d_make_root makes us leak inodes on umount (I don't know why,
> I'm not drunk enough yet.)

Oh, well, a little early in the morning, but that's a solveable problem.

> I haven't tried it yet but I'm almost 100%
> positive if I just clear out DISCONNECTED we will BUG_ON() in
> d_splice_alias() when we go to walk into the subvol we've mounted with
> the default subvol option.

I'd expect it not to BUG_ON(), but instead to fall into the !new case
and create a second alias for that directory.  Which probably actually
causes the same problems d_make_root does.

> So what I think I need to do is use use
> d_materialise_unique in our lookup function instead of
> d_splice_alias() and keep using d_obtain_alias() in get_default_root()
> and just clear DISOCONNECTED on success.  Does this sound crazy to
> anybody else?  Thanks,

Probably not crazy.

--b.

> 
> Josef
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJS/kaBAAoJEANb+wAKly3BXp8P/RiWEPYsJXwmD9N2JamQZawa
> p5Fh507ZX3PRaOAZ6zgWB2uN4xRQtqFpsIjoZl6c2NoxnR4L89d4x/ExBF39WE2t
> PmK3Ot/KJ6GXBVHSyexWJMix8R8u40eKHJcyywqj35HhHML47Ll1fy6k2vmRno5q
> FVP+E0kLiBOH0W2ae8o7kPImLp2Ue7moDvExlqKTI3jeRPQI5u+/lH4uj8oVQXx4
> /HNoZ7+htzi0Eb+mA+ve0k2/efiUlPloerE0oFUaCrrOF0HMLAVoXxhiDR7eDM96
> nZ1LHZ422DOBRCBQ6kpDo7iAebQyRKx6rdkdr/n5/5Bs0iTevg3bWy8WEbMP+NvW
> OR+mQ4r3X8J7t91Dp15OJuiEPhyH6lb2McPcxq/ozRDcGR0enZ1iYGl/GNw02eaw
> 1/JS4+nwokLCcEvTiW16n8sLGv+iU4fXawisjdKs76KQGO+rzdOd2msYjnOF8ZgT
> YTO4k689qlWJu6TtY8iC0fRStIksTAMesMmCoCBl+2zkGsHMS9tMQt3kzIF91UP4
> WTuZOBxdqByKmQiWy0INKhYXOSVxAUs27JuaHPIBUz65fDDu80IW+2Ih1sro73y/
> +t9+7vSueviUub1azUocEdYxuc8mDQ7totpzrJBYHCF7C1T2DZV5nEpGUHD/d19R
> 3QwsV2oPiu230jh+nJR2
> =VIUq
> -----END PGP SIGNATURE-----

Re: find_fh_dentry returned a DISCONNECTED directory

[Josef Bacik]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/14/2014 11:45 AM, J. Bruce Fields wrote:
> On Fri, Feb 14, 2014 at 11:38:25AM -0500, Josef Bacik wrote:
> 
> 
> On 02/14/2014 11:14 AM, J. Bruce Fields wrote:
>>>> On Fri, Feb 14, 2014 at 07:49:35AM -0800, Eric W. Biederman
>>>> wrote:
>>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>>>> 
>>>>>> On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W.
>>>>>> Biederman wrote:
>>>>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>>>>>> 
>>>>>>>> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W.
>>>>>>>> Biederman wrote:
>>>>>>>>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>>>>>>>> 
>>>>>>>>>> Yesterday you passed on a report of this printk
>>>>>>>>>> from nfsdfh.c firing:
>>>>>>>>>> 
>>>>>>>>>> printk("nfsd: find_fh_dentry returned a
>>>>>>>>>> DISCONNECTED directory: %pd2\n", dentry);
>>>>>>>>>> 
>>>>>>>>>> I think the dentry probably comes from the
>>>>>>>>>> FILEID_ROOT case of:
>>>>>>>>>> 
>>>>>>>>>> if (fileid_type == FILEID_ROOT) dentry = 
>>>>>>>>>> dget(exp->ex_path.dentry); else { dentry = 
>>>>>>>>>> exportfs_decode_fh(exp->ex_path.mnt, fid,
>>>>>>>>>> data_left, fileid_type, nfsd_acceptable, exp); }
>>>>>>>>>> 
>>>>>>>>>> In that case the dentry was found using ordinary 
>>>>>>>>>> filesystem lookups, so doesn't go through the
>>>>>>>>>> same DISCONNECTED-clearing logic as in the case
>>>>>>>>>> of lookups by filehandle.
>>>>>>>>>> 
>>>>>>>>>> Probably they have an export root that's not a 
>>>>>>>>>> filesystem root, and the lookups happened in the
>>>>>>>>>> right order?
>>>>>>>>>> 
>>>>>>>>>> I suspect that's fine, and that the printk is
>>>>>>>>>> just stupid, but maybe we should clear
>>>>>>>>>> DISCONNECTED when possible on normal lookups.
>>>>>>>>>> The following is my attempt, though I'm not sure
>>>>>>>>>> if d_alloc is the right place to do this.  In any
>>>>>>>>>> case it might help confirm this is what's
>>>>>>>>>> happening.
>>>>>>>>>> 
>>>>>>>>>> So if you pass along this patch to the person who
>>>>>>>>>> was seeing that printk I'd be interested in the
>>>>>>>>>> results.
>>>>>>>>> 
>>>>>>>>> I have been reading through the dentry code for
>>>>>>>>> other reasons and your patch definitely won't
>>>>>>>>> change anything. __d_alloc sets d_flags = 0.
>>>>>>>>> Therefore d_alloc always returns with d_flags ==
>>>>>>>>> 0.
>>>>>>>> 
>>>>>>>> You're right, of course.  I wasn't thinking
>>>>>>>> straight.
>>>>>>>> 
>>>>>>>> So the only dentries with DISCONNECTED set are those 
>>>>>>>> created with d_obtain_alias, which is normally only
>>>>>>>> used when you're looking up by filehandle.
>>>>>>>> 
>>>>>>>> Except btrfs has a weird use in get_default_root().
>>>>>>>> So maybe they were running into the dentry that
>>>>>>>> created?
>>>>>>>> 
>>>>>>>> So btrfs should probably be using something else, I'm
>>>>>>>> not sure what.
>>>>>>> 
>>>>>>> The nfs client also has the case where it uses
>>>>>>> DISCONNECTED dentries for directories that are not root
>>>>>>> on the server. Which seems very similiar to the btrfs
>>>>>>> case.
>>>>>> 
>>>>>> I don't think there's any reason for those to be flagged 
>>>>>> DISCONNECTED either.
>>>>> 
>>>>> The only practical difference between the two cases is how 
>>>>> quickly it is desirable to connect the entries.
>>>>> 
>>>>> The disconnected dentries processed by exportfs are
>>>>> dentries that we want to connect immediately, and it is an
>>>>> error/problem to have the disconnected after processing.
>>>>> 
>>>>> The dentries that are the roots of file systems we want to 
>>>>> connect them if we get the chance with d_materialise_unique
>>>>> but we don't care if they go long periods without being
>>>>> connected.
>>>>> 
>>>>> 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.)
>>>> 
>>>>> 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.
>>>> 
>>>>> 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.
>>>> 
>>>> (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?)
>>>> 
>>>>> 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.
>>>> 
>>>> 
>>>> 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.)
>>>> 
> 
> Ok using d_make_root makes us leak inodes on umount (I don't know
> why, I'm not drunk enough yet.)
> 
>> Oh, well, a little early in the morning, but that's a solveable
>> problem.
> 
> I haven't tried it yet but I'm almost 100% positive if I just clear
> out DISCONNECTED we will BUG_ON() in d_splice_alias() when we go to
> walk into the subvol we've mounted with the default subvol option.
> 
>> I'd expect it not to BUG_ON(), but instead to fall into the !new
>> case and create a second alias for that directory.  Which
>> probably actually causes the same problems d_make_root does.
> 

(weird that thunderbird is treating my replies as the most recent
reply, but whatever)

So neither of these cases happened, I just didn't see _anything_ in
the directory when I mounted the fs.  Once I mounted the parent subvol
and walked into the default subvol I could see everything properly.
This is with clearing DISCONNECTED and still using d_splice_alias() in
lookup.

Once I switched d_splice_alias() to d_materialise_unique() in
btrfs_lookup() everything went back to working as normal.  So I'm
calling it a win and forgetting we ever had this conversation.  Thanks,

Josef
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS/kwfAAoJEANb+wAKly3BHSIP/jx01XDxT98Tk4TCRz2R12Cb
HmgWZ8etu82MPp/KChuXnidxJU00H7BOfaY4vs1q8XFz+MZg3ZCiazp5zu2RVPct
yjMYRY7XPEQcce0Oj5h40dfhclJ61eSupuUd7u+gNpNErsUdaFCpbtXGqVeGAuI4
lMbi8Tqd5YKNa3e+bs8OHEGRG+pQv1Ak2D+dcS+KDn3/Np38wC9eGRsQsZocubQf
EpjERcX3R8ug3k5C/sX15SkpVfHWZS2ydL0ypnOVAI5XZD9ufT+HyyAqhgFZFPlK
WhDTWH54gnq6e2RxU3bKN+q7VzwBD2L/82wHk0/bykXDL+56JRJ0MMptBimWt2M2
QH7+aUN3pM/hm0TiW3qC6WkfM4WBdEME/Mzf8s5vVJT4gQj7u0Xh3RlOX0Y+OEwV
MvICzlXNNT9jbp7EHHzDeJuSbvpLmvVOL24Tf9UmjYO2WXHIJK2bSW1ai2V5v+zx
jZ+NeAg8tfkpxKANbDxtOAzrehHTk8gPimsO3QXFlRvUyUggZdd524Chl07dxnQZ
JNvn4C9JrRkYE32d4dKrjmJvnQXsFKw1KlCvp14nLpQP6q5+J70KRo85NtcpK3fo
tUU24jzCD7m6W+Y5tT5rJgDLz7IAmTZ/1WTI9bc9YMk61ci7GlLE0g4hYbTWfS5h
Q0A5+xddyQT/I5HtCoPZ
=dBiR
-----END PGP SIGNATURE-----

Re: find_fh_dentry returned a DISCONNECTED directory

[Eric W. Biederman]

Josef Bacik <jbacik@fb.com> writes:

> So neither of these cases happened, I just didn't see _anything_ in
> the directory when I mounted the fs.  Once I mounted the parent subvol
> and walked into the default subvol I could see everything properly.
> This is with clearing DISCONNECTED and still using d_splice_alias() in
> lookup.
>
> Once I switched d_splice_alias() to d_materialise_unique() in
> btrfs_lookup() everything went back to working as normal.  So I'm
> calling it a win and forgetting we ever had this conversation.
> Thanks,

Yes. d_splice_alias won't find anything unless DCACHE_DISCONNECTED is
set, and dmaterialse_unique is definitely what you want.

Eric

Re: find_fh_dentry returned a DISCONNECTED directory

[Eric W. Biederman]

Josef Bacik <jbacik@fb.com> writes:

> Ok using d_make_root makes us leak inodes on umount (I don't know why,
> I'm not drunk enough yet.)  I haven't tried it yet but I'm almost 100%
> positive if I just clear out DISCONNECTED we will BUG_ON() in
> d_splice_alias() when we go to walk into the subvol we've mounted with
> the default subvol option.  So what I think I need to do is use use
> d_materialise_unique in our lookup function instead of
> d_splice_alias() and keep using d_obtain_alias() in get_default_root()
> and just clear DISOCONNECTED on success.  Does this sound crazy to
> anybody else?  Thanks,

That sounds exactly right.

Eric

Re: find_fh_dentry returned a DISCONNECTED directory

[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

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 09:02:30AM -0800, Eric W. Biederman wrote:
> "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.

You're right, I forgot that.

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

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

I'm all for it.

bfields@pepper:linux-2.6$ git grep DCACHE_DISCONNECTED|wc -l
24

So it's not so terrible.  All but a few are in dcache.c and expfs.c.

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

OK.

--b.

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 05:19:29PM -0500, J. Bruce Fields wrote:
> On Fri, Feb 14, 2014 at 09:02:30AM -0800, Eric W. Biederman wrote:
> > "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.
> 
> You're right, I forgot that.
> 
> > >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.
> 
> Sounds good.

So, something like this.

(Untested, and will also screw up nilfs2 until we fix d_splice_alias not
to require DCACHE_DISCONNECTED dentries.)

--b.

commit c2bc8557e935b24caa2174403b093bc00dc62112
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Feb 14 17:35:37 2014 -0500

    dcache: d_obtain_alias callers don't all want DISCONNECTED
    
    There are a few d_obtain_alias callers that are using it to get the
    root of a filesystem which may already have an alias somewhere else.
    
    This is not the same as the filehandle-lookup case, and none of them
    actually need DCACHE_DISCONNECTED set.
    
    In the btrfs case this was causing a spurious printk from
    nfsd/nfsfh.c:fh_verify when it found an unexpected DCACHE_DISCONNECTED
    dentry.
    
    Which isn't really a serious problem, but it would really be clearer if
    we reserved DCACHE_DISCONNECTED for those cases where it's actually
    needed.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 97cc241..07ce96b 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -925,7 +925,7 @@ setup_root:
 		return dget(sb->s_root);
 	}
 
-	return d_obtain_alias(inode);
+	return d_obtain_alias_root(inode);
 }
 
 static int btrfs_fill_super(struct super_block *sb,
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 2df963f..9666950 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -732,7 +732,7 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
 				goto out;
 			}
 		} else {
-			root = d_obtain_alias(inode);
+			root = d_obtain_alias_root(inode);
 		}
 		ceph_init_dentry(root);
 		dout("open_root_inode success, root dentry is %p\n", root);
diff --git a/fs/dcache.c b/fs/dcache.c
index b4572fa..f30f783 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1832,25 +1832,7 @@ struct dentry *d_find_any_alias(struct inode *inode)
 }
 EXPORT_SYMBOL(d_find_any_alias);
 
-/**
- * d_obtain_alias - find or allocate a dentry for a given inode
- * @inode: inode to allocate the dentry for
- *
- * Obtain a dentry for an inode resulting from NFS filehandle conversion or
- * similar open by handle operations.  The returned dentry may be anonymous,
- * or may have a full name (if the inode was already in the cache).
- *
- * When called on a directory inode, we must ensure that the inode only ever
- * has one dentry.  If a dentry is found, that is returned instead of
- * allocating a new one.
- *
- * On successful return, the reference to the inode has been transferred
- * to the dentry.  In case of an error the reference on the inode is released.
- * To make it easier to use in export operations a %NULL or IS_ERR inode may
- * be passed in and will be the error will be propagate to the return value,
- * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
- */
-struct dentry *d_obtain_alias(struct inode *inode)
+struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 {
 	static const struct qstr anonstring = QSTR_INIT("/", 1);
 	struct dentry *tmp;
@@ -1881,7 +1863,10 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	}
 
 	/* attach a disconnected dentry */
-	add_flags = d_flags_for_inode(inode) | DCACHE_DISCONNECTED;
+	add_flags = d_flags_for_inode(inode);
+
+	if (disconnected)
+		add_flags |= DCACHE_DISCONNECTED;
 
 	spin_lock(&tmp->d_lock);
 	tmp->d_inode = inode;
@@ -1902,9 +1887,53 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	iput(inode);
 	return res;
 }
+
+/**
+ * d_obtain_alias - find or allocate a DISCONNECTED dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain a dentry for an inode resulting from NFS filehandle conversion or
+ * similar open by handle operations.  The returned dentry may be anonymous,
+ * or may have a full name (if the inode was already in the cache).
+ *
+ * When called on a directory inode, we must ensure that the inode only ever
+ * has one dentry.  If a dentry is found, that is returned instead of
+ * allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and the error will be propagated to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
+ */
+struct dentry *d_obtain_alias(struct inode *inode)
+{
+	return __d_obtain_alias(inode, 1);
+}
 EXPORT_SYMBOL(d_obtain_alias);
 
 /**
+ * d_obtain_alias_root - find or allocate a dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain an IS_ROOT dentry for the root of a filesystem.
+ *
+ * We must ensure that directory inodes only ever have one dentry.  If a
+ * dentry is found, that is returned instead of allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry.  In case of an error the reference on the inode is
+ * released.  A %NULL or IS_ERR inode may be passed in and will be the
+ * error will be propagate to the return value, with a %NULL @inode
+ * replaced by ERR_PTR(-ESTALE).
+ */
+struct dentry *d_obtain_alias_root(struct inode *inode)
+{
+	return __d_obtain_alias(inode, 0);
+}
+EXPORT_SYMBOL(d_obtain_alias_root);
+
+/**
  * d_add_ci - lookup or allocate new dentry with case-exact name
  * @inode:  the inode case-insensitive lookup has found
  * @dentry: the negative dentry that was passed to the parent's lookup func
diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index 66984a9..7b32522 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -112,7 +112,7 @@ struct dentry *nfs_get_root(struct super_block *sb, struct nfs_fh *mntfh,
 	 * if the dentry tree reaches them; however if the dentry already
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
-	ret = d_obtain_alias(inode);
+	ret = d_obtain_alias_root(inode);
 	if (IS_ERR(ret)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
 		goto out;
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 7ac2a12..a518a73 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -942,7 +942,7 @@ static int nilfs_get_root_dentry(struct super_block *sb,
 			iput(inode);
 		}
 	} else {
-		dentry = d_obtain_alias(inode);
+		dentry = d_obtain_alias_root(inode);
 		if (IS_ERR(dentry)) {
 			ret = PTR_ERR(dentry);
 			goto failed_dentry;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index bf72e9a..2973aab 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -247,6 +247,7 @@ extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
 extern struct dentry *d_find_any_alias(struct inode *inode);
 extern struct dentry * d_obtain_alias(struct inode *);
+extern struct dentry * d_obtain_alias_root(struct inode *);
 extern void shrink_dcache_sb(struct super_block *);
 extern void shrink_dcache_parent(struct dentry *);
 extern void shrink_dcache_for_umount(struct super_block *);

Re: find_fh_dentry returned a DISCONNECTED directory

[Josef Bacik]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 02/13/2014 10:30 PM, J. Bruce Fields wrote:
> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>>> Yesterday you passed on a report of this printk from nfsdfh.c
>>> firing:
>>> 
>>> printk("nfsd: find_fh_dentry returned a DISCONNECTED directory:
>>> %pd2\n", dentry);
>>> 
>>> I think the dentry probably comes from the FILEID_ROOT case
>>> of:
>>> 
>>> if (fileid_type == FILEID_ROOT) dentry =
>>> dget(exp->ex_path.dentry); else { dentry =
>>> exportfs_decode_fh(exp->ex_path.mnt, fid, data_left,
>>> fileid_type, nfsd_acceptable, exp); }
>>> 
>>> In that case the dentry was found using ordinary filesystem
>>> lookups, so doesn't go through the same DISCONNECTED-clearing
>>> logic as in the case of lookups by filehandle.
>>> 
>>> Probably they have an export root that's not a filesystem root,
>>> and the lookups happened in the right order?
>>> 
>>> I suspect that's fine, and that the printk is just stupid, but
>>> maybe we should clear DISCONNECTED when possible on normal
>>> lookups.  The following is my attempt, though I'm not sure if
>>> d_alloc is the right place to do this.  In any case it might
>>> help confirm this is what's happening.
>>> 
>>> So if you pass along this patch to the person who was seeing
>>> that printk I'd be interested in the results.
>> 
>> I have been reading through the dentry code for other reasons and
>> your patch definitely won't change anything. __d_alloc sets
>> d_flags = 0. Therefore d_alloc always returns with d_flags == 0.
> 
> You're right, of course.  I wasn't thinking straight.
> 
> So the only dentries with DISCONNECTED set are those created with 
> d_obtain_alias, which is normally only used when you're looking up
> by filehandle.
> 
> Except btrfs has a weird use in get_default_root().  So maybe they
> were running into the dentry that created?
> 
> So btrfs should probably be using something else, I'm not sure
> what.
> 

Urgh sorry for the wild goose chase Bruce, I'll fix this up,

Josef
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS/iVdAAoJEANb+wAKly3BfE0P/2U3wV18Dr4IRzZN/sOIlv4s
4ni9wPbGdOocySmFTvqAFDxixRzbfW+5JCigbFuTZFHpuJTffnCBwoXJVoIrmnWw
dfStqJBOG/yrAKNK+2gV1W8CYdNwKD5/QYt/9MNj+Txn2PQXiNJIPPaiKiiJfyQz
Ht+ABvyTDb00YcsAss5ldbYDVCvZ6UzfWZZ9j2lfDbAL4zsF2p3YUcANTiNKPe01
5crJ1//5bvlFNFAgLqJNJSq1JlHdbD3ANr86Cv1ikDercmvyIeuUY1Yk5kpH/+p+
BqQATQ3qvJ9C6U/sbZyBaNa86Jt7NyxgD4HgmyeTQHtk7H42Jv2HSrnPy31MYF44
VksMTkaSNFrR6RUI88X82JHuax/bhT8MBjBuXbrD50AnFXzZuUT70lcUGDfIoUni
+BgHzaCa06DHgzZCEmnP0/KEy6f5Zty9eEMMgqL30WtZGsKPZA98RsobuV3IVw67
dW6bnmdx9kIZRhyUM/hh8Cqyep7Y54lxBzUtG7KMYZDNP9tZsjCtWxWIq+j9htYW
RJ6KPs9rnjdJoRp7Ca8jGaTbuOa8OmhS62C9sFmmiXbuh40KNbSKea+rO63roUs5
5MwqYjqv8OvHNukxtRQyX2FxZwJojx8IUXf18urJnzvffCOAw2tg47GRkbRKVE51
WqW/C6HcSxxOOUr92Lup
=ZYeX
-----END PGP SIGNATURE-----

Re: find_fh_dentry returned a DISCONNECTED directory

[Josef Bacik]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 02/13/2014 10:30 PM, J. Bruce Fields wrote:
> On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>> 
>>> Yesterday you passed on a report of this printk from nfsdfh.c
>>> firing:
>>> 
>>> printk("nfsd: find_fh_dentry returned a DISCONNECTED directory:
>>> %pd2\n", dentry);
>>> 
>>> I think the dentry probably comes from the FILEID_ROOT case
>>> of:
>>> 
>>> if (fileid_type == FILEID_ROOT) dentry =
>>> dget(exp->ex_path.dentry); else { dentry =
>>> exportfs_decode_fh(exp->ex_path.mnt, fid, data_left,
>>> fileid_type, nfsd_acceptable, exp); }
>>> 
>>> In that case the dentry was found using ordinary filesystem
>>> lookups, so doesn't go through the same DISCONNECTED-clearing
>>> logic as in the case of lookups by filehandle.
>>> 
>>> Probably they have an export root that's not a filesystem root,
>>> and the lookups happened in the right order?
>>> 
>>> I suspect that's fine, and that the printk is just stupid, but
>>> maybe we should clear DISCONNECTED when possible on normal
>>> lookups.  The following is my attempt, though I'm not sure if
>>> d_alloc is the right place to do this.  In any case it might
>>> help confirm this is what's happening.
>>> 
>>> So if you pass along this patch to the person who was seeing
>>> that printk I'd be interested in the results.
>> 
>> I have been reading through the dentry code for other reasons and
>> your patch definitely won't change anything. __d_alloc sets
>> d_flags = 0. Therefore d_alloc always returns with d_flags == 0.
> 
> You're right, of course.  I wasn't thinking straight.
> 
> So the only dentries with DISCONNECTED set are those created with 
> d_obtain_alias, which is normally only used when you're looking up
> by filehandle.
> 
> Except btrfs has a weird use in get_default_root().  So maybe they
> were running into the dentry that created?
> 
> So btrfs should probably be using something else, I'm not sure
> what.
> 

Course now that I look at it I'm not sure what to do.  We know the
location of the inode we want to use as our root dentry, but we could
already have a dentry for this in cache which is why we use
d_obtain_alias().  If we use d_make_root() and then wander into the
directory later we end up with inodes left over.  So should we just
build a path to the location and do the path lookup stuff so we have a
valid dentry?  Thanks,

Josef
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS/jKfAAoJEANb+wAKly3B1q4P/1wgkd/w6QI6A3c40Qw5Oyfk
gTOFcSrCnGpd59wOzCu3B8fKuVygBweJ0vwYMlnNgGjCzqidTt37LCD+rRb1iz0w
BZVSvWS5v8qeaW+qQJJSQH83NX8v+L6zc/5D1stwPIfctXf0WvSijmacV2/BGRgJ
PqZVXd4yQhd24cqonXUzqNswiFOmPUPs8xrSz3NaR4GcRFVMLuRuubpf/pWkotmI
mldm8p+SCrDiQHgXEYYHKW1rHKjxKU4GIc9+Dsx0Jjnv26ITiooM8pY2tWW8w98S
CQv54VQGzzAmyNHQxmv/3sxPg9K4CL+8t7Z4nLQ043K9U9itd+YU/v/vvnzAXvob
KX5amGZc0cWyYHu9e/9i4HXOJmrmEUdwl5megeI47V/LStzkRUggeha+tRzm4xQW
+nT4dpueUK1ezDDPvJdOF9id9WNWaGwxKqg6Mqj9+tsnmOwYll2zaNuoOVUZacWY
Qmv+1JS6gdE8/qLTVjp9I4nTdGN9Cr/sE1EwBVM/GFbCQzAxe4m4uKnXtJ28+Noo
LOg/u27GSssO6oUFA8Mj6dnTEOKtm5QOMDD+4lxRSyWuAHCOvNg6I4SRykHb1o1I
OexIp6DK1Siofj6mAFGG5AZunXYH8uR8aOH4Ctva04vC1D6TlZsI5ycgE6V51NUk
Rv4ICvuwjuu3nZRLLKve
=KK7m
-----END PGP SIGNATURE-----

Re: find_fh_dentry returned a DISCONNECTED directory

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 10:13:36AM -0500, Josef Bacik wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 02/13/2014 10:30 PM, J. Bruce Fields wrote:
> > On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >> 
> >>> Yesterday you passed on a report of this printk from nfsdfh.c
> >>> firing:
> >>> 
> >>> printk("nfsd: find_fh_dentry returned a DISCONNECTED directory:
> >>> %pd2\n", dentry);
> >>> 
> >>> I think the dentry probably comes from the FILEID_ROOT case
> >>> of:
> >>> 
> >>> if (fileid_type == FILEID_ROOT) dentry =
> >>> dget(exp->ex_path.dentry); else { dentry =
> >>> exportfs_decode_fh(exp->ex_path.mnt, fid, data_left,
> >>> fileid_type, nfsd_acceptable, exp); }
> >>> 
> >>> In that case the dentry was found using ordinary filesystem
> >>> lookups, so doesn't go through the same DISCONNECTED-clearing
> >>> logic as in the case of lookups by filehandle.
> >>> 
> >>> Probably they have an export root that's not a filesystem root,
> >>> and the lookups happened in the right order?
> >>> 
> >>> I suspect that's fine, and that the printk is just stupid, but
> >>> maybe we should clear DISCONNECTED when possible on normal
> >>> lookups.  The following is my attempt, though I'm not sure if
> >>> d_alloc is the right place to do this.  In any case it might
> >>> help confirm this is what's happening.
> >>> 
> >>> So if you pass along this patch to the person who was seeing
> >>> that printk I'd be interested in the results.
> >> 
> >> I have been reading through the dentry code for other reasons and
> >> your patch definitely won't change anything. __d_alloc sets
> >> d_flags = 0. Therefore d_alloc always returns with d_flags == 0.
> > 
> > You're right, of course.  I wasn't thinking straight.
> > 
> > So the only dentries with DISCONNECTED set are those created with 
> > d_obtain_alias, which is normally only used when you're looking up
> > by filehandle.
> > 
> > Except btrfs has a weird use in get_default_root().  So maybe they
> > were running into the dentry that created?
> > 
> > So btrfs should probably be using something else, I'm not sure
> > what.
> > 
> 
> Course now that I look at it I'm not sure what to do.  We know the
> location of the inode we want to use as our root dentry, but we could
> already have a dentry for this in cache which is why we use
> d_obtain_alias().

That's exactly the sort of situation that d_obtain_alias() is meant to
help you with.

It's just that you don't want DCACHE_DISCONNECTED set.  I'm not even
sure if that causes any actual bug, but it seems confusing at least.

Maybe it would be worth making a d_obtain_alias_for_root() or something
for yours and nfs's use.

--b.


> If we use d_make_root() and then wander into the
> directory later we end up with inodes left over.  So should we just
> build a path to the location and do the path lookup stuff so we have a
> valid dentry?  Thanks,
> 
> Josef
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJS/jKfAAoJEANb+wAKly3B1q4P/1wgkd/w6QI6A3c40Qw5Oyfk
> gTOFcSrCnGpd59wOzCu3B8fKuVygBweJ0vwYMlnNgGjCzqidTt37LCD+rRb1iz0w
> BZVSvWS5v8qeaW+qQJJSQH83NX8v+L6zc/5D1stwPIfctXf0WvSijmacV2/BGRgJ
> PqZVXd4yQhd24cqonXUzqNswiFOmPUPs8xrSz3NaR4GcRFVMLuRuubpf/pWkotmI
> mldm8p+SCrDiQHgXEYYHKW1rHKjxKU4GIc9+Dsx0Jjnv26ITiooM8pY2tWW8w98S
> CQv54VQGzzAmyNHQxmv/3sxPg9K4CL+8t7Z4nLQ043K9U9itd+YU/v/vvnzAXvob
> KX5amGZc0cWyYHu9e/9i4HXOJmrmEUdwl5megeI47V/LStzkRUggeha+tRzm4xQW
> +nT4dpueUK1ezDDPvJdOF9id9WNWaGwxKqg6Mqj9+tsnmOwYll2zaNuoOVUZacWY
> Qmv+1JS6gdE8/qLTVjp9I4nTdGN9Cr/sE1EwBVM/GFbCQzAxe4m4uKnXtJ28+Noo
> LOg/u27GSssO6oUFA8Mj6dnTEOKtm5QOMDD+4lxRSyWuAHCOvNg6I4SRykHb1o1I
> OexIp6DK1Siofj6mAFGG5AZunXYH8uR8aOH4Ctva04vC1D6TlZsI5ycgE6V51NUk
> Rv4ICvuwjuu3nZRLLKve
> =KK7m
> -----END PGP SIGNATURE-----