Re: [PATCH 01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage()

[Nikolay Borisov <nborisov@suse.com>]

On 5.11.20 г. 12:15 ч., Qu Wenruo wrote:
> 
> 

<snip>

>>
>> The endio routine cares about logical file contiguity as evidenced by
>> the fact it uses page_offset() to calculate 'start', however our recent
>> discussion on irc with the contiguity in csums bios clearly showed that
>> we can have bios which contains pages that are contiguous in their disk
>> byte nr but not in their logical offset, in fact Josef even mentioned on
>> slack that a single bio can contain pages for different inodes so long
>> as their logical disk byte nr are contiguous. I think this is not an
>> issue in this case because you are doing the unlock on a bvec
>> granularity but just the above statement is somewhat misleading.
> 
> Right, forgot the recent discovered bvec contig problem.
> 
> But still, the contig check condition is still correct, just the commit
> message needs some update.
> 
> Another off-topic question is, should we allow such on-disk bytenr only
> merge (to improve the IO output), or don't allow them to provide a
> simpler endio function?

Can't answer that without quantifying what the performance impact is so
we can properly judge complexity/performance trade-off!

<snip>

>> I suspect for large bios with a lot of bvecs this would likely increase
>> latency because we will now invoke endio_readpage_release_extent
>> proportionally to the number of bvecs.
> 
> I believe the same situation.
> 
> Now we need to do extent_io tree operations for each bvec.
> We can slightly reduce the overhead if we have something like globally
> cached extent_state.
> 
> Your comment indeed implies we should do better extent contig cache,
> other than completely relying on extent io tree.
> 
> Maybe I could find some good way to improve the situation, while still
> avoid doing the existing dancing.

First I'd like to have numbers showing what the overhead otherwise it
will be impossible to tell if whatever approach you choose brings any
improvements.

<snip>

>> Also bear in mind that this happens in a critical endio context, which
>> uses GFP_ATOMIC allocations so if we get ENOSPC it would be rather bad.
> 
> I know you mean -ENOMEM.

Yep, my bad.

> 
> But the true is, except the leading/tailing sector of the extent, we
> shouldn't really cause extra split/allocation.

That's something you assume so the real behavior might be different,
again I think we need to experiment to better understand the behavior.

<snip>

>> I definitely like the new code however without quantifying what's the
>> increase of number of calls of endio_readpage_release_extent I'd rather
>> not merge it.
> 
> Your point stands.
> 
> I could add a new wrapper to do the same thing, but with a small help
> from some new structure to really record the
> inode/extent_start/extent_len internally.
> 
> The end result should be the same in the endio function, but much easier
> to read. (The complex part would definite have more comment)
> 
> What about this solution?

IMO the best course of action is to measure the length of extents being
unlocked in the old version of the code and the number of bvecs in a
bio. That way you would be able to extrapolate with the new version of
the code how many more calls to extent unlock would have been made. This
will tell you how effective this optimisation really is and if it's
worth keeping around.

<snip>