Re: cleanup btrfs bio handling, part 2 v3

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

On 2022/5/4 20:25, Christoph Hellwig wrote:
> Hi all,
>
> this series removes the need to allocate a separate object for I/O
> completions for all read and some write I/Os, and reduced the memory
> usage of the low-level bios cloned by btrfs_map_bio by using plain bios
> instead of the much larger btrfs_bio.

The most important part of this series is the patch "btrfs: remove
btrfs_end_io_wq()" I guess.

As a guy growing up mostly reading btrfs code, the btrfs_end_io_wq()
function looks straightforward to me initially, just put everything into
wq context and call it a day.

But as I started to look into how other fses handle their delayed work,
the btrfs way turns more "unique" other than straightforward.


So yes, aligning us to other fses, by only delaying into wq context when
necessary, should be a good thing.
Not reducing the memory allocation for the btrfs_end_io_wq structure,
but also making the atomic part of the endio function executed with less
delay.

But here I'm not sure if the btrfs way is a legacy from the past, or has
some hidden intention (possible related to performance?).

So do you mind to do some basic benchmark for read and write and see how
the throughput and latency changed?
(I guess we can get slightly reduced latency without really damping
everything else).

With such benchmark the series would be more convincing, and I can learn
some tricks on how to do proper performance tests.


Also mentioning how other fses handling delayed work may help guys like
me to get an understanding of the more common practice used in other fses.

Thanks,
Qu
>
> Changes since v2:
>   - rebased to the latests misc-next tree
>   - fixed an incorrectly assigned bi_end_io handler in the raid56 code
>
> Changes since v1:
>   - rebased to the latests misc-next tree
>   - don't call destroy_workqueue on NULL workqueues
>   - drop a marginal and slightly controversial cleanup to btrfs_map_bio
>
> Diffstat:
>   compression.c |   41 +++++-------
>   compression.h |    7 +-
>   ctree.h       |   14 ++--
>   disk-io.c     |  148 +++++-----------------------------------------
>   disk-io.h     |   11 ---
>   extent_io.c   |   35 ++++-------
>   extent_io.h   |    1
>   inode.c       |  162 +++++++++++++++++++--------------------------------
>   raid56.c      |  109 +++++++++++++---------------------
>   super.c       |   13 ----
>   volumes.c     |  184 +++++++++++++++++++++++++++-------------------------------
>   volumes.h     |    8 ++
>   12 files changed, 262 insertions(+), 471 deletions(-)