diff mbox

[07/28] ocfs2: fix a tiny case that inode can not removed.

Message ID 55de3999.S27PDhRmJdx2Acho%akpm@linux-foundation.org (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Morton Aug. 26, 2015, 10:11 p.m. UTC 
From: jiangyiwen <jiangyiwen@huawei.com>
Subject: ocfs2: fix a tiny case that inode can not removed.

When running dirop_fileop_racer we found a case that inode
can not removed.

2 nodes, say Node A and Node B, mount the same ocfs2 volume. Create
two dirs /race/1/ and /race/2/ in the filesystem.

Node A                            Node B
rm -r /race/2/
                                  mv /race/1/ /race/2/
call ocfs2_unlink(), get
the EX mode of /race/2/
                                  wait for B unlock /race/2/
decrease i_nlink of /race/2/ to 0,
and add inode of /race/2/ into
orphan dir, unlock /race/2/
                                  got EX mode of /race/2/. because
                                  /race/1/ is dir, so inc i_nlink
                                  of /race/2/ and update into disk,
                                  unlock /race/2/
because i_nlink of /race/2/
is not zero, this inode will
always remain in orphan dir

This patch fixes this case by test whether i_nlink of new dir is zero.

Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/namei.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Mark Fasheh Aug. 28, 2015, 11:26 p.m. UTC | #1 
On Wed, Aug 26, 2015 at 03:11:37PM -0700, Andrew Morton wrote:
> From: jiangyiwen <jiangyiwen@huawei.com>
> Subject: ocfs2: fix a tiny case that inode can not removed.
> 
> When running dirop_fileop_racer we found a case that inode
> can not removed.
> 
> 2 nodes, say Node A and Node B, mount the same ocfs2 volume. Create
> two dirs /race/1/ and /race/2/ in the filesystem.
> 
> Node A                            Node B
> rm -r /race/2/
>                                   mv /race/1/ /race/2/
> call ocfs2_unlink(), get
> the EX mode of /race/2/
>                                   wait for B unlock /race/2/
> decrease i_nlink of /race/2/ to 0,
> and add inode of /race/2/ into
> orphan dir, unlock /race/2/
>                                   got EX mode of /race/2/. because
>                                   /race/1/ is dir, so inc i_nlink
>                                   of /race/2/ and update into disk,
>                                   unlock /race/2/
> because i_nlink of /race/2/
> is not zero, this inode will
> always remain in orphan dir
> 
> This patch fixes this case by test whether i_nlink of new dir is zero.


Good catch, thanks for this.


> diff -puN fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed fs/ocfs2/namei.c
> --- a/fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed
> +++ a/fs/ocfs2/namei.c
> @@ -1284,6 +1284,15 @@ static int ocfs2_rename(struct inode *ol
>  	}
>  	parents_locked = 1;
>  
> +	if (!new_dir->i_nlink) {
> +		mlog(ML_ERROR, "new dir %llu has been removed, inode %llu "
> +				"can not be moved into it.",
> +				(unsigned long long)new_dir->i_ino,
> +				(unsigned long long)old_inode->i_ino);

This is something that is rare, but not an error or otherwise outstanding
condition. Therefore printing to the kernel log is not advisable and may
confuse users. Please turn that into a debug print or remove it all
together, thanks.
	--Mark

--
Mark Fasheh
Jiangyiwen Aug. 31, 2015, 7:28 a.m. UTC | #2 
On 2015/8/29 7:26, Mark Fasheh wrote:
> On Wed, Aug 26, 2015 at 03:11:37PM -0700, Andrew Morton wrote:
>> From: jiangyiwen <jiangyiwen@huawei.com>
>> Subject: ocfs2: fix a tiny case that inode can not removed.
>>
>> When running dirop_fileop_racer we found a case that inode
>> can not removed.
>>
>> 2 nodes, say Node A and Node B, mount the same ocfs2 volume. Create
>> two dirs /race/1/ and /race/2/ in the filesystem.
>>
>> Node A                            Node B
>> rm -r /race/2/
>>                                   mv /race/1/ /race/2/
>> call ocfs2_unlink(), get
>> the EX mode of /race/2/
>>                                   wait for B unlock /race/2/
>> decrease i_nlink of /race/2/ to 0,
>> and add inode of /race/2/ into
>> orphan dir, unlock /race/2/
>>                                   got EX mode of /race/2/. because
>>                                   /race/1/ is dir, so inc i_nlink
>>                                   of /race/2/ and update into disk,
>>                                   unlock /race/2/
>> because i_nlink of /race/2/
>> is not zero, this inode will
>> always remain in orphan dir
>>
>> This patch fixes this case by test whether i_nlink of new dir is zero.
> 
> 
> Good catch, thanks for this.
> 
> 
>> diff -puN fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed fs/ocfs2/namei.c
>> --- a/fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed
>> +++ a/fs/ocfs2/namei.c
>> @@ -1284,6 +1284,15 @@ static int ocfs2_rename(struct inode *ol
>>  	}
>>  	parents_locked = 1;
>>  
>> +	if (!new_dir->i_nlink) {
>> +		mlog(ML_ERROR, "new dir %llu has been removed, inode %llu "
>> +				"can not be moved into it.",
>> +				(unsigned long long)new_dir->i_ino,
>> +				(unsigned long long)old_inode->i_ino);
> 
> This is something that is rare, but not an error or otherwise outstanding
> condition. Therefore printing to the kernel log is not advisable and may
> confuse users. Please turn that into a debug print or remove it all
> together, thanks.
> 	--Mark
> 
> --
> Mark Fasheh
> 
> .
> 
OK, thanks Mark, I will resend a new version immediately.

Thanks,
Yiwen Jiang
diff mbox

Patch

diff -puN fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed fs/ocfs2/namei.c
--- a/fs/ocfs2/namei.c~ocfs2-fix-a-tiny-case-that-inode-can-not-removed
+++ a/fs/ocfs2/namei.c
@@ -1284,6 +1284,15 @@  static int ocfs2_rename(struct inode *ol
 	}
 	parents_locked = 1;
 
+	if (!new_dir->i_nlink) {
+		mlog(ML_ERROR, "new dir %llu has been removed, inode %llu "
+				"can not be moved into it.",
+				(unsigned long long)new_dir->i_ino,
+				(unsigned long long)old_inode->i_ino);
+		status = -EACCES;
+		goto bail;
+	}
+
 	/* make sure both dirs have bhs
 	 * get an extra ref on old_dir_bh if old==new */
 	if (!new_dir_bh) {