On 05/02/2020 19:16, Christoph Hellwig wrote:
> 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.
Good question, I guess it's saver to always FUA the SBs
>
>> + bio->bi_opf = REQ_OP_WRITE | op_flags;
>
> You could kill the op_flags variable and just assign everything directly
> to bio->bi_opf.
>
Then I'd still have to deal with the NOBARRIER mount option here so
op_flags is nice to have for readability.
