Re: [dm-devel] [PATCH v5 16/20] dm-crypt: check if adding pages to clone bio fails

[Mikulas Patocka]

On Tue, 2 May 2023, Johannes Thumshirn 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().
> 
> This way we can mark bio_add_pages as __must_check.
> 
> Reviewed-by: Damien Le Moal <damien.lem...@opensource.wdc.com>
> Signed-off-by: Johannes Thumshirn <johannes.thumsh...@wdc.com>
> ---
>  drivers/md/dm-crypt.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 8b47b913ee83..b234dc089cee 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1693,7 +1693,14 @@ 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)) {
> +                     mempool_free(page, &cc->page_pool);
> +                     crypt_free_buffer_pages(cc, clone);
> +                     bio_put(clone);
> +                     gfp_mask |= __GFP_DIRECT_RECLAIM;
> +                     goto retry;
> +
> +             }
>  
>               remaining_size -= len;
>       }

Hi

I nack this. This just adds code that can't ever be executed.

dm-crypt already allocates enough entries in the vector (see "unsigned int 
nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;"), so bio_add_page can't 
fail.

If you want to add __must_check to bio_add_page, you should change the 
dm-crypt code to:
if (!bio_add_page(clone, page, len, 0)) {
        WARN(1, "this can't happen");
        return NULL;
}
and not write recovery code for a can't-happen case.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel