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

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

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.

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.
>