Re: find_fh_dentry returned a DISCONNECTED directory

["J. Bruce Fields" <bfields@fieldses.org>]

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