Re: [Cluster-devel] [PATCH v6 15/20] md: raid1: check if adding pages to resync bio fails
On Tue, May 30, 2023 at 9:25 PM Christoph Hellwig 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
Re: [Cluster-devel] [PATCH v6 13/20] md: check for failure when adding pages in alloc_behind_master_bio
On Tue, May 30, 2023 at 8:50 AM Johannes Thumshirn
wrote:
>
> alloc_behind_master_bio() can possibly add multiple pages to a bio, but it
> is not checking for the return value of bio_add_page() if adding really
> succeeded.
>
> Check if the page adding succeeded and if not bail out.
>
> Reviewed-by: Damien Le Moal
> Signed-off-by: Johannes Thumshirn
Acked-by: Song Liu
> ---
> drivers/md/raid1.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 68a9e2d9985b..8283ef177f6c 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1147,7 +1147,10 @@ static void alloc_behind_master_bio(struct r1bio
> *r1_bio,
> if (unlikely(!page))
> goto free_pages;
>
> - bio_add_page(behind_bio, page, len, 0);
> + if (!bio_add_page(behind_bio, page, len, 0)) {
> + free_page(page);
> + goto free_pages;
> + }
>
> size -= len;
> i++;
> --
> 2.40.1
>
Re: [Cluster-devel] [PATCH v3 17/19] md: raid1: check if adding pages to resync bio fails
On Wed, Apr 19, 2023 at 7:11 AM Johannes Thumshirn wrote: > > From: Johannes Thumshirn > > 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 > Reviewed-by: Damien Le Moal Acked-by: Song Liu Thanks!
Re: [Cluster-devel] [PATCH v2 17/19] md: raid1: check if adding pages to resync bio fails
On Tue, Apr 4, 2023 at 1:26 AM Johannes Thumshirn wrote: > > On 31/03/2023 20:13, Song Liu wrote: > > On Thu, Mar 30, 2023 at 3:44 AM 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 > >> Reviewed-by: Damien Le Moal > >> --- > >> 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
Re: [Cluster-devel] [PATCH v2 17/19] md: raid1: check if adding pages to resync bio fails
On Thu, Mar 30, 2023 at 3:44 AM 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
> Reviewed-by: Damien Le Moal
> ---
> 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
>
Re: [Cluster-devel] [PATCH v2 16/19] md: raid1: use __bio_add_page for adding single page to bio
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn wrote: > > The sync request code uses bio_add_page() to add a page to a newly created > bio. > bio_add_page() can fail, but the return value is never checked. > > Use __bio_add_page() as adding a single page to a newly created bio is > guaranteed to succeed. > > This brings us a step closer to marking bio_add_page() as __must_check. > > Signed-off-by: Johannes Thumshirn > Reviewed-by: Damien Le Moal Acked-by: Song Liu Thanks!
Re: [Cluster-devel] [PATCH v2 07/19] md: raid5: use __bio_add_page to add single page to new bio
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn wrote: > > The raid5-ppl submission code uses bio_add_page() to add a page to a > newly created bio. bio_add_page() can fail, but the return value is never > checked. For adding consecutive pages, the return is actually checked and > a new bio is allocated if adding the page fails. > > Use __bio_add_page() as adding a single page to a newly created bio is > guaranteed to succeed. > > This brings us a step closer to marking bio_add_page() as __must_check. > > Signed-off-by: Johannes Thumshirn > Reviewed-by: Damien Le Moal Acked-by: Song Liu
Re: [Cluster-devel] [PATCH v2 06/19] md: raid5-log: use __bio_add_page to add single page
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn wrote: > > The raid5 log metadata submission code uses bio_add_page() to add a page > to a newly created bio. bio_add_page() can fail, but the return value is > never checked. > > Use __bio_add_page() as adding a single page to a newly created bio is > guaranteed to succeed. > > This brings us a step closer to marking bio_add_page() as __must_check. > > Signed-off-by: Johannes Thumshirn > Reviewed-by: Damien Le Moal Acked-by: Song Liu
Re: [Cluster-devel] [PATCH v2 05/19] md: use __bio_add_page to add single page
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn wrote: > > The md-raid superblock writing code uses bio_add_page() to add a page to a > newly created bio. bio_add_page() can fail, but the return value is never > checked. > > Use __bio_add_page() as adding a single page to a newly created bio is > guaranteed to succeed. > > This brings us a step closer to marking bio_add_page() as __must_check. > > Signed-of_-by: Johannes Thumshirn > Reviewed-by: Damien Le Moal Acked-by: Song Liu
Re: [Cluster-devel] [PATCH v2 15/19] md: check for failure when adding pages in alloc_behind_master_bio
On Thu, Mar 30, 2023 at 3:44 AM Johannes Thumshirn wrote: > > alloc_behind_master_bio() can possibly add multiple pages to a bio, but it > is not checking for the return value of bio_add_page() if adding really > succeeded. > > Check if the page adding succeeded and if not bail out. > > Signed-off-by: Johannes Thumshirn > Reviewed-by: Damien Le Moal > --- > drivers/md/raid1.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index 68a9e2d9985b..bd7c339a84a1 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -1147,7 +1147,8 @@ static void alloc_behind_master_bio(struct r1bio > *r1_bio, > if (unlikely(!page)) > goto free_pages; > > - bio_add_page(behind_bio, page, len, 0); > + if (!bio_add_page(behind_bio, page, len, 0)) > + goto free_pages; We will leak page here, no? Thanks, Song
