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

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

[Eric Wheeler]

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

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.

--
Eric Wheeler

> 
> 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-devel@linux.ewheeler.net>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: stable@vger.kernel.org
> ---
>  block/bio.c           |  1 +
>  drivers/md/dm-crypt.c | 11 +++++++++--
>  include/linux/bio.h   | 23 +++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index db85c57..1a529ff 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -584,6 +584,7 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src)
>  	bio->bi_iter = bio_src->bi_iter;
>  	bio->bi_io_vec = bio_src->bi_io_vec;
>  
> +	bio_set_prio(bio, bio_prio(bio_src));
>  	bio_clone_blkcg_association(bio, bio_src);
>  }
>  EXPORT_SYMBOL(__bio_clone_fast);
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 9b99ee9..ea7e102 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1133,6 +1133,7 @@ static void clone_init(struct dm_crypt_io *io, struct bio *clone)
>  	clone->bi_private = io;
>  	clone->bi_end_io  = crypt_endio;
>  	clone->bi_bdev    = cc->dev->bdev;
> +	bio_set_prio(clone, bio_prio(io->base_bio));
>  	bio_set_op_attrs(clone, bio_op(io->base_bio), bio_flags(io->base_bio));
>  }
>  
> @@ -2070,10 +2071,16 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
>  	io->ctx.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);
>  			kcryptd_queue_read(io);
> -	} else
> +		}
> +	} else {
> +		if (!ioprio_valid(bio_prio(io->base_bio)))
> +			bio_set_task_prio(io->base_bio, current);
>  		kcryptd_queue_crypt(io);
> +	}
>  
>  	return DM_MAPIO_SUBMITTED;
>  }
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 97cb48f..0b4f8bb 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -62,6 +62,29 @@
>  #define bio_sectors(bio)	((bio)->bi_iter.bi_size >> 9)
>  #define bio_end_sector(bio)	((bio)->bi_iter.bi_sector + bio_sectors((bio)))
>  
> +/* Set the bio's ioprio to that of the task's io_context->ioprio, if any.
> + * Always returns io_context->ioprio (or 0 if none); bio can be NULL.
> + */
> +static inline unsigned short bio_set_task_prio(
> +	struct bio *bio, struct task_struct *task)
> +{
> +	struct io_context *ioc;
> +	unsigned short ioprio = 0;
> +
> +	ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
> +	if (ioc) {
> +		if (ioprio_valid(ioc->ioprio)) {
> +			ioprio = ioc->ioprio;
> +			put_io_context(ioc);
> +		}
> +
> +		if (bio != NULL && ioprio_valid(ioprio))
> +			bio_set_prio(bio, ioc->ioprio);
> +	}
> +
> +	return ioprio;
> +}
> +
>  /*
>   * Check whether this bio carries any data or not. A NULL bio is allowed.
>   */
> -- 
> 1.8.3.1
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Mike Snitzer]

On Fri, Dec 16 2016 at  5:29pm -0500,
Eric Wheeler <dm-devel@lists.ewheeler.net> 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

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Kent Overstreet]

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.

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Mike Snitzer]

On Sun, Dec 18 2016 at  5:54pm -0500,
Kent Overstreet <kent.overstreet@gmail.com> 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 stable@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.

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Kent Overstreet]

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.

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Eric Wheeler]

On Sat, 17 Dec 2016, Mike Snitzer wrote:
> On Fri, Dec 16 2016 at  5:29pm -0500,
> Eric Wheeler <dm-devel@lists.ewheeler.net> 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

Re: dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

[Mike Snitzer]

On Thu, Dec 29 2016 at 11:08pm -0500,
Eric Wheeler <dm-devel@lists.ewheeler.net> wrote:

> On Sat, 17 Dec 2016, Mike Snitzer wrote:
> > On Fri, Dec 16 2016 at  5:29pm -0500,
> > Eric Wheeler <dm-devel@lists.ewheeler.net> 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?

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

[Eric Wheeler]

On Thu, 5 Jan 2017, Mike Snitzer wrote:

> On Thu, Dec 29 2016 at 11:08pm -0500,
> Eric Wheeler <dm-devel@lists.ewheeler.net> wrote:
> 
> > On Sat, 17 Dec 2016, Mike Snitzer wrote:
> > > On Fri, Dec 16 2016 at  5:29pm -0500,
> > > Eric Wheeler <dm-devel@lists.ewheeler.net> 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?

We discovered this by noticing that bcache never receives bios marked with 
ioprios that were submitted for writing by dm-crypt, but this seems to be 
an issue with dm-crypt specifically. Bcache gets ioprio information from 
upper-level drivers submitting from the original process context just 
fine. If the information is lost to bcache, it is certainly lost to the IO 
scheduler. Of course if ioprios are mapped between processes in some other 
way that I am missing, then please point that out for me.

Interestingly, before bio_ioprio was split out from bi_rw in about 
4.2-4.4, dm-crypt reads were marked fine because they do not usually get 
punted to another thread and complete from the submitting process. After 
splitting bi_ioprio out of bi_rw, reads broke because the simple cloning 
assignment of dst_bio->bi_rw = src_bio->bi_rw loses the priority bits. Of 
course this continues now that bi_rw has been pulled and changed into 
bi_opf.

I suspect there are other places where bi_rw assignments lost bi_ioprio 
assignments long ago, and this probably needs some research and 
fixup---but that is a separate issue from the dm-crypt issue.
 
> Or put differently: what is the easiest test to run against a dm-crypt
> device to verify that ioprio is being passed through?

You would probably need to modify dm-crypt just before it calls 
generic_make_request on the way out of writing bio's after encrypting 
either in kcryptd_crypt_write_io_submit or dmcrypt_write and calling 
something like WARN_ONCE if ioprio_valid(bio_prio(bio)).

Then set your IO priority using ionice on some process writing with direct 
IO to the mapper device and see if you get any warnings:
	ionice -c3 dd bs=1 of=/dev/mapper/cryptotest if=/dev/zero oflag=direct count=1

My guess is that you will not since bios are cloned without copying ioprio 
information and submitted from processes different from those of the 
calling thread.

I am curious what you find on your end, please let me know if you have any 
questions and thank you for your help!

--
Eric Wheeler

 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>