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

From: Song Liu <song@kernel.org>
To: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Cc: Jens Axboe <axboe@kernel.dk>, Christoph Hellwig <hch@lst.de>,
	Hannes Reinecke <hare@suse.de>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Damien Le Moal <damien.lemoal@wdc.com>,
	Ming Lei <ming.lei@redhat.com>,
	linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org, dm-devel@redhat.com,
	linux-raid@vger.kernel.org, Mike Snitzer <snitzer@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Dave Kleikamp <shaggy@kernel.org>,
	jfs-discussion@lists.sourceforge.net, cluster-devel@redhat.com,
	Bob Peterson <rpeterso@redhat.com>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org,
	Damien Le Moal <damien.lemoal@opensource.wdc.com>
Subject: Re: [PATCH v2 17/19] md: raid1: check if adding pages to resync bio fails
Date: Fri, 31 Mar 2023 11:13:51 -0700	[thread overview]
Message-ID: <CAPhsuW7a+mpn+VprfA2mC5Fc+M9BFq8i6d-y+-o5G1u5dOsk2Q@mail.gmail.com> (raw)
In-Reply-To: <8b8a3bb2db8c5183ef36c1810f2ac776ac526327.1680172791.git.johannes.thumshirn@wdc.com>

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
>