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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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---
On Fri, Jul 22 2022 at 12:41P -0400,
Christoph Hellwig <[email protected]> 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
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.
On Fri, Jul 22 2022 at 2:12P -0400,
Bart Van Assche <[email protected]> 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
On Fri, Jul 22 2022 at 5:38P -0400,
Nathan Huckleberry <[email protected]> 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 <[email protected]>
> ---
> 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 */
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
On Mon, Jul 25 2022 at 11:06P -0400,
Eric Biggers <[email protected]> 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
On 2022-07-22 13:12:36 [-0400], Mike Snitzer wrote:
> On Fri, Jul 22 2022 at 12:41P -0400,
> Christoph Hellwig <[email protected]> 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