ocfs2: use OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod error path

mark the inode with flag OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod, so we can kill the inode in case of error.

Signed-off-by Li Dongyang <[email protected]>

---
 fs/ocfs2/namei.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

fs/ocfs2/namei.c +0 -5

index 4ce4416..f989823 100644
@@ -447,11 +447,6 @@ leave:
 
 	ocfs2_free_dir_lookup_result(&lookup);
 
-	if ((status < 0) && inode) {
-		clear_nlink(inode);
-		iput(inode);
-	}
-
 	if (inode_ac)
 		ocfs2_free_alloc_context(inode_ac);
 

Quoted 25 lines · Li Dongyang

mark the inode with flag OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod, so we can kill the inode in case of error.

Signed-off-by Li Dongyang <[email protected]>

---
 fs/ocfs2/namei.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

fs/ocfs2/namei.c +0 -5

index 4ce4416..f989823 100644
@@ -447,11 +447,6 @@ leave:
 
 	ocfs2_free_dir_lookup_result(&lookup);
 
-	if ((status < 0) && inode) {
-		clear_nlink(inode);
-		iput(inode);
-	}
-
 	if (inode_ac)
 		ocfs2_free_alloc_context(inode_ac);
 

fs/ocfs2/namei.c leave: · line 463

Quoted 8 lines

fs/ocfs2/namei.c +5 -0

@@ -461,6 +456,17 @@ leave:
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
 
+	/*
+	We should call iput after the i_mutex of the bitmap
+	been unlocked in ocfs2_free_alloc_context, or the ocfs2_delete_inode
+	will mutex_lock again.
+	*/

Comment style here is wrong. It should look like:

/*
* We should call iput after the i_mutex of the bitmap been
* unlocked in ocfs2_free_alloc_context, or the
* ocfs2_delete_inode will mutex_lock again.
*/

fs/ocfs2/namei.c leave: · line 468

Quoted 5 lines

fs/ocfs2/namei.c +5 -0

+	if ((status < 0) && inode) {
+		OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR;
+		clear_nlink(inode);
+		iput(inode);
+	}

So, I realized that this doesn't fix all the issues we have.

If you look at symlink and mknod, you'll see that we can get here _after_ the inode name has been added to the directory. Removing the inode then would leave a dangling directory reference. The following patch tries to
fix the issue. Can you please confirm that it works?

If you want to pull the whole set of patches (with some cleanups by me),
you can get it at:

git pull
git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2-mark.git
skip_delete_inode ---Mark

--
Mark Fasheh

From: Mark Fasheh <[email protected]>

ocfs2: Add directory entry later in ocfs2_symlink() and ocfs2_mknod()

If we get a failure during creation of an inode we'll allow the orphan code to remove the inode, which is correct. However, we need to ensure that we don't get any errors after the call to ocfs2_add_entry(), otherwise we
could leave a dangling directory reference. The solution is simple - in both cases, all I had to do was move ocfs2_dentry_attach_lock() above the ocfs2_add_entry() call.

Signed-off-by Mark Fasheh <[email protected]>

---
 fs/ocfs2/namei.c |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

fs/ocfs2/namei.c +25 -15

index 8ff035e..4cbb18f 100644
@@ -408,23 +408,28 @@ static int ocfs2_mknod(struct inode *dir,
 		}
 	}
 
-	status = ocfs2_add_entry(handle, dentry, inode,
-				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
-				 &lookup);
-	if (status < 0) {
+	/*
+	 * Do this before adding the entry to the directory. We add
+	 * also set d_op after success so that ->d_iput() will cleanup
+	 * the dentry lock even if ocfs2_add_entry() fails below.
+	 */
+	status = ocfs2_dentry_attach_lock(dentry, inode,
+					  OCFS2_I(dir)->ip_blkno);
+	if (status) {
 		mlog_errno(status);
 		goto leave;
 	}
+	dentry->d_op = &ocfs2_dentry_ops;
 
-	status = ocfs2_dentry_attach_lock(dentry, inode,
-					  OCFS2_I(dir)->ip_blkno);
-	if (status) {
+	status = ocfs2_add_entry(handle, dentry, inode,
+				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
+				 &lookup);
+	if (status < 0) {
 		mlog_errno(status);
 		goto leave;
 	}
 
 	insert_inode_hash(inode);
-	dentry->d_op = &ocfs2_dentry_ops;
 	d_instantiate(dentry, inode);
 	status = 0;
 leave:

@@ -1777,22 +1782,27 @@ static int ocfs2_symlink(struct inode *dir,
 		}
 	}
 
-	status = ocfs2_add_entry(handle, dentry, inode,
-				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
-				 &lookup);
-	if (status < 0) {
+	/*
+	 * Do this before adding the entry to the directory. We add
+	 * also set d_op after success so that ->d_iput() will cleanup
+	 * the dentry lock even if ocfs2_add_entry() fails below.
+	 */
+	status = ocfs2_dentry_attach_lock(dentry, inode, OCFS2_I(dir)->ip_blkno);
+	if (status) {
 		mlog_errno(status);
 		goto bail;
 	}
+	dentry->d_op = &ocfs2_dentry_ops;
 
-	status = ocfs2_dentry_attach_lock(dentry, inode, OCFS2_I(dir)->ip_blkno);
-	if (status) {
+	status = ocfs2_add_entry(handle, dentry, inode,
+				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
+				 &lookup);
+	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
 	}
 
 	insert_inode_hash(inode);
-	dentry->d_op = &ocfs2_dentry_ops;
 	d_instantiate(dentry, inode);
 bail:
 	if (status < 0 && did_quota)

Quoted 51 lines · On Saturday 24 April 2010 03:59:00 Mark Fasheh wrote:

Quoted 25 lines · Li Dongyang

mark the inode with flag OCFS2_INODE_SKIP_ORPHAN_DIR in ocfs2_mknod, so we can kill the inode in case of error.

Signed-off-by Li Dongyang <[email protected]>

---
 fs/ocfs2/namei.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

fs/ocfs2/namei.c +0 -5

index 4ce4416..f989823 100644
@@ -447,11 +447,6 @@ leave:
 
 	ocfs2_free_dir_lookup_result(&lookup);
 
-	if ((status < 0) && inode) {
-		clear_nlink(inode);
-		iput(inode);
-	}
-
 	if (inode_ac)
 		ocfs2_free_alloc_context(inode_ac);
 

fs/ocfs2/namei.c leave: · line 463

Quoted 8 lines

fs/ocfs2/namei.c +5 -0

@@ -461,6 +456,17 @@ leave:
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
 
+	/*
+	We should call iput after the i_mutex of the bitmap
+	been unlocked in ocfs2_free_alloc_context, or the ocfs2_delete_inode
+	will mutex_lock again.
+	*/

Comment style here is wrong. It should look like:

/*
* We should call iput after the i_mutex of the bitmap been
* unlocked in ocfs2_free_alloc_context, or the
* ocfs2_delete_inode will mutex_lock again.
*/

fs/ocfs2/namei.c leave: · line 468

Quoted 5 lines

fs/ocfs2/namei.c +5 -0

+	if ((status < 0) && inode) {
+		OCFS2_I(inode)->ip_flags |= OCFS2_INODE_SKIP_ORPHAN_DIR;
+		clear_nlink(inode);
+		iput(inode);
+	}

So, I realized that this doesn't fix all the issues we have.

Quoted 4 lines

If you look at symlink and mknod, you'll see that we can get here _after_ the inode name has been added to the directory. Removing the inode then would leave a dangling directory reference. The following patch tries to
fix the issue. Can you please confirm that it works?

Thanks for pointing out this and fixing the comment style and the
description, I've tested with your patch and it works for me.
Br,
Li Dongyang

Quoted 97 lines

If you want to pull the whole set of patches (with some cleanups by me),
you can get it at:

git pull
git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2-mark.git
skip_delete_inode ---Mark

--
Mark Fasheh

From: Mark Fasheh <[email protected]>

ocfs2: Add directory entry later in ocfs2_symlink() and ocfs2_mknod()

If we get a failure during creation of an inode we'll allow the orphan code to remove the inode, which is correct. However, we need to ensure that we don't get any errors after the call to ocfs2_add_entry(), otherwise we could leave a dangling directory reference. The solution is simple - in both cases, all I had to do was move ocfs2_dentry_attach_lock() above the ocfs2_add_entry() call.

Signed-off-by Mark Fasheh <[email protected]>

---
 fs/ocfs2/namei.c |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

fs/ocfs2/namei.c +24 -13

index 8ff035e..4cbb18f 100644
@@ -408,23 +408,28 @@ static int ocfs2_mknod(struct inode *dir,
 		}
 	}
 
-	status = ocfs2_add_entry(handle, dentry, inode,
-				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
-				 &lookup);
-	if (status < 0) {
+	/*
+	 * Do this before adding the entry to the directory. We add
+	 * also set d_op after success so that ->d_iput() will cleanup
+	 * the dentry lock even if ocfs2_add_entry() fails below.
+	 */
+	status = ocfs2_dentry_attach_lock(dentry, inode,
+					  OCFS2_I(dir)->ip_blkno);
+	if (status) {
 		mlog_errno(status);
 		goto leave;
 	}
+	dentry->d_op = &ocfs2_dentry_ops;
 
-	status = ocfs2_dentry_attach_lock(dentry, inode,
-					  OCFS2_I(dir)->ip_blkno);
-	if (status) {
+	status = ocfs2_add_entry(handle, dentry, inode,
+				 OCFS2_I(inode)->ip_blkno, parent_fe_bh,
+				 &lookup);
+	if (status < 0) {
 		mlog_errno(status);
 		goto leave;
 	}
 
 	insert_inode_hash(inode);
-	dentry->d_op = &ocfs2_dentry_ops;
 	d_instantiate(dentry, inode);
 	status = 0;
 leave:

@@ -1777,22 +1782,27 @@ static int ocfs2_symlink(struct inode *dir,
 		}
 	}
 
-	status = ocfs2_add_entry(handle, dentry, inode,
-				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
-				 &lookup);
-	if (status < 0) {
+	/*
+	 * Do this before adding the entry to the directory. We add
+	 * also set d_op after success so that ->d_iput() will cleanup
+	 * the dentry lock even if ocfs2_add_entry() fails below.
+	 */
+	status = ocfs2_dentry_attach_lock(dentry, inode,
 OCFS2_I(dir)->ip_blkno); +	if (status) {
 		mlog_errno(status);
 		goto bail;
 	}
+	dentry->d_op = &ocfs2_dentry_ops;
 
-	status = ocfs2_dentry_attach_lock(dentry, inode,
 OCFS2_I(dir)->ip_blkno); -	if (status) {
+	status = ocfs2_add_entry(handle, dentry, inode,
+				 le64_to_cpu(fe->i_blkno), parent_fe_bh,
+				 &lookup);
+	if (status < 0) {
 		mlog_errno(status);
 		goto bail;
 	}
 

Quoted 5 lines

fs/ocfs2/namei.c +0 -1

 	insert_inode_hash(inode);
-	dentry->d_op = &ocfs2_dentry_ops;
 	d_instantiate(dentry, inode);
 bail:

if (status < 0 && did_quota)

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel