Re: [PATCH v4 1/5] btrfs: use the page-cache for super block reading

From: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: David Sterba <dsterba@suse.cz>,
	Nikolay Borisov <nborisov@suse.com>,
	Josef Bacik <josef@toxicpanda.com>,
	"linux-btrfs @ vger . kernel . org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v4 1/5] btrfs: use the page-cache for super block reading
Date: Thu, 6 Feb 2020 08:17:20 +0000	[thread overview]
Message-ID: <SN4PR0401MB359854D81C504BBE28B8B2EB9B1D0@SN4PR0401MB3598.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200205165319.GA6326@infradead.org

On 05/02/2020 17:53, Christoph Hellwig wrote:
> On Wed, Feb 05, 2020 at 11:38:27PM +0900, Johannes Thumshirn wrote:
>> Super-block reading in BTRFS is done using buffer_heads. Buffer_heads have
>> some drawbacks, like not being able to propagate errors from the lower
>> layers.
>>
>> Directly use the page cache for reading the super-blocks from disk or
>> invalidating an on-disk super-block. We have to use the page-cache so to
>> avoid races between mkfs and udev. See also 6f60cbd3ae44 ("btrfs: access
>> superblock via pagecache in scan_one_device").
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> ---
>> Changes to v3:
>> - Use read_cache_pages() and write_one_page() for IO (hch)
>> - Changed subject (David)
>> - Dropped Josef's R-b due to change
>>
>> Changes to v2:
>> - open-code kunmap() + put_page() (David)
>> - fix double kunmap() (David)
>> - don't use bi_set_op_attrs() (David)
>>
>> Changes to v1:
>> - move 'super_page' into for-loop in btrfs_scratch_superblocks() (Nikolay)
>> - switch to using pagecahce instead of alloc_pages() (Nikolay, David)
>> ---
>>   fs/btrfs/disk-io.c | 78 +++++++++++++++++++++++++---------------------
>>   fs/btrfs/disk-io.h |  4 +--
>>   fs/btrfs/volumes.c | 57 +++++++++++++++++----------------
>>   fs/btrfs/volumes.h |  2 --
>>   4 files changed, 76 insertions(+), 65 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 28622de9e642..bc14ef1aadda 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2617,11 +2617,12 @@ int __cold open_ctree(struct super_block *sb,
>>   	u64 features;
>>   	u16 csum_type;
>>   	struct btrfs_key location;
>> -	struct buffer_head *bh;
>>   	struct btrfs_super_block *disk_super;
>>   	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
>>   	struct btrfs_root *tree_root;
>>   	struct btrfs_root *chunk_root;
>> +	struct page *super_page;
>> +	u8 *superblock;
> 
> I thought you agree to turn this into a struct btrfs_super_block
> pointer?

As stated in the cover letter, I lost track of the TODOs ;-)

>>   	bytenr = btrfs_sb_offset(copy_num);
>>   	if (bytenr + BTRFS_SUPER_INFO_SIZE >= i_size_read(bdev->bd_inode))
>>   		return -EINVAL;
>>   
>> -	bh = __bread(bdev, bytenr / BTRFS_BDEV_BLOCKSIZE, BTRFS_SUPER_INFO_SIZE);
>> -	/*
>> -	 * If we fail to read from the underlying devices, as of now
>> -	 * the best option we have is to mark it EIO.
>> -	 */
>> -	if (!bh)
>> -		return -EIO;
>> +	gfp_mask = mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL;
>> +	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, gfp_mask);
>> +	if (IS_ERR_OR_NULL(page))
>> +		return -ENOMEM;
> 
> Why do you need the __GFP_NOFAIL given that failures are handled
> properly here?  Also I think instead of using mapping_gfp_constraint you
> can use GFP_NOFS directly here.

OK

>>   
>> -	super = (struct btrfs_super_block *)bh->b_data;
>> +	super = kmap(page);
>>   	if (btrfs_super_bytenr(super) != bytenr ||
>>   		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>> -		brelse(bh);
>> +		kunmap(page);
>> +		put_page(page);
>>   		return -EINVAL;
>>   	}
>> +	kunmap(page);
> 
> Also last time I wondered why we can't leave the page mapped for the
> caller and also return the virtual address?  That would keep the
> callers a little cleaner.  Note that you don't need to pass the
> struct page in that case as the unmap helper can use kmap_to_page (and
> I think a helper would be really nice for the unmap and put anyway).
> 

There's btrfs_release_disk_super() but David didn't like the use of it 
in v2 of this series. But when using a 'struct btrfs_disk_super' instead 
of a 'struct page' I think he could be ok.