ocfs2: fall back to buffer IO when append dio is disabled with file hole existing
diff mbox

Message ID 20171218135358.cee63b6019681cd7cfc569ee@linux-foundation.org
State New
Headers show

Commit Message

Andrew Morton Dec. 18, 2017, 9:53 p.m. UTC
On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei@h3c.com> wrote:

> Before ocfs2 supporting allocating clusters while doing append-dio, all append
> dio will fall back to buffer io to allocate clusters firstly. Also, when it
> steps on a file hole, it will fall back to buffer io, too. But for current
> code, writing to file hole will leverage dio to allocate clusters. This is not
> right, since whether append-io is enabled tells the capability whether ocfs2 can
> allocate space while doing dio.
> So introduce file hole check function back into ocfs2.
> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will fall
> back to buffer IO to allocate clusters.
> 

hm, that's a bit hard to understand.  Hopefully reviewers will know
what's going on ;)

> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>   	return ret;
>   }
>   
> +/*
> + * Will look for holes and unwritten extents in the range starting at
> + * pos for count bytes (inclusive).
> + */
> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
> +				       size_t count)
> +{
> +	int ret = 0;
> +	unsigned int extent_flags;
> +	u32 cpos, clusters, extent_len, phys_cpos;
> +	struct super_block *sb = inode->i_sb;
> +
> +	cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits;
> +	clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos;
> +
> +	while (clusters) {
> +		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len,
> +					 &extent_flags);
> +		if (ret < 0) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
> +		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
> +			ret = 1;
> +			break;
> +		}
> +
> +		if (extent_len > clusters)
> +			extent_len = clusters;
> +
> +		clusters -= extent_len;
> +		cpos += extent_len;
> +	}
> +out:
> +	return ret;
> +}

A few thoughts:

- a function which does "check_foo" isn't well named.  Because the
  reader cannot determine whether the return value means "foo is true"
  or "foo is false".

  So a better name for this function is ocfs2_range_has_holes(), so
  the reader immediately understand what its return value *means".

  Also a bool return value is more logical.

- Mixing "goto out" with "break" as above is a bit odd.

So...

Comments

Joseph Qi Dec. 19, 2017, 1:24 a.m. UTC | #1
On 17/12/19 05:53, Andrew Morton wrote:
> On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei@h3c.com> wrote:
> 
>> Before ocfs2 supporting allocating clusters while doing append-dio, all append
>> dio will fall back to buffer io to allocate clusters firstly. Also, when it
>> steps on a file hole, it will fall back to buffer io, too. But for current
>> code, writing to file hole will leverage dio to allocate clusters. This is not
>> right, since whether append-io is enabled tells the capability whether ocfs2 can
>> allocate space while doing dio.
>> So introduce file hole check function back into ocfs2.
>> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will fall
>> back to buffer IO to allocate clusters.
>>
> 
> hm, that's a bit hard to understand.  Hopefully reviewers will know
> what's going on ;)> 
Also suggest rearrange the description:)

>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>>   	return ret;
>>   }
>>   
>> +/*
>> + * Will look for holes and unwritten extents in the range starting at
>> + * pos for count bytes (inclusive).
>> + */
>> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>> +				       size_t count)
>> +{
>> +	int ret = 0;
>> +	unsigned int extent_flags;
>> +	u32 cpos, clusters, extent_len, phys_cpos;
>> +	struct super_block *sb = inode->i_sb;
>> +
>> +	cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits;
>> +	clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos;
>> +
>> +	while (clusters) {
>> +		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len,
>> +					 &extent_flags);
>> +		if (ret < 0) {
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
>> +
>> +		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +
>> +		if (extent_len > clusters)
>> +			extent_len = clusters;
>> +
>> +		clusters -= extent_len;
>> +		cpos += extent_len;
>> +	}
>> +out:
>> +	return ret;
>> +}
> 
> A few thoughts:
> 
> - a function which does "check_foo" isn't well named.  Because the
>   reader cannot determine whether the return value means "foo is true"
>   or "foo is false".
> 
>   So a better name for this function is ocfs2_range_has_holes(), so
>   the reader immediately understand what its return value *means".
> 
>   Also a bool return value is more logical.
> 
> - Mixing "goto out" with "break" as above is a bit odd.
> 
> So...
> 
> 
> --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
> +++ a/fs/ocfs2/aops.c
> @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
>   * Will look for holes and unwritten extents in the range starting at
>   * pos for count bytes (inclusive).
>   */
> -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
> -				       size_t count)
> +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
>  {
> -	int ret = 0;
> +	bool ret = false;

I have a different opinion here. If we change return value from int to
bool, the error returned by ocfs2_get_clusters cannot be reflected. So
I'd prefer the original version.

Thanks,
Joseph

>  	unsigned int extent_flags;
>  	u32 cpos, clusters, extent_len, phys_cpos;
>  	struct super_block *sb = inode->i_sb;
> @@ -2489,8 +2488,8 @@ static int ocfs2_check_range_for_holes(s
>  		}
>  
>  		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
> -			ret = 1;
> -			break;
> +			ret = true;
> +			goto out;
>  		}
>  
>  		if (extent_len > clusters)
> _
>
Andrew Morton Dec. 19, 2017, 1:27 a.m. UTC | #2
On Tue, 19 Dec 2017 09:24:17 +0800 Joseph Qi <jiangqi903@gmail.com> wrote:

> > --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
> > +++ a/fs/ocfs2/aops.c
> > @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
> >   * Will look for holes and unwritten extents in the range starting at
> >   * pos for count bytes (inclusive).
> >   */
> > -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
> > -				       size_t count)
> > +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
> >  {
> > -	int ret = 0;
> > +	bool ret = false;
> 
> I have a different opinion here. If we change return value from int to
> bool, the error returned by ocfs2_get_clusters cannot be reflected. So
> I'd prefer the original version.

But that error code is not used?
Joseph Qi Dec. 19, 2017, 1:39 a.m. UTC | #3
On 17/12/19 09:27, Andrew Morton wrote:
> On Tue, 19 Dec 2017 09:24:17 +0800 Joseph Qi <jiangqi903@gmail.com> wrote:
> 
>>> --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
>>> +++ a/fs/ocfs2/aops.c
>>> @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
>>>   * Will look for holes and unwritten extents in the range starting at
>>>   * pos for count bytes (inclusive).
>>>   */
>>> -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>>> -				       size_t count)
>>> +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
>>>  {
>>> -	int ret = 0;
>>> +	bool ret = false;
>>
>> I have a different opinion here. If we change return value from int to
>> bool, the error returned by ocfs2_get_clusters cannot be reflected. So
>> I'd prefer the original version.
> 
> But that error code is not used?
> 
IMO, since ocfs2_get_clusters will get io involved, the caller shouldn't
ignore its error.

Something like:
ret = ocfs2_check_range_for_holes();
if (ret < 0) {
	mlog_errno(ret);
	goto out;
}

Then check if append dio feature has been enabled as well as write range
and hole.

Thanks,
Joseph
Changwei Ge Dec. 19, 2017, 3 a.m. UTC | #4
On 2017/12/19 5:54, Andrew Morton wrote:
> On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei@h3c.com> wrote:
> 
>> Before ocfs2 supporting allocating clusters while doing append-dio, all append
>> dio will fall back to buffer io to allocate clusters firstly. Also, when it
>> steps on a file hole, it will fall back to buffer io, too. But for current
>> code, writing to file hole will leverage dio to allocate clusters. This is not
>> right, since whether append-io is enabled tells the capability whether ocfs2 can
>> allocate space while doing dio.
>> So introduce file hole check function back into ocfs2.
>> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will fall
>> back to buffer IO to allocate clusters.
>>
> 
> hm, that's a bit hard to understand.  Hopefully reviewers will know
> what's going on ;)
Hi Andrew,
Sorry for mess up the changelog.
I will enrich the changelog and elaborate my intention further in the next version
of this patch.


> 
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>>    	return ret;
>>    }
>>    
>> +/*
>> + * Will look for holes and unwritten extents in the range starting at
>> + * pos for count bytes (inclusive).
>> + */
>> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>> +				       size_t count)
>> +{
>> +	int ret = 0;
>> +	unsigned int extent_flags;
>> +	u32 cpos, clusters, extent_len, phys_cpos;
>> +	struct super_block *sb = inode->i_sb;
>> +
>> +	cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits;
>> +	clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos;
>> +
>> +	while (clusters) {
>> +		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len,
>> +					 &extent_flags);
>> +		if (ret < 0) {
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
>> +
>> +		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
>> +			ret = 1;
>> +			break;
>> +		}
>> +
>> +		if (extent_len > clusters)
>> +			extent_len = clusters;
>> +
>> +		clusters -= extent_len;
>> +		cpos += extent_len;
>> +	}
>> +out:
>> +	return ret;
>> +}
> 
> A few thoughts:
> 
> - a function which does "check_foo" isn't well named.  Because the
>    reader cannot determine whether the return value means "foo is true"
>    or "foo is false".

Very true.

> 
>    So a better name for this function is ocfs2_range_has_holes(), so
>    the reader immediately understand what its return value *means".
> 
>    Also a bool return value is more logical.

I prefer int value.
It's my fault here since I didn't check the return value for other exception
scenario.

> 
> - Mixing "goto out" with "break" as above is a bit odd.

Agree.

> 
> So...
> 
> 
> --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
> +++ a/fs/ocfs2/aops.c
> @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
>    * Will look for holes and unwritten extents in the range starting at
>    * pos for count bytes (inclusive).
>    */
> -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
> -				       size_t count)
> +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
>   {
> -	int ret = 0;
> +	bool ret = false;
>   	unsigned int extent_flags;
>   	u32 cpos, clusters, extent_len, phys_cpos;
>   	struct super_block *sb = inode->i_sb;
> @@ -2489,8 +2488,8 @@ static int ocfs2_check_range_for_holes(s
>   		}
>   
>   		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
> -			ret = 1;
> -			break;
> +			ret = true;
> +			goto out;
>   		}

Ok, I will take this into account.

Thanks,
Changwei

>   
>   		if (extent_len > clusters)
> _
> 
>
Changwei Ge Dec. 19, 2017, 3:02 a.m. UTC | #5
On 2017/12/19 9:24, Joseph Qi wrote:
> 
> On 17/12/19 05:53, Andrew Morton wrote:
>> On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei@h3c.com> wrote:
>>
>>> Before ocfs2 supporting allocating clusters while doing append-dio, all append
>>> dio will fall back to buffer io to allocate clusters firstly. Also, when it
>>> steps on a file hole, it will fall back to buffer io, too. But for current
>>> code, writing to file hole will leverage dio to allocate clusters. This is not
>>> right, since whether append-io is enabled tells the capability whether ocfs2 can
>>> allocate space while doing dio.
>>> So introduce file hole check function back into ocfs2.
>>> Once ocfs2 is doing dio upon a file hole with append-dio disabled, it will fall
>>> back to buffer IO to allocate clusters.
>>>
>>
>> hm, that's a bit hard to understand.  Hopefully reviewers will know
>> what's going on ;)>
> Also suggest rearrange the description:)

Hi Joseph,
OK, I will elaborate the intention of this patch further in the next version.

> 
>>> --- a/fs/ocfs2/aops.c
>>> +++ b/fs/ocfs2/aops.c
>>> @@ -2414,6 +2414,44 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>>>    	return ret;
>>>    }
>>>    
>>> +/*
>>> + * Will look for holes and unwritten extents in the range starting at
>>> + * pos for count bytes (inclusive).
>>> + */
>>> +static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>>> +				       size_t count)
>>> +{
>>> +	int ret = 0;
>>> +	unsigned int extent_flags;
>>> +	u32 cpos, clusters, extent_len, phys_cpos;
>>> +	struct super_block *sb = inode->i_sb;
>>> +
>>> +	cpos = pos >> OCFS2_SB(sb)->s_clustersize_bits;
>>> +	clusters = ocfs2_clusters_for_bytes(sb, pos + count) - cpos;
>>> +
>>> +	while (clusters) {
>>> +		ret = ocfs2_get_clusters(inode, cpos, &phys_cpos, &extent_len,
>>> +					 &extent_flags);
>>> +		if (ret < 0) {
>>> +			mlog_errno(ret);
>>> +			goto out;
>>> +		}
>>> +
>>> +		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
>>> +			ret = 1;
>>> +			break;
>>> +		}
>>> +
>>> +		if (extent_len > clusters)
>>> +			extent_len = clusters;
>>> +
>>> +		clusters -= extent_len;
>>> +		cpos += extent_len;
>>> +	}
>>> +out:
>>> +	return ret;
>>> +}
>>
>> A few thoughts:
>>
>> - a function which does "check_foo" isn't well named.  Because the
>>    reader cannot determine whether the return value means "foo is true"
>>    or "foo is false".
>>
>>    So a better name for this function is ocfs2_range_has_holes(), so
>>    the reader immediately understand what its return value *means".
>>
>>    Also a bool return value is more logical.
>>
>> - Mixing "goto out" with "break" as above is a bit odd.
>>
>> So...
>>
>>
>> --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
>> +++ a/fs/ocfs2/aops.c
>> @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
>>    * Will look for holes and unwritten extents in the range starting at
>>    * pos for count bytes (inclusive).
>>    */
>> -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>> -				       size_t count)
>> +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
>>   {
>> -	int ret = 0;
>> +	bool ret = false;
> 
> I have a different opinion here. If we change return value from int to
> bool, the error returned by ocfs2_get_clusters cannot be reflected. So
> I'd prefer the original version.

Yes, it's my mistake here.
You have to forgive me.:)

Thanks,
Changwei

> 
> Thanks,
> Joseph
> 
>>   	unsigned int extent_flags;
>>   	u32 cpos, clusters, extent_len, phys_cpos;
>>   	struct super_block *sb = inode->i_sb;
>> @@ -2489,8 +2488,8 @@ static int ocfs2_check_range_for_holes(s
>>   		}
>>   
>>   		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
>> -			ret = 1;
>> -			break;
>> +			ret = true;
>> +			goto out;
>>   		}
>>   
>>   		if (extent_len > clusters)
>> _
>>
>
Changwei Ge Dec. 19, 2017, 3:03 a.m. UTC | #6
On 2017/12/19 9:39, Joseph Qi wrote:
> 
> 
> On 17/12/19 09:27, Andrew Morton wrote:
>> On Tue, 19 Dec 2017 09:24:17 +0800 Joseph Qi <jiangqi903@gmail.com> wrote:
>>
>>>> --- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
>>>> +++ a/fs/ocfs2/aops.c
>>>> @@ -2469,10 +2469,9 @@ static int ocfs2_dio_end_io(struct kiocb
>>>>    * Will look for holes and unwritten extents in the range starting at
>>>>    * pos for count bytes (inclusive).
>>>>    */
>>>> -static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
>>>> -				       size_t count)
>>>> +static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
>>>>   {
>>>> -	int ret = 0;
>>>> +	bool ret = false;
>>>
>>> I have a different opinion here. If we change return value from int to
>>> bool, the error returned by ocfs2_get_clusters cannot be reflected. So
>>> I'd prefer the original version.
>>
>> But that error code is not used?
>>
> IMO, since ocfs2_get_clusters will get io involved, the caller shouldn't
> ignore its error.
> 
> Something like:
> ret = ocfs2_check_range_for_holes();
> if (ret < 0) {
> 	mlog_errno(ret);
> 	goto out;
> }
> 
> Then check if append dio feature has been enabled as well as write range
> and hole.

Accept this point.

> 
> Thanks,
> Joseph
>

Patch
diff mbox

--- a/fs/ocfs2/aops.c~ocfs2-fall-back-to-buffer-io-when-append-dio-is-disabled-with-file-hole-existing-fix
+++ a/fs/ocfs2/aops.c
@@ -2469,10 +2469,9 @@  static int ocfs2_dio_end_io(struct kiocb
  * Will look for holes and unwritten extents in the range starting at
  * pos for count bytes (inclusive).
  */
-static int ocfs2_check_range_for_holes(struct inode *inode, loff_t pos,
-				       size_t count)
+static bool ocfs2_range_has_holes(struct inode *inode, loff_t pos, size_t count)
 {
-	int ret = 0;
+	bool ret = false;
 	unsigned int extent_flags;
 	u32 cpos, clusters, extent_len, phys_cpos;
 	struct super_block *sb = inode->i_sb;
@@ -2489,8 +2488,8 @@  static int ocfs2_check_range_for_holes(s
 		}
 
 		if (phys_cpos == 0 || (extent_flags & OCFS2_EXT_UNWRITTEN)) {
-			ret = 1;
-			break;
+			ret = true;
+			goto out;
 		}
 
 		if (extent_len > clusters)