diff mbox series

[v2,17/19] md: raid1: check if adding pages to resync bio fails

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

Commit Message

Johannes Thumshirn March 30, 2023, 10:43 a.m. UTC
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(-)

Comments

Song Liu March 31, 2023, 6:13 p.m. UTC | #1
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
>
Johannes Thumshirn April 4, 2023, 8:26 a.m. UTC | #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
Song Liu April 10, 2023, 4:06 p.m. UTC | #3
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 mbox series

Patch

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;