[Ocfs2-devel] [PATCH] ocfs2: fix readonly issue in ocfs2_unlink()
Younger Liu
younger.liu at huawei.com
Thu Jun 27 22:52:20 PDT 2013
On 2013/6/28 5:58, Andrew Morton wrote:
> On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <younger.liu at 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 at huawei.com>
More information about the Ocfs2-devel
mailing list