Re: blk-mq: introduce BLK_STS_DEV_RESOURCE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 20, 2018 at 10:52:01PM -0500, Mike Snitzer wrote:
> On Sat, Jan 20 2018 at  7:57pm -0500,
> Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> 
> > On Sat, Jan 20, 2018 at 12:30:02PM -0500, Mike Snitzer wrote:
> > > On Sat, Jan 20 2018 at  8:48am -0500,
> > > Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> > > 
> > > > This status is returned from driver to block layer if device related
> > > > resource is run out of, but driver can guarantee that IO dispatch is
> > > > triggered in future when the resource is available.
> > > > 
> > > > This patch converts some drivers to use this return value. Meantime
> > > > if driver returns BLK_STS_RESOURCE and S_SCHED_RESTART is marked, run
> > > > queue after 10ms for avoiding IO hang.
> > > > 
> > > > Suggested-by: Jens Axboe <axboe@xxxxxxxxx>
> > > > Cc: Mike Snitzer <snitzer@xxxxxxxxxx>
> > > > Cc: Laurence Oberman <loberman@xxxxxxxxxx>
> > > > Cc: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> > > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > > > ---
> > > >  block/blk-core.c             |  1 +
> > > >  block/blk-mq.c               | 20 ++++++++++++++++----
> > > >  drivers/block/null_blk.c     |  2 +-
> > > >  drivers/block/virtio_blk.c   |  2 +-
> > > >  drivers/block/xen-blkfront.c |  2 +-
> > > >  drivers/scsi/scsi_lib.c      |  6 +++---
> > > >  include/linux/blk_types.h    |  7 +++++++
> > > >  7 files changed, 30 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > > index 01f271d40825..6e97e0bf8178 100644
> > > > --- a/block/blk-mq.c
> > > > +++ b/block/blk-mq.c
> > > > @@ -1226,7 +1226,8 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> > > >  		}
> > > >  
> > > >  		ret = q->mq_ops->queue_rq(hctx, &bd);
> > > > -		if (ret == BLK_STS_RESOURCE) {
> > > > +		if ((ret == BLK_STS_RESOURCE) ||
> > > > +				(ret == BLK_STS_DEV_RESOURCE)) {
> > > >  			/*
> > > >  			 * If an I/O scheduler has been configured and we got a
> > > >  			 * driver tag for the next request already, free it
> > > 
> > > Just a nit, but this should be on one line.
> > 
> > It is too long, and my editor starts to highlight/complain it, :-)
> 
> Look at the lines immediately following it, your isn't longer than
> them..

OK.

> 
> > > > @@ -1764,6 +1775,7 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
> > > >  		*cookie = new_cookie;
> > > >  		break;
> > > >  	case BLK_STS_RESOURCE:
> > > > +	case BLK_STS_DEV_RESOURCE:
> > > >  		__blk_mq_requeue_request(rq);
> > > >  		break;
> > > >  	default:
> > > 
> > > It seems the strategy for BLK_STS_DEV_RESOURCE and BLK_STS_RESOURCE is
> > > too muddled: calling __blk_mq_requeue_request() for both will cause
> > > underlying blk-mq driver to retain the request, won't it?
> > 
> > blk_mq_request_issue_directly() is used by driver(dm-rq) on underlying
> > queue, and driver need to deal with underlying queue busy, now we simply
> > free the (underlying)request and feedback the busy status to blk-mq via
> > dm-rq.
> > 
> > Except for blk_mq_request_issue_directly(), this request need to be
> > requeued, and is retained by blk-mq in hctx->dispatch_list.
> > 
> > The difference is that if driver returns BLK_STS_DEV_RESOURCE, the queue
> > will be rerun when resource is available, so don't need to run queue after
> > a delay for avoiding IO hang explicitly.
> 
> Yes, I understand the intent.
> 
> > > > @@ -1826,7 +1838,7 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> > > >  	hctx_lock(hctx, &srcu_idx);
> > > >  
> > > >  	ret = __blk_mq_try_issue_directly(hctx, rq, cookie, false);
> > > > -	if (ret == BLK_STS_RESOURCE)
> > > > +	if ((ret == BLK_STS_RESOURCE) || (ret == BLK_STS_DEV_RESOURCE))
> > > >  		blk_mq_sched_insert_request(rq, false, true, false);
> > > >  	else if (ret != BLK_STS_OK)
> > > >  		blk_mq_end_request(rq, ret);
> > > 
> > > For this normal (non dm-mpath) case the request gets re-inserted;
> > > dm-mpath must avoid that.
> > > 
> > > But with dm-mpath, which instead uses blk_mq_request_issue_directly(),
> > > we're driving IO with stacked blk-mq drivers.  If the underlying blk-mq 
> > > driver (e.g. scsi-mq or nvme) is made to retain the request, using
> > > __blk_mq_issue_directly()'s call to __blk_mq_requeue_request() above,
> > > then dm-mpath will not have the ability to requeue the request without
> > > conflicting with the underlying blk-mq driver, will it?
> > 
> > No, as I explained, the exception is blk_mq_request_issue_directly(),
> > and now dm-rq simply frees it(and in my original version, this request
> > is cached for underlying queue, and reused in next dispatch), for others,
> > the request is retained in hctx->dispatch_list, and owned by blk-mq.
> >
> > > Or am I'm misunderstanding what __blk_mq_requeue_request() is doing?
> 
> OK I was misunderstanding __blk_mq_requeue_request().  Seems
> __blk_mq_requeue_request() is effectively resetting a request for
> reuse.  Not retaining the request for reissue.
> 
> > > dm_mq_queue_rq
> > > -> multipath_clone_and_map
> > >    -> blk_get_request (scsi_mq)
> > >       -> if error, dm-mpath conditionally requeues (w/ or w/o delay)
> > 
> > Yes, with this patch, most of times blk-mq will run queue w/ delay
> > because SCHED_RESTART is set after the 1st STS_RESOURCE from dm-rq
> > .queue_rq()
> > 
> > >       -> if BLK_STS_OK then blk_mq_request_issue_directly() gets called
> > > -> dm_dispatch_clone_request
> > >    -> blk_mq_request_issue_directly
> > >       -> __blk_mq_try_issue_directly
> > >          -> __blk_mq_issue_directly
> > >             -> q->mq_ops->queue_rq (this is the underlying scsi_mq)
> > >                -> a BLK_STS_RESOURCE return here is how Bart was able to cause stalls
> > 
> > The stall only happens when SCHED_RESTART is set and the dm-rq queue is
> > idle(no any in-flight requests), that is exactly what this patch tries to
> > address as suggested by Jens.
> > 
> > >             -> __blk_mq_requeue_request, if BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE **1
> > >    -> (return from blk_mq_request_issue_directly)
> > >    -> if BLK_STS_RESOURCE, the dm-mpath request is released using blk_put_request();
> > >                            and DM_MAPIO_REQUEUE is returned to dm_mq_queue_rq **2
> > 
> > Right.
> > 
> > > -> if DM_MAPIO_REQUEUE return from map_request()'s call to dm_dispatch_clone_request:
> > >    BLK_STS_RESOURCE is returned from dm-mpath's dm_mq_queue_rq
> > 
> > Right.
> > 
> > > 
> > > The redundant queueing (both to underlying blk-mq at **1 above, and
> > > upper layer blk-mq at **2 above) is what I'm concerned about.
> > > 
> > > Hope this is clear.
> > 
> > Yeah, it is quite clear.
> 
> Well I thought there was a problem with __blk_mq_issue_directly calling
> __blk_mq_requeue_request for dm-rq on scsi-mq.. guess not.
> 
> > I also have other dm-mpath specific questions:
> > 
> > 1) when any STS_RESOURCE is returned from underlying queue either
> > because of blk_get_request() or underlying .queue_rq() for a while,
> > will dm-mpath try to switch to other path?
> 
> If dm_requeue_original_request() is used, and then we reenter
> multipath_clone_and_map() then a new path may be selected.  Depending on
> whether there are multiple paths and which path selector is being used.

OK.

> 
> > 2) what is the basic path switch policy of dm-mpath?
> 
> It is selectable, there are 3:
> round-robin, queue-length and service-time
> 
> A path will be failed in multipath_end_io() if IO returns with a
> retryable error (as determined by blk_path_error()).  This ensures the
> path that experienced the failure will not be selected again until
> reinstated.
> 
> > 3) is it possible to move the check of 'ti->type->busy' into
> > .clone_and_map_rq()? if it is possible, this way might be more
> > effective to detect underlying queue busy.
> 
> Can you elaborate further why it'd make a difference?
> 
> It'd be somewhat odd to push that busy check down into
> .clone_and_map_rq().

>From source code of multipath_busy() and multipath_clone_and_map():

1) they both check if there is path available, so this part in
multipath_busy() can be done in multipath_clone_and_map() too, I guess.

2) multipath_busy() also check if there is any non-busy path(underlying
queue), but never records the non-busy path for further uses:

- pgpath_busy() calls blk_lld_busy() to check if underlying queue is
  busy, this way is easy to cause race, because queue can become not
  busy at the same time. Also blk_lld_busy() always return false in
  case of blk-mq.

- that means multipath_busy() isn't needed for blk-mq, but for non-mq,
  the check on busy may avoid to waste CPU for doing unnecessary allocation
  which may be a bit expensive, but this still depends on that the selected
  path in multipath_clone_and_map() isn't busy. 

That is why I raise the question in previous email.

> 
> Are you looking to check the selected path's blk-mq queue before calling
> blk_get_request()?  Why is that any different than blk_get_request()
> returning busy?

Maybe the check of 'ti->type->busy' can be killed at least in dm_mq_queue_rq().

> 
> > Actually this patch may has another issue: if the default run queue
> > delay(in this patch, it is 10ms) is too short, the timer may expire
> > before any in-flight underlying request completes, then we may
> > dequeue too quick, and sequential IO performance can be hurt too.
> 
> That's the problem with any arbitrary timer based solution.  No value
> that you pick is going to be correct for all hardware.
>  
> > But my previous patch in github doesn't have this issue.
> > 
> > 	https://github.com/ming1/linux/commit/dfd672c998283a110247152237a9916b8264f3ec
> > 
> > Jens, what do you think of this issue? Or do we need to worry about
> > it?
> 
> As your follow-up reply pointed out, blk_get_request_notify() is likely
> the better way forward to address this race.

Yeah, please ignore this patch, and I will post them out soon for
review, but the new patches have to be against both dm and block tree.

-- 
Ming



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux