Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio
From: Eric Wheeler <hidden>
Date: 2016-12-30 04:08:11
Also in:
dm-devel, linux-bcache
On Sat, 17 Dec 2016, Mike Snitzer wrote:
On Fri, Dec 16 2016 at 5:29pm -0500, Eric Wheeler [off-list ref] wrote:quoted
On Wed, 14 Dec 2016, Eric Wheeler wrote:quoted
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/607I 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.
quoted
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