Re: block: fix direct dispatch issue failure for clones

[Mike Snitzer <snitzer@redhat.com>]

On Thu, Dec 06 2018 at  8:13pm -0500,
Jens Axboe <axboe@kernel.dk> wrote:

> On 12/6/18 6:04 PM, jianchao.wang wrote:
> > 
> > 
> > On 12/7/18 6:20 AM, Jens Axboe wrote:
> >> After the direct dispatch corruption fix, we permanently disallow direct
> >> dispatch of non read/write requests. This works fine off the normal IO
> >> path, as they will be retried like any other failed direct dispatch
> >> request. But for the blk_insert_cloned_request() that only DM uses to
> >> bypass the bottom level scheduler, we always first attempt direct
> >> dispatch. For some types of requests, that's now a permanent failure,
> >> and no amount of retrying will make that succeed.
> >>
> >> Don't use direct dispatch off the cloned insert path, always just use
> >> bypass inserts. This still bypasses the bottom level scheduler, which is
> >> what DM wants.
> >>
> >> Fixes: ffe81d45322c ("blk-mq: fix corruption with direct issue")
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>
> >> ---
> >>
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index deb56932f8c4..4c44e6fa0d08 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -2637,7 +2637,8 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *
> >>  		 * bypass a potential scheduler on the bottom device for
> >>  		 * insert.
> >>  		 */
> >> -		return blk_mq_request_issue_directly(rq);
> >> +		blk_mq_request_bypass_insert(rq, true);
> >> +		return BLK_STS_OK;
> >>  	}
> >>  
> >>  	spin_lock_irqsave(q->queue_lock, flags);
> >>
> > Not sure about this because it will break the merging promotion for request based DM
> > from Ming.
> > 396eaf21ee17c476e8f66249fb1f4a39003d0ab4
> > (blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback)
> > 
> > We could use some other way to fix this.
> 
> That really shouldn't matter as this is the cloned insert, merging should
> have been done on the original request.

Reading the header of 396eaf21ee17c476e8f66249fb1f4a39003d0ab4 brings me
back to relatively recent hell.

Thing is, dm-rq was the original justification and consumer of
blk_mq_request_issue_directly -- but Ming's later use of directly issuing
requests has forced fixes that didn't consider the original valid/safe
use of the interface that is now too rigid.  dm-rq needs the
functionality the blk_mq_request_issue_directly interface provides.

Sorry to say we cannot lose the sequential IO performance improvements
that the IO merging feedback loop gives us.