Re: [PATCH 5/6] scsi: core: don't limit per-LUN queue depth for SSD when HBA needs

From: Ming Lei <ming.lei@redhat.com>
To: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Sathya Prakash <sathya.prakash@broadcom.com>,
	Chaitra P B <chaitra.basappa@broadcom.com>,
	Suganath Prabu Subramani  <suganath-prabu.subramani@broadcom.com>,
	Kashyap Desai <kashyap.desai@broadcom.com>,
	Sumit Saxena <sumit.saxena@broadcom.com>,
	Shivasharan S <shivasharan.srikanteshwara@broadcom.com>,
	"Ewan D . Milne" <emilne@redhat.com>,
	Christoph Hellwig <hch@lst.de>, Hannes Reinecke <hare@suse.de>,
	Bart Van Assche <bart.vanassche@wdc.com>
Subject: Re: [PATCH 5/6] scsi: core: don't limit per-LUN queue depth for SSD when HBA needs
Date: Fri, 24 Jan 2020 09:59:57 +0800	[thread overview]
Message-ID: <20200124015957.GA17387@ming.t460p> (raw)
In-Reply-To: <yq1sgk5ejix.fsf@oracle.com>

Hi Martin,

On Thu, Jan 23, 2020 at 08:21:42PM -0500, Martin K. Petersen wrote:
> 
> Ming,
> 
> > However, it depends on if the target device returns the congestion to
> > host. From my observation, looks there isn't such feedback from NVMe
> > target.
> 
> It happens all the time with SCSI devices. It is imperative that this
> keeps working.
> 
> > Even if there was such SSD target which provides such congestion
> > feedback, bypassing .device_busy won't cause big effect too since
> > blk-mq's SCHED_RESTART will retry this IO returning STS_RESOURCE only
> > after another in-flight one is completed.
> 
> The reason we back off is that it allows the device to recover by
> temporarily reducing its workload. In addition, the lower queue depth
> alleviates the risk of commands timing out leading to application I/O
> failures.

The timeout risk may only happen when driver/device doesn't return
congestion feedback, meantime the host queue depth is big enough.

So far we don't see such issue on NVMe which hw queue depth is 1023, and
the hw queue count is often 32+, and not see such timeout report
when there are so many inflight IOs(32 * 1023) on single LUN.

Also megaraid sas's queue depth is much less than (32 * 1023), so it
seems much unlikely to happen.

Megaraid guys, could you clarify if it is one issue? Kashyap, Sumit
and Shivasharan?

> 
> > At least, Broadcom guys tests this patch on megaraid raid and the
> > results shows that big improvement was got, that is why the flag is
> > only set on megaraid host.
> 
> I do not question that it improves performance. That's not my point.
> 
> > In theory, .track_queue_depth may only improve sequential IO's
> > performance for HDD., not very effective for SSD. Or just save a bit
> > CPU cycles in case of SSD.
> 
> This is not about performance. This is about how the system behaves when
> a device is starved for resources or experiencing transient failures.

Could you explain a bit how this patch changes the system beaviror? I
understand the EH just retries the incompleted requests, which total
number is just less than host queue depth.

Thanks,
Ming