ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

PATCH New Younger Liu 3 participants 6 messages Jun 19, 2013 – Jun 20, 2013 OCFS2

Share & tools

Younger Liu

author

2013-06-19 08:57 UTC #0 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.

So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.

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

---
 fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

fs/ocfs2/namei.c +22 ~~-16~~

index f53471d..f2da854 100644
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	/* we're a cluster, and nlink can change on disk from
 	 * underneath us... */
 	orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;

@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 				   orphan_dir_bh, lookup);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
-	}
-
-	/*
-	 * We're going to journal the change of i_flags and i_orphaned_slot.
-	 * It's safe anyway, though some callers may duplicate the journaling.
-	 * Journaling within the func just make the logic look more
-	 * straightforward.
-	 */
-	status = ocfs2_journal_access_di(handle,
-					 INODE_CACHE(inode),
-					 fe_bh,
-					 OCFS2_JOURNAL_ACCESS_WRITE);
-	if (status < 0) {
-		mlog_errno(status);
-		goto leave;
+		goto rollback;
 	}
 
 	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);

@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
 				   osb->slot_num);
 
+rollback:
+	if (status < 0 && S_ISDIR(inode->i_mode)) {
+		ocfs2_add_links_count(orphan_fe, -1);
+		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+	}
+
 leave:
 	brelse(orphan_dir_bh);

-- 1.7.9.7

Jeff Liu

2013-06-19 10:53 UTC #1 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

Hey Younger,

Quoted 11 lines · Younger Liu

While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.

Yes, and this has been introduced by commits 3939fda4. Please see my comments below.

Quoted 57 lines

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

---
 fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

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

index f53471d..f2da854 100644
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	/* we're a cluster, and nlink can change on disk from
 	 * underneath us... */
 	orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;

@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 				   orphan_dir_bh, lookup);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
-	}
-
-	/*
-	 * We're going to journal the change of i_flags and i_orphaned_slot.
-	 * It's safe anyway, though some callers may duplicate the journaling.
-	 * Journaling within the func just make the logic look more
-	 * straightforward.
-	 */
-	status = ocfs2_journal_access_di(handle,
-					 INODE_CACHE(inode),
-					 fe_bh,
-					 OCFS2_JOURNAL_ACCESS_WRITE);
-	if (status < 0) {
-		mlog_errno(status);
-		goto leave;
+		goto rollback;
 	}
 
 	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);

fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb, · line 2061

Quoted 5 lines

fs/ocfs2/namei.c +2 -0

@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
 				   osb->slot_num);
 
+rollback:
+	if (status < 0 && S_ISDIR(inode->i_mode)) {

^^^^ <-- But that might not be a directory, right? :-P
How about the following?

	if (status < 0) {
		if (S_ISDIR(inode->i_mode))
			ocfs2_add_links_count(orphan_fe, -1);
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
	}

fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb,

Quoted 8 lines

fs/ocfs2/namei.c +4 -1

+			ocfs2_add_links_count(orphan_fe, -1);
+		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+	}
+
 leave:
 	brelse(orphan_dir_bh);
 
-- 1.7.9.7

Thanks,
-Jeff

Younger Liu

author

2013-06-20 01:25 UTC #2 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

Hi Jeff

Quoted 12 lines · Jeff Liu

Hey Younger,
Quoted 8 lines · Younger Liu
While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.

Quoted 5 lines

Quoted 3 lines
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.
Yes, and this has been introduced by commits 3939fda4.

I do not find the patch with 3939fda4. Can you tell me the link? Thanks.

Quoted 68 lines

Please see my comments below.

Quoted 57 lines

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

---
 fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

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

index f53471d..f2da854 100644
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	/* we're a cluster, and nlink can change on disk from
 	 * underneath us... */
 	orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;

@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 				   orphan_dir_bh, lookup);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
-	}
-
-	/*
-	 * We're going to journal the change of i_flags and i_orphaned_slot.
-	 * It's safe anyway, though some callers may duplicate the journaling.
-	 * Journaling within the func just make the logic look more
-	 * straightforward.
-	 */
-	status = ocfs2_journal_access_di(handle,
-					 INODE_CACHE(inode),
-					 fe_bh,
-					 OCFS2_JOURNAL_ACCESS_WRITE);
-	if (status < 0) {
-		mlog_errno(status);
-		goto leave;
+		goto rollback;
 	}
 
 	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);

fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb, · line 2061

Quoted 5 lines

fs/ocfs2/namei.c +2 -0

@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
 				   osb->slot_num);
 
+rollback:
+	if (status < 0 && S_ISDIR(inode->i_mode)) {

^^^^ <-- But that might not be a directory, right? :-P
How about the following?

Quoted 6 lines

	if (status < 0) {
		if (S_ISDIR(inode->i_mode))
			ocfs2_add_links_count(orphan_fe, -1);
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
	}

Of course, that may be not a directory.
But if it is not a directory, there is no change for orphan_dir_inode->i_nlink. So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.

Quoted 15 lines

fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb,
Quoted 8 lines
fs/ocfs2/namei.c +4 -1
+			ocfs2_add_links_count(orphan_fe, -1);
+		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+	}
+
 leave:
 	brelse(orphan_dir_bh);
 
-- 1.7.9.7 
Thanks,
-Jeff
.

Li Zefan

2013-06-20 01:42 UTC #3 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

? 2013/6/20 9:25, Younger Liu ??:

Quoted 15 lines

Hi Jeff
Quoted 12 lines · Jeff Liu
Hey Younger,
Quoted 8 lines · Younger Liu
While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.
Yes, and this has been introduced by commits 3939fda4.
I do not find the patch with 3939fda4. Can you tell me the link? Thanks.

# git show 3939fda4
commit 3939fda4b389993caf8741df5739b3e49f33a263
Author: Tristan Ye <[email protected]>
Date:   Fri Mar 19 09:21:09 2010 +0800

Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.

Currently, some callers were missing to journal the dirty inode after adding it to orphan dir.

Now we're going to journal such modifications within the ocfs2_orphan_add() itself, It's safe to do so, though some existing caller may duplicate this, and it makes the logic look more straightforward anyway.

Signed-off-by Tristan Ye <[email protected]>

Signed-off-by Joel Becker <[email protected]>

Jeff Liu

2013-06-20 05:52 UTC #4 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

In reply to Younger Liu

Quoted 99 lines · Younger Liu

Hi Jeff
Quoted 12 lines · Jeff Liu
Hey Younger,
Quoted 8 lines · Younger Liu
While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.
Quoted 5 lines
Quoted 3 lines
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.
Yes, and this has been introduced by commits 3939fda4.
I do not find the patch with 3939fda4. Can you tell me the link? Thanks.
Quoted 68 lines
Please see my comments below.
Quoted 57 lines
Signed-off-by Younger Liu <[email protected]>
---
 fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
fs/ocfs2/namei.c +16 ~~-16~~
index f53471d..f2da854 100644
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	/* we're a cluster, and nlink can change on disk from
 	 * underneath us... */
 	orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 				   orphan_dir_bh, lookup);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
-	}
-
-	/*
-	 * We're going to journal the change of i_flags and i_orphaned_slot.
-	 * It's safe anyway, though some callers may duplicate the journaling.
-	 * Journaling within the func just make the logic look more
-	 * straightforward.
-	 */
-	status = ocfs2_journal_access_di(handle,
-					 INODE_CACHE(inode),
-					 fe_bh,
-					 OCFS2_JOURNAL_ACCESS_WRITE);
-	if (status < 0) {
-		mlog_errno(status);
-		goto leave;
+		goto rollback;
 	}
 
 	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb, · line 2061
Quoted 5 lines
fs/ocfs2/namei.c +2 -0
@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
 				   osb->slot_num);
 
+rollback:
+	if (status < 0 && S_ISDIR(inode->i_mode)) {
^^^^ <-- But that might not be a directory, right? :-P
How about the following?
Quoted 6 lines
	if (status < 0) {
		if (S_ISDIR(inode->i_mode))
			ocfs2_add_links_count(orphan_fe, -1);
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
	}

Quoted 3 lines

Of course, that may be not a directory.
But if it is not a directory, there is no change for orphan_dir_inode->i_nlink. So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.

	if (S_ISDIR(inode->i_mode))
                ocfs2_add_links_count(orphan_fe, 1);
        set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe)); 
	^^^^^^^^^
		So we set nlink against orphan_dir_inode no matter IS_DIR(inode->i_mode) or not.
        ocfs2_journal_dirty(handle, orphan_dir_bh);
		Any then orphan_dir buffer got dirty.
	
	if (status < 0 && S_ISDIR(inode->i_mode)) {
		In this way, AFAICS the trans rollback can be triggered only if
		the target inode is a dir.
	}

	As a result, should we rollback in case of:
	if (status < 0)
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));

Thanks,
-Jeff

Quoted 17 lines

Quoted 16 lines
fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb,
Quoted 9 lines
fs/ocfs2/namei.c +4 -1
+			ocfs2_add_links_count(orphan_fe, -1);
+		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+	}
+
 leave:
 	brelse(orphan_dir_bh);
 
-- 1.7.9.7 
Thanks,
-Jeff
.

Younger Liu

author

2013-06-20 06:33 UTC #5 lore raw

PATCH 1/1 ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

Ok, thanks.
I will resent the patch.

Quoted 148 lines · Jeff Liu

Quoted 99 lines · Younger Liu
Hi Jeff
Quoted 12 lines · Jeff Liu
Hey Younger,
Quoted 8 lines · Younger Liu
While adding a file into orphan dir in ocfs2_orphan_add(), it calls __ocfs2_add_entry() before ocfs2_journal_access_di(). If ocfs2_journal_access_di() failed, the file is added into orphan dir, and orphan dir dinode updated, but file dinode has not been updated.
Accordingly, the data is not consistent between file dinode and orphan dir.
Quoted 5 lines
Quoted 3 lines
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(), and if ocfs2_journal_access_di() failed, orphan_fe and orphan_dir_inode->i_nlink need rollback.
Yes, and this has been introduced by commits 3939fda4.
I do not find the patch with 3939fda4. Can you tell me the link? Thanks.
Quoted 68 lines
Please see my comments below.
Quoted 57 lines
Signed-off-by Younger Liu <[email protected]>
---
 fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
fs/ocfs2/namei.c +16 ~~-16~~
index f53471d..f2da854 100644
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}
 
+	/*
+	 * We're going to journal the change of i_flags and i_orphaned_slot.
+	 * It's safe anyway, though some callers may duplicate the journaling.
+	 * Journaling within the func just make the logic look more
+	 * straightforward.
+	 */
+	status = ocfs2_journal_access_di(handle,
+					 INODE_CACHE(inode),
+					 fe_bh,
+					 OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto leave;
+	}
+
 	/* we're a cluster, and nlink can change on disk from
 	 * underneath us... */
 	orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 				   orphan_dir_bh, lookup);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
-	}
-
-	/*
-	 * We're going to journal the change of i_flags and i_orphaned_slot.
-	 * It's safe anyway, though some callers may duplicate the journaling.
-	 * Journaling within the func just make the logic look more
-	 * straightforward.
-	 */
-	status = ocfs2_journal_access_di(handle,
-					 INODE_CACHE(inode),
-					 fe_bh,
-					 OCFS2_JOURNAL_ACCESS_WRITE);
-	if (status < 0) {
-		mlog_errno(status);
-		goto leave;
+		goto rollback;
 	}
 
 	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
fs/ocfs2/namei.c static int ocfs2_orphan_add(struct ocfs2_super *osb, · line 2061
Quoted 5 lines
fs/ocfs2/namei.c +2 -0
@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 	trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
 				   osb->slot_num);
 
+rollback:
+	if (status < 0 && S_ISDIR(inode->i_mode)) {
^^^^ <-- But that might not be a directory, right? :-P
How about the following?
Quoted 6 lines
	if (status < 0) {
		if (S_ISDIR(inode->i_mode))
			ocfs2_add_links_count(orphan_fe, -1);
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
	}
Quoted 3 lines
Of course, that may be not a directory.
But if it is not a directory, there is no change for orphan_dir_inode->i_nlink. So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.
	if (S_ISDIR(inode->i_mode))
                ocfs2_add_links_count(orphan_fe, 1);
        set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe)); 
	^^^^^^^^^
		So we set nlink against orphan_dir_inode no matter IS_DIR(inode->i_mode) or not.
        ocfs2_journal_dirty(handle, orphan_dir_bh);
		Any then orphan_dir buffer got dirty.
	
	if (status < 0 && S_ISDIR(inode->i_mode)) {
		In this way, AFAICS the trans rollback can be triggered only if
		the target inode is a dir.
	}

	As a result, should we rollback in case of:
	if (status < 0)
		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
Thanks,
-Jeff
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
fs/ocfs2/namei.c +4 -0
+			ocfs2_add_links_count(orphan_fe, -1);
+		set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+	}
+
 leave:
 	brelse(orphan_dir_bh);
 
Quoted 9 lines
Quoted 8 lines
Quoted 1 line
-- 1.7.9.7
Thanks,
-Jeff
.
.