diff mbox

ocfs2: should not use le32_add_cpu to set ocfs2_dinode i_flags

Message ID 51A56B1C.30606@huawei.com
State New, archived
Headers show

Commit Message

Joseph Qi May 29, 2013, 2:42 a.m. UTC
If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
corresponding flag corrupted. So we should change it to bitwise and/or
operation.

Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Shen Canquan May 29, 2013, 7:02 a.m. UTC | #1
On 2013/5/29 10:42, Joseph Qi wrote:
> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
> corresponding flag corrupted. So we should change it to bitwise and/or
> operation.
I think it should be use bitwise and or operation, because one bit stand 
for one meaning.
>
> Signed-off-by: Joseph Qi<joseph.qi@huawei.com>
> ---
>   fs/ocfs2/namei.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 04ee1b5..3a5269a 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
>
>   	fe->i_last_eb_blk = 0;
>   	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
> -	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>   	fe->i_atime = fe->i_ctime = fe->i_mtime =
>   		cpu_to_le64(CURRENT_TIME.tv_sec);
>   	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
> @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>   		goto leave;
>   	}
>
> -	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>   	OCFS2_I(inode)->ip_flags&= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
>
>   	/* Record which orphan dir our inode now resides
> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
>   	}
>
>   	di = (struct ocfs2_dinode *)di_bh->b_data;
> -	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
> +	di->i_flags&= cpu_to_le32(~OCFS2_ORPHANED_FL);
>   	di->i_orphaned_slot = 0;
>   	set_nlink(inode, 1);
>   	ocfs2_set_links_count(di, inode->i_nlink);
jeff.liu May 29, 2013, 7:57 a.m. UTC | #2
Thanks for your patch, Joseph.

On 05/29/2013 10:42 AM, Joseph Qi wrote:

> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
> corresponding flag corrupted. So we should change it to bitwise and/or
> operation.
> 
> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
> ---
>  fs/ocfs2/namei.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 04ee1b5..3a5269a 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
> 
>  	fe->i_last_eb_blk = 0;
>  	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
> -	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>  	fe->i_atime = fe->i_ctime = fe->i_mtime =
>  		cpu_to_le64(CURRENT_TIME.tv_sec);
>  	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
> @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>  		goto leave;
>  	}
> 
> -	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>  	OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
> 
>  	/* Record which orphan dir our inode now resides
> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
>  	}
> 
>  	di = (struct ocfs2_dinode *)di_bh->b_data;
> -	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
> +	di->i_flags &= cpu_to_le32(~OCFS2_ORPHANED_FL);

Hmm? This is wrong.  Instead it should be:
di->i_flags &= ~cpu_to_le32(OCFS2_ORPHANED_FL);

>  	di->i_orphaned_slot = 0;
>  	set_nlink(inode, 1);
>  	ocfs2_set_links_count(di, inode->i_nlink);


-Jeff
Shen Canquan May 29, 2013, 8:12 a.m. UTC | #3
On 2013/5/29 15:57, Jeff Liu wrote:
> Thanks for your patch, Joseph.
>
> On 05/29/2013 10:42 AM, Joseph Qi wrote:
>
>> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
>> corresponding flag corrupted. So we should change it to bitwise and/or
>> operation.
>>
>> Signed-off-by: Joseph Qi<joseph.qi@huawei.com>
>> ---
>>   fs/ocfs2/namei.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>> index 04ee1b5..3a5269a 100644
>> --- a/fs/ocfs2/namei.c
>> +++ b/fs/ocfs2/namei.c
>> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
>>
>>   	fe->i_last_eb_blk = 0;
>>   	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
>> -	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
>> +	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>>   	fe->i_atime = fe->i_ctime = fe->i_mtime =
>>   		cpu_to_le64(CURRENT_TIME.tv_sec);
>>   	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
>> @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>   		goto leave;
>>   	}
>>
>> -	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
>> +	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>>   	OCFS2_I(inode)->ip_flags&= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
>>
>>   	/* Record which orphan dir our inode now resides
>> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
>>   	}
>>
>>   	di = (struct ocfs2_dinode *)di_bh->b_data;
>> -	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
>> +	di->i_flags&= cpu_to_le32(~OCFS2_ORPHANED_FL);
> Hmm? This is wrong.  Instead it should be:
> di->i_flags&= ~cpu_to_le32(OCFS2_ORPHANED_FL);

  in ocfs2_remove_inode function , it has the following code:
     di->i_dtime = cpu_to_le64(CURRENT_TIME.tv_sec);
     di->i_flags &= cpu_to_le32(~(OCFS2_VALID_FL | OCFS2_ORPHANED_FL));

I think at first clear the flag and then convert to the little end.
>>   	di->i_orphaned_slot = 0;
>>   	set_nlink(inode, 1);
>>   	ocfs2_set_links_count(di, inode->i_nlink);
>
> -Jeff
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
>
jeff.liu May 29, 2013, 10:17 a.m. UTC | #4
On 05/29/2013 04:12 PM, shencanquan wrote:

> On 2013/5/29 15:57, Jeff Liu wrote:
>> Thanks for your patch, Joseph.
>>
>> On 05/29/2013 10:42 AM, Joseph Qi wrote:
>>
>>> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
>>> corresponding flag corrupted. So we should change it to bitwise and/or
>>> operation.
>>>
>>> Signed-off-by: Joseph Qi<joseph.qi@huawei.com>
>>> ---
>>>   fs/ocfs2/namei.c |    6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>> index 04ee1b5..3a5269a 100644
>>> --- a/fs/ocfs2/namei.c
>>> +++ b/fs/ocfs2/namei.c
>>> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
>>>
>>>       fe->i_last_eb_blk = 0;
>>>       strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
>>> -    le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
>>> +    fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>>>       fe->i_atime = fe->i_ctime = fe->i_mtime =
>>>           cpu_to_le64(CURRENT_TIME.tv_sec);
>>>       fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
>>> @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super
>>> *osb,
>>>           goto leave;
>>>       }
>>>
>>> -    le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
>>> +    fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>>>       OCFS2_I(inode)->ip_flags&= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
>>>
>>>       /* Record which orphan dir our inode now resides
>>> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode
>>> *dir,
>>>       }
>>>
>>>       di = (struct ocfs2_dinode *)di_bh->b_data;
>>> -    le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
>>> +    di->i_flags&= cpu_to_le32(~OCFS2_ORPHANED_FL);
>> Hmm? This is wrong.  Instead it should be:
>> di->i_flags&= ~cpu_to_le32(OCFS2_ORPHANED_FL);
> 
>  in ocfs2_remove_inode function , it has the following code:
>     di->i_dtime = cpu_to_le64(CURRENT_TIME.tv_sec);
>     di->i_flags &= cpu_to_le32(~(OCFS2_VALID_FL | OCFS2_ORPHANED_FL));
> 
> I think at first clear the flag and then convert to the little end.

Call cpu_to_xxx(~value) or (~cpu_to_xxx(value)) will be evaluated end up
with the same result, but the later manner would looks a bit neater IMO.

Thanks,
-Jeff
Joseph Qi May 30, 2013, 4:40 a.m. UTC | #5
You are right. Thanks for your review comments.
I will do the corresponding modification and resend the patch.

On 2013/5/29 18:17, Jeff Liu wrote:
> On 05/29/2013 04:12 PM, shencanquan wrote:
> 
>> On 2013/5/29 15:57, Jeff Liu wrote:
>>> Thanks for your patch, Joseph.
>>>
>>> On 05/29/2013 10:42 AM, Joseph Qi wrote:
>>>
>>>> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
>>>> corresponding flag corrupted. So we should change it to bitwise and/or
>>>> operation.
>>>>
>>>> Signed-off-by: Joseph Qi<joseph.qi@huawei.com>
>>>> ---
>>>>   fs/ocfs2/namei.c |    6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>>> index 04ee1b5..3a5269a 100644
>>>> --- a/fs/ocfs2/namei.c
>>>> +++ b/fs/ocfs2/namei.c
>>>> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
>>>>
>>>>       fe->i_last_eb_blk = 0;
>>>>       strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
>>>> -    le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
>>>> +    fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>>>>       fe->i_atime = fe->i_ctime = fe->i_mtime =
>>>>           cpu_to_le64(CURRENT_TIME.tv_sec);
>>>>       fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
>>>> @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super
>>>> *osb,
>>>>           goto leave;
>>>>       }
>>>>
>>>> -    le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
>>>> +    fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>>>>       OCFS2_I(inode)->ip_flags&= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
>>>>
>>>>       /* Record which orphan dir our inode now resides
>>>> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode
>>>> *dir,
>>>>       }
>>>>
>>>>       di = (struct ocfs2_dinode *)di_bh->b_data;
>>>> -    le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
>>>> +    di->i_flags&= cpu_to_le32(~OCFS2_ORPHANED_FL);
>>> Hmm? This is wrong.  Instead it should be:
>>> di->i_flags&= ~cpu_to_le32(OCFS2_ORPHANED_FL);
>>
>>  in ocfs2_remove_inode function , it has the following code:
>>     di->i_dtime = cpu_to_le64(CURRENT_TIME.tv_sec);
>>     di->i_flags &= cpu_to_le32(~(OCFS2_VALID_FL | OCFS2_ORPHANED_FL));
>>
>> I think at first clear the flag and then convert to the little end.
> 
> Call cpu_to_xxx(~value) or (~cpu_to_xxx(value)) will be evaluated end up
> with the same result, but the later manner would looks a bit neater IMO.
> 
> Thanks,
> -Jeff
> 
> .
>
diff mbox

Patch

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 04ee1b5..3a5269a 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -522,7 +522,7 @@  static int __ocfs2_mknod_locked(struct inode *dir,

 	fe->i_last_eb_blk = 0;
 	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
-	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
+	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
 	fe->i_atime = fe->i_ctime = fe->i_mtime =
 		cpu_to_le64(CURRENT_TIME.tv_sec);
 	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec =
@@ -2044,7 +2044,7 @@  static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}

-	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
+	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
 	OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR;

 	/* Record which orphan dir our inode now resides
@@ -2434,7 +2434,7 @@  int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
 	}

 	di = (struct ocfs2_dinode *)di_bh->b_data;
-	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
+	di->i_flags &= cpu_to_le32(~OCFS2_ORPHANED_FL);
 	di->i_orphaned_slot = 0;
 	set_nlink(inode, 1);
 	ocfs2_set_links_count(di, inode->i_nlink);