ocfs2: fix readonly issue in ocfs2_unlink()

PATCH New Younger Liu 3 participants 4 messages Jun 27, 2013 – Jun 28, 2013 OCFS2

Share & tools

Younger Liu

author

2013-06-27 03:05 UTC #0 lore raw

PATCH 1/1 ocfs2: fix readonly issue in ocfs2_unlink()

While deleting a file with ocfs2_unlink(), there is a bug in this function. This bug will result in filesystem read-only.

After calling ocfs2_orphan_add(), the file which will be deleted is added into orphan dir. If ocfs2_delete_entry() fails, the file still exists in the parent dir.
And this scenario introduces a conflict of metadata.

If a file is added into orphan dir, when we put inode of the file with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in ocfs2_remove_inode(), and then write back to disk.

But as previously mentioned, the file still exists in the parent dir. On other nodes, the file can be still accessed. When first read the file with ocfs2_read_blocks() from disk, It will check and avalidate inode using ocfs2_validate_inode_block().
So File system will be readonly because the inode is invalid. In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).

Signed-off-by Younger Liu <[email protected]>

Signed-off-by Jensen <[email protected]>

---
 fs/ocfs2/namei.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

fs/ocfs2/namei.c +10 ~~-10~~

index e3fce49..ddb4915 100644
@@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0;
+	int child_locked = 0, is_unlinkable = 0;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);

@@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
 			mlog_errno(status);
 			goto leave;
 		}
+		is_unlinkable = 1;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));

@@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
 
 	fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
-	if (inode_is_unlinkable(inode)) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-	}
-
 	/* delete the name from the parent dir */
 	status = ocfs2_delete_entry(handle, dir, &lookup);
 	if (status < 0) {

@@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
 		mlog_errno(status);
 		if (S_ISDIR(inode->i_mode))
 			inc_nlink(dir);
+		goto leave;
+	}
+
+	if (is_unlinkable) {
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
+					  &orphan_insert, orphan_dir);
+		if (status < 0)
+			mlog_errno(status);
 	}
 
 leave:

Signature · 1 line

1.7.9.7

Andrew Morton

2013-06-27 21:58 UTC #1 lore raw

PATCH 1/1 ocfs2: fix readonly issue in ocfs2_unlink()

Quoted 24 lines · On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <[email protected]> wrote:

While deleting a file with ocfs2_unlink(), there is a bug in this function. This bug will result in filesystem read-only.
After calling ocfs2_orphan_add(), the file which will be deleted is added into orphan dir. If ocfs2_delete_entry() fails, the file still exists in the parent dir.
And this scenario introduces a conflict of metadata.
If a file is added into orphan dir, when we put inode of the file with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in ocfs2_remove_inode(), and then write back to disk.
But as previously mentioned, the file still exists in the parent dir. On other nodes, the file can be still accessed. When first read the file with ocfs2_read_blocks() from disk, It will check and avalidate inode using ocfs2_validate_inode_block().
So File system will be readonly because the inode is invalid. In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).
...

fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 793

Quoted 5 lines

fs/ocfs2/namei.c +1 -1

@@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0;
+	int child_locked = 0, is_unlinkable = 0;

Please note that the surrounding code was carful to use the one-definition-per-line convention. That's a good convention - more readable, less patch rejects during code evolution, leaves room for a nice little comment.

Also, type `bool' would have been appropraite here.

Quoted 30 lines

fs/ocfs2/namei.c +1 -9

 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);

@@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
 			mlog_errno(status);
 			goto leave;
 		}
+		is_unlinkable = 1;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));

@@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
 
 	fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
-	if (inode_is_unlinkable(inode)) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-	}
-
 	/* delete the name from the parent dir */
 	status = ocfs2_delete_entry(handle, dir, &lookup);
 	if (status < 0) {

@@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
 		mlog_errno(status);
 		if (S_ISDIR(inode->i_mode))

fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 926

Quoted 10 lines

fs/ocfs2/namei.c +8 -0

 			inc_nlink(dir);
+		goto leave;
+	}
+
+	if (is_unlinkable) {
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
+					  &orphan_insert, orphan_dir);
+		if (status < 0)
+			mlog_errno(status);
 	}

This is yet another ocfs2 function which reports the same error two times. ho hum.

Please review:

fs/ocfs2/namei.c renamed from fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix +5 -4

@@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0, is_unlinkable = 0;
+	int child_locked = 0;
+	bool is_unlinkable = false;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);

@@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di
 			mlog_errno(status);
 			goto leave;
 		}
-		is_unlinkable = 1;
+		is_unlinkable = true;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));

@@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di
 	}
 
 	if (is_unlinkable) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh,
+				orphan_name, &orphan_insert, orphan_dir);
 		if (status < 0)
 			mlog_errno(status);
 	}

Younger Liu

author

2013-06-28 05:52 UTC #2 lore raw

PATCH 1/1 ocfs2: fix readonly issue in ocfs2_unlink()

Quoted 74 lines · Andrew Morton

Quoted 24 lines · On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <[email protected]> wrote:
While deleting a file with ocfs2_unlink(), there is a bug in this function. This bug will result in filesystem read-only.
After calling ocfs2_orphan_add(), the file which will be deleted is added into orphan dir. If ocfs2_delete_entry() fails, the file still exists in the parent dir.
And this scenario introduces a conflict of metadata.
If a file is added into orphan dir, when we put inode of the file with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in ocfs2_remove_inode(), and then write back to disk.
But as previously mentioned, the file still exists in the parent dir. On other nodes, the file can be still accessed. When first read the file with ocfs2_read_blocks() from disk, It will check and avalidate inode using ocfs2_validate_inode_block().
So File system will be readonly because the inode is invalid. In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).
...
fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 793
Quoted 5 lines
fs/ocfs2/namei.c +1 -1
@@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0;
+	int child_locked = 0, is_unlinkable = 0;
Please note that the surrounding code was carful to use the one-definition-per-line convention. That's a good convention - more readable, less patch rejects during code evolution, leaves room for a nice little comment.
Also, type `bool' would have been appropraite here.
Quoted 35 lines
fs/ocfs2/namei.c +5 -9
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
@@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
 			mlog_errno(status);
 			goto leave;
 		}
+		is_unlinkable = 1;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
@@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
 
 	fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
-	if (inode_is_unlinkable(inode)) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-	}
-
 	/* delete the name from the parent dir */
 	status = ocfs2_delete_entry(handle, dir, &lookup);
 	if (status < 0) {
@@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
 		mlog_errno(status);
 		if (S_ISDIR(inode->i_mode))
 			inc_nlink(dir);
+		goto leave;
+	}
+
+	if (is_unlinkable) {

Quoted 9 lines

fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 926
Quoted 5 lines
fs/ocfs2/namei.c +4 -0
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
+					  &orphan_insert, orphan_dir);
+		if (status < 0)
+			mlog_errno(status);
 	}
This is yet another ocfs2 function which reports the same error two times. ho hum.

Thanks for your review.

Quoted 34 lines

Please review:

fs/ocfs2/namei.c renamed from fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix +5 -4

@@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0, is_unlinkable = 0;
+	int child_locked = 0;
+	bool is_unlinkable = false;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);

@@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di
 			mlog_errno(status);
 			goto leave;
 		}
-		is_unlinkable = 1;
+		is_unlinkable = true;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));

@@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di
 	}
 
 	if (is_unlinkable) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh,
+				orphan_name, &orphan_insert, orphan_dir);
 		if (status < 0)
 			mlog_errno(status);
 	}

Quoted 2 lines

_

...

This patch looks fine to me. I also test it, and the result is fine. You can consider to add:

Reviewed-by Younger Liu <[email protected]>

Jeff Liu

2013-06-28 06:38 UTC #3 lore raw

PATCH 1/1 ocfs2: fix readonly issue in ocfs2_unlink()

Quoted 124 lines · Younger Liu

Quoted 74 lines · Andrew Morton
Quoted 24 lines · On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <[email protected]> wrote:
While deleting a file with ocfs2_unlink(), there is a bug in this function. This bug will result in filesystem read-only.
After calling ocfs2_orphan_add(), the file which will be deleted is added into orphan dir. If ocfs2_delete_entry() fails, the file still exists in the parent dir.
And this scenario introduces a conflict of metadata.
If a file is added into orphan dir, when we put inode of the file with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in ocfs2_remove_inode(), and then write back to disk.
But as previously mentioned, the file still exists in the parent dir. On other nodes, the file can be still accessed. When first read the file with ocfs2_read_blocks() from disk, It will check and avalidate inode using ocfs2_validate_inode_block().
So File system will be readonly because the inode is invalid. In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).
...
fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 793
Quoted 5 lines
fs/ocfs2/namei.c +1 -1
@@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0;
+	int child_locked = 0, is_unlinkable = 0;
Please note that the surrounding code was carful to use the one-definition-per-line convention. That's a good convention - more readable, less patch rejects during code evolution, leaves room for a nice little comment.
Also, type `bool' would have been appropraite here.
Quoted 35 lines
fs/ocfs2/namei.c +5 -9
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
@@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
 			mlog_errno(status);
 			goto leave;
 		}
+		is_unlinkable = 1;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
@@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
 
 	fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
-	if (inode_is_unlinkable(inode)) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-	}
-
 	/* delete the name from the parent dir */
 	status = ocfs2_delete_entry(handle, dir, &lookup);
 	if (status < 0) {
@@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
 		mlog_errno(status);
 		if (S_ISDIR(inode->i_mode))
 			inc_nlink(dir);
+		goto leave;
+	}
+
+	if (is_unlinkable) {
Quoted 9 lines
fs/ocfs2/namei.c static int ocfs2_unlink(struct inode *dir, · line 926
Quoted 5 lines
fs/ocfs2/namei.c +4 -0
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
+					  &orphan_insert, orphan_dir);
+		if (status < 0)
+			mlog_errno(status);
 	}
This is yet another ocfs2 function which reports the same error two times. ho hum.
Thanks for your review.
Quoted 36 lines
Please review:
fs/ocfs2/namei.c renamed from fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix +5 -4
@@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0, is_unlinkable = 0;
+	int child_locked = 0;
+	bool is_unlinkable = false;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
@@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di
 			mlog_errno(status);
 			goto leave;
 		}
-		is_unlinkable = 1;
+		is_unlinkable = true;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
@@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di
 	}
 
 	if (is_unlinkable) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh,
+				orphan_name, &orphan_insert, orphan_dir);
 		if (status < 0)
 			mlog_errno(status);
 	}
_
...

Quoted 3 lines

This patch looks fine to me. I also test it, and the result is fine. You can consider to add:
Reviewed-by Younger Liu <[email protected]>

I'd like to have a minor change for the naming conventions of inode_is_unlinkable() to avoid any possible conflicts to the VFS exported methods in the future, i.e, s/inode_is_unlinkable/ocfs2_inode_is_unlinkable/

How about below changes in addition to above fix up?

fs/ocfs2/namei.c +2 -2

index d11cf81..6708d29 100644
@@ -773,7 +773,7 @@ static int ocfs2_remote_dentry_delete(struct dentry *dentry)
        return ret;
 }
 
-static inline int inode_is_unlinkable(struct inode *inode)
+static inline int ocfs2_inode_is_unlinkable(struct inode *inode)
 {
        if (S_ISDIR(inode->i_mode)) {
                if (inode->i_nlink == 2)

@@ -866,7 +866,7 @@ static int ocfs2_unlink(struct inode *dir,
                goto leave;
        }
 
-       if (inode_is_unlinkable(inode)) {
+       if (ocfs2_inode_is_unlinkable(inode)) {
                status = ocfs2_prepare_orphan_dir(osb, &orphan_dir,
                                                  OCFS2_I(inode)->ip_blkno,
                                                  orphan_name, &orphan_insert);

Thanks,
-Jeff