[Ocfs2-devel] [PATCH] ocfs2: fix readonly issue in ocfs2_unlink()

[Younger Liu <younger.liu@huawei.com>]

On 2013/6/28 5:58, Andrew Morton wrote:
> On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <younger.liu@huawei.com> 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).
>>
>> ...
>>
>> --- a/fs/ocfs2/namei.c
>> +++ b/fs/ocfs2/namei.c
>> @@ -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.
> 
>>  	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);
>>  	}
> 
> This is yet another ocfs2 function which reports the same error two
> times.  ho hum.
> 
Thanks for your review.

> Please review:
> 
> --- a/fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix
> +++ a/fs/ocfs2/namei.c
> @@ -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);
>  	}
> _
> 
...

This patch looks fine to me. I also test it, and the result is fine. 
You can consider to add:
Reviewed-by: Younger Liu <younger.liu@huawei.com>