On Fri, Jul 22 2022 at 5:38P -0400,
Nathan Huckleberry <nh...@google.com> wrote:
> Using tasklets for disk verification can reduce IO latency. When there are
> accelerated hash instructions it is often better to compute the hash
> immediately
> using a tasklet rather than deferring verification to a work-queue. This
> reduces time spent waiting to schedule work-queue jobs, but requires spending
> slightly more time in interrupt context.
>
> If the dm-bufio cache does not have the required hashes we fallback to
> the work-queue implementation.
>
> The following shows a speed comparison of random reads on a dm-verity device.
> The dm-verity device uses a 1G ramdisk for data and a 1G ramdisk for hashes.
> One test was run using tasklets and one test was run using the existing
> work-queue solution. Both tests were run when the dm-bufio cache was hot.
> The
> tasklet implementation performs significantly better since there is no time
> waiting for work-queue jobs to be scheduled.
>
> # /data/fio /data/tasklet.fio | grep READ
> READ: bw=181MiB/s (190MB/s), 181MiB/s-181MiB/s (190MB/s-190MB/s), io=512MiB
> (537MB), run=2827-2827msec
> # /data/fio /data/workqueue.fio | grep READ
> READ: bw=23.6MiB/s (24.8MB/s), 23.6MiB/s-23.6MiB/s (24.8MB/s-24.8MB/s),
> io=512MiB (537MB), run=21688-21688msec
>
> Signed-off-by: Nathan Huckleberry <nh...@google.com>
> ---
> drivers/md/dm-bufio.c | 14 +++---
> drivers/md/dm-verity-target.c | 87 ++++++++++++++++++++++++++++++-----
> drivers/md/dm-verity.h | 5 ++
> 3 files changed, 87 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 3edeca7cfca6..039f39c29594 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -92,7 +92,7 @@ struct dm_bufio_client {
> s8 sectors_per_block_bits;
> void (*alloc_callback)(struct dm_buffer *);
> void (*write_callback)(struct dm_buffer *);
> - bool may_sleep;
> + bool get_may_sleep;
>
> struct kmem_cache *slab_buffer;
> struct kmem_cache *slab_cache;
> @@ -170,7 +170,7 @@ struct dm_buffer {
>
> static void dm_bufio_lock(struct dm_bufio_client *c)
> {
> - if (c->may_sleep)
> + if (c->get_may_sleep)
> mutex_lock_nested(&c->lock, dm_bufio_in_request());
> else
> spin_lock_irqsave_nested(&c->spinlock, c->spinlock_flags,
> dm_bufio_in_request());
> @@ -178,7 +178,7 @@ static void dm_bufio_lock(struct dm_bufio_client *c)
>
> static int dm_bufio_trylock(struct dm_bufio_client *c)
> {
> - if (c->may_sleep)
> + if (c->get_may_sleep)
> return mutex_trylock(&c->lock);
> else
> return spin_trylock_irqsave(&c->spinlock, c->spinlock_flags);
> @@ -186,7 +186,7 @@ static int dm_bufio_trylock(struct dm_bufio_client *c)
>
> static void dm_bufio_unlock(struct dm_bufio_client *c)
> {
> - if (c->may_sleep)
> + if (c->get_may_sleep)
> mutex_unlock(&c->lock);
> else
> spin_unlock_irqrestore(&c->spinlock, c->spinlock_flags);
> @@ -890,7 +890,7 @@ static struct dm_buffer
> *__alloc_buffer_wait_no_callback(struct dm_bufio_client
> * be allocated.
> */
> while (1) {
> - if (dm_bufio_cache_size_latch != 1 && c->may_sleep) {
> + if (dm_bufio_cache_size_latch != 1 && c->get_may_sleep) {
> b = alloc_buffer(c, GFP_NOWAIT | __GFP_NORETRY |
> __GFP_NOMEMALLOC | __GFP_NOWARN);
> if (b)
> return b;
> @@ -1761,9 +1761,9 @@ struct dm_bufio_client *dm_bufio_client_create(struct
> block_device *bdev, unsign
> c->alloc_callback = alloc_callback;
> c->write_callback = write_callback;
>
> - c->may_sleep = true;
> + c->get_may_sleep = true;
> if (flags & DM_BUFIO_GET_CANT_SLEEP)
> - c->may_sleep = false;
> + c->get_may_sleep = false;
>
> for (i = 0; i < LIST_SIZE; i++) {
> INIT_LIST_HEAD(&c->lru[i]);
I ended up inverting the logic and went with the name "no_sleep", see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.20&id=fa9b59cc264f350c1e34ea784ac4c12fcee1aed1
My reasoning is that: the bufio change has broader implications than
just the _get method. So I expanded the scope of the naming to reflect
that the locking and buffers allocations will not sleep if
DM_BUFIO_CLIENT_NO_SLEEP is set.
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 5d3fc58a3c34..e4d1b674a278 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -34,6 +34,7 @@
> #define DM_VERITY_OPT_PANIC "panic_on_corruption"
> #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
> #define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
> +#define DM_VERITY_OPT_TASKLET_VERIFY "try_verify_in_tasklet"
>
> #define DM_VERITY_OPTS_MAX (3 + DM_VERITY_OPTS_FEC + \
> DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
> @@ -286,7 +287,20 @@ static int verity_verify_level(struct dm_verity *v,
> struct dm_verity_io *io,
>
> verity_hash_at_level(v, block, level, &hash_block, &offset);
>
> - data = dm_bufio_read(v->bufio, hash_block, &buf);
> + if (io->in_tasklet)
> + data = dm_bufio_get(v->bufio, hash_block, &buf);
> + else
> + data = dm_bufio_read(v->bufio, hash_block, &buf);
> +
> + if (data == NULL) {
> + /*
> + * We're running as a tasklet and the hash was not in the bufio
> + * cache. Return early and resume execution from a work-queue
> + * so that we can read the hash from disk.
> + */
> + return -EAGAIN;
> + }
> +
> if (IS_ERR(data))
> return PTR_ERR(data);
>
I tweaked various things in the dm-verity-target.c portion of this
patch. Ranging from whitespace and style tweaks to code flow tweaks.
I'll include an incremental patch at the end of this email.
<snip>
> @@ -1156,7 +1217,7 @@ static int verity_ctr(struct dm_target *ti, unsigned
> argc, char **argv)
> goto bad;
> }
>
> - v->tfm = crypto_alloc_ahash(v->alg_name, 0, 0);
> + v->tfm = crypto_alloc_ahash(v->alg_name, 0, CRYPTO_ALG_ASYNC);
> if (IS_ERR(v->tfm)) {
> ti->error = "Cannot initialize hash function";
> r = PTR_ERR(v->tfm);
This hunk that adds the CRYPTO_ALG_ASYNC flag _seems_ unrelated.
> @@ -1281,7 +1343,8 @@ static int verity_ctr(struct dm_target *ti, unsigned
> argc, char **argv)
> }
>
> /* WQ_UNBOUND greatly improves performance when running on ramdisk */
> - v->verify_wq = alloc_workqueue("kverityd", WQ_CPU_INTENSIVE |
> WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus());
> + v->verify_wq = alloc_workqueue("kverityd", WQ_HIGHPRI | WQ_MEM_RECLAIM |
> + WQ_UNBOUND,
> num_online_cpus());
> if (!v->verify_wq) {
> ti->error = "Cannot allocate workqueue";
> r = -ENOMEM;
Likewise, the removal of WQ_CPU_INTENSIVE _seems_ unrelated to this
patch. If you'd like these 2 changes made please send an incremental
patch with a header that explains these changes.
Here is an earlier incremental patch that I folded into this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.20&id=a5beaa11a1a225860fdf9ae80f0bd54bf9125c4c
(Also, if the performance you quoted in the commit header depends on
the 2 above flags changes (which I dropped) please let me know.)
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 73215a4baf1f..356a11f36100 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -221,7 +221,7 @@ static int verity_handle_err(struct dm_verity *v, enum
verity_block_type type,
struct mapped_device *md = dm_table_get_md(v->ti->table);
/* Corruption should be visible in device status in all modes */
- v->hash_failed = 1;
+ v->hash_failed = true;
if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
goto out;
@@ -287,20 +287,19 @@ static int verity_verify_level(struct dm_verity *v,
struct dm_verity_io *io,
verity_hash_at_level(v, block, level, &hash_block, &offset);
- if (io->in_tasklet)
+ if (io->in_tasklet) {
data = dm_bufio_get(v->bufio, hash_block, &buf);
- else
+ if (data == NULL) {
+ /*
+ * In tasklet and the hash was not in the bufio cache.
+ * Return early and resume execution from a work-queue
+ * to read the hash from disk.
+ */
+ return -EAGAIN;
+ }
+ } else
data = dm_bufio_read(v->bufio, hash_block, &buf);
- if (data == NULL) {
- /*
- * We're running as a tasklet and the hash was not in the bufio
- * cache. Return early and resume execution from a work-queue
- * so that we can read the hash from disk.
- */
- return -EAGAIN;
- }
-
if (IS_ERR(data))
return PTR_ERR(data);
@@ -321,14 +320,12 @@ static int verity_verify_level(struct dm_verity *v,
struct dm_verity_io *io,
if (likely(memcmp(verity_io_real_digest(v, io), want_digest,
v->digest_size) == 0))
aux->hash_verified = 1;
- else if (io->in_tasklet) {
+ else if (io->in_tasklet)
/*
* FEC code cannot be run in a tasklet since it may
- * sleep. We need to resume execution in a work-queue
- * to handle FEC.
+ * sleep, so fallback to using a work-queue.
*/
return -EAGAIN;
- }
else if (verity_fec_decode(v, io,
DM_VERITY_BLOCK_TYPE_METADATA,
hash_block, data, NULL) == 0)
@@ -527,13 +524,6 @@ static int verity_verify_io(struct dm_verity_io *io)
return r;
continue;
- } else if (io->in_tasklet) {
- /*
- * FEC code cannot be run in a tasklet since it may
- * sleep. We need to resume execution in a work-queue
- * to handle FEC.
- */
- return -EAGAIN;
}
r = verity_hash_init(v, req, &wait);
@@ -555,8 +545,14 @@ static int verity_verify_io(struct dm_verity_io *io)
if (v->validated_blocks)
set_bit(cur_block, v->validated_blocks);
continue;
+ } else if (io->in_tasklet) {
+ /*
+ * FEC code cannot be run in a tasklet since it may
+ * sleep, so fallback to using a work-queue.
+ */
+ return -EAGAIN;
} else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA,
- cur_block, NULL, &start) == 0) {
+ cur_block, NULL, &start) == 0) {
continue;
} else {
if (bio->bi_status) {
@@ -603,22 +599,20 @@ static void verity_finish_io(struct dm_verity_io *io,
blk_status_t status)
static void verity_work(struct work_struct *w)
{
struct dm_verity_io *io = container_of(w, struct dm_verity_io, work);
- int err;
- io->in_tasklet = false;
- err = verity_verify_io(io);
-
- verity_finish_io(io, errno_to_blk_status(err));
+ verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
}
static void verity_tasklet(unsigned long data)
{
- struct dm_verity_io *io = (struct dm_verity_io *) data;
+ struct dm_verity_io *io = (struct dm_verity_io *)data;
int err;
io->in_tasklet = true;
err = verity_verify_io(io);
- if (err) {
+ if (err == -EAGAIN) {
+ /* fallback to retrying with work-queue */
+ io->in_tasklet = false;
INIT_WORK(&io->work, verity_work);
queue_work(io->v->verify_wq, &io->work);
return;
@@ -1069,13 +1063,13 @@ static int verity_parse_opt_args(struct dm_arg_set *as,
struct dm_verity *v,
} else if (!strcasecmp(arg_name, DM_VERITY_OPT_TASKLET_VERIFY))
{
v->use_tasklet = true;
continue;
- }
- else if (verity_is_fec_opt_arg(arg_name)) {
+ } else if (verity_is_fec_opt_arg(arg_name)) {
r = verity_fec_parse_opt_args(as, v, &argc, arg_name);
if (r)
return r;
continue;
+
} else if (verity_verify_is_sig_opt_arg(arg_name)) {
r = verity_verify_sig_parse_opt_args(as, v,
verify_args,
@@ -1083,7 +1077,6 @@ static int verity_parse_opt_args(struct dm_arg_set *as,
struct dm_verity *v,
if (r)
return r;
continue;
-
}
ti->error = "Unrecognized verity feature request";
@@ -1127,7 +1120,6 @@ static int verity_ctr(struct dm_target *ti, unsigned
argc, char **argv)
}
ti->private = v;
v->ti = ti;
- v->use_tasklet = false;
r = verity_fec_ctr_alloc(v);
if (r)
@@ -1216,7 +1208,7 @@ static int verity_ctr(struct dm_target *ti, unsigned
argc, char **argv)
goto bad;
}
- v->tfm = crypto_alloc_ahash(v->alg_name, 0, CRYPTO_ALG_ASYNC);
+ v->tfm = crypto_alloc_ahash(v->alg_name, 0, 0);
if (IS_ERR(v->tfm)) {
ti->error = "Cannot initialize hash function";
r = PTR_ERR(v->tfm);
@@ -1326,8 +1318,8 @@ static int verity_ctr(struct dm_target *ti, unsigned
argc, char **argv)
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, v->use_tasklet ?
- DM_BUFIO_CLIENT_NO_SLEEP : 0);
+ dm_bufio_alloc_callback, NULL,
+ v->use_tasklet ? DM_BUFIO_CLIENT_NO_SLEEP : 0);
if (IS_ERR(v->bufio)) {
ti->error = "Cannot initialize dm-bufio";
r = PTR_ERR(v->bufio);
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index 4e65f93bd8d6..26fbe553ca15 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -51,11 +51,11 @@ struct dm_verity {
unsigned char hash_dev_block_bits; /* log2(hash blocksize) */
unsigned char hash_per_block_bits; /* log2(hashes in hash block) */
unsigned char levels; /* the number of tree levels */
- bool use_tasklet; /* try to verify in tasklet before work-queue */
unsigned char version;
unsigned digest_size; /* digest size for the current hash algorithm */
unsigned int ahash_reqsize;/* the size of temporary space for crypto */
- int hash_failed; /* set to 1 if hash of any block failed */
+ bool hash_failed:1; /* set to 1 if hash of any block failed */
+ bool use_tasklet:1; /* try to verify in tasklet before work-queue */
enum verity_mode mode; /* mode for handling verification errors */
unsigned corrupted_errs;/* Number of errors for corrupted blocks */
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
