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

[David Sterba <dsterba@suse.cz>]

On Fri, Feb 14, 2020 at 12:24:33AM +0900, Johannes Thumshirn wrote:
> +		bio = bio_alloc(GFP_NOFS, 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));
>  
>  		/*
> -		 * we fua the first super.  The others we allow
> +		 * We fua the first super.  The others we allow
>  		 * to go down lazy.
>  		 */
> -		op_flags = REQ_SYNC | REQ_META | REQ_PRIO;
> +		bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_META | REQ_PRIO;
>  		if (i == 0 && !btrfs_test_opt(device->fs_info, NOBARRIER))
> -			op_flags |= REQ_FUA;
> -		ret = btrfsic_submit_bh(REQ_OP_WRITE, op_flags, bh);
> -		if (ret)
> -			errors++;
> +			bio->bi_opf |= REQ_FUA;
> +
> +		btrfsic_submit_bio(bio);

btrfsic_submit_bh forwards the return values, and could increase error
counter, now btrfsic_submit_bio does not forward submit_bio return
value.

I traced it to generic_make_request, the comment says it does not return
error, so not handling it is ok.

>  static int wait_dev_supers(struct btrfs_device *device, int max_mirrors)

>  	for (i = 0; i < max_mirrors; i++) {
> +		struct page *page;
> +
>  		bytenr = btrfs_sb_offset(i);
>  		if (bytenr + BTRFS_SUPER_INFO_SIZE >=
>  		    device->commit_total_bytes)
>  			break;
>  
> -		bh = __find_get_block(device->bdev,
> -				      bytenr / BTRFS_BDEV_BLOCKSIZE,
> -				      BTRFS_SUPER_INFO_SIZE);
> -		if (!bh) {
> +		page = find_get_page(device->bdev->bd_inode->i_mapping,
> +				     bytenr >> PAGE_SHIFT);
> +		if (!page) {
>  			errors++;
>  			if (i == 0)
>  				primary_failed = true;
>  			continue;
>  		}
> -		wait_on_buffer(bh);
> -		if (!buffer_uptodate(bh)) {
> +		wait_on_page_locked(page);

And here it's checking the result that could be set by
btrfs_end_super_write (SetPageError). The waiting part is due to the
PG_Locked bit. Please add some comments.

> +		if (PageError(page)) {
>  			errors++;
>  			if (i == 0)
>  				primary_failed = true;
>  		}
>  
>  		/* drop our reference */
> -		brelse(bh);
> +		put_page(page);
>  
>  		/* drop the reference from the writing run */
> -		brelse(bh);
> +		put_page(page);
>  	}
>  
>  	/* log error, force error return */
> -- 
> 2.24.1