[Cluster-devel] [gfs2 patches in vfs.git]
PATCH 1/1 [Cluster-devel] [gfs2 patches in vfs.git]
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.
PATCH 1/3 gfs2: bugger off early if O_CREAT open finds a directory
--- fs/gfs2/inode.c | 5 +++++ 1 file changed, 5 insertions(+)
fs/gfs2/inode.c +5 -0
@@ -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)) {
Signature · 1 line
1.7.10.4
PATCH 1/3 gfs2: bugger off early if O_CREAT open finds a directory
----- Original Message -----
Quoted 21 lines
Signed-off-by Al Viro <[email protected]>--- fs/gfs2/inode.c | 5 +++++ 1 file changed, 5 insertions(+)fs/gfs2/inode.c +5
-0@@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir, structdentry *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);
Quoted 5 lines
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
PATCH 1/3 gfs2: bugger off early if O_CREAT open finds a directory
Quoted 19 lines · Bob Peterson
----- Original Message -----
Quoted 18 lines
Signed-off-by Al Viro <[email protected]>--- fs/gfs2/inode.c | 5 +++++ 1 file changed, 5 insertions(+)fs/gfs2/inode.c +4
-0@@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir, structdentry *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;
Quoted 8 lines
Quoted 4 lines
fs/gfs2/inode.c +1
-0+ } 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).
PATCH 1/3 gfs2: bugger off early if O_CREAT open finds a directory
----- Original Message -----
Quoted 31 lines · Bob Peterson
Quoted 21 lines
----- Original Message -----
Quoted 20 lines
Signed-off-by Al Viro <[email protected]>--- fs/gfs2/inode.c | 5 +++++ 1 file changed, 5 insertions(+)fs/gfs2/inode.c +0
-0@@ -624,6 +624,11 @@ static int gfs2_create_inode(struct inode *dir,struct
dentry *dentry,fs/gfs2/inode.c +5
-0inode =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; + }Quoted 8 lines
fs/gfs2/inode.c static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, · line 634
Quoted 3 lines
fs/gfs2/inode.c +0
-0d =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.
Quoted 12 lines
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 ofd_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 whered_splice_alias()is called for an already hashed dentry (another isd_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
PATCH 2/3 gfs2_create_inode(): don't bother with d_splice_alias()
In reply to Al Virodentry 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.
--- fs/gfs2/inode.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)
fs/gfs2/inode.c +4 -14
@@ -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;
Signature · 1 line
1.7.10.4
PATCH 3/3 gfs2_atomic_open(): simplify the use of finish_no_open()
In reply to Al ViroIn ->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...
--- fs/gfs2/inode.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
fs/gfs2/inode.c +2 -5
@@ -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 ; }
Signature · 1 line
1.7.10.4