diff mbox

[V2] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

Message ID 51C2B662.2000206@huawei.com
State New, archived
Headers show

Commit Message

Younger Liu June 20, 2013, 7:59 a.m. UTC
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 <younger.liu@huawei.com>
Cc: Jie Liu <jeff.liu@oracle.com>
---
 fs/ocfs2/namei.c |   39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

Comments

Younger Liu June 20, 2013, 8:46 a.m. UTC | #1
On 2013/6/20 15:59, Younger Liu wrote:
> 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. 
> 

This bug is introduced by commits 3939fda4.

> Signed-off-by: Younger Liu <younger.liu@huawei.com>
> Cc: Jie Liu <jeff.liu@oracle.com>
> ---
>  fs/ocfs2/namei.c |   39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index f53471d..087c58b 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -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,13 @@ 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) {
> +		if (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);
>  
>
Andrew Morton June 26, 2013, 9:49 p.m. UTC | #2
On Thu, 20 Jun 2013 15:59:30 +0800 Younger Liu <younger.liu@huawei.com> wrote:

> 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. 
> 
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,

ocfs2_orphan_add() will call mlog_errno(status) two times for the same
error in many cases.  That's not a big problem, but is sloppy and asks
for a cleanup.
Younger Liu June 28, 2013, 6:30 a.m. UTC | #3
On 2013/6/27 5:49, Andrew Morton wrote:
> On Thu, 20 Jun 2013 15:59:30 +0800 Younger Liu <younger.liu@huawei.com> wrote:
> 
>> 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. 
>>
>> --- a/fs/ocfs2/namei.c
>> +++ b/fs/ocfs2/namei.c
>> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
> 
> ocfs2_orphan_add() will call mlog_errno(status) two times for the same
> error in many cases.  That's not a big problem, but is sloppy and asks
> for a cleanup.
> 
> 
> 
Thanks for your review, I will resent the patch in a moment.
					Younger
diff mbox

Patch

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f53471d..087c58b 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -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,13 @@  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) {
+		if (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);