| Message ID | cfea13b2a1649e4c295b020f2713660c879ef898.1678777941.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror() | expand |
On 14.03.23 08:36, Qu Wenruo wrote: > There is really no need to go through the super complex scrub_sectors() > to just handle super blocks. > > This patch will introduce a dedicated function (less than 50 lines) to > handle super block scrubing. > > This new function will introduce a behavior change, instead of using the > complex but concurrent scrub_bio system, here we just go > submit-and-wait. > > There is really not much sense to care the performance of super block > scrubbing. It only has 3 super blocks at most, and they are all scattered > around the devices already. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index 3cdf73277e7e..e765eb8b8bcf 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -4243,18 +4243,59 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx, > return ret; > } > > +static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, > + struct page *page, u64 physical, u64 generation) > +{ > + struct btrfs_fs_info *fs_info = sctx->fs_info; > + struct bio_vec bvec; > + struct bio bio; > + struct btrfs_super_block *sb = page_address(page); > + int ret; > + > + bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ); > + bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT; > + bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0); > + ret = submit_bio_wait(&bio); > + bio_uninit(&bio); > + I don't think bio_uninit() is needed here. You're not attaching any cgroup information, bio integrity or crypto context to it. Or can that be attached down the stack?
On Tue, Mar 14, 2023 at 12:05:52PM +0000, Johannes Thumshirn wrote: > On 14.03.23 08:36, Qu Wenruo wrote: > > There is really no need to go through the super complex scrub_sectors() > > to just handle super blocks. > > > > This patch will introduce a dedicated function (less than 50 lines) to > > handle super block scrubing. > > > > This new function will introduce a behavior change, instead of using the > > complex but concurrent scrub_bio system, here we just go > > submit-and-wait. > > > > There is really not much sense to care the performance of super block > > scrubbing. It only has 3 super blocks at most, and they are all scattered > > around the devices already. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > --- > > fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 46 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > > index 3cdf73277e7e..e765eb8b8bcf 100644 > > --- a/fs/btrfs/scrub.c > > +++ b/fs/btrfs/scrub.c > > @@ -4243,18 +4243,59 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx, > > return ret; > > } > > > > +static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, > > + struct page *page, u64 physical, u64 generation) > > +{ > > + struct btrfs_fs_info *fs_info = sctx->fs_info; > > + struct bio_vec bvec; > > + struct bio bio; > > + struct btrfs_super_block *sb = page_address(page); > > + int ret; > > + > > + bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ); > > + bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT; > > + bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0); > > + ret = submit_bio_wait(&bio); > > + bio_uninit(&bio); > > + > > I don't think bio_uninit() is needed here. You're not attaching any cgroup information, > bio integrity or crypto context to it. Or can that be attached down the stack? It may not be needed due to functional reasons but I'd rather keep it there to pair with bio_init.
On 2023/3/14 20:05, Johannes Thumshirn wrote: > On 14.03.23 08:36, Qu Wenruo wrote: >> There is really no need to go through the super complex scrub_sectors() >> to just handle super blocks. >> >> This patch will introduce a dedicated function (less than 50 lines) to >> handle super block scrubing. >> >> This new function will introduce a behavior change, instead of using the >> complex but concurrent scrub_bio system, here we just go >> submit-and-wait. >> >> There is really not much sense to care the performance of super block >> scrubbing. It only has 3 super blocks at most, and they are all scattered >> around the devices already. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 46 insertions(+), 8 deletions(-) >> >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >> index 3cdf73277e7e..e765eb8b8bcf 100644 >> --- a/fs/btrfs/scrub.c >> +++ b/fs/btrfs/scrub.c >> @@ -4243,18 +4243,59 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx, >> return ret; >> } >> >> +static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, >> + struct page *page, u64 physical, u64 generation) >> +{ >> + struct btrfs_fs_info *fs_info = sctx->fs_info; >> + struct bio_vec bvec; >> + struct bio bio; >> + struct btrfs_super_block *sb = page_address(page); >> + int ret; >> + >> + bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ); >> + bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT; >> + bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0); >> + ret = submit_bio_wait(&bio); >> + bio_uninit(&bio); >> + > > I don't think bio_uninit() is needed here. You're not attaching any cgroup information, > bio integrity or crypto context to it. Or can that be attached down the stack? > It's mostly to pair with bio_init(). Although the bio has no CGROUP nor crypto context, it's still better to explicitly pair bio_init() with bio_uninit(). Thanks, Qu
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 3cdf73277e7e..e765eb8b8bcf 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -4243,18 +4243,59 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx, return ret; } +static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, + struct page *page, u64 physical, u64 generation) +{ + struct btrfs_fs_info *fs_info = sctx->fs_info; + struct bio_vec bvec; + struct bio bio; + struct btrfs_super_block *sb = page_address(page); + int ret; + + bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ); + bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT; + bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0); + ret = submit_bio_wait(&bio); + bio_uninit(&bio); + + if (ret < 0) + return ret; + ret = btrfs_check_super_csum(fs_info, sb); + if (ret != 0) { + btrfs_err_rl(fs_info, + "super block at physical %llu devid %llu has bad csum", + physical, dev->devid); + return -EIO; + } + if (btrfs_super_generation(sb) != generation) { + btrfs_err_rl(fs_info, +"super block at physical %llu devid %llu has bad generation, has %llu expect %llu", + physical, dev->devid, + btrfs_super_generation(sb), generation); + return -EUCLEAN; + } + + ret = btrfs_validate_super(fs_info, sb, -1); + return ret; +} + static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, struct btrfs_device *scrub_dev) { int i; u64 bytenr; u64 gen; - int ret; + int ret = 0; + struct page *page; struct btrfs_fs_info *fs_info = sctx->fs_info; if (BTRFS_FS_ERROR(fs_info)) return -EROFS; + page = alloc_page(GFP_KERNEL); + if (!page) + return -ENOMEM; + /* Seed devices of a new filesystem has their own generation. */ if (scrub_dev->fs_devices != fs_info->fs_devices) gen = scrub_dev->generation; @@ -4269,15 +4310,12 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, if (!btrfs_check_super_location(scrub_dev, bytenr)) continue; - ret = scrub_sectors(sctx, bytenr, BTRFS_SUPER_INFO_SIZE, bytenr, - scrub_dev, BTRFS_EXTENT_FLAG_SUPER, gen, i, - NULL, bytenr); + ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); if (ret) - return ret; + break; } - wait_event(sctx->list_wait, atomic_read(&sctx->bios_in_flight) == 0); - - return 0; + __free_page(page); + return ret; } static void scrub_workers_put(struct btrfs_fs_info *fs_info)
There is really no need to go through the super complex scrub_sectors() to just handle super blocks. This patch will introduce a dedicated function (less than 50 lines) to handle super block scrubing. This new function will introduce a behavior change, instead of using the complex but concurrent scrub_bio system, here we just go submit-and-wait. There is really not much sense to care the performance of super block scrubbing. It only has 3 super blocks at most, and they are all scattered around the devices already. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 8 deletions(-)