Re: [PATCH 06/10] btrfs: transfer the bio counter reference to the raid submission helpers

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

On 2022/6/20 15:47, Christoph Hellwig wrote:
> On Mon, Jun 20, 2022 at 05:50:53AM +0800, Qu Wenruo wrote:
>> In fact, the bio counter for btrfs_map_bio() is just increased and to
>> allow the real bios (either the RAID56 code, or submit_stripe_bio()) to
>> grab extra counter to cover the full lifespan of the real bio.
>>
>> Thus I don't think there is any bio counter to be "transferred" here.

Well, your reply already make it very clear that that question is incorrect.

>
> What is the real bio?

At least to me, it is more like a meta bio representing the write for
logical address.

E.g. if we have a write for a logical address, and the underlying chunk
is RAID1, then when both copies finished their endio, the bio counter
should be 0.
(For current code base, during the bio assemble/submission, the counter
can easily go beyond 1, which can be cleaned up in the future).

>
> In the parity raid case there is:
>
>   1) the upper level btrfs_bio, which is handed off to
>      raid56_parity_write / raid56_parity_recover.
>      It then to the bios list of the rbio and is eventually completed
>   2) lower-level RAID bios, which have no direct connection to the
>      btrfs_bio, as they are all driven off the rbio-based state
>      machine
>
> For the non-parity case we have
>
>   1) the upper level btrfs_bio, which is submitted to the actual devic
>      as the last mirror (write case) or the only bio (read case)
>   2) the clones for the non-last mirror writes
>
> btrfs_submit_bio calls btrfs_bio_counter_inc_blocked before
> __btrfs_map_block to protect against device replace operations,
> and that protection needs to last until the last bio using the
> mapping returned from __btrfs_map_block has completed.
>
> So we don't need an extra count for the parity case.

Yep, I can totally agree now.

>  In fact we
> don't really need an extra count either for the non-parity case
> except for additional mirror writes.  So maybe things are cleaned
> up if we also add this patch (which relies on the previous ones in
> this series):

Sure, the series is already a little large, we can definitely do more
cleanup later.

But in that case, mind to put this patch into the series do all the bio
counter cleanups?

AFAIK there is no dependency in the patchset on this patch, it would be
much better to be in a series doing bio counters.

Thanks,
Qu
>
> ---
>  From 6351835b133ce00e2d65a6b2a398678b45426947 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 20 Jun 2022 09:43:48 +0200
> Subject: btrfs: don't take a bio_counter reference for cloned bios
>
> There is no need for multiple bio_counter references for a single I/O.
> Just release the reference when completing the bio and avoid additional
> counter roundtrips.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/ctree.h       | 1 -
>   fs/btrfs/dev-replace.c | 5 -----
>   fs/btrfs/volumes.c     | 7 ++-----
>   3 files changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 1347c92234a56..6857897c77108 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3972,7 +3972,6 @@ static inline void btrfs_init_full_stripe_locks_tree(
>
>   /* dev-replace.c */
>   void btrfs_bio_counter_inc_blocked(struct btrfs_fs_info *fs_info);
> -void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info);
>   void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount);
>
>   static inline void btrfs_bio_counter_dec(struct btrfs_fs_info *fs_info)
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index a7dd6ba25e990..aa435d04e8ef3 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -1288,11 +1288,6 @@ int __pure btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace)
>   	return 1;
>   }
>
> -void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info)
> -{
> -	percpu_counter_inc(&fs_info->dev_replace.bio_counter);
> -}
> -
>   void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount)
>   {
>   	percpu_counter_sub(&fs_info->dev_replace.bio_counter, amount);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 33a232c897d14..86e200d2000f9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6647,14 +6647,14 @@ static void btrfs_end_bio(struct bio *bio)
>   		}
>   	}
>
> -	btrfs_bio_counter_dec(bioc->fs_info);
> -
>   	if (bio != orig_bio) {
>   		bio_endio(orig_bio);
>   		bio_put(bio);
>   		return;
>   	}
>
> +	btrfs_bio_counter_dec(bioc->fs_info);
> +
>   	/*
>   	 * Only send an error to the higher layers if it is beyond the tolerance
>   	 * threshold.
> @@ -6698,8 +6698,6 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc,
>   	bio->bi_end_io = btrfs_end_bio;
>   	bio->bi_iter.bi_sector = physical >> 9;
>
> -	btrfs_bio_counter_inc_noblocked(fs_info);
> -
>   	if (!dev || !dev->bdev ||
>   	    test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) ||
>   	    (btrfs_op(bio) == BTRFS_MAP_WRITE &&
> @@ -6781,7 +6779,6 @@ void btrfs_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>
>   		submit_stripe_bio(bioc, bio, dev_nr, should_clone);
>   	}
> -	btrfs_bio_counter_dec(fs_info);
>   }
>
>   static bool dev_args_match_fs_devices(const struct btrfs_dev_lookup_args *args,