[v1,31/38] nfs: replace d_add with d_splice_alias in atomic_open
diff mbox

Message ID 1447761180-4250-32-git-send-email-jeff.layton@primarydata.com
State New
Headers show

Commit Message

Jeff Layton Nov. 17, 2015, 11:52 a.m. UTC
From: Peng Tao <tao.peng@primarydata.com>

It's a trival change but follows knfsd export document that asks
for d_splice_alias during lookup.

Signed-off-by: Peng Tao <tao.peng@primarydata.com>
---
 fs/nfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

bfields@fieldses.org Nov. 19, 2015, 8:06 p.m. UTC | #1
On Tue, Nov 17, 2015 at 06:52:53AM -0500, Jeff Layton wrote:
> From: Peng Tao <tao.peng@primarydata.com>
> 
> It's a trival change but follows knfsd export document that asks
> for d_splice_alias during lookup.

This is a bug even before you start exporting, isn't it?

OK, I see, in the atomic_open case we're probably only dealing with a
positive dentry for a regular file at this point, in which case
d_splice_alias is really just d_add....

I'm not sure this patch is really necessary, then.

--b.


> 
> Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> ---
>  fs/nfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index ce5a21861074..a4df1137878e 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1534,7 +1534,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  		switch (err) {
>  		case -ENOENT:
>  			d_drop(dentry);
> -			d_add(dentry, NULL);
> +			d_splice_alias(NULL, dentry);
>  			nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
>  			break;
>  		case -EISDIR:
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Nov. 19, 2015, 8:52 p.m. UTC | #2
On Thu, Nov 19, 2015 at 3:06 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Tue, Nov 17, 2015 at 06:52:53AM -0500, Jeff Layton wrote:
> > From: Peng Tao <tao.peng@primarydata.com>
> >
> > It's a trival change but follows knfsd export document that asks
> > for d_splice_alias during lookup.
>
> This is a bug even before you start exporting, isn't it?
>
> OK, I see, in the atomic_open case we're probably only dealing with a
> positive dentry for a regular file at this point, in which case
> d_splice_alias is really just d_add....
>
> I'm not sure this patch is really necessary, then.
>

d_splice_alias() also instantly reduces to d_add() whenever the
'inode' argument is NULL.

Cheers
  Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Nov. 19, 2015, 8:59 p.m. UTC | #3
On Thu, 19 Nov 2015 15:06:51 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Nov 17, 2015 at 06:52:53AM -0500, Jeff Layton wrote:
> > From: Peng Tao <tao.peng@primarydata.com>
> > 
> > It's a trival change but follows knfsd export document that asks
> > for d_splice_alias during lookup.
> 
> This is a bug even before you start exporting, isn't it?
> 
> OK, I see, in the atomic_open case we're probably only dealing with a
> positive dentry for a regular file at this point, in which case
> d_splice_alias is really just d_add....
> 
> I'm not sure this patch is really necessary, then.
> 
> --b.
> 
> 
> > 
> > Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> > ---
> >  fs/nfs/dir.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index ce5a21861074..a4df1137878e 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -1534,7 +1534,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> >  		switch (err) {
> >  		case -ENOENT:
> >  			d_drop(dentry);
> > -			d_add(dentry, NULL);
> > +			d_splice_alias(NULL, dentry);
> >  			nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
> >  			break;
> >  		case -EISDIR:
> > -- 
> > 2.4.3

Ahh right -- good point. d_splice_alias does this:

        if (!inode) {
                __d_instantiate(dentry, NULL);
                goto out;
        }
[...]
out:
	security_d_instantiate(dentry, inode);
        d_rehash(dentry);
        return NULL;

...which is exactly what d_add does. So yeah, that change isn't
strictly necessary. Still, it might be more future-proof to use
d_splice_alias there if the semantics ever change...
bfields@fieldses.org Nov. 19, 2015, 10:32 p.m. UTC | #4
On Thu, Nov 19, 2015 at 03:59:31PM -0500, Jeff Layton wrote:
> Ahh right -- good point. d_splice_alias does this:
> 
>         if (!inode) {
>                 __d_instantiate(dentry, NULL);
>                 goto out;
>         }
> [...]
> out:
> 	security_d_instantiate(dentry, inode);
>         d_rehash(dentry);
>         return NULL;
> 
> ...which is exactly what d_add does. So yeah, that change isn't
> strictly necessary. Still, it might be more future-proof to use
> d_splice_alias there if the semantics ever change...

Could be, I don't know.

May as well apply it (or not) independently of this series, though.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index ce5a21861074..a4df1137878e 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1534,7 +1534,7 @@  int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		switch (err) {
 		case -ENOENT:
 			d_drop(dentry);
-			d_add(dentry, NULL);
+			d_splice_alias(NULL, dentry);
 			nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
 			break;
 		case -EISDIR: