Re: [PATCH v3 09/12] btrfs: scrub: introduce a writeback helper for scrub_stripe

[David Sterba <dsterba@suse.cz>]

On Mon, Mar 20, 2023 at 10:12:55AM +0800, Qu Wenruo wrote:
> +static void scrub_write_endio(struct btrfs_bio *bbio)
> +{
> +	struct scrub_stripe *stripe = bbio->private;
> +	struct btrfs_fs_info *fs_info = stripe->bg->fs_info;
> +	struct bio_vec *bvec;
> +	unsigned long flags;
> +	int sector_nr = calc_sector_number(stripe, bio_first_bvec_all(&bbio->bio));
> +	int bio_size = 0;
> +	int i;
> +
> +	bio_for_each_bvec_all(bvec, &bbio->bio, i)
> +		bio_size += bvec->bv_len;

The types should match, bv_len is u32 so it should be summed to a u32
type (bio_size).

> +
> +	if (bbio->bio.bi_status) {
> +		spin_lock_irqsave(&stripe->write_error_lock, flags);
> +		bitmap_set(&stripe->write_error_bitmap, sector_nr,
> +				bio_size >> fs_info->sectorsize_bits);
> +		spin_unlock_irqrestore(&stripe->write_error_lock, flags);
> +	}
> +	bio_put(&bbio->bio);
> +
> +	if (atomic_dec_and_test(&stripe->pending_io))
> +		wake_up(&stripe->io_wait);
> +}
> +
> +/*
> + * Submit the write bio(s) for the sectors specified by @write_bitmap.
> + *
> + * Here we utilize btrfs_submit_scrub_write(), which has some extra benefits:
> + *
> + * - Only needs logical bytenr and mirror_num
> + *   Just like the scrub read path
> + *
> + * - Would only result writes to the specified mirror
> + *   Unlike the regular writeback path, which would write back to all stripes
> + *
> + * - Handle dev-replace and read-repair writeback differently
> + */
> +void scrub_write_sectors(struct scrub_ctx *sctx,
> +			struct scrub_stripe *stripe,
> +			unsigned long write_bitmap, bool dev_replace)
> +{
> +	struct btrfs_fs_info *fs_info = stripe->bg->fs_info;
> +	struct btrfs_bio *bbio = NULL;
> +	bool zoned = btrfs_is_zoned(fs_info);
> +	int sector_nr;
> +
> +	for_each_set_bit(sector_nr, &write_bitmap, stripe->nr_sectors) {
> +		struct page *page = scrub_stripe_get_page(stripe, sector_nr);
> +		unsigned int pgoff = scrub_stripe_get_page_offset(stripe,
> +								  sector_nr);
> +		int ret;
> +
> +		/* We should only writeback sectors covered by an extent. */
> +		ASSERT(test_bit(sector_nr, &stripe->extent_sector_bitmap));
> +
> +		/* Can not merge with previous sector, submit the current one. */
> +		if (bbio && sector_nr && !test_bit(sector_nr - 1, &write_bitmap)) {
> +			fill_writer_pointer_gap(sctx, stripe->physical +
> +					(sector_nr << fs_info->sectorsize_bits));
> +			atomic_inc(&stripe->pending_io);
> +			btrfs_submit_scrub_write(fs_info, bbio,
> +						 stripe->mirror_num, dev_replace);
> +			/* For zoned writeback, QD must be 1. */

QD means queue depth? I'd rather spell it out, QD is not a common
abbreviation in our code.

> +			if (zoned)
> +				wait_scrub_stripe_io(stripe);
> +			bbio = NULL;
> +		}
> +		if (!bbio) {
> +			bbio = btrfs_bio_alloc(BTRFS_STRIPE_LEN >> PAGE_SHIFT,

					       SCRUB_STRIPE_PAGES

> +					REQ_OP_WRITE, NULL, scrub_write_endio, stripe);
> +			/* Backed by mempool */
> +			ASSERT(bbio);

Not sure if this is the first that does it but we could drop the comment
and assert around btrfs_bio_alloc because it's documented in the
function.

> +
> +			bbio->bio.bi_iter.bi_sector = (stripe->logical +
> +				(sector_nr << fs_info->sectorsize_bits)) >>
> +				SECTOR_SHIFT;
> +		}
> +		ret = bio_add_page(&bbio->bio, page, fs_info->sectorsize, pgoff);
> +		ASSERT(ret == fs_info->sectorsize);
> +	}
> +	if (bbio) {
> +		fill_writer_pointer_gap(sctx, bbio->bio.bi_iter.bi_sector <<
> +					SECTOR_SHIFT);
> +		atomic_inc(&stripe->pending_io);
> +		btrfs_submit_scrub_write(fs_info, bbio, stripe->mirror_num,
> +					 dev_replace);
> +		if (zoned)
> +			wait_scrub_stripe_io(stripe);
> +	}
> +}