* [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
@ 2022-07-22 9:38 Nathan Huckleberry
2022-07-22 9:38 ` [PATCH 1/3] dm-bufio: Add flags for dm_bufio_client_create Nathan Huckleberry
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Nathan Huckleberry @ 2022-07-22 9:38 UTC (permalink / raw)
To: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer
Cc: Eric Biggers, Sami Tolvanen, Nathan Huckleberry
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.
A tasklet can only be used for verification if all the required hashes are
already in the dm-bufio cache. If verification cannot be done in a tasklet, we
fallback the existing work-queue implementation.
To allow tasklets to query the dm-bufio cache, the dm-bufio code must not sleep.
This patchset adds a flag to dm-bufio that disallows sleeping.
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
Nathan Huckleberry (3):
dm-bufio: Add flags for dm_bufio_client_create
dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP
dm-verity: Add try_verify_in_tasklet
drivers/md/dm-bufio.c | 29 +++++--
drivers/md/dm-ebs-target.c | 3 +-
drivers/md/dm-integrity.c | 2 +-
drivers/md/dm-snap-persistent.c | 2 +-
drivers/md/dm-verity-fec.c | 4 +-
drivers/md/dm-verity-target.c | 87 ++++++++++++++++---
drivers/md/dm-verity.h | 5 ++
drivers/md/persistent-data/dm-block-manager.c | 3 +-
include/linux/dm-bufio.h | 8 +-
9 files changed, 119 insertions(+), 24 deletions(-)
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] dm-bufio: Add flags for dm_bufio_client_create 2022-07-22 9:38 [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Nathan Huckleberry @ 2022-07-22 9:38 ` Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 2/3] dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP Nathan Huckleberry ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Nathan Huckleberry @ 2022-07-22 9:38 UTC (permalink / raw) To: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer Cc: Eric Biggers, Sami Tolvanen, Nathan Huckleberry Add a flags argument to dm_bufio_client_create and update all the callers. This is in preparation to add the DM_BUFIO_GET_CANT_SLEEP flag. Signed-off-by: Nathan Huckleberry <nhuck@google.com> --- drivers/md/dm-bufio.c | 3 ++- drivers/md/dm-ebs-target.c | 3 ++- drivers/md/dm-integrity.c | 2 +- drivers/md/dm-snap-persistent.c | 2 +- drivers/md/dm-verity-fec.c | 4 ++-- drivers/md/dm-verity-target.c | 2 +- drivers/md/persistent-data/dm-block-manager.c | 3 ++- include/linux/dm-bufio.h | 3 ++- 8 files changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c index 5ffa1dcf84cf..ad5603eb12e3 100644 --- a/drivers/md/dm-bufio.c +++ b/drivers/md/dm-bufio.c @@ -1717,7 +1717,8 @@ static unsigned long dm_bufio_shrink_count(struct shrinker *shrink, struct shrin struct dm_bufio_client *dm_bufio_client_create(struct block_device *bdev, unsigned block_size, unsigned reserved_buffers, unsigned aux_size, void (*alloc_callback)(struct dm_buffer *), - void (*write_callback)(struct dm_buffer *)) + void (*write_callback)(struct dm_buffer *), + unsigned int flags) { int r; struct dm_bufio_client *c; diff --git a/drivers/md/dm-ebs-target.c b/drivers/md/dm-ebs-target.c index 0221fa63f888..c90f9b9b1f02 100644 --- a/drivers/md/dm-ebs-target.c +++ b/drivers/md/dm-ebs-target.c @@ -312,7 +312,8 @@ static int ebs_ctr(struct dm_target *ti, unsigned int argc, char **argv) goto bad; } - ec->bufio = dm_bufio_client_create(ec->dev->bdev, to_bytes(ec->u_bs), 1, 0, NULL, NULL); + ec->bufio = dm_bufio_client_create(ec->dev->bdev, to_bytes(ec->u_bs), 1, + 0, NULL, NULL, 0); if (IS_ERR(ec->bufio)) { ti->error = "Cannot create dm bufio client"; r = PTR_ERR(ec->bufio); diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c index 3d5a0ce123c9..a508073d8414 100644 --- a/drivers/md/dm-integrity.c +++ b/drivers/md/dm-integrity.c @@ -4439,7 +4439,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned argc, char **argv) } ic->bufio = dm_bufio_client_create(ic->meta_dev ? ic->meta_dev->bdev : ic->dev->bdev, - 1U << (SECTOR_SHIFT + ic->log2_buffer_sectors), 1, 0, NULL, NULL); + 1U << (SECTOR_SHIFT + ic->log2_buffer_sectors), 1, 0, NULL, NULL, 0); if (IS_ERR(ic->bufio)) { r = PTR_ERR(ic->bufio); ti->error = "Cannot initialize dm-bufio"; diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c index 3bb5cff5d6fc..aaa699749c3b 100644 --- a/drivers/md/dm-snap-persistent.c +++ b/drivers/md/dm-snap-persistent.c @@ -494,7 +494,7 @@ static int read_exceptions(struct pstore *ps, client = dm_bufio_client_create(dm_snap_cow(ps->store->snap)->bdev, ps->store->chunk_size << SECTOR_SHIFT, - 1, 0, NULL, NULL); + 1, 0, NULL, NULL, 0); if (IS_ERR(client)) return PTR_ERR(client); diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c index cea2b3789736..23cffce56403 100644 --- a/drivers/md/dm-verity-fec.c +++ b/drivers/md/dm-verity-fec.c @@ -749,7 +749,7 @@ int verity_fec_ctr(struct dm_verity *v) f->bufio = dm_bufio_client_create(f->dev->bdev, f->io_size, - 1, 0, NULL, NULL); + 1, 0, NULL, NULL, 0); if (IS_ERR(f->bufio)) { ti->error = "Cannot initialize FEC bufio client"; return PTR_ERR(f->bufio); @@ -765,7 +765,7 @@ int verity_fec_ctr(struct dm_verity *v) f->data_bufio = dm_bufio_client_create(v->data_dev->bdev, 1 << v->data_dev_block_bits, - 1, 0, NULL, NULL); + 1, 0, NULL, NULL, 0); if (IS_ERR(f->data_bufio)) { ti->error = "Cannot initialize FEC data bufio client"; return PTR_ERR(f->data_bufio); diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index d6dbd47492a8..5d3fc58a3c34 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -1266,7 +1266,7 @@ 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); + dm_bufio_alloc_callback, NULL, 0); if (IS_ERR(v->bufio)) { ti->error = "Cannot initialize dm-bufio"; r = PTR_ERR(v->bufio); diff --git a/drivers/md/persistent-data/dm-block-manager.c b/drivers/md/persistent-data/dm-block-manager.c index 54c089a50b15..11935864f50f 100644 --- a/drivers/md/persistent-data/dm-block-manager.c +++ b/drivers/md/persistent-data/dm-block-manager.c @@ -391,7 +391,8 @@ struct dm_block_manager *dm_block_manager_create(struct block_device *bdev, bm->bufio = dm_bufio_client_create(bdev, block_size, max_held_per_thread, sizeof(struct buffer_aux), dm_block_manager_alloc_callback, - dm_block_manager_write_callback); + dm_block_manager_write_callback, + 0); if (IS_ERR(bm->bufio)) { r = PTR_ERR(bm->bufio); kfree(bm); diff --git a/include/linux/dm-bufio.h b/include/linux/dm-bufio.h index 90bd558a17f5..e21480715255 100644 --- a/include/linux/dm-bufio.h +++ b/include/linux/dm-bufio.h @@ -24,7 +24,8 @@ struct dm_bufio_client * dm_bufio_client_create(struct block_device *bdev, unsigned block_size, unsigned reserved_buffers, unsigned aux_size, void (*alloc_callback)(struct dm_buffer *), - void (*write_callback)(struct dm_buffer *)); + void (*write_callback)(struct dm_buffer *), + unsigned int flags); /* * Release a buffered IO cache. -- 2.37.1.359.gd136c6c3e2-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP 2022-07-22 9:38 [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 1/3] dm-bufio: Add flags for dm_bufio_client_create Nathan Huckleberry @ 2022-07-22 9:38 ` Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 3/3] dm-verity: Add try_verify_in_tasklet Nathan Huckleberry 2022-07-22 16:41 ` [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Christoph Hellwig 3 siblings, 0 replies; 12+ messages in thread From: Nathan Huckleberry @ 2022-07-22 9:38 UTC (permalink / raw) To: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer Cc: Eric Biggers, Sami Tolvanen, Nathan Huckleberry Add an optional flag that ensures dm_bufio_get does not sleep. This allows the dm-bufio cache to be queried from interrupt context. To ensure that dm-bufio does not sleep, dm-bufio must use a spinlock instead of a mutex. Additionally, to avoid deadlocks, special care must be taken so that dm-bufio does not sleep while holding the spinlock. DM_BUFIO_GET_CANT_SLEEP is useful in some contexts, such as dm-verity, so that we can query the dm-bufio cache in a tasklet. If the required data is cached, processing can be handled immediately in the tasklet instead of waiting for a work-queue job to be scheduled. This can reduce latency when there is high CPU load and memory pressure. Signed-off-by: Nathan Huckleberry <nhuck@google.com> --- drivers/md/dm-bufio.c | 26 ++++++++++++++++++++++---- include/linux/dm-bufio.h | 5 +++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c index ad5603eb12e3..3edeca7cfca6 100644 --- a/drivers/md/dm-bufio.c +++ b/drivers/md/dm-bufio.c @@ -81,6 +81,8 @@ */ struct dm_bufio_client { struct mutex lock; + spinlock_t spinlock; + unsigned long spinlock_flags; struct list_head lru[LIST_SIZE]; unsigned long n_buffers[LIST_SIZE]; @@ -90,6 +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; struct kmem_cache *slab_buffer; struct kmem_cache *slab_cache; @@ -167,17 +170,26 @@ struct dm_buffer { static void dm_bufio_lock(struct dm_bufio_client *c) { - mutex_lock_nested(&c->lock, dm_bufio_in_request()); + if (c->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()); } static int dm_bufio_trylock(struct dm_bufio_client *c) { - return mutex_trylock(&c->lock); + if (c->may_sleep) + return mutex_trylock(&c->lock); + else + return spin_trylock_irqsave(&c->spinlock, c->spinlock_flags); } static void dm_bufio_unlock(struct dm_bufio_client *c) { - mutex_unlock(&c->lock); + if (c->may_sleep) + mutex_unlock(&c->lock); + else + spin_unlock_irqrestore(&c->spinlock, c->spinlock_flags); } /*----------------------------------------------------------------*/ @@ -878,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) { + if (dm_bufio_cache_size_latch != 1 && c->may_sleep) { b = alloc_buffer(c, GFP_NOWAIT | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); if (b) return b; @@ -1041,6 +1053,7 @@ static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, if (nf == NF_GET && unlikely(test_bit(B_READING, &b->state))) return NULL; + b->hold_count++; __relink_lru(b, test_bit(B_DIRTY, &b->state) || test_bit(B_WRITING, &b->state)); @@ -1748,12 +1761,17 @@ 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; + if (flags & DM_BUFIO_GET_CANT_SLEEP) + c->may_sleep = false; + for (i = 0; i < LIST_SIZE; i++) { INIT_LIST_HEAD(&c->lru[i]); c->n_buffers[i] = 0; } mutex_init(&c->lock); + spin_lock_init(&c->spinlock); INIT_LIST_HEAD(&c->reserved_buffers); c->need_reserved_buffers = reserved_buffers; diff --git a/include/linux/dm-bufio.h b/include/linux/dm-bufio.h index e21480715255..2a78f0cb8e71 100644 --- a/include/linux/dm-bufio.h +++ b/include/linux/dm-bufio.h @@ -17,6 +17,11 @@ struct dm_bufio_client; struct dm_buffer; +/* + * Flags for dm_bufio_client_create + */ +#define DM_BUFIO_GET_CANT_SLEEP 0x1 + /* * Create a buffered IO cache on a given device */ -- 2.37.1.359.gd136c6c3e2-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] dm-verity: Add try_verify_in_tasklet 2022-07-22 9:38 [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 1/3] dm-bufio: Add flags for dm_bufio_client_create Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 2/3] dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP Nathan Huckleberry @ 2022-07-22 9:38 ` Nathan Huckleberry 2022-07-26 1:58 ` Mike Snitzer 2022-07-22 16:41 ` [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Christoph Hellwig 3 siblings, 1 reply; 12+ messages in thread From: Nathan Huckleberry @ 2022-07-22 9:38 UTC (permalink / raw) To: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer Cc: Eric Biggers, Sami Tolvanen, Nathan Huckleberry 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 <nhuck@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]); 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); @@ -307,6 +321,14 @@ 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) { + /* + * 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; + } else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_METADATA, hash_block, data, NULL) == 0) @@ -474,13 +496,12 @@ static int verity_verify_io(struct dm_verity_io *io) bool is_zero; struct dm_verity *v = io->v; struct bvec_iter start; - unsigned b; struct crypto_wait wait; struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size); - for (b = 0; b < io->n_blocks; b++) { + for (; io->block_idx < io->n_blocks; io->block_idx++) { int r; - sector_t cur_block = io->block + b; + sector_t cur_block = io->block + io->block_idx; struct ahash_request *req = verity_io_hash_req(v, io); if (v->validated_blocks && @@ -527,6 +548,13 @@ 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. We need to resume execution in a work-queue + * to handle FEC. + */ + return -EAGAIN; } else if (verity_fec_decode(v, io, DM_VERITY_BLOCK_TYPE_DATA, cur_block, NULL, &start) == 0) @@ -567,7 +595,8 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status) bio->bi_end_io = io->orig_bi_end_io; bio->bi_status = status; - verity_fec_finish_io(io); + if (!io->in_tasklet) + verity_fec_finish_io(io); bio_endio(bio); } @@ -575,8 +604,28 @@ 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(verity_verify_io(io))); + verity_finish_io(io, errno_to_blk_status(err)); +} + +static void verity_tasklet(unsigned long data) +{ + struct dm_verity_io *io = (struct dm_verity_io *) data; + int err; + + io->in_tasklet = true; + err = verity_verify_io(io); + if (err) { + INIT_WORK(&io->work, verity_work); + queue_work(io->v->verify_wq, &io->work); + return; + } + + verity_finish_io(io, errno_to_blk_status(err)); } static void verity_end_io(struct bio *bio) @@ -589,8 +638,14 @@ static void verity_end_io(struct bio *bio) return; } - INIT_WORK(&io->work, verity_work); - queue_work(io->v->verify_wq, &io->work); + io->block_idx = 0; + if (io->v->use_tasklet) { + tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io); + tasklet_schedule(&io->tasklet); + } else { + INIT_WORK(&io->work, verity_work); + queue_work(io->v->verify_wq, &io->work); + } } /* @@ -1012,7 +1067,12 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, return r; continue; - } else if (verity_is_fec_opt_arg(arg_name)) { + } else if (!strcasecmp(arg_name, DM_VERITY_OPT_TASKLET_VERIFY)) { + v->use_tasklet = true; + continue; + } + + else if (verity_is_fec_opt_arg(arg_name)) { r = verity_fec_parse_opt_args(as, v, &argc, arg_name); if (r) return r; @@ -1068,6 +1128,7 @@ 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) @@ -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); @@ -1266,7 +1327,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, 0); + dm_bufio_alloc_callback, NULL, v->use_tasklet ? + DM_BUFIO_GET_CANT_SLEEP : 0); if (IS_ERR(v->bufio)) { ti->error = "Cannot initialize dm-bufio"; r = PTR_ERR(v->bufio); @@ -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; diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h index 4e769d13473a..4e65f93bd8d6 100644 --- a/drivers/md/dm-verity.h +++ b/drivers/md/dm-verity.h @@ -13,6 +13,7 @@ #include <linux/dm-bufio.h> #include <linux/device-mapper.h> +#include <linux/interrupt.h> #include <crypto/hash.h> #define DM_VERITY_MAX_LEVELS 63 @@ -50,6 +51,7 @@ 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 */ @@ -76,10 +78,13 @@ struct dm_verity_io { sector_t block; unsigned n_blocks; + unsigned int block_idx; + bool in_tasklet; struct bvec_iter iter; struct work_struct work; + struct tasklet_struct tasklet; /* * Three variably-size fields follow this struct: -- 2.37.1.359.gd136c6c3e2-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] dm-verity: Add try_verify_in_tasklet 2022-07-22 9:38 ` [PATCH 3/3] dm-verity: Add try_verify_in_tasklet Nathan Huckleberry @ 2022-07-26 1:58 ` Mike Snitzer 2022-07-26 3:06 ` Eric Biggers 0 siblings, 1 reply; 12+ messages in thread From: Mike Snitzer @ 2022-07-26 1:58 UTC (permalink / raw) To: Nathan Huckleberry Cc: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer, Eric Biggers, Sami Tolvanen On Fri, Jul 22 2022 at 5:38P -0400, Nathan Huckleberry <nhuck@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 <nhuck@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 */ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] dm-verity: Add try_verify_in_tasklet 2022-07-26 1:58 ` Mike Snitzer @ 2022-07-26 3:06 ` Eric Biggers 2022-07-26 4:13 ` Mike Snitzer 0 siblings, 1 reply; 12+ messages in thread From: Eric Biggers @ 2022-07-26 3:06 UTC (permalink / raw) To: Mike Snitzer Cc: Nathan Huckleberry, linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer, Sami Tolvanen On Mon, Jul 25, 2022 at 09:58:39PM -0400, Mike Snitzer wrote: > > > @@ -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. I believe it's needed to ensure that only a synchronous algorithm is allocated, so that verity_hash_update() doesn't have to sleep during the tasklet. It should be conditional on v->use_tasklet, though. > @@ -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; > - } Doesn't this need to be: r = -EAGAIN; goto release_ret_r; - Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] dm-verity: Add try_verify_in_tasklet 2022-07-26 3:06 ` Eric Biggers @ 2022-07-26 4:13 ` Mike Snitzer 0 siblings, 0 replies; 12+ messages in thread From: Mike Snitzer @ 2022-07-26 4:13 UTC (permalink / raw) To: Eric Biggers Cc: Mike Snitzer, linux-kernel, Nathan Huckleberry, dm-devel, Sami Tolvanen, Alasdair Kergon On Mon, Jul 25 2022 at 11:06P -0400, Eric Biggers <ebiggers@kernel.org> wrote: > On Mon, Jul 25, 2022 at 09:58:39PM -0400, Mike Snitzer wrote: > > > > > @@ -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. > > I believe it's needed to ensure that only a synchronous algorithm is allocated, > so that verity_hash_update() doesn't have to sleep during the tasklet. It > should be conditional on v->use_tasklet, though. Ah yes, it is a mask, that makes sense. I can now see why it was being set unconditionally given dm-verity's optional ctr args aren't processed until after the crypto_alloc_ahash() call. And of course verity_parse_opt_args() depends on non-optional args related to the tfm.... gah! Do you have a sense for what the implications are for always setting CRYPTO_ALG_ASYNC like Nathan had? Will it disallow certain tfm that may already be in use by some users? > > @@ -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; > > - } > > > Doesn't this need to be: > > r = -EAGAIN; > goto release_ret_r; Yes, good catch. Thanks, Mike ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity 2022-07-22 9:38 [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Nathan Huckleberry ` (2 preceding siblings ...) 2022-07-22 9:38 ` [PATCH 3/3] dm-verity: Add try_verify_in_tasklet Nathan Huckleberry @ 2022-07-22 16:41 ` Christoph Hellwig 2022-07-22 17:12 ` Mike Snitzer 2022-07-22 18:12 ` [dm-devel] " Bart Van Assche 3 siblings, 2 replies; 12+ messages in thread From: Christoph Hellwig @ 2022-07-22 16:41 UTC (permalink / raw) To: Nathan Huckleberry Cc: linux-kernel, dm-devel, Alasdair Kergon, Mike Snitzer, Eric Biggers, Sami Tolvanen, Thomas Gleixner, Sebastian Andrzej Siewior We've been tying to kill off task lets for about 15 years. I don't think adding new users will make you a whole lot of friends.. On Fri, Jul 22, 2022 at 09:38:20AM +0000, Nathan Huckleberry 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. > > A tasklet can only be used for verification if all the required hashes are > already in the dm-bufio cache. If verification cannot be done in a tasklet, we > fallback the existing work-queue implementation. > > To allow tasklets to query the dm-bufio cache, the dm-bufio code must not sleep. > This patchset adds a flag to dm-bufio that disallows sleeping. > > 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 > > Nathan Huckleberry (3): > dm-bufio: Add flags for dm_bufio_client_create > dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP > dm-verity: Add try_verify_in_tasklet > > drivers/md/dm-bufio.c | 29 +++++-- > drivers/md/dm-ebs-target.c | 3 +- > drivers/md/dm-integrity.c | 2 +- > drivers/md/dm-snap-persistent.c | 2 +- > drivers/md/dm-verity-fec.c | 4 +- > drivers/md/dm-verity-target.c | 87 ++++++++++++++++--- > drivers/md/dm-verity.h | 5 ++ > drivers/md/persistent-data/dm-block-manager.c | 3 +- > include/linux/dm-bufio.h | 8 +- > 9 files changed, 119 insertions(+), 24 deletions(-) > > -- > 2.37.1.359.gd136c6c3e2-goog > ---end quoted text--- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity 2022-07-22 16:41 ` [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Christoph Hellwig @ 2022-07-22 17:12 ` Mike Snitzer 2022-08-15 11:35 ` Sebastian Andrzej Siewior 2022-07-22 18:12 ` [dm-devel] " Bart Van Assche 1 sibling, 1 reply; 12+ messages in thread From: Mike Snitzer @ 2022-07-22 17:12 UTC (permalink / raw) To: Christoph Hellwig Cc: Nathan Huckleberry, linux-kernel, dm-devel, Alasdair Kergon, Eric Biggers, Sami Tolvanen, Thomas Gleixner, Sebastian Andrzej Siewior On Fri, Jul 22 2022 at 12:41P -0400, Christoph Hellwig <hch@infradead.org> wrote: > We've been tying to kill off task lets for about 15 years. I don't > think adding new users will make you a whole lot of friends.. I don't have perspective on how serious that effort is. But ~2 years ago DM introduced another consumer of tasklets in dm-crypt, see: 39d42fa96ba1 dm crypt: add flags to optionally bypass kcryptd workqueues Given that, and other numerous users, is the effort to remove tasklets valid? What is the alternative to tasklets? Mike ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity 2022-07-22 17:12 ` Mike Snitzer @ 2022-08-15 11:35 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 12+ messages in thread From: Sebastian Andrzej Siewior @ 2022-08-15 11:35 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, Nathan Huckleberry, linux-kernel, dm-devel, Alasdair Kergon, Eric Biggers, Sami Tolvanen, Thomas Gleixner On 2022-07-22 13:12:36 [-0400], Mike Snitzer wrote: > On Fri, Jul 22 2022 at 12:41P -0400, > Christoph Hellwig <hch@infradead.org> wrote: > > > We've been tying to kill off task lets for about 15 years. I don't > > think adding new users will make you a whole lot of friends.. > > I don't have perspective on how serious that effort is. But ~2 years > ago DM introduced another consumer of tasklets in dm-crypt, see: > 39d42fa96ba1 dm crypt: add flags to optionally bypass kcryptd workqueues I tried to get rid of the in_atomic() as it appeared work "magic" in there and in ended in a pointless discussion… > Given that, and other numerous users, is the effort to remove tasklets > valid? What is the alternative to tasklets? The tasklets end up as anonymous load in the system. It is usually not visible due to the way accounting usually works (yes we do have full accounting) and you can't distinguish between work from USB-cam, storage, … if everything is fed into the same context. This becomes a problem on a smaller/ slower system of one softirq throttles the other (say the webcam processing gets delayed due to other tasklets). With the tasklet/BH context you need to disable BH while acquiring a spin_lock() so this ends up a per-CPU BKL since a random spin_lock_bh() is also synchronised again the timer as well as large parts of the networking subsystem and so on. This seems not to bother anyone in general it becomes a problem on PREEMPT_RT where this becomes visible. In general, a tasklet runs after the interrupt handler and were introduced a long time ago, before we had threaded interrupts available. Therefore threaded interrupts are a good substitute. > Mike Sebastian ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dm-devel] [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity 2022-07-22 16:41 ` [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Christoph Hellwig 2022-07-22 17:12 ` Mike Snitzer @ 2022-07-22 18:12 ` Bart Van Assche 2022-07-22 18:33 ` Mike Snitzer 1 sibling, 1 reply; 12+ messages in thread From: Bart Van Assche @ 2022-07-22 18:12 UTC (permalink / raw) To: Christoph Hellwig, Nathan Huckleberry Cc: Sebastian Andrzej Siewior, Mike Snitzer, linux-kernel, Eric Biggers, dm-devel, Sami Tolvanen, Thomas Gleixner, Alasdair Kergon On 7/22/22 09:41, Christoph Hellwig wrote: > We've been tying to kill off task lets for about 15 years. I don't > think adding new users will make you a whole lot of friends.. +1 for not using tasklets. At least in Android the real-time thread scheduling latency is important. Tasklets are not visible to the scheduler and hence cause latency spikes for real-time threads. These latency spikes can be observed by users, e.g. if the real-time thread is involved in audio playback. Bart. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity 2022-07-22 18:12 ` [dm-devel] " Bart Van Assche @ 2022-07-22 18:33 ` Mike Snitzer 0 siblings, 0 replies; 12+ messages in thread From: Mike Snitzer @ 2022-07-22 18:33 UTC (permalink / raw) To: Bart Van Assche Cc: Christoph Hellwig, Nathan Huckleberry, Sebastian Andrzej Siewior, Mike Snitzer, linux-kernel, Eric Biggers, dm-devel, Sami Tolvanen, Thomas Gleixner, Alasdair Kergon On Fri, Jul 22 2022 at 2:12P -0400, Bart Van Assche <bvanassche@acm.org> wrote: > On 7/22/22 09:41, Christoph Hellwig wrote: > > We've been tying to kill off task lets for about 15 years. I don't > > think adding new users will make you a whole lot of friends.. > > +1 for not using tasklets. At least in Android the real-time thread > scheduling latency is important. Tasklets are not visible to the scheduler > and hence cause latency spikes for real-time threads. These latency spikes > can be observed by users, e.g. if the real-time thread is involved in audio > playback. OK, then android wouldn't set the _optional_ "try_verify_in_tasklet" Mike ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-08-15 11:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-22 9:38 [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 1/3] dm-bufio: Add flags for dm_bufio_client_create Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 2/3] dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP Nathan Huckleberry 2022-07-22 9:38 ` [PATCH 3/3] dm-verity: Add try_verify_in_tasklet Nathan Huckleberry 2022-07-26 1:58 ` Mike Snitzer 2022-07-26 3:06 ` Eric Biggers 2022-07-26 4:13 ` Mike Snitzer 2022-07-22 16:41 ` [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity Christoph Hellwig 2022-07-22 17:12 ` Mike Snitzer 2022-08-15 11:35 ` Sebastian Andrzej Siewior 2022-07-22 18:12 ` [dm-devel] " Bart Van Assche 2022-07-22 18:33 ` Mike Snitzer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).