| Message ID | 20221213084123.309790-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] btrfs: cleanup raid56_parity_write | expand |
On 2022/12/13 16:41, Christoph Hellwig wrote: > The bio_list is only filled by scrub_assemble_read_bios when > successful, so don't try to walk it and put the bios on any > failure before the successful call to recover_assemble_read_bios. > Also initialize bio_list at initialization time. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/raid56.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 5dab0685e17dd5..2a5857d42fff20 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -2754,31 +2754,32 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, > > static int scrub_rbio(struct btrfs_raid_bio *rbio) > { > + struct bio_list bio_list = BIO_EMPTY_LIST; > bool need_check = false; > - struct bio_list bio_list; > int sector_nr; > int ret; > struct bio *bio; > > - 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); > if (ret < 0) > - goto cleanup; > + return ret; > > submit_read_bios(rbio, &bio_list); > wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0); > > /* We may have some failures, recover the failed sectors first. */ > ret = recover_scrub_rbio(rbio); > - if (ret < 0) > - goto cleanup; > + if (ret < 0) { > + while ((bio = bio_list_pop(&bio_list))) > + bio_put(bio); > + return ret; > + } Do we still need the cleanup? IIRC after submit_read_bios() (or be more safe, after wait_event()), we should no longer touch @bio_list anymore. Thus the cleanup itself is no longer needed AFAIK. Thanks, Qu > > /* > * We have every sector properly prepared. Can finish the scrub > @@ -2796,12 +2797,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio) > } > } > return ret; > - > -cleanup: > - while ((bio = bio_list_pop(&bio_list))) > - bio_put(bio); > - > - return ret; > } > > static void scrub_rbio_work_locked(struct work_struct *work)
On Tue, Dec 13, 2022 at 04:53:33PM +0800, Qu Wenruo wrote: >> ret = scrub_assemble_read_bios(rbio, &bio_list); >> if (ret < 0) >> - goto cleanup; >> + return ret; >> submit_read_bios(rbio, &bio_list); >> wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0); >> /* We may have some failures, recover the failed sectors first. */ >> ret = recover_scrub_rbio(rbio); >> - if (ret < 0) >> - goto cleanup; >> + if (ret < 0) { >> + while ((bio = bio_list_pop(&bio_list))) >> + bio_put(bio); >> + return ret; >> + } > > Do we still need the cleanup? IIRC after submit_read_bios() (or be more > safe, after wait_event()), we should no longer touch @bio_list anymore. Oh, true. submit_read_bios does the list_pop, so we don't need this here, and in recover_rbio either. And looking at it a little more I think the are could use even more cleanup by: - moving the wait_event for stripes_pending into submit_read_bios. - moving the submit_read_bios *_assemble_read_bios and stop passing the bio_list entirely, which removes all the confusion about who cleans it up. What do you think of this series (still needs testing before I can post it): http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-raid56-cleanups
On 2022/12/13 21:24, Christoph Hellwig wrote: > On Tue, Dec 13, 2022 at 04:53:33PM +0800, Qu Wenruo wrote: >>> ret = scrub_assemble_read_bios(rbio, &bio_list); >>> if (ret < 0) >>> - goto cleanup; >>> + return ret; >>> submit_read_bios(rbio, &bio_list); >>> wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0); >>> /* We may have some failures, recover the failed sectors first. */ >>> ret = recover_scrub_rbio(rbio); >>> - if (ret < 0) >>> - goto cleanup; >>> + if (ret < 0) { >>> + while ((bio = bio_list_pop(&bio_list))) >>> + bio_put(bio); >>> + return ret; >>> + } >> >> Do we still need the cleanup? IIRC after submit_read_bios() (or be more >> safe, after wait_event()), we should no longer touch @bio_list anymore. > > Oh, true. submit_read_bios does the list_pop, so we don't need > this here, and in recover_rbio either. And looking at it a little more > I think the are could use even more cleanup by: > > - moving the wait_event for stripes_pending into submit_read_bios. > - moving the submit_read_bios *_assemble_read_bios and stop passing > the bio_list entirely, which removes all the confusion about > who cleans it up. > > What do you think of this series (still needs testing before I can > post it): > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/btrfs-raid56-cleanups The code change all looks good, I'd just prefer some naming changes. One personal thing is, I'd prefer "_wait_" for functions that may wait for IOs. Thus submit_read_bios() may be better renamed to submit_read_wait_bios(). (bios means it still directly handle a bio list, or just submit_read_wait_bio_list()?) Another changes is, since there is no bio_list out of the <work>_read_bios(), the "bios" naming can be removed. Something like <work>_read_wait() may be a little better. Thanks, Qu
On Wed, Dec 14, 2022 at 07:32:00AM +0800, Qu Wenruo wrote: > > One personal thing is, I'd prefer "_wait_" for functions that may wait for > IOs. Thus submit_read_bios() may be better renamed to > submit_read_wait_bios(). > (bios means it still directly handle a bio list, or just > submit_read_wait_bio_list()?) I can do either one. > > Another changes is, since there is no bio_list out of the > <work>_read_bios(), the "bios" naming can be removed. > Something like <work>_read_wait() may be a little better. Sure.
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 5dab0685e17dd5..2a5857d42fff20 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -2754,31 +2754,32 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio, static int scrub_rbio(struct btrfs_raid_bio *rbio) { + struct bio_list bio_list = BIO_EMPTY_LIST; bool need_check = false; - struct bio_list bio_list; int sector_nr; int ret; struct bio *bio; - 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); if (ret < 0) - goto cleanup; + return ret; submit_read_bios(rbio, &bio_list); wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0); /* We may have some failures, recover the failed sectors first. */ ret = recover_scrub_rbio(rbio); - if (ret < 0) - goto cleanup; + if (ret < 0) { + while ((bio = bio_list_pop(&bio_list))) + bio_put(bio); + return ret; + } /* * We have every sector properly prepared. Can finish the scrub @@ -2796,12 +2797,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio) } } return ret; - -cleanup: - while ((bio = bio_list_pop(&bio_list))) - bio_put(bio); - - return ret; } static void scrub_rbio_work_locked(struct work_struct *work)
The bio_list is only filled by scrub_assemble_read_bios when successful, so don't try to walk it and put the bios on any failure before the successful call to recover_assemble_read_bios. Also initialize bio_list at initialization time. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/raid56.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)