From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: find_fh_dentry returned a DISCONNECTED directory Date: Fri, 14 Feb 2014 07:49:35 -0800 Message-ID: <877g8x1ypc.fsf@xmission.com> References: <20140213212701.GB21982@fieldses.org> <8738jm1ss3.fsf@xmission.com> <20140214033030.GC21982@fieldses.org> <87ob2az5fc.fsf@xmission.com> <20140214144640.GE21982@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain Cc: jbacik@fb.com, linux-fsdevel@vger.kernel.org To: "J. Bruce Fields" Return-path: Received: from out02.mta.xmission.com ([166.70.13.232]:35325 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752195AbaBNPtn (ORCPT ); Fri, 14 Feb 2014 10:49:43 -0500 In-Reply-To: <20140214144640.GE21982@fieldses.org> (J. Bruce Fields's message of "Fri, 14 Feb 2014 09:46:40 -0500") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: "J. Bruce Fields" writes: > On Thu, Feb 13, 2014 at 08:25:43PM -0800, Eric W. Biederman wrote: >> "J. Bruce Fields" writes: >> >> > On Thu, Feb 13, 2014 at 03:45:16PM -0800, Eric W. Biederman wrote: >> >> "J. Bruce Fields" 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