Re: [PATCH RFC v2 02/12] btrfs: always save bio::bi_iter into btrfs_bio::iter before submitting

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

On 2022/4/28 21:32, Christoph Hellwig wrote:
> On Wed, Apr 27, 2022 at 03:18:48PM +0800, Qu Wenruo wrote:
>> Lower level bio mapping, from driver to dm, even btrfs chunk mapping
>> can modify bio::bi_iter.
>>
>> This prevents us from doing two things:
>>
>> - Iterate the bio range of a cloned bio
>>    This is only utilized by direct IO, thus it's already using
>>    btrfs_bio::iter, which is populated in btrfs_bio_clone_partial().
>>
>> - Grab the original logical bytenr of a bio
>>    This will be utilized by incoming read repair patches.
>>
>> So to make sure all btrfs_bio submitted to own a proper iter, this patch
>> will assigned btrfs_bio::iter in the following call sites:
>
> Independent of what we want to do with the saved iter in the future
> I don't think this is an improvement.  The place where we can start
> advancing the iter is after the bio is submit by btrfs_map_bio, so that
> is the one central place where it should be saved.  I actually had
> a patch like that in one my my wip branches:
>
> ---
> From: Christoph Hellwig <hch@lst.de>
> Subject: btrfs: centralize stashing away ->bi_iter in btrfs_map_bio
>
> Once a bio is submitted to a driver, the driver can advance bi_iter.
> Save to the copy in the btrfs_bio just before that can happen rather
> than in random places high up in the stack.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/extent_io.c | 7 +------
>   fs/btrfs/volumes.c   | 3 +++
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 07888cce3bce6..002b1ea92e398 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2683,7 +2683,6 @@ int btrfs_repair_one_sector(struct inode *inode,
>   	}
>
>   	bio_add_page(repair_bio, page, failrec->len, pgoff);
> -	repair_bbio->iter = repair_bio->bi_iter;
>
>   	btrfs_debug(btrfs_sb(inode->i_sb),
>   		    "repair read error: submitting new read to mirror %d",
> @@ -3209,14 +3208,11 @@ struct bio *btrfs_bio_alloc(unsigned int nr_iovecs)
>
>   struct bio *btrfs_bio_clone(struct block_device *bdev, struct bio *bio)
>   {
> -	struct btrfs_bio *bbio;
>   	struct bio *new;
>
>   	/* Bio allocation backed by a bioset does not fail */
>   	new = bio_alloc_clone(bdev, bio, GFP_NOFS, &btrfs_bioset);
> -	bbio = btrfs_bio(new);
> -	btrfs_bio_init(bbio);
> -	bbio->iter = bio->bi_iter;
> +	btrfs_bio_init(btrfs_bio(new));
>   	return new;
>   }
>
> @@ -3235,7 +3231,6 @@ struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size)
>   	btrfs_bio_init(bbio);
>
>   	bio_trim(bio, offset >> 9, size >> 9);
> -	bbio->iter = bio->bi_iter;
>   	return bio;
>   }
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 748614b00ffa2..f282a58a12344 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6750,6 +6750,9 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>   	int total_devs;
>   	struct btrfs_io_context *bioc = NULL;
>
> +	/* stash away the iter as the low-level processing can advance it */
> +	btrfs_bio(bio)->iter = bio->bi_iter;

The problem here is, if any endio function needs to grab the original
bio, and btrfs submit bio hooks failed before btrfs_map_bio(), like some
failure from btrfs_bio_wq_end_io(), then we will call bio_endio(), and
the endio just got an empty iter.

Thanks,
Qu

> +
>   	length = bio->bi_iter.bi_size;
>   	map_length = length;
>