Re: [PATCH] dm-verity: all verity devices share the same dm_io_client and recheck_pool.

LongPing Wei <weilongping@xxxxxxxx> · Thu, 24 Apr 2025 16:35:59 +0800

Hi, Mikulas

Thanks for your suggestion. I will test it.

LongPing

On 2025/4/24 2:46, Mikulas Patocka wrote:
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 <weilongping@xxxxxxxx>
---
  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










