Re: dm crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Fri, Dec 07 2018 at 2:43pm -0500,
Christoph Hellwig wrote:
> On Thu, Dec 06, 2018 at 05:15:07PM -0500, Mike Snitzer wrote:
> > From: Eric Wheeler
> >
> > 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
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Thu, Dec 29 2016 at 11:08pm -0500, Eric Wheeler wrote: > On Sat, 17 Dec 2016, Mike Snitzer wrote: > > On Fri, Dec 16 2016 at 5:29pm -0500, > > Eric Wheeler wrote: > > > On Wed, 14 Dec 2016, Eric Wheeler wrote: > > > > 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. > > > > > > The motivation of this patch is for ioprio-driven writebackup/bypass > > > hinting inside bcache when bcache is under dm-crypt which Jens is picking > > > up for 4.10: > > > https://lkml.org/lkml/2016/12/6/607 > > > > I now see your commits: > > b71de4659fba4e42c7 bcache: introduce per-process ioprio-based > > bypass/writeback hints > > 82e7192711c3855038 bcache: documentation for ioprio cache hinting > > > > You'd think this is the type of thing that you'd have proposed to a > > wider audience. > > ( Its not really relevant to this bugfix, but please see this thread since > you were curious about a wider audience discussion: > https://www.redhat.com/archives/dm-devel/2016-July/msg00556.html ) > > The note above in the previous email was intended to explain how we > discovered the dm-crypt problem, purely as an example use case. The > stable commit note discusses the real issue: lost elevator hints. > > This commit fixes elevator ioprio hints passing through dm-crypt and is > not intended to address dm-cache, nor enable a bcache feature. > > All impementations using ioprio hints beneath dm-crypt would > benefit---most importantly, _CFQ_ ! > > As it is, all ioprio hints passing through dm-crypt are lost to the > elevator; the elevator looses those useful bits because of queuing to > another thread for crypto operations. How did you test the change? Or put differently: what is the easiest test to run against a dm-crypt device to verify that ioprio is being passed through? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Sat, 17 Dec 2016, Mike Snitzer wrote: > On Fri, Dec 16 2016 at 5:29pm -0500, > Eric Wheeler wrote: > > On Wed, 14 Dec 2016, Eric Wheeler wrote: > > > 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. > > > > The motivation of this patch is for ioprio-driven writebackup/bypass > > hinting inside bcache when bcache is under dm-crypt which Jens is picking > > up for 4.10: > > https://lkml.org/lkml/2016/12/6/607 > > I now see your commits: > b71de4659fba4e42c7 bcache: introduce per-process ioprio-based > bypass/writeback hints > 82e7192711c3855038 bcache: documentation for ioprio cache hinting > > You'd think this is the type of thing that you'd have proposed to a > wider audience. ( Its not really relevant to this bugfix, but please see this thread since you were curious about a wider audience discussion: https://www.redhat.com/archives/dm-devel/2016-July/msg00556.html ) The note above in the previous email was intended to explain how we discovered the dm-crypt problem, purely as an example use case. The stable commit note discusses the real issue: lost elevator hints. This commit fixes elevator ioprio hints passing through dm-crypt and is not intended to address dm-cache, nor enable a bcache feature. All impementations using ioprio hints beneath dm-crypt would benefit---most importantly, _CFQ_ ! As it is, all ioprio hints passing through dm-crypt are lost to the elevator; the elevator looses those useful bits because of queuing to another thread for crypto operations. > > The ioprio aware schedulers like CFQ and BFQ also benefit with more > > knowledge about the bio's when passing through dm-crypt. It would be > > great if this can be accepted for 4.10, too. > > It also needs more review, testing and possible re-working. Each DM > target shouldn't have to worry about these details (though I do grant > that dm-crypt.c:clone_init call to bio_set_prio makes sense). The patch is trivial: +5 lines in dm-crypt.c (excluding `if` bracing), a helper function in bio.h, and a 1-line fixup in bio.c. Thus, the stable@ inclusion since it would probably patch down to v3.x somewhere and help everyone who uses dm-crypt with ionice. Its only 4.10-rc1, and as a bugfix it could be accepted as a stable commit for 4.10-rc2 or later if you are willing to help dm-crypt users who use ionice. Indeed, if you so choose, you could both accept this commit and still re-work it in 4.11. This is to benefit everyone, using kernels both old and new. Cheers, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Sun, Dec 18, 2016 at 06:17:55PM -0500, Mike Snitzer wrote: > Spinning it as a pure bugfix is a reach (as Eric's header documents the > patch, the case is weak for cc'ing stable). Reality is the change is > needed to enable a new bcache feature. I'm not going to rush > feature-enabling change that arrived in the last minute. I do agree that it can wait for 4.11 and isn't a candidate for stable, I should've said that earlier. > > > and you can't fault Eric for not > > wanting to do work on dm-cache too when all he's trying to do is solve a > > particular real world problem. He should be able to do that without having > > to > > dive into dm-cache code too. > > > > Furthermore, pretty much every filesystem has private ioctls and interfaces > > - > > this is no different. > > You're completely misreading my having raised dm-cache. I was poinitng > out that Eric's patch to enable a new bcache feature ontop of dm-crypt > was too late. Had Eric known of this limitation sooner or thought to > engage me or the rest of dm-devel then DM could've been fixed with > precision during the 4.10 development window. > > I wasn't saying Eric should've worked on dm-cache. But had he raised > this bcache feature to my attention, in the context of missing ioprio > and why dm-cache would/could use it in the future too, then it'd have > been all the better. Simple as that. I was trying to be helpful. Not > trying to be a PITA in any way. I took your email to mean that the original patches shouldn't have gone in without involving dm-cache. If that wasn't what you meant, let's just chalk this up to a miscommunication. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Sun, Dec 18 2016 at 5:54pm -0500, Kent Overstreet wrote: > On Sat, Dec 17, 2016 at 10:58:00AM -0500, Mike Snitzer wrote: > > The time for 4.10 inclusion has passed. This needs to wait until 4.11. > > > > It also needs more review, testing and possible re-working. Each DM > > target shouldn't have to worry about these details (though I do grant > > that dm-crypt.c:clone_init call to bio_set_prio makes sense). > > > > A more generic solution is needed (likely in DM core). > > > > A while ago, Vivek floated a patch that spoke to the need for iocontext > > (for the purposes of cgroups): > > https://patchwork.kernel.org/patch/8485451/ > > > > I don't consider your patch too dissimilar. But it just needs to be > > worked on during a development window. On to 4.11 ;) > > Mike, this current patch is a pure bugfix, Spinning it as a pure bugfix is a reach (as Eric's header documents the patch, the case is weak for cc'ing stable). Reality is the change is needed to enable a new bcache feature. I'm not going to rush feature-enabling change that arrived in the last minute. > and you can't fault Eric for not > wanting to do work on dm-cache too when all he's trying to do is solve a > particular real world problem. He should be able to do that without having to > dive into dm-cache code too. > > Furthermore, pretty much every filesystem has private ioctls and interfaces - > this is no different. You're completely misreading my having raised dm-cache. I was poinitng out that Eric's patch to enable a new bcache feature ontop of dm-crypt was too late. Had Eric known of this limitation sooner or thought to engage me or the rest of dm-devel then DM could've been fixed with precision during the 4.10 development window. I wasn't saying Eric should've worked on dm-cache. But had he raised this bcache feature to my attention, in the context of missing ioprio and why dm-cache would/could use it in the future too, then it'd have been all the better. Simple as that. I was trying to be helpful. Not trying to be a PITA in any way. > If you dm guys want to standardize something, that's awesome - and in > hindsight, > it wouldn't have been a bad idea to CC you guys on the original patches. But > please keep in mind Eric's not a full time kernel developer, and don't crap on > his work just becausue he's not working on dm-cache too. You don't get to make this something it isn't. I didn't crap on anything. This line of development will be pursued in Januaray when I get back from holiday break. If there is a sta...@vger.kernel.org worthy change that comes from that development then so be it. Could be that the change(s) will land during 4.10-rc but I cannot put time to it until after January 2. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Sat, Dec 17, 2016 at 10:58:00AM -0500, Mike Snitzer wrote: > The time for 4.10 inclusion has passed. This needs to wait until 4.11. > > It also needs more review, testing and possible re-working. Each DM > target shouldn't have to worry about these details (though I do grant > that dm-crypt.c:clone_init call to bio_set_prio makes sense). > > A more generic solution is needed (likely in DM core). > > A while ago, Vivek floated a patch that spoke to the need for iocontext > (for the purposes of cgroups): > https://patchwork.kernel.org/patch/8485451/ > > I don't consider your patch too dissimilar. But it just needs to be > worked on during a development window. On to 4.11 ;) Mike, this current patch is a pure bugfix, and you can't fault Eric for not wanting to do work on dm-cache too when all he's trying to do is solve a particular real world problem. He should be able to do that without having to dive into dm-cache code too. Furthermore, pretty much every filesystem has private ioctls and interfaces - this is no different. If you dm guys want to standardize something, that's awesome - and in hindsight, it wouldn't have been a bad idea to CC you guys on the original patches. But please keep in mind Eric's not a full time kernel developer, and don't crap on his work just becausue he's not working on dm-cache too. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
On Fri, Dec 16 2016 at 5:29pm -0500, Eric Wheeler wrote: > On Wed, 14 Dec 2016, Eric Wheeler wrote: > > 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. > > The motivation of this patch is for ioprio-driven writebackup/bypass > hinting inside bcache when bcache is under dm-crypt which Jens is picking > up for 4.10: > https://lkml.org/lkml/2016/12/6/607 I now see your commits: b71de4659fba4e42c7 bcache: introduce per-process ioprio-based bypass/writeback hints 82e7192711c3855038 bcache: documentation for ioprio cache hinting You'd think this is the type of thing that you'd have proposed to a wider audience. I'm sure you're aware that dm-cache exists? If you did then it would've been on my radar with a shot of having DM in shape in time for 4.10. > Without assigning the ioprio before queuing to the en/decrypt queues, > bcache isn't notified of the priority---and presumably neither is the IO > scheduler. > > The ioprio aware schedulers like CFQ and BFQ also benefit with more > knowledge about the bio's when passing through dm-crypt. It would be > great if this can be accepted for 4.10, too. The time for 4.10 inclusion has passed. This needs to wait until 4.11. It also needs more review, testing and possible re-working. Each DM target shouldn't have to worry about these details (though I do grant that dm-crypt.c:clone_init call to bio_set_prio makes sense). A more generic solution is needed (likely in DM core). A while ago, Vivek floated a patch that spoke to the need for iocontext (for the purposes of cgroups): https://patchwork.kernel.org/patch/8485451/ I don't consider your patch too dissimilar. But it just needs to be worked on during a development window. On to 4.11 ;) Mike -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
