[05/15] freevxfs_lookup(): use d_splice_alias()
diff mbox

Message ID 20180513213017.31269-5-viro@ZenIV.linux.org.uk
State New
Headers show

Commit Message

Al Viro May 13, 2018, 9:30 p.m. UTC
From: Al Viro <viro@zeniv.linux.org.uk>

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/freevxfs/vxfs_lookup.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

hch@lst.de May 14, 2018, 8:41 a.m. UTC | #1
On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

It looks a little cleaner, so this is fine, but is there also any
other reason?  A little changelog would be nice.
Al Viro May 14, 2018, 3:26 p.m. UTC | #2
On Mon, May 14, 2018 at 10:41:02AM +0200, Christoph Hellwig wrote:
> On Sun, May 13, 2018 at 10:30:07PM +0100, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> It looks a little cleaner, so this is fine, but is there also any
> other reason?  A little changelog would be nice.

The same changelog would be duplicated in a bunch of commits in that
series and I'm not sure how much is too much.  Let me put it that
way:
	* d_splice_alias() has better calling conventions for use
in ->lookup() - it tends to reduce the number of branches nicely.
	* d_add() in ->lookup() is no-go for anything exportable;
we did conversion for exported stuff back then, and it's documented
in Documentation/filesystems/nfs/Exporting, but people fail to RTFM
when converting more filesystems to exportability.  Preempting that
kind of bugs is a good idea.
	* the fewer stray d_add() we have sitting around, the better -
each needs a proof that this particular invocation won't end up with
multiple aliases of a directory inode.  d_splice_alias() is *NOT*
a universal replacement, but in a lot of ->lookup() instances we can
use it sanely; it can't be done quite blindly (the stuff that hangs
something off the dentry needs a careful look), but most of the time
it's fine - on any filesystem that doesn't play with ->d_fsdata, for
starters.  And in all cases such replacement in ->lookup() is
"it's no worse than before" - it's just that some cases need more
than just that conversion before they can be made exportable (see
e.g. 9p for example of that kind of extra work;
        res = d_splice_alias(inode, dentry);
        if (!res)
                v9fs_fid_add(dentry, fid);
        else if (!IS_ERR(res))
                v9fs_fid_add(res, fid);
        else
                p9_client_clunk(fid);
in there, when we need to decide which dentry to slap the fid onto)

The bunch in this series is the trivial part of conversions.  Next
group is where a bit more attention is needed and the last is procfs
nest of horrors.

I'm honestly not sure how much of the above makes sense as a boilerplate
to go into all those commits.  Suggestions?

Patch
diff mbox

diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c
index ce4785fd81c6..a51425634f65 100644
--- a/fs/freevxfs/vxfs_lookup.c
+++ b/fs/freevxfs/vxfs_lookup.c
@@ -193,13 +193,9 @@  vxfs_lookup(struct inode *dip, struct dentry *dp, unsigned int flags)
 		return ERR_PTR(-ENAMETOOLONG);
 				 
 	ino = vxfs_inode_by_name(dip, dp);
-	if (ino) {
+	if (ino)
 		ip = vxfs_iget(dip->i_sb, ino);
-		if (IS_ERR(ip))
-			return ERR_CAST(ip);
-	}
-	d_add(dp, ip);
-	return NULL;
+	return d_splice_alias(ip, dp);
 }
 
 /**