| Message ID | 20140115151749.GF23999@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-01-15 at 10:17 -0500, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > d_splice_alias can create duplicate directory aliases (in the !new > case), or (in the new case) d_move without holding appropriate locks. It can d_move, because the dentry is known to be disconnected, i.e. it doesn't have a parent for which we could obtain the lock. > d_materialise_unique deals with both of these problems. (The latter > seems to be dealt by trylocks (see __d_unalias), which look like they > could cause spurious lookup failures--but that's at least better than > corrupting the dcache.) > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/dcache.c | 25 +------------------------ > 1 file changed, 1 insertion(+), 24 deletions(-) > > Only lightly tested.... If this is right, then we can also just ditch > d_splice_alias completely, and clean up the various d_find_alias's. > > I think the only reason we have both d_splice_alias and > d_materialise_unique is that the former was written for exportable > filesystems and the latter for distributed filesystems. > > But we have at least one exportable filesystem (fuse) using > d_materialise_unique. And I doubt d_splice_alias was ever completely > correct even for on-disk filesystems. > > Am I missing some subtlety? One subtle difference is that for a non-directory d_splice_alias() will reconnect a DCACHE_DISCONNECTED dentry if one exists, while d_materialise_unique() will not. Does this matter in practice? The small number of extra dentries probably does not matter. Thanks, Miklos > > --b. > > diff --git a/fs/dcache.c b/fs/dcache.c > index 4bdb300..da82fa9 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -1926,33 +1926,10 @@ EXPORT_SYMBOL(d_obtain_alias); > */ > struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry) > { > - struct dentry *new = NULL; > - > if (IS_ERR(inode)) > return ERR_CAST(inode); > > - if (inode && S_ISDIR(inode->i_mode)) { > - spin_lock(&inode->i_lock); > - new = __d_find_alias(inode, 1); > - if (new) { > - BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED)); > - spin_unlock(&inode->i_lock); > - security_d_instantiate(new, inode); > - d_move(new, dentry); > - iput(inode); > - } else { > - /* already taking inode->i_lock, so d_add() by hand */ > - __d_instantiate(dentry, inode); > - spin_unlock(&inode->i_lock); > - security_d_instantiate(dentry, inode); > - d_rehash(dentry); > - } > - } else { > - d_instantiate(dentry, inode); > - if (d_unhashed(dentry)) > - d_rehash(dentry); > - } > - return new; > + return d_materialise_unique(dentry, inode); > } > EXPORT_SYMBOL(d_splice_alias); > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 15, 2014 at 06:34:56PM +0100, Miklos Szeredi wrote: > On Wed, 2014-01-15 at 10:17 -0500, J. Bruce Fields wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > d_splice_alias can create duplicate directory aliases (in the !new > > case), or (in the new case) d_move without holding appropriate locks. > > It can d_move, because the dentry is known to be disconnected, i.e. it > doesn't have a parent for which we could obtain the lock. DCACHE_DISCONNECTED doesn't mean that. When you lookup a dentry by filehandle that dentry is initially marked DCACHE_DISCONNECTED. It is cleared only after reconnect_path() has verified that the dentry is reachable all the way from the root. So !DCACHE_DISCONNECTED implies that the dentry is connected all the way up to the root, but the converse is not true. This has been a source of confusion, but it is explained in Documentation/filesystems/nfs/Exporting. Recently I've cleaned up a few odd uses of DCACHE_DISCONNECTED and rewritten reconnect_path(), partly as an attempt to clarify the situation. Let me know if any of that doesn't look right to you.... > > d_materialise_unique deals with both of these problems. (The latter > > seems to be dealt by trylocks (see __d_unalias), which look like they > > could cause spurious lookup failures--but that's at least better than > > corrupting the dcache.) > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/dcache.c | 25 +------------------------ > > 1 file changed, 1 insertion(+), 24 deletions(-) > > > > Only lightly tested.... If this is right, then we can also just ditch > > d_splice_alias completely, and clean up the various d_find_alias's. > > > > I think the only reason we have both d_splice_alias and > > d_materialise_unique is that the former was written for exportable > > filesystems and the latter for distributed filesystems. > > > > But we have at least one exportable filesystem (fuse) using > > d_materialise_unique. And I doubt d_splice_alias was ever completely > > correct even for on-disk filesystems. > > > > Am I missing some subtlety? > > One subtle difference is that for a non-directory d_splice_alias() will > reconnect a DCACHE_DISCONNECTED dentry if one exists, while > d_materialise_unique() will not. Actually until f80de2cde10350b8d146e375ff8b634e72e6a827 "dcache: don't clear DCACHE_DISCONNECTED too early", it was the reverse: d_materialise_unique cleared DISCONNECTED and d_splice_alias (correctly) did not. The only place where it should be cleared is reconnect_path(). > Does this matter in practice? The small number of extra dentries > probably does not matter. Directories are assumed to have unique aliases. When they don't, the kernel can deadlock or crash. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 15, 2014 at 6:57 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > On Wed, Jan 15, 2014 at 06:34:56PM +0100, Miklos Szeredi wrote: >> On Wed, 2014-01-15 at 10:17 -0500, J. Bruce Fields wrote: >> > From: "J. Bruce Fields" <bfields@redhat.com> >> > >> > d_splice_alias can create duplicate directory aliases (in the !new >> > case), or (in the new case) d_move without holding appropriate locks. >> >> It can d_move, because the dentry is known to be disconnected, i.e. it >> doesn't have a parent for which we could obtain the lock. > > DCACHE_DISCONNECTED doesn't mean that. You're right, but I'm also right, because __d_find_alias() will check IS_ROOT() too. So only "root" disconnected dentries will be moved. > > When you lookup a dentry by filehandle that dentry is initially marked > DCACHE_DISCONNECTED. It is cleared only after reconnect_path() has > verified that the dentry is reachable all the way from the root. > > So !DCACHE_DISCONNECTED implies that the dentry is connected all the way > up to the root, but the converse is not true. > > This has been a source of confusion, but it is explained in > Documentation/filesystems/nfs/Exporting. Recently I've cleaned up a few > odd uses of DCACHE_DISCONNECTED and rewritten reconnect_path(), partly > as an attempt to clarify the situation. > > Let me know if any of that doesn't look right to you.... > >> > d_materialise_unique deals with both of these problems. (The latter >> > seems to be dealt by trylocks (see __d_unalias), which look like they >> > could cause spurious lookup failures--but that's at least better tha >> > corrupting the dcache.) >> > >> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> >> > --- >> > fs/dcache.c | 25 +------------------------ >> > 1 file changed, 1 insertion(+), 24 deletions(-) >> > >> > Only lightly tested.... If this is right, then we can also just ditch >> > d_splice_alias completely, and clean up the various d_find_alias's. >> > >> > I think the only reason we have both d_splice_alias and >> > d_materialise_unique is that the former was written for exportable >> > filesystems and the latter for distributed filesystems. >> > >> > But we have at least one exportable filesystem (fuse) using >> > d_materialise_unique. And I doubt d_splice_alias was ever completely >> > correct even for on-disk filesystems. >> > >> > Am I missing some subtlety? >> >> One subtle difference is that for a non-directory d_splice_alias() will >> reconnect a DCACHE_DISCONNECTED dentry if one exists, while >> d_materialise_unique() will not. > > Actually until f80de2cde10350b8d146e375ff8b634e72e6a827 "dcache: don't > clear DCACHE_DISCONNECTED too early", it was the reverse: > d_materialise_unique cleared DISCONNECTED and d_splice_alias (correctly) > did not. > > The only place where it should be cleared is reconnect_path(). > >> Does this matter in practice? The small number of extra dentries >> probably does not matter. > > Directories are assumed to have unique aliases. When they don't, the > kernel can deadlock or crash. What I meant is that d_materialise_unique() will currently not reuse disconnected *nondirectory* dentries, hence there may be more aliases than necessary. This could easily be fixed, though. Thanks, Miklos > > --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 15, 2014 at 07:25:11PM +0100, Miklos Szeredi wrote: > On Wed, Jan 15, 2014 at 6:57 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > On Wed, Jan 15, 2014 at 06:34:56PM +0100, Miklos Szeredi wrote: > >> On Wed, 2014-01-15 at 10:17 -0500, J. Bruce Fields wrote: > >> > From: "J. Bruce Fields" <bfields@redhat.com> > >> > > >> > d_splice_alias can create duplicate directory aliases (in the !new > >> > case), or (in the new case) d_move without holding appropriate locks. > >> > >> It can d_move, because the dentry is known to be disconnected, i.e. it > >> doesn't have a parent for which we could obtain the lock. > > > > DCACHE_DISCONNECTED doesn't mean that. > > You're right, but I'm also right, because __d_find_alias() will check > IS_ROOT() too. So only "root" disconnected dentries will be moved. You're right, I forgot that check. > >> One subtle difference is that for a non-directory d_splice_alias() will > >> reconnect a DCACHE_DISCONNECTED dentry if one exists, while > >> d_materialise_unique() will not. ... > >> Does this matter in practice? The small number of extra dentries > >> probably does not matter. > > > > Directories are assumed to have unique aliases. When they don't, the > > kernel can deadlock or crash. > > What I meant is that d_materialise_unique() will currently not reuse > disconnected *nondirectory* dentries, hence there may be more aliases > than necessary. This could easily be fixed, though. And, sorry, I did miss that you said "non-directory". But I think you have that backwards: d_splice_alias looks like: if (inode && S_ISDIR(inode->i_mode)) { ... } else { d_instantiate(dentry, inode); if (d_unhashed(dentry)) d_rehash(dentry); } So it ignores any existing aliases in the non-directory case. d_materialise_unique by contrast calls __d_instantiate_unique, which looks like it should avoid adding duplicates. So I think switching everyone to d_materialiase_unique would result in fewer dentries. But I've never seen any complaint about the issue and like you don't see a reason this would matter much either way. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 15, 2014 at 10:17:49AM -0500, bfields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > d_splice_alias can create duplicate directory aliases (in the !new > case), or (in the new case) d_move without holding appropriate locks. > > d_materialise_unique deals with both of these problems. (The latter > seems to be dealt by trylocks (see __d_unalias), which look like they > could cause spurious lookup failures--but that's at least better than > corrupting the dcache.) > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/dcache.c | 25 +------------------------ > 1 file changed, 1 insertion(+), 24 deletions(-) > > Only lightly tested.... If this is right, then we can also just ditch > d_splice_alias completely, and clean up the various d_find_alias's. > > I think the only reason we have both d_splice_alias and > d_materialise_unique is that the former was written for exportable > filesystems and the latter for distributed filesystems. > > But we have at least one exportable filesystem (fuse) using > d_materialise_unique. And I doubt d_splice_alias was ever completely > correct even for on-disk filesystems. > > Am I missing some subtlety? Hm, I just noticed: commit 0d0d110720d7960b77c03c9f2597faaff4b484ae Author: Miklos Szeredi <mszeredi@suse.cz> Date: Mon Sep 16 14:52:00 2013 +0200 GFS2: d_splice_alias() can't return error unless it was given an IS_ERR(inode), which isn't the case here. So clean up the unnecessary error handling in gfs2_create_inode(). This paves the way for real fixes (hence the stable Cc). Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> Signed-off-by: Steven Whitehouse <swhiteho@redhat.com> Cc: stable@vger.kernel.org While the statement is true for the current implementation of d_splice_alias, I don't think it's actually true for any correct implementation of d_splice_alias, which must be able to return at least -ELOOP in the directory case. Does gfs2 need fixing? --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 16, 2014 at 4:41 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > And, sorry, I did miss that you said "non-directory". But I think you > have that backwards: d_splice_alias looks like: > > if (inode && S_ISDIR(inode->i_mode)) { > ... > } else { > d_instantiate(dentry, inode); > if (d_unhashed(dentry)) > d_rehash(dentry); > } > > So it ignores any existing aliases in the non-directory case. Okay. > > d_materialise_unique by contrast calls __d_instantiate_unique, which > looks like it should avoid adding duplicates. > > So I think switching everyone to d_materialiase_unique would result in > fewer dentries. But I've never seen any complaint about the issue and > like you don't see a reason this would matter much either way. So, yes, d_materialise_unique() looks like it has superior functionality compared to d_splice_alias(). Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, 2014-01-16 at 11:10 -0500, J. Bruce Fields wrote: > On Wed, Jan 15, 2014 at 10:17:49AM -0500, bfields wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > d_splice_alias can create duplicate directory aliases (in the !new > > case), or (in the new case) d_move without holding appropriate locks. > > > > d_materialise_unique deals with both of these problems. (The latter > > seems to be dealt by trylocks (see __d_unalias), which look like they > > could cause spurious lookup failures--but that's at least better than > > corrupting the dcache.) > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/dcache.c | 25 +------------------------ > > 1 file changed, 1 insertion(+), 24 deletions(-) > > > > Only lightly tested.... If this is right, then we can also just ditch > > d_splice_alias completely, and clean up the various d_find_alias's. > > > > I think the only reason we have both d_splice_alias and > > d_materialise_unique is that the former was written for exportable > > filesystems and the latter for distributed filesystems. > > > > But we have at least one exportable filesystem (fuse) using > > d_materialise_unique. And I doubt d_splice_alias was ever completely > > correct even for on-disk filesystems. > > > > Am I missing some subtlety? > > Hm, I just noticed: > > commit 0d0d110720d7960b77c03c9f2597faaff4b484ae > Author: Miklos Szeredi <mszeredi@suse.cz> > Date: Mon Sep 16 14:52:00 2013 +0200 > > GFS2: d_splice_alias() can't return error > > unless it was given an IS_ERR(inode), which isn't the case here. So clean > up the unnecessary error handling in gfs2_create_inode(). > > This paves the way for real fixes (hence the stable Cc). > > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> > Signed-off-by: Steven Whitehouse <swhiteho@redhat.com> > Cc: stable@vger.kernel.org > > While the statement is true for the current implementation of > d_splice_alias, I don't think it's actually true for any correct > implementation of d_splice_alias, which must be able to return at least > -ELOOP in the directory case. Does gfs2 need fixing? > > --b. Yes, in that case, probably in two places, Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 15, 2014 at 10:17:49AM -0500, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > d_splice_alias can create duplicate directory aliases (in the !new > case), or (in the new case) d_move without holding appropriate locks. > > d_materialise_unique deals with both of these problems. (The latter > seems to be dealt by trylocks (see __d_unalias), which look like they > could cause spurious lookup failures--but that's at least better than > corrupting the dcache.) I'm a bit worried about those spurious failures, maybe we should retry in that case? Also looking over the changes I wonder if the explicit cecking for aliases for every non-directory might have a major performance impact, all the dcache growling already was a major issues in NFS workloads years ago and I dumb it's become any better. Also looking at this area I'd like to suggest that if you end up merging the two I'd continue using the d_splice_alias name and calling conventions. Also the inode == NULL case really should be split out from d_materialise_unique into a separate helper. It shares almost no code, is entirely undocumented to the point that I don't really understand what the purpose is, and the only caller that can get there (fuse) already branches around that case in the caller anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 17, 2014 at 04:17:23AM -0800, Christoph Hellwig wrote: > On Wed, Jan 15, 2014 at 10:17:49AM -0500, J. Bruce Fields wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > d_splice_alias can create duplicate directory aliases (in the !new > > case), or (in the new case) d_move without holding appropriate locks. > > > > d_materialise_unique deals with both of these problems. (The latter > > seems to be dealt by trylocks (see __d_unalias), which look like they > > could cause spurious lookup failures--but that's at least better than > > corrupting the dcache.) > > I'm a bit worried about those spurious failures, maybe we should > retry in that case? Maybe so. I'm not sure how. d_materialise_unique is called from lookup and we'd need to at least drop the parent i_mutex to give a concurrent rename a chance to progress. I think NFS or cluster filesystem clients could hit this case with: host A host B --------- ------------------------- process 1 process 1 process 2 --------- --------- --------- mkdir foo/X mv foo/X bar/ stat bar/X mv baz qux When (B,1) looks up X in bar it finds that X still has an alias in foo, tries to rename that alias to bar/X, but can't because the current baz->qux rename is holding the rename mutex. So __d_unalias and the lookup return -EBUSY. None of those operations are particularly fast, so I'm a bit surprised we haven't already heard complaints. I must be missing some reason this doesn't happen. I guess I should set up a test. > Also looking over the changes I wonder if the explicit cecking for > aliases for every non-directory might have a major performance impact, > all the dcache growling already was a major issues in NFS workloads > years ago and I dumb it's become any better. This only happens on the first (uncached) lookup. So we've already acquired a bunch of locks and probably done a round trip to a disk or a server--is walking a (typically short) list really something to worry about? > Also looking at this area I'd like to suggest that if you end up > merging the two I'd continue using the d_splice_alias name and > calling conventions. OK, I guess I don't care which one we keep. > Also the inode == NULL case really should be split out from > d_materialise_unique into a separate helper. It shares almost no > code, is entirely undocumented to the point that I don't really > understand what the purpose is, and the only caller that can get > there (fuse) already branches around that case in the caller anyway. I think I see what you mean, I can fix that. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 17, 2014 at 10:39:17AM -0500, J. Bruce Fields wrote: > On Fri, Jan 17, 2014 at 04:17:23AM -0800, Christoph Hellwig wrote: > > Also the inode == NULL case really should be split out from > > d_materialise_unique into a separate helper. It shares almost no > > code, is entirely undocumented to the point that I don't really > > understand what the purpose is, and the only caller that can get > > there (fuse) already branches around that case in the caller anyway. > > I think I see what you mean, I can fix that. Actually: - two callers (fuse and nfs) take advantage of the NULL case. - d_splice_alias handles inode == NULL in the same way, and almost every caller takes advantage of that. So at least we wouldn't want to actually make the caller handle this case. But maybe there's still some opportunity for cleanup or documentation. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 17, 2014 at 04:03:43PM -0500, J. Bruce Fields wrote:
> - d_splice_alias handles inode == NULL in the same way,
Actually, not exactly; simplifying a bit, in the NULL case they do:
d_splice_alias:
__d_instantiate(dentry, NULL);
security_d_instantiate(dentry, NULL);
if (d_unhashed(dentry))
d_rehash(dentry);
d_materialise_unique:
BUG_ON(!d_unhashed(dentry));
__d_instantiate(dentry, NULL);
d_rehash(dentry);
security_d_instantiate(dentry, NULL);
and a comment on d_splice_alias says
Cluster filesystems may call this function with a negative,
hashed dentry. In that case, we know that the inode will be a
regular file, and also this will only occur during atomic_open.
I don't understand those callers. But I guess it would be easy enough
to handle in d_materialise_unique.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/dcache.c b/fs/dcache.c index 4bdb300..da82fa9 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1926,33 +1926,10 @@ EXPORT_SYMBOL(d_obtain_alias); */ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry) { - struct dentry *new = NULL; - if (IS_ERR(inode)) return ERR_CAST(inode); - if (inode && S_ISDIR(inode->i_mode)) { - spin_lock(&inode->i_lock); - new = __d_find_alias(inode, 1); - if (new) { - BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED)); - spin_unlock(&inode->i_lock); - security_d_instantiate(new, inode); - d_move(new, dentry); - iput(inode); - } else { - /* already taking inode->i_lock, so d_add() by hand */ - __d_instantiate(dentry, inode); - spin_unlock(&inode->i_lock); - security_d_instantiate(dentry, inode); - d_rehash(dentry); - } - } else { - d_instantiate(dentry, inode); - if (d_unhashed(dentry)) - d_rehash(dentry); - } - return new; + return d_materialise_unique(dentry, inode); } EXPORT_SYMBOL(d_splice_alias);