Re: [PATCH v2 6/6] btrfs: remove buffer_heads form superblock mirror integrity checking

From: Nikolay Borisov <nborisov@suse.com>
To: Johannes Thumshirn <johannes.thumshirn@wdc.com>,
	David Sterba <dsterba@suse.com>
Cc: "linux-btrfs @ vger . kernel . org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v2 6/6] btrfs: remove buffer_heads form superblock mirror integrity checking
Date: Thu, 23 Jan 2020 16:23:23 +0200	[thread overview]
Message-ID: <11e78494-f8ee-bc04-69ba-d0f9b35b4792@suse.com> (raw)
In-Reply-To: <20200123081849.23397-7-johannes.thumshirn@wdc.com>

On 23.01.20 г. 10:18 ч., Johannes Thumshirn wrote:
> The integrity checking code for the superblock mirrors is the last remaining
> user of buffer_heads in BTRFS, change it to using plain BIOs as well.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> ---
> Changes to v1:
> - Convert from alloc_page() to find_or_create_page()
> ---
>  fs/btrfs/check-integrity.c | 44 +++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index 4f6db2fe482a..45b88bcd6cbb 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -77,7 +77,6 @@
>  
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> -#include <linux/buffer_head.h>
>  #include <linux/mutex.h>
>  #include <linux/genhd.h>
>  #include <linux/blkdev.h>
> @@ -762,28 +761,47 @@ static int btrfsic_process_superblock_dev_mirror(
>  	struct btrfs_fs_info *fs_info = state->fs_info;
>  	struct btrfs_super_block *super_tmp;
>  	u64 dev_bytenr;
> -	struct buffer_head *bh;
>  	struct btrfsic_block *superblock_tmp;
>  	int pass;
>  	struct block_device *const superblock_bdev = device->bdev;
> +	struct page *page;
> +	struct bio bio;
> +	struct bio_vec bio_vec;
> +	struct address_space *mapping = superblock_bdev->bd_inode->i_mapping;
> +	gfp_t gfp_mask;
> +	int ret;
>  
>  	/* super block bytenr is always the unmapped device bytenr */
>  	dev_bytenr = btrfs_sb_offset(superblock_mirror_num);
>  	if (dev_bytenr + BTRFS_SUPER_INFO_SIZE > device->commit_total_bytes)
>  		return -1;
> -	bh = __bread(superblock_bdev, dev_bytenr / BTRFS_BDEV_BLOCKSIZE,
> -		     BTRFS_SUPER_INFO_SIZE);
> -	if (NULL == bh)
> +
> +	gfp_mask = mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL;
> +
> +	page = find_or_create_page(mapping, dev_bytenr >> PAGE_SHIFT, gfp_mask);
> +	if (!page)
> +		return -1;
> +
> +	bio_init(&bio, &bio_vec, 1);
> +	bio.bi_iter.bi_sector = dev_bytenr >> SECTOR_SHIFT;
> +	bio_set_dev(&bio, superblock_bdev);
> +	bio_set_op_attrs(&bio, REQ_OP_READ, 0);
> +	bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0);
> +
> +	ret = submit_bio_wait(&bio);
> +	if (ret)
>  		return -1;
> -	super_tmp = (struct btrfs_super_block *)
> -	    (bh->b_data + (dev_bytenr & (BTRFS_BDEV_BLOCKSIZE - 1)));
> +
> +	unlock_page(page);

This is safe since it's part of the integrity code which gets called
during mount so presumably we can't have a transaction commit while this
is running. I'd prefer an explicit mention of that in the changelog.

<snip>