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

[David Sterba <dsterba@suse.cz>]

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

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.