Re: [PATCH RFC 0/3] btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context

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

On 2021/10/7 03:38, David Sterba wrote:
> On Wed, Sep 22, 2021 at 04:27:03PM +0800, Qu Wenruo wrote:
>> Currently btrfs_io_context is utilized as both bio->bi_private for
>> mirrored stripes, and stripes mapping for RAID56.
>>
>> This makes some members like btrfs_io_context::private to be there.
>>
>> But in fact, since almost all bios in btrfs have btrfs_bio structure
>> appended, we don't really need to reuse bio::bi_private for mirrored
>> profiles.
>>
>> So this patchset will:
>>
>> - Introduce btrfs_bio::bioc member
>>    So that btrfs_io_context don't need to hold @private.
>>    This modification is still a net increase for memory usage, just
>>    a trade-off between btrfs_io_context and btrfs_bio.
>>
>> - Replace btrfs_bio::device with btrfs_bio::stripe_num
>>    This reclaim the memory usage increase for btrfs_bio.
>>
>>    But unfortunately, due to the short life time of btrfs_io_context,
>>    we don't have as good device status accounting as the old code.
>>
>>    Now for csum error, we can no longer distinguish source and target
>>    device of dev-replace.
>>
>>    This is the biggest blockage, and that's why it's RFC.
>>
>> The result of the patchset is:
>>
>> btrfs_bio:		size: 240 -> 240
>> btrfs_io_context:	size: 88 -> 80
>>
>> Although to really reduce btrfs_bio, the main target should be
>> csum_inline.
>>
>> Qu Wenruo (3):
>>    btrfs: add btrfs_bio::bioc pointer for further modification
>>    btrfs: remove redundant parameters for submit_stripe_bio()
>>    btrfs: replace btrfs_bio::device member with stripe_num
>
> Can you please refresh the patchset on top current misc-next?
>

Please discard the patchset for now.

Unfortunately I have found one critical member abuses making such
refactor unfeasible (at least for now).

- btrfs_bio::logical abuse
   Direct IO uses btrfs_bio::logical as file_offset, while no other
   call sites really utilize btrfs_bio::logical at all.

   This means, we can't simply rely on btrfs_map_bio() to verify if
   btrfs_bio::logical is correctly initialized.

   This is a big alert, if we can't do ASSERT() to verify if one member
   is properly initialized, then it's just going to happen.

   This also means, for direct IO, we can't use @mirror_num with @logical
   to grab the device with IO failure.

So for now, although it may save 8 bytes, unless we solve the DIO mess,
we can't continue.

Thanks,
Qu