[Cluster-devel] [gfs2 patches in vfs.git]

[Cluster-devel] [gfs2 patches in vfs.git]

[Al Viro]

This stuff is around ->atomic_open() and my main goal here is to eliminate the
cases when d_splice_alias() is used on a hashed dentry.  Patches in question
sit in vfs.git#for-gfs2, and that branch isn't going to be rebased; I'd prefer
if it was pulled instead of having them applied manually.

Shortlog:
Al Viro (3):
      gfs2: bugger off early if O_CREAT open finds a directory
      gfs2_create_inode(): don't bother with d_splice_alias()
      gfs2_atomic_open(): simplify the use of finish_no_open()

Diffstat:
 fs/gfs2/inode.c |   26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

Patches themselves are in followups.

[Cluster-devel] [PATCH 1/3] gfs2: bugger off early if O_CREAT open finds a directory

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/gfs2/inode.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index c4ed823..310e248 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 	inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
 	error = PTR_ERR(inode);
 	if (!IS_ERR(inode)) {
+		if (S_ISDIR(inode->i_mode)) {
+			iput(inode);
+			inode = ERR_PTR(-EISDIR);
+			goto fail_gunlock;
+		}
 		d = d_splice_alias(inode, dentry);
 		error = PTR_ERR(d);
 		if (IS_ERR(d)) {
-- 
1.7.10.4

[Cluster-devel] [PATCH 2/3] gfs2_create_inode(): don't bother with d_splice_alias()

[Al Viro]

dentry is always hashed and negative, inode - non-error, non-NULL and
non-directory.  In such conditions d_splice_alias() is equivalent to
"d_instantiate(dentry, inode) and return NULL", which simplifies the
downstream code and is consistent with the "have to create a new object"
case.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/gfs2/inode.c |   18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 310e248..ce0cf9a 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -596,7 +596,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 	struct gfs2_inode *dip = GFS2_I(dir), *ip;
 	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
 	struct gfs2_glock *io_gl;
-	struct dentry *d;
 	int error, free_vfs_inode = 0;
 	u32 aflags = 0;
 	unsigned blocks = 1;
@@ -629,22 +628,13 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 			inode = ERR_PTR(-EISDIR);
 			goto fail_gunlock;
 		}
-		d = d_splice_alias(inode, dentry);
-		error = PTR_ERR(d);
-		if (IS_ERR(d)) {
-			inode = ERR_CAST(d);
-			goto fail_gunlock;
-		}
+		d_instantiate(dentry, inode);
 		error = 0;
 		if (file) {
-			if (S_ISREG(inode->i_mode)) {
-				WARN_ON(d != NULL);
+			if (S_ISREG(inode->i_mode))
 				error = finish_open(file, dentry, gfs2_open_common, opened);
-			} else {
-				error = finish_no_open(file, d);
-			}
-		} else {
-			dput(d);
+			else
+				error = finish_no_open(file, NULL);
 		}
 		gfs2_glock_dq_uninit(ghs);
 		return error;
-- 
1.7.10.4

[Cluster-devel] [PATCH 3/3] gfs2_atomic_open(): simplify the use of finish_no_open()

[Al Viro]

In ->atomic_open(inode, dentry, file, opened) calling finish_no_open(file, NULL)
is equivalent to dget(dentry); return finish_no_open(file, dentry);

No need to open-code that...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/gfs2/inode.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index ce0cf9a..6e29174 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1249,11 +1249,8 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,
 	if (d != NULL)
 		dentry = d;
 	if (dentry->d_inode) {
-		if (!(*opened & FILE_OPENED)) {
-			if (d == NULL)
-				dget(dentry);
-			return finish_no_open(file, dentry);
-		}
+		if (!(*opened & FILE_OPENED))
+			return finish_no_open(file, d);
 		dput(d);
 		return 0;
 	}
-- 
1.7.10.4

[Cluster-devel] [PATCH 1/3] gfs2: bugger off early if O_CREAT open finds a directory

[Bob Peterson]

----- Original Message -----
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/gfs2/inode.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index c4ed823..310e248 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir, struct
> dentry *dentry,
>  	inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
>  	error = PTR_ERR(inode);
>  	if (!IS_ERR(inode)) {
> +		if (S_ISDIR(inode->i_mode)) {
> +			iput(inode);
> +			inode = ERR_PTR(-EISDIR);
> +			goto fail_gunlock;
> +		}
>  		d = d_splice_alias(inode, dentry);
>  		error = PTR_ERR(d);
>  		if (IS_ERR(d)) {
> --
> 1.7.10.4
> 
> 

Hm. Seems wrong that it should return 0 if the dirent exists (mkdir of a
directory that already exists) but it looks like it already behaves that way.
So I guess so. It may warrant further investigation.

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/3] gfs2: bugger off early if O_CREAT open finds a directory

[Al Viro]

On Wed, Nov 19, 2014 at 03:33:29PM -0500, Bob Peterson wrote:
> ----- Original Message -----
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  fs/gfs2/inode.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > index c4ed823..310e248 100644
> > --- a/fs/gfs2/inode.c
> > +++ b/fs/gfs2/inode.c
> > @@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir, struct
> > dentry *dentry,
> >  	inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
> >  	error = PTR_ERR(inode);
> >  	if (!IS_ERR(inode)) {
> > +		if (S_ISDIR(inode->i_mode)) {
> > +			iput(inode);
> > +			inode = ERR_PTR(-EISDIR);
> > +			goto fail_gunlock;
> > +		}
> >  		d = d_splice_alias(inode, dentry);
> >  		error = PTR_ERR(d);
> >  		if (IS_ERR(d)) {

> Hm. Seems wrong that it should return 0 if the dirent exists (mkdir of a
> directory that already exists) but it looks like it already behaves that way.
> So I guess so. It may warrant further investigation.

It doesn't.  Note that !S_ISREG(mode) || excl in there - *anything* other
than ->create() will treat existing entries with the same name as EEXIST.
The only case when we can possibly get into that if (!IS_ERR(inode)) is
->create() hitting an existing file.  And with this change it narrows to
"->create() hitting an existing non-directory".  That allows the next patch
to use d_instantiate() instead of d_splice_alias() - for non-directories
it's the same thing, since dentry is already hashed here and we don't need
to avoid multiple aliases.  Which kills one of the two places in the
tree where d_splice_alias() is called for an already hashed dentry (another
is d_add_ci() and that call also goes down - see vfs.git#for-next for that
one).

[Cluster-devel] [PATCH 1/3] gfs2: bugger off early if O_CREAT open finds a directory

[Bob Peterson]

----- Original Message -----
> On Wed, Nov 19, 2014 at 03:33:29PM -0500, Bob Peterson wrote:
> > ----- Original Message -----
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > ---
> > >  fs/gfs2/inode.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > index c4ed823..310e248 100644
> > > --- a/fs/gfs2/inode.c
> > > +++ b/fs/gfs2/inode.c
> > > @@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir,
> > > struct
> > > dentry *dentry,
> > >  	inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
> > >  	error = PTR_ERR(inode);
> > >  	if (!IS_ERR(inode)) {
> > > +		if (S_ISDIR(inode->i_mode)) {
> > > +			iput(inode);
> > > +			inode = ERR_PTR(-EISDIR);
> > > +			goto fail_gunlock;
> > > +		}
> > >  		d = d_splice_alias(inode, dentry);
> > >  		error = PTR_ERR(d);
> > >  		if (IS_ERR(d)) {
> 
> > Hm. Seems wrong that it should return 0 if the dirent exists (mkdir of a
> > directory that already exists) but it looks like it already behaves that
> > way.
> > So I guess so. It may warrant further investigation.
> 
> It doesn't.  Note that !S_ISREG(mode) || excl in there - *anything* other
> than ->create() will treat existing entries with the same name as EEXIST.
> The only case when we can possibly get into that if (!IS_ERR(inode)) is
> ->create() hitting an existing file.  And with this change it narrows to
> "->create() hitting an existing non-directory".  That allows the next patch
> to use d_instantiate() instead of d_splice_alias() - for non-directories
> it's the same thing, since dentry is already hashed here and we don't need
> to avoid multiple aliases.  Which kills one of the two places in the
> tree where d_splice_alias() is called for an already hashed dentry (another
> is d_add_ci() and that call also goes down - see vfs.git#for-next for that
> one).
> 
Okay, thanks for clearing that up.

ACK

Bob Peterson
Red Hat File Systems