| Message ID | 8b8a3bb2db8c5183ef36c1810f2ac776ac526327.1680172791.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Song Liu |
| Headers | show |
| Series | bio: check return values of bio_add_page | expand |
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn <johannes.thumshirn@wdc.com> wrote: > > Check if adding pages to resync bio fails and if bail out. > > As the comment above suggests this cannot happen, WARN if it actually > happens. > > This way we can mark bio_add_pages as __must_check. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/md/raid1-10.c | 7 ++++++- > drivers/md/raid10.c | 12 ++++++++++-- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c > index e61f6cad4e08..c21b6c168751 100644 > --- a/drivers/md/raid1-10.c > +++ b/drivers/md/raid1-10.c > @@ -105,7 +105,12 @@ static void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > * won't fail because the vec table is big > * enough to hold all these pages > */ We know these won't fail. Shall we just use __bio_add_page? Thanks, Song > - bio_add_page(bio, page, len, 0); > + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { > + bio->bi_status = BLK_STS_RESOURCE; > + bio_endio(bio); > + return; > + } > + > size -= len; > } while (idx++ < RESYNC_PAGES && size > 0); > } > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 6c66357f92f5..5682dba52fd3 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -3808,7 +3808,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr, > * won't fail because the vec table is big enough > * to hold all these pages > */ > - bio_add_page(bio, page, len, 0); > + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { > + bio->bi_status = BLK_STS_RESOURCE; > + bio_endio(bio); > + goto giveup; > + } > } > nr_sectors += len>>9; > sector_nr += len>>9; > @@ -4989,7 +4993,11 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, > * won't fail because the vec table is big enough > * to hold all these pages > */ > - bio_add_page(bio, page, len, 0); > + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { > + bio->bi_status = BLK_STS_RESOURCE; > + bio_endio(bio); > + return sectors_done; /* XXX: is this correct? */ > + } > } > sector_nr += len >> 9; > nr_sectors += len >> 9; > -- > 2.39.2 >
On 31/03/2023 20:13, Song Liu wrote: > On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn > <johannes.thumshirn@wdc.com> wrote: >> >> Check if adding pages to resync bio fails and if bail out. >> >> As the comment above suggests this cannot happen, WARN if it actually >> happens. >> >> This way we can mark bio_add_pages as __must_check. >> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/md/raid1-10.c | 7 ++++++- >> drivers/md/raid10.c | 12 ++++++++++-- >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c >> index e61f6cad4e08..c21b6c168751 100644 >> --- a/drivers/md/raid1-10.c >> +++ b/drivers/md/raid1-10.c >> @@ -105,7 +105,12 @@ static void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, >> * won't fail because the vec table is big >> * enough to hold all these pages >> */ > > We know these won't fail. Shall we just use __bio_add_page? We could yes, but I kind of like the assert() style warning. But of cause ultimately your call. Byte, Johannes
On Tue, Apr 4, 2023 at 1:26 AM Johannes Thumshirn <jth@kernel.org> wrote: > > On 31/03/2023 20:13, Song Liu wrote: > > On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn > > <johannes.thumshirn@wdc.com> wrote: > >> > >> Check if adding pages to resync bio fails and if bail out. > >> > >> As the comment above suggests this cannot happen, WARN if it actually > >> happens. > >> > >> This way we can mark bio_add_pages as __must_check. > >> > >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > >> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > >> --- > >> drivers/md/raid1-10.c | 7 ++++++- > >> drivers/md/raid10.c | 12 ++++++++++-- > >> 2 files changed, 16 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c > >> index e61f6cad4e08..c21b6c168751 100644 > >> --- a/drivers/md/raid1-10.c > >> +++ b/drivers/md/raid1-10.c > >> @@ -105,7 +105,12 @@ static void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > >> * won't fail because the vec table is big > >> * enough to hold all these pages > >> */ > > > > We know these won't fail. Shall we just use __bio_add_page? > > We could yes, but I kind of like the assert() style warning. > But of cause ultimately your call. The assert() style warning is fine. In this case, please remove the "won't fail ..." comments. Thanks, Song
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c index e61f6cad4e08..c21b6c168751 100644 --- a/drivers/md/raid1-10.c +++ b/drivers/md/raid1-10.c @@ -105,7 +105,12 @@ static void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, * won't fail because the vec table is big * enough to hold all these pages */ - bio_add_page(bio, page, len, 0); + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { + bio->bi_status = BLK_STS_RESOURCE; + bio_endio(bio); + return; + } + size -= len; } while (idx++ < RESYNC_PAGES && size > 0); } diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 6c66357f92f5..5682dba52fd3 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -3808,7 +3808,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr, * won't fail because the vec table is big enough * to hold all these pages */ - bio_add_page(bio, page, len, 0); + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { + bio->bi_status = BLK_STS_RESOURCE; + bio_endio(bio); + goto giveup; + } } nr_sectors += len>>9; sector_nr += len>>9; @@ -4989,7 +4993,11 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, * won't fail because the vec table is big enough * to hold all these pages */ - bio_add_page(bio, page, len, 0); + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { + bio->bi_status = BLK_STS_RESOURCE; + bio_endio(bio); + return sectors_done; /* XXX: is this correct? */ + } } sector_nr += len >> 9; nr_sectors += len >> 9;