Re: [PATCH v4 2/5] btrfs: use BIOs instead of buffer_heads from superblock writeout

[Christoph Hellwig <hch@infradead.org>]

On Wed, Feb 05, 2020 at 11:38:28PM +0900, Johannes Thumshirn wrote:
> +static void btrfs_end_super_write(struct bio *bio)
>  {
> +	struct btrfs_device *device = bio->bi_private;
> +	struct bio_vec *bvec;
> +	struct bvec_iter_all iter_all;
> +	struct page *page;
> +
> +	bio_for_each_segment_all(bvec, bio, iter_all) {
> +		page = bvec->bv_page;
> +
> +		if (blk_status_to_errno(bio->bi_status)) {

Nit: this could simply check bio->bi_status without a conversion.

> +			btrfs_warn_rl_in_rcu(device->fs_info,
> +					     "lost page write due to IO error on %s",
> +					     rcu_str_deref(device->name));

But maybe you want to print the error here?

> +	gfp_mask = mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL;

Same comment on the ask as in the previous patch.

> +		u8 *ptr;

I'd use a typed pointer here again..

> +		ptr = kmap(page);
> +		memcpy(ptr, sb, BTRFS_SUPER_INFO_SIZE);

With which you could do a struct assignment here and very slightly
improve type safety.

> @@ -3497,9 +3506,23 @@ static int write_dev_supers(struct btrfs_device *device,
>  		op_flags = REQ_SYNC | REQ_META | REQ_PRIO;
>  		if (i == 0 && !btrfs_test_opt(device->fs_info, NOBARRIER))
>  			op_flags |= REQ_FUA;

Question on the existing code:  why is it safe to not use FUA for the
subsequent superblocks?

> +
> +		/*
> +		 * Directly use BIOs here instead of relying on the page-cache
> +		 * to do I/O, so we don't loose the ability to do integrity
> +		 * checking.
> +		 */
> +		bio = bio_alloc(gfp_mask, 1);
> +		bio_set_dev(bio, device->bdev);
> +		bio->bi_iter.bi_sector = bytenr >> SECTOR_SHIFT;
> +		bio->bi_private = device;
> +		bio->bi_end_io = btrfs_end_super_write;
> +		bio_add_page(bio, page, BTRFS_SUPER_INFO_SIZE,
> +			     offset_in_page(bytenr));

Missing return value check.  But given that it is a single page and
can't error out please switch to __bio_add_page here.

> +		bio->bi_opf = REQ_OP_WRITE | op_flags;

You could kill the op_flags variable and just assign everything directly
to bio->bi_opf.