[Ocfs2-devel] [PATCH] ocfs2: fall back to buffer IO when append dio is disabled with file hole existing

[Joseph Qi]

On 17/12/19 05:53, Andrew Morton wrote:
> On Mon, 18 Dec 2017 12:06:21 +0000 Changwei Ge <ge.changwei at 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)
> _
>