| Message ID | 20230111062335.1023353-8-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] btrfs: cleanup raid56_parity_write | expand |
On 2023/1/11 14:23, Christoph Hellwig wrote: > Instead of filling in a bio_list and submitting the bios in the only > caller, do that in scrub_assemble_read_bios. This removes the > need to pass the bio_list, and also makes it clear that the extra > bio_list cleanup in the caller is entirely pointless. Rename the > function to scrub_read_bios to make it clear that the bios are not > only assembled. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Qu Wenruo <wqu@suse.com> Originally the idea of passing bio_list around is reuse the function to submit and wait. But your cleanup has shown it can be smaller and simpler even without reusing the same submit and wait function. Thanks, Qu > --- > fs/btrfs/raid56.c | 36 +++++++++++++----------------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 88404a6b0b98e7..374c3873169b3f 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -2668,14 +2668,12 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio) > return ret; > } > > -static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, > - struct bio_list *bio_list) > +static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio) > { > + struct bio_list bio_list = BIO_EMPTY_LIST; > int total_sector_nr; > int ret = 0; > > - ASSERT(bio_list_size(bio_list) == 0); > - > /* Build a list of bios to read all the missing parts. */ > for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors; > total_sector_nr++) { > @@ -2704,42 +2702,38 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, > if (sector->uptodate) > continue; > > - ret = rbio_add_io_sector(rbio, bio_list, sector, stripe, > + ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe, > sectornr, REQ_OP_READ); > - if (ret) > - goto error; > + if (ret) { > + bio_list_put(&bio_list); > + return ret; > + } > } > + > + submit_read_wait_bio_list(rbio, &bio_list); > return 0; > -error: > - bio_list_put(bio_list); > - return ret; > } > > static int scrub_rbio(struct btrfs_raid_bio *rbio) > { > bool need_check = false; > - struct bio_list bio_list; > int sector_nr; > int ret; > > - bio_list_init(&bio_list); > - > ret = alloc_rbio_essential_pages(rbio); > if (ret) > - goto cleanup; > + return ret; > > bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors); > > - ret = scrub_assemble_read_bios(rbio, &bio_list); > + ret = scrub_assemble_read_bios(rbio); > if (ret < 0) > - goto cleanup; > - > - submit_read_wait_bio_list(rbio, &bio_list); > + return ret; > > /* We may have some failures, recover the failed sectors first. */ > ret = recover_scrub_rbio(rbio); > if (ret < 0) > - goto cleanup; > + return ret; > > /* > * We have every sector properly prepared. Can finish the scrub > @@ -2757,10 +2751,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio) > } > } > return ret; > - > -cleanup: > - bio_list_put(&bio_list); > - return ret; > } > > static void scrub_rbio_work_locked(struct work_struct *work)
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 88404a6b0b98e7..374c3873169b3f 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -2668,14 +2668,12 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio) return ret; } -static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, - struct bio_list *bio_list) +static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio) { + struct bio_list bio_list = BIO_EMPTY_LIST; int total_sector_nr; int ret = 0; - ASSERT(bio_list_size(bio_list) == 0); - /* Build a list of bios to read all the missing parts. */ for (total_sector_nr = 0; total_sector_nr < rbio->nr_sectors; total_sector_nr++) { @@ -2704,42 +2702,38 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, if (sector->uptodate) continue; - ret = rbio_add_io_sector(rbio, bio_list, sector, stripe, + ret = rbio_add_io_sector(rbio, &bio_list, sector, stripe, sectornr, REQ_OP_READ); - if (ret) - goto error; + if (ret) { + bio_list_put(&bio_list); + return ret; + } } + + submit_read_wait_bio_list(rbio, &bio_list); return 0; -error: - bio_list_put(bio_list); - return ret; } static int scrub_rbio(struct btrfs_raid_bio *rbio) { bool need_check = false; - struct bio_list bio_list; int sector_nr; int ret; - bio_list_init(&bio_list); - ret = alloc_rbio_essential_pages(rbio); if (ret) - goto cleanup; + return ret; bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors); - ret = scrub_assemble_read_bios(rbio, &bio_list); + ret = scrub_assemble_read_bios(rbio); if (ret < 0) - goto cleanup; - - submit_read_wait_bio_list(rbio, &bio_list); + return ret; /* We may have some failures, recover the failed sectors first. */ ret = recover_scrub_rbio(rbio); if (ret < 0) - goto cleanup; + return ret; /* * We have every sector properly prepared. Can finish the scrub @@ -2757,10 +2751,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio) } } return ret; - -cleanup: - bio_list_put(&bio_list); - return ret; } static void scrub_rbio_work_locked(struct work_struct *work)
Instead of filling in a bio_list and submitting the bios in the only caller, do that in scrub_assemble_read_bios. This removes the need to pass the bio_list, and also makes it clear that the extra bio_list cleanup in the caller is entirely pointless. Rename the function to scrub_read_bios to make it clear that the bios are not only assembled. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/raid56.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-)