[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Wengang Wang]

Hi Joel/Tao,

I don't know the reflink very well, so please ignore this patch if I am wrong.

I think in ocfs2_prepare_inode_for_write(), we disable DIO write if the inode
has reflink.
If am right, the way we determine if the inode has reflink is wrong in case
(!has_refcount && direct_io).

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/file.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 06ccf6a..77ebb6e 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1775,7 +1775,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
 					 int *direct_io,
 					 int *has_refcount)
 {
-	int ret = 0, meta_level = 0;
+	int ret = 0, meta_level = 0, refcount = 0;
 	struct inode *inode = dentry->d_inode;
 	loff_t saved_pos, end;
 
@@ -1834,6 +1834,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
 							       saved_pos,
 							       count,
 							       &meta_level);
+			refcount = 1;
 			if (has_refcount)
 				*has_refcount = 1;
 		}
@@ -1859,7 +1860,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
 			break;
 		}
 
-		if (has_refcount && *has_refcount == 1) {
+		if (refcount) {
 			*direct_io = 0;
 			break;
 		}
-- 
1.6.6

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Tao Ma]

Hi wengang,

Wengang Wang wrote:
> Hi Joel/Tao,
> 
> I don't know the reflink very well, so please ignore this patch if I am wrong.
> 
> I think in ocfs2_prepare_inode_for_write(), we disable DIO write if the inode
> has reflink.
> If am right, the way we determine if the inode has reflink is wrong in case
> (!has_refcount && direct_io).
I just check the caller, all these 2 parameters are either set or NULL 
simultaneously. You patch only make sense in (!has_refcount && 
direct_io), but currently we don't have such a case. So why bother 
adding redundant code for a not-exist case?

Regards,
Tao

> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  fs/ocfs2/file.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 06ccf6a..77ebb6e 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1775,7 +1775,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  					 int *direct_io,
>  					 int *has_refcount)
>  {
> -	int ret = 0, meta_level = 0;
> +	int ret = 0, meta_level = 0, refcount = 0;
>  	struct inode *inode = dentry->d_inode;
>  	loff_t saved_pos, end;
>  
> @@ -1834,6 +1834,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  							       saved_pos,
>  							       count,
>  							       &meta_level);
> +			refcount = 1;
>  			if (has_refcount)
>  				*has_refcount = 1;
>  		}
> @@ -1859,7 +1860,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  			break;
>  		}
>  
> -		if (has_refcount && *has_refcount == 1) {
> +		if (refcount) {
>  			*direct_io = 0;
>  			break;
>  		}

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Wengang Wang]

Hi Tao,

On 10-02-05 08:58, Tao Ma wrote:
> Hi wengang,
>
> Wengang Wang wrote:
>> Hi Joel/Tao,
>>
>> I don't know the reflink very well, so please ignore this patch if I am wrong.
>>
>> I think in ocfs2_prepare_inode_for_write(), we disable DIO write if the inode
>> has reflink.
>> If am right, the way we determine if the inode has reflink is wrong in case
>> (!has_refcount && direct_io).
> I just check the caller, all these 2 parameters are either set or NULL  
> simultaneously. You patch only make sense in (!has_refcount &&  
> direct_io), but currently we don't have such a case. So why bother  
> adding redundant code for a not-exist case?

Yes that current calling has no problem. But such interface has potential danger
for callers in future.
If you don't like change code, I think it's better to add comment that
has_refcount and direct_io must be both NULL or both non-NULL.


regards,
wengang.

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Tao Ma]

Wengang Wang wrote:
> Hi Tao,
> 
> On 10-02-05 08:58, Tao Ma wrote:
>> Hi wengang,
>>
>> Wengang Wang wrote:
>>> Hi Joel/Tao,
>>>
>>> I don't know the reflink very well, so please ignore this patch if I am wrong.
>>>
>>> I think in ocfs2_prepare_inode_for_write(), we disable DIO write if the inode
>>> has reflink.
>>> If am right, the way we determine if the inode has reflink is wrong in case
>>> (!has_refcount && direct_io).
>> I just check the caller, all these 2 parameters are either set or NULL  
>> simultaneously. You patch only make sense in (!has_refcount &&  
>> direct_io), but currently we don't have such a case. So why bother  
>> adding redundant code for a not-exist case?
> 
> Yes that current calling has no problem. But such interface has potential danger
> for callers in future.
> If you don't like change code, I think it's better to add comment that
> has_refcount and direct_io must be both NULL or both non-NULL.
Add it please as you wish.

Regards,
Tao

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Wengang Wang]

On 10-02-05 10:30, Tao Ma wrote:
>
>
> Wengang Wang wrote:
>> Hi Tao,
>>
>> On 10-02-05 08:58, Tao Ma wrote:
>>> Hi wengang,
>>>
>>> Wengang Wang wrote:
>>>> Hi Joel/Tao,
>>>>
>>>> I don't know the reflink very well, so please ignore this patch if I am wrong.
>>>>
>>>> I think in ocfs2_prepare_inode_for_write(), we disable DIO write if the inode
>>>> has reflink.
>>>> If am right, the way we determine if the inode has reflink is wrong in case
>>>> (!has_refcount && direct_io).
>>> I just check the caller, all these 2 parameters are either set or 
>>> NULL  simultaneously. You patch only make sense in (!has_refcount &&  
>>> direct_io), but currently we don't have such a case. So why bother   
>>> adding redundant code for a not-exist case?
>>
>> Yes that current calling has no problem. But such interface has potential danger
>> for callers in future.
>> If you don't like change code, I think it's better to add comment that
>> has_refcount and direct_io must be both NULL or both non-NULL.
> Add it please as you wish.

It's not the best way I want to do. I persist what I have post :).

regards,
wengang.

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Joel Becker]

On Thu, Feb 04, 2010 at 04:20:18PM -0500, Wengang Wang wrote:
> index 06ccf6a..77ebb6e 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1775,7 +1775,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  					 int *direct_io,
>  					 int *has_refcount)
>  {
> -	int ret = 0, meta_level = 0;
> +	int ret = 0, meta_level = 0, refcount = 0;
>  	struct inode *inode = dentry->d_inode;
>  	loff_t saved_pos, end;
>  
> @@ -1834,6 +1834,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  							       saved_pos,
>  							       count,
>  							       &meta_level);
> +			refcount = 1;
>  			if (has_refcount)
>  				*has_refcount = 1;
>  		}

	Tao is right that this is redundant - we're setting two refcount
bits, which is pointless.
	Here's the thing: we know that refcounted extents mean no direct
io.  Instead of adding your 'refcount' bit or Tao's comment, why not
just clear direct_io right here?

@@ -1834,6 +1834,8 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
 							       saved_pos,
 							       count,
 							       &meta_level);
 			if (has_refcount)
 				*has_refcount = 1;
+			if (direct_io)
+				*direct_io = 0;
 		}

It's safe to clear if it exists - whether it was zero before, it must be
zero now.

Joel

-- 

"War doesn't determine who's right; war determines who's left."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH] ocfs2: fix a refcount condition checking

[Tao Ma]

Joel Becker wrote:
> On Thu, Feb 04, 2010 at 04:20:18PM -0500, Wengang Wang wrote:
>> index 06ccf6a..77ebb6e 100644
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -1775,7 +1775,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>>  					 int *direct_io,
>>  					 int *has_refcount)
>>  {
>> -	int ret = 0, meta_level = 0;
>> +	int ret = 0, meta_level = 0, refcount = 0;
>>  	struct inode *inode = dentry->d_inode;
>>  	loff_t saved_pos, end;
>>  
>> @@ -1834,6 +1834,7 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>>  							       saved_pos,
>>  							       count,
>>  							       &meta_level);
>> +			refcount = 1;
>>  			if (has_refcount)
>>  				*has_refcount = 1;
>>  		}
> 
> 	Tao is right that this is redundant - we're setting two refcount
> bits, which is pointless.
> 	Here's the thing: we know that refcounted extents mean no direct
> io.  Instead of adding your 'refcount' bit or Tao's comment, why not
> just clear direct_io right here?
> 
> @@ -1834,6 +1834,8 @@ static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
>  							       saved_pos,
>  							       count,
>  							       &meta_level);
>  			if (has_refcount)
>  				*has_refcount = 1;
> +			if (direct_io)
> +				*direct_io = 0;
>  		}
> 
> It's safe to clear if it exists - whether it was zero before, it must be
> zero now.
yeah, this is better.
btw, we also need to remove the latter lines.

-	if (has_refcount && *has_refcount == 1) {
-		*direct_io = 0;
-		break;
-}

Regards,
Tao