Hi
I nack this series of patches, because sharing mempools (or dm_io_client)
is prone to deadlocks.
If you have two dm-verity devices stacked on each other, the upper device
may deplete the mempool and the lower device has no way to do forward
progress, resulting in a deadlock.
For example, suppose that you have dm-snapshot on the top of dm-verity on
the top of another dm-verity device. If the kernel needs to write back
dirty memory, it calls the dm-snapshot target, the dm-snapshot reads data
from the upper dm-verity device, the upper dm-verity device depletes the
shared mempool and the lower dm-verity device has no way to make forward
progress - deadlock.
If you want to reduce memory consumption, set reserved_bio_based_ios to 1,
that should be safe. (setting it to 0 is prone to deadlock).
I remember that the one of the first things that I did at Red Hat 17 years
ago was removing shared resources from device mapper and making them
per-device :-)
Mikulas
On Wed, 23 Apr 2025, LongPing Wei wrote:
> The size of biovec's memory of one dm_io_client will be
> reserved_bio_based_ios(default value is 16) * 4KiB memory.
> When the number of dm-verity devices is dozens,
> the total memory size will be several MiBs.
>
> The number of dm-verity devices is 59 on my test device,
> and the size of biovec's memory here will be 3.87MiB.
>
> This commit will reduce the memory usage of dm-verity
> by making all verity devices share the same dm_io_client and recheck_pool.
>
> Fixes: 9177f3c0dea61 ("dm-verity: recheck the hash after a failure")
>
> Signed-off-by: LongPing Wei <[email protected]>
> ---
> drivers/md/dm-verity-target.c | 71 ++++++++++++++++++++++++-----------
> drivers/md/dm-verity.h | 3 --
> 2 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 3c427f18a04b..986c3386aa59 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -72,6 +72,9 @@ struct dm_verity_prefetch_work {
> unsigned int n_blocks;
> };
>
> +static struct dm_io_client *verity_io_client;
> +static mempool_t verity_recheck_pool;
> +
> /*
> * Auxiliary structure appended to each dm-bufio buffer. If the value
> * hash_verified is nonzero, hash of the block has been verified.
> @@ -449,14 +452,14 @@ static noinline int verity_recheck(struct dm_verity *v,
> struct dm_verity_io *io,
> struct dm_io_request io_req;
> struct dm_io_region io_loc;
>
> - page = mempool_alloc(&v->recheck_pool, GFP_NOIO);
> + page = mempool_alloc(&verity_recheck_pool, GFP_NOIO);
> buffer = page_to_virt(page);
>
> io_req.bi_opf = REQ_OP_READ;
> io_req.mem.type = DM_IO_KMEM;
> io_req.mem.ptr.addr = buffer;
> io_req.notify.fn = NULL;
> - io_req.client = v->io;
> + io_req.client = verity_io_client;
> io_loc.bdev = v->data_dev->bdev;
> io_loc.sector = cur_block << (v->data_dev_block_bits - SECTOR_SHIFT);
> io_loc.count = 1 << (v->data_dev_block_bits - SECTOR_SHIFT);
> @@ -478,7 +481,7 @@ static noinline int verity_recheck(struct dm_verity *v,
> struct dm_verity_io *io,
> memcpy(dest, buffer, 1 << v->data_dev_block_bits);
> r = 0;
> free_ret:
> - mempool_free(page, &v->recheck_pool);
> + mempool_free(page, &verity_recheck_pool);
>
> return r;
> }
> @@ -1075,10 +1078,6 @@ static void verity_dtr(struct dm_target *ti)
> if (v->verify_wq)
> destroy_workqueue(v->verify_wq);
>
> - mempool_exit(&v->recheck_pool);
> - if (v->io)
> - dm_io_client_destroy(v->io);
> -
> if (v->bufio)
> dm_bufio_client_destroy(v->bufio);
>
> @@ -1625,20 +1624,6 @@ static int verity_ctr(struct dm_target *ti, unsigned
> int argc, char **argv)
> }
> v->hash_blocks = hash_position;
>
> - r = mempool_init_page_pool(&v->recheck_pool, 1, 0);
> - if (unlikely(r)) {
> - ti->error = "Cannot allocate mempool";
> - goto bad;
> - }
> -
> - v->io = dm_io_client_create();
> - if (IS_ERR(v->io)) {
> - r = PTR_ERR(v->io);
> - v->io = NULL;
> - ti->error = "Cannot allocate dm io";
> - goto bad;
> - }
> -
> v->bufio = dm_bufio_client_create(v->hash_dev->bdev,
> 1 << v->hash_dev_block_bits, 1, sizeof(struct buffer_aux),
> dm_bufio_alloc_callback, NULL,
> @@ -1822,7 +1807,49 @@ static struct target_type verity_target = {
> .preresume = verity_preresume,
> #endif /* CONFIG_SECURITY */
> };
> -module_dm(verity);
> +
> +static int __init dm_verity_init(void)
> +{
> + int r;
> +
> + r = mempool_init_page_pool(&verity_recheck_pool, 1, 0);
> + if (unlikely(r)) {
> + DMERR("Cannot allocate mempool");
> + goto mempool_init_page_pool_failed;
> + }
> +
> + verity_io_client = dm_io_client_create();
> + if (IS_ERR(verity_io_client)) {
> + r = PTR_ERR(verity_io_client);
> + verity_io_client = NULL;
> + DMERR("Cannot allocate dm io");
> + goto dm_io_client_create_failed;
> + }
> +
> + r = dm_register_target(&(verity_target));
> + if (unlikely(r))
> + goto dm_register_target_failed;
> + return r;
> +
> +dm_register_target_failed:
> + dm_io_client_destroy(verity_io_client);
> + verity_io_client = NULL;
> +dm_io_client_create_failed:
> + mempool_exit(&verity_recheck_pool);
> +mempool_init_page_pool_failed:
> + return r;
> +}
> +module_init(dm_verity_init);
> +
> +static void __exit dm_verity_exit(void)
> +{
> + dm_unregister_target(&(verity_target));
> + mempool_exit(&verity_recheck_pool);
> + if (verity_io_client)
> + dm_io_client_destroy(verity_io_client);
> + verity_io_client = NULL;
> +}
> +module_exit(dm_verity_exit)
>
> /*
> * Check whether a DM target is a verity target.
> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
> index 8cbb57862ae1..f28454f81249 100644
> --- a/drivers/md/dm-verity.h
> +++ b/drivers/md/dm-verity.h
> @@ -75,9 +75,6 @@ struct dm_verity {
> unsigned long *validated_blocks; /* bitset blocks validated */
>
> char *signature_key_desc; /* signature keyring reference */
> -
> - struct dm_io_client *io;
> - mempool_t recheck_pool;
> };
>
> struct dm_verity_io {
> --
> 2.34.1
>
