Re: [PATCH v3 1/5] btrfs: remove buffer heads from super block reading

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 28/01/2020 12:47, Christoph Hellwig wrote:
> On Tue, Jan 28, 2020 at 12:59:27AM +0900, Johannes Thumshirn wrote:
>>   	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;
> 
> Any good reason this isn't a struct btrfs_super_block * instead?

No not really.

[...]

>> @@ -2873,8 +2880,9 @@ int __cold open_ctree(struct super_block *sb,
>>   	 * following bytes up to INFO_SIZE, the checksum is calculated from
>>   	 * the whole block of INFO_SIZE
>>   	 */
>> -	memcpy(fs_info->super_copy, bh->b_data, sizeof(*fs_info->super_copy));
>> -	brelse(bh);
>> +	memcpy(fs_info->super_copy, superblock, sizeof(*fs_info->super_copy));
>> +	kunmap(super_page);
>> +	put_page(super_page);
> 
> Would it make sense to move the code up to here in a helper to
> simplify the error handling?

There is btrfs_release_disk_super() already but David didn't like it's 
use here.

> 
>>   
>>   int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>> -			struct buffer_head **bh_ret)
>> +			struct page **super_page)
>>   {
>> -	struct buffer_head *bh;
>>   	struct btrfs_super_block *super;
>> +	struct bio_vec bio_vec;
>> +	struct bio bio;
>> +	struct page *page;
>>   	u64 bytenr;
>> +	struct address_space *mapping = bdev->bd_inode->i_mapping;
>> +	gfp_t gfp_mask;
>> +	int ret;
>>   
>>   	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);
>> +	gfp_mask = mapping_gfp_constraint(mapping, ~__GFP_FS) | __GFP_NOFAIL;
>> +	page = find_or_create_page(mapping, bytenr >> PAGE_SHIFT, gfp_mask);
> 
> Why not simply use read_cache_page_gfp to find or read the page?

right

> 
>> -	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);
>>   
>> -	*bh_ret = bh;
>> +	*super_page = page;
> 
> Given that both callers need the kernel virtual address, why not leave it
> kmapped?  OTOH if you use read_cache_page_gfp, we could just kill
> btrfs_read_dev_one_super and open code the call to read_cache_page_gfp
> and btrfs_super_bytenr / btrfs_super_magic in the callers.

Sounds like a good idea, I'll have a look into it.

> 
>> +	bio_init(&bio, &bio_vec, 1);
>>   	for (copy_num = 0; copy_num < BTRFS_SUPER_MIRROR_MAX;
>>   		copy_num++) {
>> +		u64 bytenr = btrfs_sb_offset(copy_num);
>> +		struct page *page;
>>   
>> -		if (btrfs_read_dev_one_super(bdev, copy_num, &bh))
>> +		if (btrfs_read_dev_one_super(bdev, copy_num, &page))
>>   			continue;
>>   
>> -		disk_super = (struct btrfs_super_block *)bh->b_data;
>> +		disk_super = kmap(page) + offset_in_page(bytenr);
>>   
>>   		memset(&disk_super->magic, 0, sizeof(disk_super->magic));
>> -		set_buffer_dirty(bh);
>> -		sync_dirty_buffer(bh);
>> -		brelse(bh);
>> +
>> +		bio.bi_iter.bi_sector = bytenr >> SECTOR_SHIFT;
>> +		bio_set_dev(&bio, bdev);
>> +		bio.bi_opf = REQ_OP_WRITE;
>> +		bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE,
>> +			     offset_in_page(bytenr));
>> +
>> +		lock_page(page);
>> +		submit_bio_wait(&bio);
>> +		unlock_page(page);
>> +		kunmap(page);
>> +		put_page(page);
>> +		bio_reset(&bio);
> 
> Facoting out the code to write a single sb would clean this up a bit.
> Also no real need to keep the page kmapped when under I/O.
> 

Yup, will be doing