| Message ID | e2f96e539befa4f9d57f19ff1fc26cfc0d109435.1680108414.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | bio: check return values of bio_add_page | expand |
On 3/30/23 02:06, Johannes Thumshirn 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> > --- > 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 > */ > - bio_add_page(bio, page, len, 0); > + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { Not sure we really need the WARN_ON here... Nevertheless, Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > + 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;
On 30.03.23 01:44, Damien Le Moal wrote: > On 3/30/23 02:06, Johannes Thumshirn 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> >> --- >> 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 >> */ >> - bio_add_page(bio, page, len, 0); >> + if (WARN_ON(!bio_add_page(bio, page, len, 0))) { > Not sure we really need the WARN_ON here... > Nevertheless, > I see it as a kind of assert(). It shouldn't fail but in theory it can. > Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> Thanks -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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;
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> --- drivers/md/raid1-10.c | 7 ++++++- drivers/md/raid10.c | 12 ++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-)