Re: [PATCH v3 02/12] btrfs: introduce a new helper to submit bio for scrub

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v3 02/12] btrfs: introduce a new helper to submit bio for scrub
Date: Sat, 25 Mar 2023 16:21:41 +0800	[thread overview]
Message-ID: <9581646d-380f-2b55-07ac-2abd37822577@gmx.com> (raw)
In-Reply-To: <ZB6sQGoP9dbsgvX7@infradead.org>

On 2023/3/25 16:09, Christoph Hellwig wrote:
> On Fri, Mar 24, 2023 at 05:58:57PM +0800, Qu Wenruo wrote:
>> I have updated the branch, with a dedicated btrfs_bio::fs_info,
>> btrfs_scrub_bio_alloc() and always populate that new fs_info.
>>
>> Mind to have a pre-check?
>>
>> https://github.com/adam900710/linux/commit/0f6a419f035787546eec6f51b0430a05a345d4c5
> 
> I'd prefer to just pass a fs_info to btrfs_bio_init and btrfs_bio_alloc
> instead of duplicating the allocator.  With my "simplify extent_buffer
> reading and writing" series and another tiny change we'll only need the
> inode for data inodes.  But given that we've not made too much
> progress on getting that series merged I guess we'll have to add
> the duplicate allocator for now and just revert it later.

No big deal, I can go with the extra fs_info parameter for 
btrfs_bio_init() and btrfs_bio_alloc().

The main reason I go with the duplicated allocate is to remove the need 
for nr_vecs, but that's pretty minor, thus not a critical one.

> 
> I'd drop the ASSERT in btrfs_submit_bio - all allocators initialize
> the field, no need for the extra check.

OK, sounds reasonable.

> 
>> Although the next two patches are also slightly updated to take the
>> advantage of that always populated fs_info:
> 
> I think you wanted to add links here, but they are missing.
> 
>> Since the modification, I'm a little more dependent on that always populated
>> fs_info now, thus not sure if going back to a union and conditionally
>> grabbing that fs_info is a good idea.
> 
> I don't think the union is a good idea at all.  Maybe in the future
> we can move the inode into a union, but right now the csum for data
> read is the upper bound on the btrfs_bio, so I'm not sure we'll
> ever get to a point where that would help.

Yeah, let's avoid union and let fs_info always populated.

Thanks,
Qu

> 
>> Although I'm more interested why there is a super big gap between work and
>> bio (32 bytes), while the compiler still refuses to put the pointer into
>> that hole...
> 
> I don't think there is one.  pahole just reports structs strangely
> sometimes.