| Message ID | 20241104121318.16784-1-jth@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] btrfs: handle bio_split() error | expand |
On 04/11/2024 12:13, Johannes Thumshirn wrote: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Now that bio_split() can return errors, add error handling for it in > btrfs_split_bio() and ultimately btrfs_submit_chunk(). I have a couple of comments, below; However, since I am not familiar with the code, maybe they are invalid. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > > This is based on top of John Garry's series "bio_split() error handling > rework" explicitly on the patch titled "block: Rework bio_split() return > value", which are as of now (Tue Oct 29 10:02:16 2024) not yet merged into > any tree. > > Changes to v2: > - assign the split bbio to a new variable, so we can keep the old error > paths and end the original bbio > > Changes to v1: > - convert ERR_PTR to blk_status_t > - correctly fail already split bbios > --- > fs/btrfs/bio.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c > index 1f216d07eff6..7a0998d0abe3 100644 > --- a/fs/btrfs/bio.c > +++ b/fs/btrfs/bio.c > @@ -81,6 +81,9 @@ static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info, > > bio = bio_split(&orig_bbio->bio, map_length >> SECTOR_SHIFT, GFP_NOFS, > &btrfs_clone_bioset); > + if (IS_ERR(bio)) > + return ERR_CAST(bio); > + > bbio = btrfs_bio(bio); > btrfs_bio_init(bbio, fs_info, NULL, orig_bbio); > bbio->inode = orig_bbio->inode; > @@ -678,7 +681,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) > &bioc, &smap, &mirror_num); > if (error) { > ret = errno_to_blk_status(error); > - goto fail; > + goto end_bbio; eh, I am not sure why this has changed (and we now skip the "fail" label actions) > } > > map_length = min(map_length, length); > @@ -686,7 +689,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) > map_length = btrfs_append_map_length(bbio, map_length); > > if (map_length < length) { > - bbio = btrfs_split_bio(fs_info, bbio, map_length); > + struct btrfs_bio *split; > + > + split = btrfs_split_bio(fs_info, bbio, map_length); > + if (IS_ERR(split)) { > + ret = errno_to_blk_status(PTR_ERR(split)); > + goto end_bbio; Do we need to undo the btrfs_bio_counter_inc() (not shown)? > + } > + bbio = split; > bio = &bbio->bio; > } > > @@ -760,6 +770,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) > > btrfs_bio_end_io(remaining, ret); > } > +end_bbio: > btrfs_bio_end_io(bbio, ret); > /* Do not submit another chunk */ > return true;
On 13.11.24 10:51, John Garry wrote: > On 04/11/2024 12:13, Johannes Thumshirn wrote: >> From: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> >> Now that bio_split() can return errors, add error handling for it in >> btrfs_split_bio() and ultimately btrfs_submit_chunk(). > > I have a couple of comments, below; However, since I am not familiar > with the code, maybe they are invalid. > >> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> >> This is based on top of John Garry's series "bio_split() error handling >> rework" explicitly on the patch titled "block: Rework bio_split() return >> value", which are as of now (Tue Oct 29 10:02:16 2024) not yet merged into >> any tree. >> >> Changes to v2: >> - assign the split bbio to a new variable, so we can keep the old error >> paths and end the original bbio >> >> Changes to v1: >> - convert ERR_PTR to blk_status_t >> - correctly fail already split bbios >> --- >> fs/btrfs/bio.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c >> index 1f216d07eff6..7a0998d0abe3 100644 >> --- a/fs/btrfs/bio.c >> +++ b/fs/btrfs/bio.c >> @@ -81,6 +81,9 @@ static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info, >> >> bio = bio_split(&orig_bbio->bio, map_length >> SECTOR_SHIFT, GFP_NOFS, >> &btrfs_clone_bioset); >> + if (IS_ERR(bio)) >> + return ERR_CAST(bio); >> + >> bbio = btrfs_bio(bio); >> btrfs_bio_init(bbio, fs_info, NULL, orig_bbio); >> bbio->inode = orig_bbio->inode; >> @@ -678,7 +681,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >> &bioc, &smap, &mirror_num); >> if (error) { >> ret = errno_to_blk_status(error); >> - goto fail; >> + goto end_bbio; > > eh, I am not sure why this has changed (and we now skip the "fail" label > actions) Because we want to skip the 'if (map_length < length) {' part below the 'fail' label and directly go to btrfs_bio_end_io(). >> } >> >> map_length = min(map_length, length); >> @@ -686,7 +689,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >> map_length = btrfs_append_map_length(bbio, map_length); >> >> if (map_length < length) { >> - bbio = btrfs_split_bio(fs_info, bbio, map_length); >> + struct btrfs_bio *split; >> + >> + split = btrfs_split_bio(fs_info, bbio, map_length); >> + if (IS_ERR(split)) { >> + ret = errno_to_blk_status(PTR_ERR(split)); >> + goto end_bbio; > > Do we need to undo the btrfs_bio_counter_inc() (not shown)? Yes we do. > >> + } >> + bbio = split; >> bio = &bbio->bio; >> } >> >> @@ -760,6 +770,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >> >> btrfs_bio_end_io(remaining, ret); >> } >> +end_bbio: >> btrfs_bio_end_io(bbio, ret); >> /* Do not submit another chunk */ >> return true; > >
On 13/11/2024 10:08, Johannes Thumshirn wrote: Hi Johannes, Could you kindly repost this patch? Sorry for not paying attention to it further previously. BTW, on a related topic, should we check for a negative error code in btrfs_append_map_length() -> bio_split_rw_at() result? Thanks, John > On 13.11.24 10:51, John Garry wrote: >> On 04/11/2024 12:13, Johannes Thumshirn wrote: >>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> >>> Now that bio_split() can return errors, add error handling for it in >>> btrfs_split_bio() and ultimately btrfs_submit_chunk(). >> >> I have a couple of comments, below; However, since I am not familiar >> with the code, maybe they are invalid. >> >>> >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> --- >>> >>> This is based on top of John Garry's series "bio_split() error handling >>> rework" explicitly on the patch titled "block: Rework bio_split() return >>> value", which are as of now (Tue Oct 29 10:02:16 2024) not yet merged into >>> any tree. >>> >>> Changes to v2: >>> - assign the split bbio to a new variable, so we can keep the old error >>> paths and end the original bbio >>> >>> Changes to v1: >>> - convert ERR_PTR to blk_status_t >>> - correctly fail already split bbios >>> --- >>> fs/btrfs/bio.c | 15 +++++++++++++-- >>> 1 file changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c >>> index 1f216d07eff6..7a0998d0abe3 100644 >>> --- a/fs/btrfs/bio.c >>> +++ b/fs/btrfs/bio.c >>> @@ -81,6 +81,9 @@ static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info, >>> >>> bio = bio_split(&orig_bbio->bio, map_length >> SECTOR_SHIFT, GFP_NOFS, >>> &btrfs_clone_bioset); >>> + if (IS_ERR(bio)) >>> + return ERR_CAST(bio); >>> + >>> bbio = btrfs_bio(bio); >>> btrfs_bio_init(bbio, fs_info, NULL, orig_bbio); >>> bbio->inode = orig_bbio->inode; >>> @@ -678,7 +681,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >>> &bioc, &smap, &mirror_num); >>> if (error) { >>> ret = errno_to_blk_status(error); >>> - goto fail; >>> + goto end_bbio; >> >> eh, I am not sure why this has changed (and we now skip the "fail" label >> actions) > > Because we want to skip the 'if (map_length < length) {' part below the > 'fail' label and directly go to btrfs_bio_end_io(). > > >>> } >>> >>> map_length = min(map_length, length); >>> @@ -686,7 +689,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >>> map_length = btrfs_append_map_length(bbio, map_length); >>> >>> if (map_length < length) { >>> - bbio = btrfs_split_bio(fs_info, bbio, map_length); >>> + struct btrfs_bio *split; >>> + >>> + split = btrfs_split_bio(fs_info, bbio, map_length); >>> + if (IS_ERR(split)) { >>> + ret = errno_to_blk_status(PTR_ERR(split)); >>> + goto end_bbio; >> >> Do we need to undo the btrfs_bio_counter_inc() (not shown)? > > Yes we do. > >> >>> + } >>> + bbio = split; >>> bio = &bbio->bio; >>> } >>> >>> @@ -760,6 +770,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) >>> >>> btrfs_bio_end_io(remaining, ret); >>> } >>> +end_bbio: >>> btrfs_bio_end_io(bbio, ret); >>> /* Do not submit another chunk */ >>> return true; >> >> >
diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c index 1f216d07eff6..7a0998d0abe3 100644 --- a/fs/btrfs/bio.c +++ b/fs/btrfs/bio.c @@ -81,6 +81,9 @@ static struct btrfs_bio *btrfs_split_bio(struct btrfs_fs_info *fs_info, bio = bio_split(&orig_bbio->bio, map_length >> SECTOR_SHIFT, GFP_NOFS, &btrfs_clone_bioset); + if (IS_ERR(bio)) + return ERR_CAST(bio); + bbio = btrfs_bio(bio); btrfs_bio_init(bbio, fs_info, NULL, orig_bbio); bbio->inode = orig_bbio->inode; @@ -678,7 +681,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) &bioc, &smap, &mirror_num); if (error) { ret = errno_to_blk_status(error); - goto fail; + goto end_bbio; } map_length = min(map_length, length); @@ -686,7 +689,14 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) map_length = btrfs_append_map_length(bbio, map_length); if (map_length < length) { - bbio = btrfs_split_bio(fs_info, bbio, map_length); + struct btrfs_bio *split; + + split = btrfs_split_bio(fs_info, bbio, map_length); + if (IS_ERR(split)) { + ret = errno_to_blk_status(PTR_ERR(split)); + goto end_bbio; + } + bbio = split; bio = &bbio->bio; } @@ -760,6 +770,7 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num) btrfs_bio_end_io(remaining, ret); } +end_bbio: btrfs_bio_end_io(bbio, ret); /* Do not submit another chunk */ return true;