| Message ID | 20181206221507.5423-1-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dm crypt: fix lost ioprio when queuing crypto bios from task with ioprio | expand |
> 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); > 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 ;-)
On Thu, Dec 06, 2018 at 05:15:07PM -0500, Mike Snitzer wrote: > From: Eric Wheeler <dm-devel@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). 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?
On Fri, Dec 07 2018 at 2:43pm -0500, Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Dec 06, 2018 at 05:15:07PM -0500, Mike Snitzer wrote: > > From: Eric Wheeler <dm-devel@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). > > 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
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))