[Ocfs2-devel] [PATCH V3] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()

[Jeff Liu]

On 07/03/2013 05:26 AM, Andrew Morton wrote:

> On Fri, 28 Jun 2013 14:50:59 +0800 Younger Liu <younger.liu at 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. 
>>
>> This bug is introduced by commits 3939fda4.
>>
>> Compared with the PATCH V2, this patch cleans up mlog_errno(status) 
>> which was called two times for the same error.
>>
> 
> Folks, could we please get this one reviewed?

At least, this fix looks good to me.

Thanks,
-Jeff

> 
> From: Younger Liu <younger.liu at huawei.com>
> Subject: 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.
> 
> This bug was added by 3939fda4 ("Ocfs2: Journaling i_flags and
> i_orphaned_slot when adding inode to orphan dir.").
> 
> Signed-off-by: Younger Liu <younger.liu at huawei.com>
> Cc: Jie Liu <jeff.liu at oracle.com>
> Cc: Sunil Mushran <sunil.mushran at gmail.com>
> Cc: Mark Fasheh <mfasheh at suse.com>
> Cc: Joel Becker <jlbec at evilplan.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> ---
> 
>  fs/ocfs2/namei.c |   41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff -puN fs/ocfs2/namei.c~ocfs2-need-rollback-when-journal_access-failed-in-ocfs2_orphan_add fs/ocfs2/namei.c
> --- a/fs/ocfs2/namei.c~ocfs2-need-rollback-when-journal_access-failed-in-ocfs2_orphan_add
> +++ a/fs/ocfs2/namei.c
> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2
>  		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
>  				   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,11 +2057,16 @@ static int ocfs2_orphan_add(struct ocfs2
>  	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);
>  
> -	if (status)
> -		mlog_errno(status);
>  	return status;
>  }
>  
> _
>