| Message ID | 20200213152436.13276-9-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove buffer heads form superblock handling | expand |
On Fri, Feb 14, 2020 at 12:24:36AM +0900, 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> > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > --- > Changes to v7: > - Use read_Cache_page_gfp() > - Don't kmap() block device mappings (David) > > Changes to v4: > - Remove mapping_gfp_constraint() > > Changes to v2: > - Open-code kunmap() + put_page() (David) > - Remove __GFP_NOFAIL from allocation (Josef) > - Merge error paths (David) The per-patch changes are a nice bonus, let me use it to point out a thing that the reviews from previous iterations should be kept only if there are cosmetic changes (like renaming identifiers, formatting or comments) and not functional changes like the kmap/kunmap or read_cache_gfp. If the rev-by lines are there I would expect that the respective reviewers will skip reading the patch. Good if not, but without an explicit reply 'still ok' we're losing some information.
On Fri, Feb 14, 2020 at 12:24:36AM +0900, 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> > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > --- > Changes to v7: > - Use read_Cache_page_gfp() > - Don't kmap() block device mappings (David) > > Changes to v4: > - Remove mapping_gfp_constraint() > > Changes to v2: > - Open-code kunmap() + put_page() (David) > - Remove __GFP_NOFAIL from allocation (Josef) > - Merge error paths (David) > > Changes to v1: > - Convert from alloc_page() to find_or_create_page() > --- > fs/btrfs/check-integrity.c | 42 +++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c > index 4f6db2fe482a..d8d915d7beda 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,29 +761,33 @@ 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; > + 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) > + > + page = reed_cache_page_gfp(mapping, dev_bytenr >> PAGE_SHIFT, GFP_NOFS); Reed-Solomon error correction in page cache? Have I missed the news? :) CC [M] fs/btrfs/check-integrity.o fs/btrfs/check-integrity.c: In function ‘btrfsic_process_superblock_dev_mirror’: fs/btrfs/check-integrity.c:778:9: error: implicit declaration of function ‘reed_cache_page_gfp’; did you mean ‘read_cache_page_gfp’? [-Werror=implicit-function-declaration] 778 | page = reed_cache_page_gfp(mapping, dev_bytenr >> PAGE_SHIFT, GFP_NOFS); | ^~~~~~~~~~~~~~~~~~~ | read_cache_page_gfp And with that fixed there are still the following warnings. fs/btrfs/check-integrity.c:778:7: warning: assignment to ‘struct page *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion] 778 | page = reed_cache_page_gfp(mapping, dev_bytenr >> PAGE_SHIFT, GFP_NOFS); | ^ fs/btrfs/check-integrity.c:769:17: warning: unused variable ‘bio_vec’ [-Wunused-variable] 769 | struct bio_vec bio_vec; | ^~~~~~~ fs/btrfs/check-integrity.c:768:13: warning: unused variable ‘bio’ [-Wunused-variable] 768 | struct bio bio; | ^~~
diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c index 4f6db2fe482a..d8d915d7beda 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,29 +761,33 @@ 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; + 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) + + page = reed_cache_page_gfp(mapping, dev_bytenr >> PAGE_SHIFT, GFP_NOFS); + if (IS_ERR(page)) return -1; - super_tmp = (struct btrfs_super_block *) - (bh->b_data + (dev_bytenr & (BTRFS_BDEV_BLOCKSIZE - 1))); + + super_tmp = page_address(page); if (btrfs_super_bytenr(super_tmp) != dev_bytenr || btrfs_super_magic(super_tmp) != BTRFS_MAGIC || memcmp(device->uuid, super_tmp->dev_item.uuid, BTRFS_UUID_SIZE) || btrfs_super_nodesize(super_tmp) != state->metablock_size || btrfs_super_sectorsize(super_tmp) != state->datablock_size) { - brelse(bh); - return 0; + ret = 0; + goto out; } superblock_tmp = @@ -795,8 +798,8 @@ static int btrfsic_process_superblock_dev_mirror( superblock_tmp = btrfsic_block_alloc(); if (NULL == superblock_tmp) { pr_info("btrfsic: error, kmalloc failed!\n"); - brelse(bh); - return -1; + ret = -1; + goto out; } /* for superblock, only the dev_bytenr makes sense */ superblock_tmp->dev_bytenr = dev_bytenr; @@ -880,8 +883,8 @@ static int btrfsic_process_superblock_dev_mirror( mirror_num)) { pr_info("btrfsic: btrfsic_map_block(bytenr @%llu, mirror %d) failed!\n", next_bytenr, mirror_num); - brelse(bh); - return -1; + ret = -1; + goto out; } next_block = btrfsic_block_lookup_or_add( @@ -890,8 +893,8 @@ static int btrfsic_process_superblock_dev_mirror( mirror_num, NULL); if (NULL == next_block) { btrfsic_release_block_ctx(&tmp_next_block_ctx); - brelse(bh); - return -1; + ret = -1; + goto out; } next_block->disk_key = tmp_disk_key; @@ -902,16 +905,17 @@ static int btrfsic_process_superblock_dev_mirror( BTRFSIC_GENERATION_UNKNOWN); btrfsic_release_block_ctx(&tmp_next_block_ctx); if (NULL == l) { - brelse(bh); - return -1; + ret = -1; + goto out; } } } if (state->print_mask & BTRFSIC_PRINT_MASK_INITIAL_ALL_TREES) btrfsic_dump_tree_sub(state, superblock_tmp, 0); - brelse(bh); - return 0; +out: + put_page(page); + return ret; } static struct btrfsic_stack_frame *btrfsic_stack_frame_alloc(void)