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.chang...@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)
>> _
>>
>
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
