[v6,16/20] dm-crypt: check if adding pages to clone bio fails

Commit Message

Johannes Thumshirn May 30, 2023, 3:49 p.m. UTC

Check if adding pages to clone bio fails and if it does retry with
reclaim. This mirrors the behaviour of page allocation in
crypt_alloc_buffer().

This way we can mark bio_add_pages as __must_check.

Reviewed-by: Damien Le Moal <[e-mail]>
Signed-off-by: Johannes Thumshirn <[e-mail]>
---
 drivers/md/dm-crypt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Mike Snitzer May 30, 2023, 4:10 p.m. UTC | #1

On Tue, May 30 2023 at 11:49P -0400,
Johannes Thumshirn <[e-mail]> wrote:

> Check if adding pages to clone bio fails and if it does retry with
> reclaim. This mirrors the behaviour of page allocation in
> crypt_alloc_buffer().

Nope.

> This way we can mark bio_add_pages as __must_check.
> 
> Reviewed-by: Damien Le Moal <[e-mail]>
> Signed-off-by: Johannes Thumshirn <[e-mail]>

The above patch header doesn't reflect the code.

I also think __bio_add_page should be used, like my racey reply to
Mikulas vs your v6 patchbomb said, please see:
https://listman.redhat.com/archives/dm-devel/2023-May/054388.html

Thanks,
Mike

> ---
>  drivers/md/dm-crypt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 8b47b913ee83..0dd231e61757 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1693,7 +1693,10 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  
>  		len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;
>  
> -		bio_add_page(clone, page, len, 0);
> +		if (!bio_add_page(clone, page, len, 0)) {
> +			WARN_ONCE(1, "Adding page to bio failed\n");
> +			return NULL;
> +		}
>  
>  		remaining_size -= len;
>  	}
> -- 
> 2.40.1
> 
> --
> dm-devel mailing list
> [e-mail]
> https://listman.redhat.com/mailman/listinfo/dm-devel
> 

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

Johannes Thumshirn May 30, 2023, 4:20 p.m. UTC | #2

On 30.05.23 18:10, Mike Snitzer wrote:
> On Tue, May 30 2023 at 11:49P -0400,
> Johannes Thumshirn <[e-mail]> wrote:
> 
>> Check if adding pages to clone bio fails and if it does retry with
>> reclaim. This mirrors the behaviour of page allocation in
>> crypt_alloc_buffer().
> 
> Nope.
> 
>> This way we can mark bio_add_pages as __must_check.
>>
>> Reviewed-by: Damien Le Moal <[e-mail]>
>> Signed-off-by: Johannes Thumshirn <[e-mail]>
> 
> The above patch header doesn't reflect the code.
> 
> I also think __bio_add_page should be used, like my racey reply to
> Mikulas vs your v6 patchbomb said, please see:
> https://listman.redhat.com/archives/dm-devel/2023-May/054388.html

Yep that mail was racing with my send of v6.

I can send out a v7 of the series tomorrow or just that one patch updated.
Whatever Jens preferes.

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

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8b47b913ee83..0dd231e61757 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1693,7 +1693,10 @@  static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 
 		len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;
 
-		bio_add_page(clone, page, len, 0);
+		if (!bio_add_page(clone, page, len, 0)) {
+			WARN_ONCE(1, "Adding page to bio failed\n");
+			return NULL;
+		}
 
 		remaining_size -= len;
 	}