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

[Younger Liu]

Hi Jeff

On 2013/6/19 18:53, Jeff Liu wrote:
> Hey Younger,
> 
> On 06/19/2013 04:57 PM, 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.
> 
> Yes, and this has been introduced by commits 3939fda4.

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

> Please see my comments below.
> 
>>
>> Signed-off-by: Younger Liu <younger.liu at huawei.com>
>> ---
>>  fs/ocfs2/namei.c |   38 ++++++++++++++++++++++----------------
>>  1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>> index f53471d..f2da854 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,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));
> 	}
> 

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.

>> +			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
> 
> .
>