[dm-devel] [PATCH] dm crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Mike Snitzer]

From: Eric Wheeler <dm-***@lists.ewheeler.net>

Since dm-crypt queues writes (and sometimes reads) to a different kernel
thread (workqueue), the bios will dispatch from tasks with different
io_context->ioprio settings than the submitting task, thus giving
incorrect ioprio hints to the io scheduler.

By assigning the ioprio to the bio before queuing to a workqueue, the
original submitting task's io_context->ioprio setting can be retained
through the life of the bio. We only set the bio's ioprio in dm-crypt
if not already set (by somewhere else, higher in the stack).

Signed-off-by: Eric Wheeler <dm-***@linux.ewheeler.net>
Cc: Mikulas Patocka <***@redhat.com>
Cc: Alasdair Kergon <***@redhat.com>
Cc: Jens Axboe <***@kernel.dk>
Cc: ***@vger.kernel.org
Signed-off-by: Mike Snitzer <***@redhat.com>
---
block/bio.c | 24 ++++++++++++++++++++++++
drivers/md/dm-crypt.c | 15 +++++++++++++--
include/linux/bio.h | 2 ++
3 files changed, 39 insertions(+), 2 deletions(-)

[Jens, too much block code for me to take, please feel free to pick
this up, I tweaked Eric's original for 2 years ago (sorry this slipped
through the cracks Eric!):
https://patchwork.kernel.org/patch/9474535/
]

diff --git a/block/bio.c b/block/bio.c
index 0c2208a5446d..ed68fdd78547 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -647,6 +647,30 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
}
EXPORT_SYMBOL(bio_clone_fast);

+/**
+ * bio_set_task_prio - set bio's ioprio to task's ioprio, if any.
+ * @bio: bio to set the ioprio of, can be NULL
+ * @task: task of interest
+ * @gfp_flags: allocation flags, used if allocation is necessary
+ * @node: allocation node, used if allocation is necessary
+ */
+void bio_set_task_prio(struct bio *bio, struct task_struct *task,
+ gfp_t gfp_flags, int node)
+{
+ struct io_context *ioc;
+
+ if (!bio)
+ return;
+
+ ioc = get_task_io_context(current, gfp_flags, node);
+ if (ioc) {
+ if (ioprio_valid(ioc->ioprio))
+ bio_set_prio(bio, ioc->ioprio);
+ put_io_context(ioc);
+ }
+}
+EXPORT_SYMBOL(bio_set_task_prio);
+
/**
* bio_add_pc_page - attempt to add page to bio
* @q: the target queue
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 509fb20f7f86..4e3f0962f230 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1548,6 +1548,7 @@ static void clone_init(struct dm_crypt_io *io, struct bio *clone)
clone->bi_private = io;
clone->bi_end_io = crypt_endio;
bio_set_dev(clone, cc->dev->bdev);
+ bio_set_prio(clone, bio_prio(io->base_bio));
clone->bi_opf = io->base_bio->bi_opf;
}

@@ -2916,10 +2917,20 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
io->ctx.r.req = (struct skcipher_request *)(io + 1);

if (bio_data_dir(io->base_bio) == READ) {
- if (kcryptd_io_read(io, GFP_NOWAIT))
+ if (kcryptd_io_read(io, GFP_NOWAIT)) {
+ if (!ioprio_valid(bio_prio(io->base_bio))) {
+ bio_set_task_prio(io->base_bio, current,
+ GFP_NOIO, NUMA_NO_NODE);
+ }
kcryptd_queue_read(io);
- } else
+ }
+ } else {
+ if (!ioprio_valid(bio_prio(io->base_bio))) {
+ bio_set_task_prio(io->base_bio, current,
+ GFP_NOIO, NUMA_NO_NODE);
+ }
kcryptd_queue_crypt(io);
+ }

return DM_MAPIO_SUBMITTED;
}
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 056fb627edb3..93e11424e7f0 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -46,6 +46,8 @@

#define bio_prio(bio) (bio)->bi_ioprio
#define bio_set_prio(bio, prio) ((bio)->bi_ioprio = prio)
+extern void bio_set_task_prio(struct bio *bio, struct task_struct *task,
+ gfp_t gfp_flags, int node);

#define bio_iter_iovec(bio, iter) \
bvec_iter_bvec((bio)->bi_io_vec, (iter))

[Jens Axboe]

Post by Mike Snitzer
diff --git a/block/bio.c b/block/bio.c
index 0c2208a5446d..ed68fdd78547 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -647,6 +647,30 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
}
EXPORT_SYMBOL(bio_clone_fast);
+/**
+ * bio_set_task_prio - set bio's ioprio to task's ioprio, if any.
+ */
+void bio_set_task_prio(struct bio *bio, struct task_struct *task,
+ gfp_t gfp_flags, int node)
+{
+ struct io_context *ioc;
+
+ if (!bio)
+ return;
+
+ ioc = get_task_io_context(current, gfp_flags, node);
+ if (ioc) {
+ if (ioprio_valid(ioc->ioprio))
+ bio_set_prio(bio, ioc->ioprio);
+ put_io_context(ioc);
+ }
+}
+EXPORT_SYMBOL(bio_set_task_prio);

Shouldn't this just be a lookup, not a create io context? If the io
priority has been set, the ioc would exist anyway.

void bio_set_prio_from_current(struct bio *bio)
{
struct io_context *ioc = current->io_context;

if (ioc && ioprio_valid(ioc->ioprio))
bio_set_prio(bio, ioc->ioprio);
}

and rename it like so. I don't like passing in the task, and all the
users are 'current' anyway. Passing in a task_struct implies that we
could have looked it up and would need a reference to it.

And don't make it pass in bio == NULL, that's just odd.

Apart from that, looks fine ;-)

[Christoph Hellwig]

Post by Mike Snitzer
Since dm-crypt queues writes (and sometimes reads) to a different kernel
thread (workqueue), the bios will dispatch from tasks with different
io_context->ioprio settings than the submitting task, thus giving
incorrect ioprio hints to the io scheduler.
By assigning the ioprio to the bio before queuing to a workqueue, the
original submitting task's io_context->ioprio setting can be retained
through the life of the bio. We only set the bio's ioprio in dm-crypt
if not already set (by somewhere else, higher in the stack).

The scheme looks a little odd to me. Instead of requring a driver
call why don't we just make sure bios always have the submitting
tasks priority set and then make sure the lower layers can rely on it?

[Mike Snitzer]

On Fri, Dec 07 2018 at 2:43pm -0500,

Post by Christoph Hellwig

Post by Mike Snitzer
Since dm-crypt queues writes (and sometimes reads) to a different kernel
thread (workqueue), the bios will dispatch from tasks with different
io_context->ioprio settings than the submitting task, thus giving
incorrect ioprio hints to the io scheduler.
By assigning the ioprio to the bio before queuing to a workqueue, the
original submitting task's io_context->ioprio setting can be retained
through the life of the bio. We only set the bio's ioprio in dm-crypt
if not already set (by somewhere else, higher in the stack).

The scheme looks a little odd to me. Instead of requring a driver
call why don't we just make sure bios always have the submitting
tasks priority set and then make sure the lower layers can rely on it?

The original patch from Eric was from 2 years ago when
__bio_clone_fast() didn't even copy over the ioprio from the src bio:
https://patchwork.kernel.org/patch/9474535/

So the 2 hunks from that original patch that did the copying of the
ioprio (__bio_clone_fast and clone_init) are no longer needed.

Leaving only these changes as in doubt:

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 509fb20f7f86..ce8f03f52433 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2916,10 +2916,16 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
io->ctx.r.req = (struct skcipher_request *)(io + 1);

if (bio_data_dir(io->base_bio) == READ) {
- if (kcryptd_io_read(io, GFP_NOWAIT))
+ if (kcryptd_io_read(io, GFP_NOWAIT)) {
+ if (!ioprio_valid(bio_prio(io->base_bio)))
+ bio_set_prio_from_current(io->base_bio);
kcryptd_queue_read(io);
- } else
+ }
+ } else {
+ if (!ioprio_valid(bio_prio(io->base_bio)))
+ bio_set_prio_from_current(io->base_bio);
kcryptd_queue_crypt(io);
+ }

return DM_MAPIO_SUBMITTED;
}

I need to look closer at _why_ io->base_bio wouldn't have inherited the
submitting task's ioprio....

Looks like the 2yr old need for the above was simply due to
__bio_clone_fast() not copying over the ioprio. Because crypt_io_init()
assigns the cloned bio, that DM core sent crypt_map(), to io->base_bio.
Since io->base_bio is a clone it will have been assigned the original
bio's ioprio. Leaving only remaining question being whether that
original bio had its ioprio previously set?

In this case, bcache appears to be only caller of bio_set_prio(), so for
the bcache on dm-crypt case Eric cares about dm-crypt _should_ inherit
bcache's ioprio.

Long story short: you're correct (driver shouldn't need to do this) and
thankfully it looks like latest upstream code should work fine -- though
I've not tested it to verify.

Eric, it has been a long time since you had a need for these ioprio
changes, to benefit bcache stacked on dm-crypt, but if you still have an
issue with ioprio not being set (or not being from submitting task)
please let us know.

Thanks,
Mike