Re: [RFC PATCH] dm: fix excessive dm-mq context switching

[Mike Snitzer <snitzer@redhat.com>]

On Fri, Feb 05 2016 at 10:13am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:
 
> Following is RFC because it really speaks to dm-mq _needing_ a variant
> of blk_mq_complete_request() that supports partial completions.  Not
> supporting partial completions really isn't an option for DM multipath.
> 
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Fri, 5 Feb 2016 08:49:01 -0500
> Subject: [RFC PATCH] dm: fix excessive dm-mq context switching
> 
> Request-based DM's blk-mq support (dm-mq) was reported to be 50% slower
> than if an underlying null_blk device were used directly.  This biggest
> reason for this drop in performance is that blk_insert_clone_request()
> was calling blk_mq_insert_request() with @async=true.  This forced the
> use of kblockd_schedule_delayed_work_on() to run the queues which
> ushered in ping-ponging between process context (fio in this case) and
> kblockd's kworker to submit the cloned request.  The ftrace
> function_graph tracer showed:
> 
>   kworker-2013  =>   fio-12190
>   fio-12190    =>  kworker-2013
>   ...
>   kworker-2013  =>   fio-12190
>   fio-12190    =>  kworker-2013
>   ...
> 
> Fixing blk_mq_insert_request() to _not_ use kblockd to submit the cloned
> requests isn't enough to fix eliminated the oberved context switches.
> 
> In addition to this dm-mq specific blk-core fix, there were 2 DM core
> fixes to dm-mq that (when paired with the blk-core fix) completely
> eliminate the observed context switching:
> 
> 1)  don't blk_mq_run_hw_queues in blk-mq request completion
> 
>     Motivated by desire to reduce overhead of dm-mq, punting to kblockd
>     just increases context switches.
> 
>     In my testing against a really fast null_blk device there was no benefit
>     to running blk_mq_run_hw_queues() on completion (and no other blk-mq
>     driver does this).  So hopefully this change doesn't induce the need for
>     yet another revert like commit 621739b00e16ca2d !
> 
> 2)  use blk_mq_complete_request() in dm_complete_request()
> 
>     blk_complete_request() doesn't offer the traditional q->mq_ops vs
>     .request_fn branching pattern that other historic block interfaces
>     do (e.g. blk_get_request).  Using blk_mq_complete_request() for
>     blk-mq requests is important for performance but it doesn't handle
>     partial completions -- which is a pretty big problem given the
>     potential for partial completions with DM multipath due to path
>     failure(s).  As such this makes this entire patch only RFC-worthy.

> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index c683f6d..a618477 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1344,7 +1340,10 @@ static void dm_complete_request(struct request *rq, int error)
>  	struct dm_rq_target_io *tio = tio_from_request(rq);
>  
>  	tio->error = error;
> -	blk_complete_request(rq);
> +	if (!rq->q->mq_ops)
> +		blk_complete_request(rq);
> +	else
> +		blk_mq_complete_request(rq, rq->errors);
>  }
>  
>  /*

Looking closer, DM is very likely OK just using blk_mq_complete_request.

blk_complete_request() also doesn't provide native partial completion
support (it relies on the driver to do it, which DM core does):

/**
 * blk_complete_request - end I/O on a request
 * @req:      the request being processed
 *
 * Description:
 *     Ends all I/O on a request. It does not handle partial completions,
 *     unless the driver actually implements this in its completion callback
 *     through requeueing. The actual completion happens out-of-order,
 *     through a softirq handler. The user must have registered a completion
 *     callback through blk_queue_softirq_done().
 **/

blk_mq_complete_request() is effectively implemented in a comparable
fashion to blk_complete_request().  Given that DM core is providing
partial completion support by dm.c:end_clone_bio() triggering requeueing
of the request via dm-mpath.c:multipath_end_io()'s return of
DM_ENDIO_REQUEUE.

So I'm thinking I can drop the "RFC" for this patch and run with
it.. once I get Jens' feedback (hopefully) confirming my understanding.

Jens, please advise.  If you're comfortable providing your Acked-by I
can get this fix in for 4.5-rc4 or so...

Thanks!

Mike