[v6,15/20] md: raid1: check if adding pages to resync bio fails

Commit Message

Johannes Thumshirn May 30, 2023, 3:49 p.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.

Reviewed-by: Damien Le Moal <[e-mail]>
Acked-by: Song Liu <[e-mail]>
Signed-off-by: Johannes Thumshirn <[e-mail]>
---
 drivers/md/raid1-10.c | 11 ++++++-----
 drivers/md/raid10.c   | 20 ++++++++++----------
 2 files changed, 16 insertions(+), 15 deletions(-)

Comments

Christoph Hellwig May 31, 2023, 4:25 a.m. UTC | #1

To me these look like __bio_add_page candidates, but I guess Song
preferred it this way?  It'll add a bit pointless boilerplate code,
but I'm ok with that.

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Song Liu May 31, 2023, 4:58 a.m. UTC | #2

On Tue, May 30, 2023 at 9:25 PM Christoph Hellwig <[e-mail]> wrote:
>
> To me these look like __bio_add_page candidates, but I guess Song
> preferred it this way?  It'll add a bit pointless boilerplate code,
> but I'm ok with that.

We had some discussion on this in v2, and decided to keep these
assert-like WARN_ON().

Thanks,
Song

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Paul Menzel May 31, 2023, 7:54 a.m. UTC | #3

Dear Johannes,


Thank you for your patches.

Am 31.05.23 um 06:58 schrieb Song Liu:
> On Tue, May 30, 2023 at 9:25 PM Christoph Hellwig <[e-mail]> wrote:
>>
>> To me these look like __bio_add_page candidates, but I guess Song
>> preferred it this way?  It'll add a bit pointless boilerplate code,
>> but I'm ok with that.
> 
> We had some discussion on this in v2, and decided to keep these
> assert-like WARN_ON().

it’d be great if you added a summary/note of the discussion to the 
commit message.


Kind regards,

Paul

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index e61f6cad4e08..cd349e69ed77 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -101,11 +101,12 @@  static void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp,
 		struct page *page = resync_fetch_page(rp, idx);
 		int len = min_t(int, size, PAGE_SIZE);
 
-		/*
-		 * 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 4fcfcb350d2b..381c21f7fb06 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3819,11 +3819,11 @@  static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 		for (bio= biolist ; bio ; bio=bio->bi_next) {
 			struct resync_pages *rp = get_resync_pages(bio);
 			page = resync_fetch_page(rp, page_idx);
-			/*
-			 * 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;
@@ -4997,11 +4997,11 @@  static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 		if (len > PAGE_SIZE)
 			len = PAGE_SIZE;
 		for (bio = blist; bio ; bio = bio->bi_next) {
-			/*
-			 * 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;
+			}
 		}
 		sector_nr += len >> 9;
 		nr_sectors += len >> 9;