From: Bart Van Assche <hidden> Date: 2017-08-25 19:00:57
Since .initialize_rq_fn() is called from inside blk_get_request()
that function is only called for pass-through requests but not for
filesystem requests. There is agreement in the SCSI community that
the jiffies_at_alloc and retries members of struct scsi_cmnd
should be initialized at request allocation time instead of when
preparing a request. This will allow to preserve the value of these
members when requeuing a request. Since the block layer does not
yet allow block drivers to initialize filesystem requests without
overriding request_queue.make_request_fn, move the
.initialize_rq_fn() calls from blk_get_request() into
blk_mq_rq_ctx_init() and blk_rq_init().
See also https://www.spinics.net/lists/linux-scsi/msg108934.html.
Signed-off-by: Bart Van Assche <redacted>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <redacted>
Cc: Hannes Reinecke <hare@suse.com>
---
block/blk-core.c | 20 +++++++-------------
block/blk-mq.c | 4 ++++
2 files changed, 11 insertions(+), 13 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2017-08-28 08:10:17
I still disagree that we should have an indirect function call like this
in the fast path.
All that can be done by clearing or setting a flag on the first call to
->queue_rq or ->queuecommand instead. In NVMe we use RQF_DONTPREP for
that, but for SCSI we probablt can't use that given that it has more
meaning for the old request path. But how about just adding a new
RQD_DRV_INITIALIZED or similar flag?
On Mon, Aug 28, 2017 at 06:31:33PM +0000, Bart Van Assche wrote:
On Mon, 2017-08-28 at 10:10 +0200, Christoph Hellwig wrote:
quoted
I still disagree that we should have an indirect function call like this
in the fast path.
All that can be done by clearing or setting a flag on the first call to
->queue_rq or ->queuecommand instead. In NVMe we use RQF_DONTPREP for
that, but for SCSI we probably can't use that given that it has more
meaning for the old request path. But how about just adding a new
RQD_DRV_INITIALIZED or similar flag?
Hello Christoph,
Sorry but I'm not enthusiast about the proposal to introduce a
RQD_DRV_INITIALIZED or similar flag. That approach involves an annoying
behavior difference, namely that .initialize_rq_fn() would be called from
inside blk_get_request() for pass-through requests and from inside the prep
function for filesystem requests. Another disadvantage of that approach is
that the block layer core never clears request.atomic_flags completely but
only sets and clears individual flags. The SCSI core would have to follow
that model and hence code for clearing RQD_DRV_INITIALIZED would have to be
added to all request completion paths in the SCSI core.
Have you noticed that Ming Lei's patch series introduces several new atomic
operations in the hot path? I'm referring here to the BLK_MQ_S_DISPATCH_BUSY
manipulations. Have you noticed that for SCSI drivers these patches introduce
an overhead between 0.1 and 1.0 microseconds per I/O request in the hot path?
I have derived these numbers from the random write SRP performance numbers
as follows: 1/142460 - 1/142990 = 2.6 microseconds. That number has to be
multiplied with the number of I/O requests processed in parallel. The number
of jobs in Ming Lei's test was 64 but that's probably way higher than the
actual I/O parallelism.
Hi Bart,
Did you see perf regression on SRP with smaller jobs after applying
my patchset V3?
I just run the test with 16 jobs(the system has 16 CPU cores) instead of 64,
looks not see perf regression on SRP about v4.13-rc6+blk-next(2nd column) VS.
v4.13-rc6+blk-next+patch V3(3rd column):
---------------------------------------
IOPS(K) | NONE | NONE
---------------------------------------
read | 475.83 | 485.88
---------------------------------------
randread | 142.86 | 141.96
---------------------------------------
write | 483.9 | 492.39
---------------------------------------
randwrite | 124.83 | 124.53
---------------------------------------
---------------------------------------
CPU(%) | NONE | NONE
---------------------------------------
read | 15.43 | 15.81
---------------------------------------
randread | 9.87 | 9.75
---------------------------------------
write | 17.67 | 18.34
---------------------------------------
randwrite | 10.96 | 10.56
---------------------------------------
---------------------------------------
LAT(us) | NONE | NONE
---------------------------------------
read | 2.15 | 2.11
---------------------------------------
randread | 7.17 | 7.21
---------------------------------------
write | 2.11 | 2.08
---------------------------------------
randwrite | 8.2 | 8.22
---------------------------------------
---------------------------------------
MERGE(K) | NONE | NONE
---------------------------------------
read | 8798.59 | 9064.09
---------------------------------------
randread | 0.02 | 0.19
---------------------------------------
write | 8847.73 | 9102.18
---------------------------------------
randwrite | 0.03 | 0.13
---------------------------------------
--
Ming
Hi Bart,
this isn't just about performance - it's about understanability of
the I/O path.
The legacy request path already has the prep_fn, which is intended
for exactly this sort of prep work, but even that led to a lot
of confusion. For blk-mq we decided to not add it but let the called
driver in control. I really don't want to move away from that.
The passthrough requests using blk_get_requst are a special case
as the caller allocates the requests, stuffs data into them and only
then hands control to the driver, and thus we need some way to
initialize the request before handing controller to the driver in
this particular special case. And if needed would could actually
do that with explicit calls instead of the callback, although you
changed it to save a bit of code.
On Mon, 2017-08-28 at 10:10 +0200, Christoph Hellwig wrote:
quoted
All that can be done by clearing or setting a flag on the first call to
->queue_rq or ->queuecommand instead. In NVMe we use RQF_DONTPREP for
that, but for SCSI we probablt can't use that given that it has more
meaning for the old request path. But how about just adding a new
RQD_DRV_INITIALIZED or similar flag?
Hello Christoph,
More changes would have to be made to implement the above proposal
than just setting a flag at the start of .queue_rq() / .queuecommand()
for all filesystem requests. The numerous code paths that lead to a
request not being executed immediately, e.g. a SCSI host being busy or
a preparation status != BLKPREP_OK, would have to be inspected and
code would have to be added to clear the "command initialized" flag to
ensure that initialization occurs shortly before the first time a
command is executed.
The choice we have to make is to add more state information and
complicated code for keeping that state information up-to-date to the
SCSI core or making a small and easy to maintain change to the block
layer core that does not involve any new state information. That's why
I'm asking you to reconsider the patch at the start of this e-mail
thread.
I don't like this addition, and honestly I wasn't a huge fan of adding
->init() hooks to the alloc path when we initially added this earlier
this summer. The fact that we now have to make this more invasive
doesn't improve the situation at all.
So fwiw, I too would much rather see an implementation based on an RQF_
flag instead.
--
Jens Axboe
On Tue, Aug 29, 2017 at 08:15:23PM +0000, Bart Van Assche wrote:
On Tue, 2017-08-29 at 19:16 +0800, Ming Lei wrote:
quoted
Hi Bart,
Did you see perf regression on SRP with smaller jobs after applying
my patchset V3?
I just run the test with 16 jobs(the system has 16 CPU cores) instead of 64,
looks not see perf regression on SRP about v4.13-rc6+blk-next(2nd column) VS.
v4.13-rc6+blk-next+patch V3(3rd column):
---------------------------------------
IOPS(K) | NONE | NONE
---------------------------------------
read | 475.83 | 485.88
---------------------------------------
randread | 142.86 | 141.96
---------------------------------------
write | 483.9 | 492.39
---------------------------------------
randwrite | 124.83 | 124.53
---------------------------------------
[ ... ]
---------------------------------------
LAT(us) | NONE | NONE
---------------------------------------
read | 2.15 | 2.11
---------------------------------------
randread | 7.17 | 7.21
---------------------------------------
write | 2.11 | 2.08
---------------------------------------
randwrite | 8.2 | 8.22
---------------------------------------
[ ... ]
Hello Ming,
Although I would prefer to see measurement data against an SRP target system
that supports a higher workload (>1M IOPS) and
Hi Bart,
For so higher workload, I guess it often requires to increase
.cmd_per_lun.
also for a high-end NVMe drive,
My patch won't affect NVMe drive since NVMe driver doesn't become busy
usually.
I think the above data is sufficient to show that the performance impact of
your patch series is most likely small enough even for high-end SCSI initiator
drivers.