Re: [PATCH 0/2] btrfs: btrfs_dec_test_*_ordered_extent() refactor

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/12/19 上午8:26, Qu Wenruo wrote:
>
>
> On 2020/12/18 下午11:57, David Sterba wrote:
>> On Fri, Dec 18, 2020 at 01:16:59PM +0800, Qu Wenruo wrote:
>>> This small patchset is btrfs_dec_test_*_ordered_extent() refactor during
>>> subpage RW support development.
>>>
>>> This is mostly to make btrfs_dev_test_* functions more human readable
>>> and prepare it for calling btrfs_dec_test_first_ordered_extent() in
>>> btrfs_writepage_endio_finish_ordered() where we can have one or more
>>> ordered extents for one bvec.
>>>
>>> Qu Wenruo (2):
>>>    btrfs: make btrfs_dio_private::bytes to be u32
>>>    btrfs: refactor btrfs_dec_test_* functions for ordered extents
>>
>> The idea makes sense but the patches are IMO in wrong order and still
>> leave some u64/u32, eg. in btrfs_dec_test_first_ordered_pending the
>> io_size is still u64 while for btrfs_dec_test_ordered_pending it
>> switches type to u32 (as expected).
>
> That u64 is left there and the reason is explained in the 2nd patch.
>
> Mostly due to iomap requirement.
>
> But I totally get your point.

After digging more, I prefer to give up the width reduction, but just
focus on the other cleanups in the patch.

The width reduction really needs more concern, especially when iomap is
involved.

Thanks,
Qu
>
> Thanks,
> Qu
>
>>
>> The type cleanup should be done bottom-up, from the leaf functions
>> upwards. After that, the structure type can be safely switched.
>>
>> I'm not sure what to do with __endio_write_update_ordered, it can take
>> u32 for bytes but internally uses u64 for ordered_bytes, that should be
>> u32 as well. But
>>
>>   7711                 if (ordered_offset < offset + bytes) {
>>   7712                         ordered_bytes = offset + bytes -
>> ordered_offset;
>>   7713                         ordered = NULL;
>>   7714                 }
>>
>> expression on line 7712 would need a temporary variable for the u64
>> calculation and then reassign. Maybe such conversions are inevitable so
>> we have clean function API and not pass u64 just because.
>>
>> I like that the hole btrfs_dio_private gets removed so the cleanups are
>> worthwhile, but maybe not trivial.
>>