| Message ID | 20200123081849.23397-7-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: remove buffer heads form superblock handling | expand |
On 1/23/20 3:18 AM, 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; > + I don't think we need __GFP_NOFAIL here, it's just check integrity. Other than that you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
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>
On Thu, Jan 23, 2020 at 05:18:49PM +0900, Johannes Thumshirn wrote: > 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); > + btrfs_release_disk_super(page); > return 0; > } > > @@ -795,7 +813,7 @@ 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); > + btrfs_release_disk_super(page); > return -1; > } > /* for superblock, only the dev_bytenr makes sense */ > @@ -880,7 +898,7 @@ 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); > + btrfs_release_disk_super(page); > return -1; > } > > @@ -890,7 +908,7 @@ static int btrfsic_process_superblock_dev_mirror( > mirror_num, NULL); > if (NULL == next_block) { > btrfsic_release_block_ctx(&tmp_next_block_ctx); > - brelse(bh); > + btrfs_release_disk_super(page); > return -1; > } > > @@ -902,7 +920,7 @@ static int btrfsic_process_superblock_dev_mirror( > BTRFSIC_GENERATION_UNKNOWN); > btrfsic_release_block_ctx(&tmp_next_block_ctx); > if (NULL == l) { > - brelse(bh); > + btrfs_release_disk_super(page); > return -1; > } > } > @@ -910,7 +928,7 @@ static int btrfsic_process_superblock_dev_mirror( > if (state->print_mask & BTRFSIC_PRINT_MASK_INITIAL_ALL_TREES) > btrfsic_dump_tree_sub(state, superblock_tmp, 0); > > - brelse(bh); > + btrfs_release_disk_super(page); This could be the cleaned up to merge all error exits to jump to this common block. Integrity checker is an old code so nobody cared enough to clean it up, but it would be good to do it now when there are pople looking at the code. As mentioned before, btrfs_release_disk_super should be opencoded so this would make it more straightfowrard to have only one place instad of each error exit brelse replaced by put_page/kunmap. > return 0; > }
On 24/01/2020 15:22, David Sterba wrote: [...] >> >> - brelse(bh); >> + btrfs_release_disk_super(page); > > This could be the cleaned up to merge all error exits to jump to this > common block. Integrity checker is an old code so nobody cared enough to > clean it up, but it would be good to do it now when there are pople > looking at the code. > > As mentioned before, btrfs_release_disk_super should be opencoded so > this would make it more straightfowrard to have only one place instad of > each error exit brelse replaced by put_page/kunmap. Sure no problem. I already have a internal branch with put_page()/kunmap(), but before sending it out I want to address Nikolay's comments regarding the page locking and this still causes deadlocks here. Thanks, Johannes
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); + + super_tmp = kmap(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); + btrfs_release_disk_super(page); return 0; } @@ -795,7 +813,7 @@ 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); + btrfs_release_disk_super(page); return -1; } /* for superblock, only the dev_bytenr makes sense */ @@ -880,7 +898,7 @@ 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); + btrfs_release_disk_super(page); return -1; } @@ -890,7 +908,7 @@ static int btrfsic_process_superblock_dev_mirror( mirror_num, NULL); if (NULL == next_block) { btrfsic_release_block_ctx(&tmp_next_block_ctx); - brelse(bh); + btrfs_release_disk_super(page); return -1; } @@ -902,7 +920,7 @@ static int btrfsic_process_superblock_dev_mirror( BTRFSIC_GENERATION_UNKNOWN); btrfsic_release_block_ctx(&tmp_next_block_ctx); if (NULL == l) { - brelse(bh); + btrfs_release_disk_super(page); return -1; } } @@ -910,7 +928,7 @@ static int btrfsic_process_superblock_dev_mirror( if (state->print_mask & BTRFSIC_PRINT_MASK_INITIAL_ALL_TREES) btrfsic_dump_tree_sub(state, superblock_tmp, 0); - brelse(bh); + btrfs_release_disk_super(page); return 0; }
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(-)