Re: find_fh_dentry returned a DISCONNECTED directory

[ebiederm@xmission.com (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