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.
