[PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Bart Van Assche]

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 <bart.vanassche@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/blk-core.c | 20 +++++++-------------
 block/blk-mq.c   |  4 ++++
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 78a46794053e..463aa0ee36ce 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
 	rq->part = NULL;
+
+	if (q && q->initialize_rq_fn)
+		q->initialize_rq_fn(rq);
 }
 EXPORT_SYMBOL(blk_rq_init);
 
@@ -1420,21 +1423,12 @@ static struct request *blk_old_get_request(struct request_queue *q,
 struct request *blk_get_request(struct request_queue *q, unsigned int op,
 				gfp_t gfp_mask)
 {
-	struct request *req;
-
-	if (q->mq_ops) {
-		req = blk_mq_alloc_request(q, op,
+	if (q->mq_ops)
+		return blk_mq_alloc_request(q, op,
 			(gfp_mask & __GFP_DIRECT_RECLAIM) ?
 				0 : BLK_MQ_REQ_NOWAIT);
-		if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn)
-			q->mq_ops->initialize_rq_fn(req);
-	} else {
-		req = blk_old_get_request(q, op, gfp_mask);
-		if (!IS_ERR(req) && q->initialize_rq_fn)
-			q->initialize_rq_fn(req);
-	}
-
-	return req;
+	else
+		return blk_old_get_request(q, op, gfp_mask);
 }
 EXPORT_SYMBOL(blk_get_request);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 21f2cff217ce..fdb33f8a2860 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -273,6 +273,7 @@ EXPORT_SYMBOL(blk_mq_can_queue);
 static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		unsigned int tag, unsigned int op)
 {
+	struct request_queue *q = data->q;
 	struct blk_mq_tags *tags = blk_mq_tags_from_data(data);
 	struct request *rq = tags->static_rqs[tag];
 
@@ -325,6 +326,9 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->end_io_data = NULL;
 	rq->next_rq = NULL;
 
+	if (q->mq_ops->initialize_rq_fn)
+		q->mq_ops->initialize_rq_fn(rq);
+
 	data->ctx->rq_dispatched[op_is_sync(op)]++;
 	return rq;
 }
-- 
2.14.1

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Christoph Hellwig]

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?

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA4LTI4IGF0IDEwOjEwICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gSSBzdGlsbCBkaXNhZ3JlZSB0aGF0IHdlIHNob3VsZCBoYXZlIGFuIGluZGlyZWN0IGZ1
bmN0aW9uIGNhbGwgbGlrZSB0aGlzDQo+IGluIHRoZSBmYXN0IHBhdGguDQo+IA0KPiBBbGwgdGhh
dCBjYW4gYmUgZG9uZSBieSBjbGVhcmluZyBvciBzZXR0aW5nIGEgZmxhZyBvbiB0aGUgZmlyc3Qg
Y2FsbCB0bw0KPiAtPnF1ZXVlX3JxIG9yIC0+cXVldWVjb21tYW5kIGluc3RlYWQuICBJbiBOVk1l
IHdlIHVzZSBSUUZfRE9OVFBSRVAgZm9yDQo+IHRoYXQsIGJ1dCBmb3IgU0NTSSB3ZSBwcm9iYWJs
eSBjYW4ndCB1c2UgdGhhdCBnaXZlbiB0aGF0IGl0IGhhcyBtb3JlDQo+IG1lYW5pbmcgZm9yIHRo
ZSBvbGQgcmVxdWVzdCBwYXRoLiAgQnV0IGhvdyBhYm91dCBqdXN0IGFkZGluZyBhIG5ldw0KPiBS
UURfRFJWX0lOSVRJQUxJWkVEIG9yIHNpbWlsYXIgZmxhZz8NCg0KSGVsbG8gQ2hyaXN0b3BoLA0K
DQpTb3JyeSBidXQgSSdtIG5vdCBlbnRodXNpYXN0IGFib3V0IHRoZSBwcm9wb3NhbCB0byBpbnRy
b2R1Y2UgYQ0KUlFEX0RSVl9JTklUSUFMSVpFRCBvciBzaW1pbGFyIGZsYWcuIFRoYXQgYXBwcm9h
Y2ggaW52b2x2ZXMgYW4gYW5ub3lpbmcNCmJlaGF2aW9yIGRpZmZlcmVuY2UsIG5hbWVseSB0aGF0
IC5pbml0aWFsaXplX3JxX2ZuKCkgd291bGQgYmUgY2FsbGVkIGZyb20NCmluc2lkZSBibGtfZ2V0
X3JlcXVlc3QoKSBmb3IgcGFzcy10aHJvdWdoIHJlcXVlc3RzIGFuZCBmcm9tIGluc2lkZSB0aGUg
cHJlcA0KZnVuY3Rpb24gZm9yIGZpbGVzeXN0ZW0gcmVxdWVzdHMuIEFub3RoZXIgZGlzYWR2YW50
YWdlIG9mIHRoYXQgYXBwcm9hY2ggaXMNCnRoYXQgdGhlIGJsb2NrIGxheWVyIGNvcmUgbmV2ZXIg
Y2xlYXJzIHJlcXVlc3QuYXRvbWljX2ZsYWdzIGNvbXBsZXRlbHkgYnV0DQpvbmx5IHNldHMgYW5k
IGNsZWFycyBpbmRpdmlkdWFsIGZsYWdzLiBUaGUgU0NTSSBjb3JlIHdvdWxkIGhhdmUgdG8gZm9s
bG93DQp0aGF0IG1vZGVsIGFuZCBoZW5jZSBjb2RlIGZvciBjbGVhcmluZyBSUURfRFJWX0lOSVRJ
QUxJWkVEIHdvdWxkIGhhdmUgdG8gYmUNCmFkZGVkIHRvIGFsbCByZXF1ZXN0IGNvbXBsZXRpb24g
cGF0aHMgaW4gdGhlIFNDU0kgY29yZS4NCg0KSGF2ZSB5b3Ugbm90aWNlZCB0aGF0IE1pbmcgTGVp
J3MgcGF0Y2ggc2VyaWVzIGludHJvZHVjZXMgc2V2ZXJhbCBuZXcgYXRvbWljDQpvcGVyYXRpb25z
IGluIHRoZSBob3QgcGF0aD8gSSdtIHJlZmVycmluZyBoZXJlIHRvIHRoZSBCTEtfTVFfU19ESVNQ
QVRDSF9CVVNZDQptYW5pcHVsYXRpb25zLiBIYXZlIHlvdSBub3RpY2VkIHRoYXQgZm9yIFNDU0kg
ZHJpdmVycyB0aGVzZSBwYXRjaGVzIGludHJvZHVjZQ0KYW4gb3ZlcmhlYWQgYmV0d2VlbiAwLjEg
YW5kIDEuMCBtaWNyb3NlY29uZHMgcGVyIEkvTyByZXF1ZXN0IGluIHRoZSBob3QgcGF0aD8NCkkg
aGF2ZSBkZXJpdmVkIHRoZXNlIG51bWJlcnMgZnJvbSB0aGUgcmFuZG9tIHdyaXRlIFNSUCBwZXJm
b3JtYW5jZSBudW1iZXJzDQphcyBmb2xsb3dzOiAxLzE0MjQ2MCAtIDEvMTQyOTkwID0gMi42IG1p
Y3Jvc2Vjb25kcy4gVGhhdCBudW1iZXIgaGFzIHRvIGJlDQptdWx0aXBsaWVkIHdpdGggdGhlIG51
bWJlciBvZiBJL08gcmVxdWVzdHMgcHJvY2Vzc2VkIGluIHBhcmFsbGVsLiBUaGUgbnVtYmVyDQpv
ZiBqb2JzIGluIE1pbmcgTGVpJ3MgdGVzdCB3YXMgNjQgYnV0IHRoYXQncyBwcm9iYWJseSB3YXkg
aGlnaGVyIHRoYW4gdGhlDQphY3R1YWwgSS9PIHBhcmFsbGVsaXNtLg0KDQpIYXZlIHlvdSBub3Rp
Y2VkIHRoYXQgbXkgcGF0Y2ggZGlkIG5vdCBhZGQgYW55IGF0b21pYyBpbnN0cnVjdGlvbnMgdG8g
dGhlIGhvdA0KcGF0aCBidXQgb25seSBhIHJlYWQgb2YgYSBmdW5jdGlvbiBwb2ludGVyIHRoYXQg
c2hvdWxkIGFscmVhZHkgYmUgY2FjaGUgaG90Pw0KQXMgeW91IGtub3cgbW9kZXJuIENQVXMgYXJl
IGdvb2QgYXQgcHJlZGljdGluZyBicmFuY2hlcy4gQXJlIHlvdSBzdXJlIHRoYXQgbXkNCnBhdGNo
IHdpbGwgaGF2ZSBhIG1lYXN1cmFibGUgcGVyZm9ybWFuY2UgaW1wYWN0Pw0KDQpCYXJ0Lg==

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Ming Lei]

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:
> > 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

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[hch]

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.

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA4LTI5IGF0IDE5OjE2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gSGkg
QmFydCwNCj4gDQo+IERpZCB5b3Ugc2VlIHBlcmYgcmVncmVzc2lvbiBvbiBTUlAgd2l0aCBzbWFs
bGVyIGpvYnMgYWZ0ZXIgYXBwbHlpbmcNCj4gbXkgcGF0Y2hzZXQgVjM/DQo+IA0KPiBJIGp1c3Qg
cnVuIHRoZSB0ZXN0IHdpdGggMTYgam9icyh0aGUgc3lzdGVtIGhhcyAxNiBDUFUgY29yZXMpIGlu
c3RlYWQgb2YgNjQsDQo+IGxvb2tzIG5vdCBzZWUgcGVyZiByZWdyZXNzaW9uIG9uIFNSUCBhYm91
dCB2NC4xMy1yYzYrYmxrLW5leHQoMm5kIGNvbHVtbikgVlMuDQo+IHY0LjEzLXJjNitibGstbmV4
dCtwYXRjaCBWMygzcmQgY29sdW1uKToNCj4gDQo+IA0KPiAtLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0NCj4gIElPUFMoSykgIHwgICAgTk9ORSAgICAgfCAgICBOT05FDQo+
IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiByZWFkICAgICAgfCAg
ICAgIDQ3NS44MyB8ICAgICAgNDg1Ljg4DQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLQ0KPiByYW5kcmVhZCAgfCAgICAgIDE0Mi44NiB8ICAgICAgMTQxLjk2DQo+IC0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiB3cml0ZSAgICAgfCAgICAg
ICA0ODMuOSB8ICAgICAgNDkyLjM5DQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLQ0KPiByYW5kd3JpdGUgfCAgICAgIDEyNC44MyB8ICAgICAgMTI0LjUzDQo+IC0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiBbIC4uLiBdDQo+IC0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiAgTEFUKHVzKSAgfCAgICBOT05FICAg
ICB8ICAgIE5PTkUNCj4gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+
IHJlYWQgICAgICB8ICAgICAgICAyLjE1IHwgICAgICAgIDIuMTENCj4gLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+IHJhbmRyZWFkICB8ICAgICAgICA3LjE3IHwgICAg
ICAgIDcuMjENCj4gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+IHdy
aXRlICAgICB8ICAgICAgICAyLjExIHwgICAgICAgIDIuMDgNCj4gLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+IHJhbmR3cml0ZSB8ICAgICAgICAgOC4yIHwgICAgICAg
IDguMjINCj4gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+IFsgLi4u
IF0NCg0KSGVsbG8gTWluZywNCg0KQWx0aG91Z2ggSSB3b3VsZCBwcmVmZXIgdG8gc2VlIG1lYXN1
cmVtZW50IGRhdGEgYWdhaW5zdCBhbiBTUlAgdGFyZ2V0IHN5c3RlbQ0KdGhhdCBzdXBwb3J0cyBh
IGhpZ2hlciB3b3JrbG9hZCAoPjFNIElPUFMpIGFuZCBhbHNvIGZvciBhIGhpZ2gtZW5kIE5WTWUg
ZHJpdmUsDQpJIHRoaW5rIHRoZSBhYm92ZSBkYXRhIGlzIHN1ZmZpY2llbnQgdG8gc2hvdyB0aGF0
IHRoZSBwZXJmb3JtYW5jZSBpbXBhY3Qgb2YNCnlvdXIgcGF0Y2ggc2VyaWVzIGlzIG1vc3QgbGlr
ZWx5IHNtYWxsIGVub3VnaCBldmVuIGZvciBoaWdoLWVuZCBTQ1NJIGluaXRpYXRvcg0KZHJpdmVy
cy4NCg0KQmFydC4=

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA4LTI4IGF0IDEwOjEwICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gQWxsIHRoYXQgY2FuIGJlIGRvbmUgYnkgY2xlYXJpbmcgb3Igc2V0dGluZyBhIGZsYWcg
b24gdGhlIGZpcnN0IGNhbGwgdG8NCj4gLT5xdWV1ZV9ycSBvciAtPnF1ZXVlY29tbWFuZCBpbnN0
ZWFkLiAgSW4gTlZNZSB3ZSB1c2UgUlFGX0RPTlRQUkVQIGZvcg0KPiB0aGF0LCBidXQgZm9yIFND
U0kgd2UgcHJvYmFibHQgY2FuJ3QgdXNlIHRoYXQgZ2l2ZW4gdGhhdCBpdCBoYXMgbW9yZQ0KPiBt
ZWFuaW5nIGZvciB0aGUgb2xkIHJlcXVlc3QgcGF0aC4gIEJ1dCBob3cgYWJvdXQganVzdCBhZGRp
bmcgYSBuZXcNCj4gUlFEX0RSVl9JTklUSUFMSVpFRCBvciBzaW1pbGFyIGZsYWc/DQoNCkhlbGxv
IENocmlzdG9waCwNCg0KTW9yZSBjaGFuZ2VzIHdvdWxkIGhhdmUgdG8gYmUgbWFkZSB0byBpbXBs
ZW1lbnQgdGhlIGFib3ZlIHByb3Bvc2FsIHRoYW4ganVzdA0Kc2V0dGluZyBhIGZsYWcgYXQgdGhl
IHN0YXJ0IG9mIC5xdWV1ZV9ycSgpIC8gLnF1ZXVlY29tbWFuZCgpIGZvciBhbGwNCmZpbGVzeXN0
ZW0gcmVxdWVzdHMuIFRoZSBudW1lcm91cyBjb2RlIHBhdGhzIHRoYXQgbGVhZCB0byBhIHJlcXVl
c3Qgbm90IGJlaW5nDQpleGVjdXRlZCBpbW1lZGlhdGVseSwgZS5nLiBhIFNDU0kgaG9zdCBiZWlu
ZyBidXN5IG9yIGEgcHJlcGFyYXRpb24gc3RhdHVzICE9DQpCTEtQUkVQX09LLCB3b3VsZCBoYXZl
IHRvIGJlIGluc3BlY3RlZCBhbmQgY29kZSB3b3VsZCBoYXZlIHRvIGJlIGFkZGVkIHRvIGNsZWFy
DQp0aGUgImNvbW1hbmQgaW5pdGlhbGl6ZWQiIGZsYWcgdG8gZW5zdXJlIHRoYXQgaW5pdGlhbGl6
YXRpb24gb2NjdXJzIHNob3J0bHkNCmJlZm9yZSB0aGUgZmlyc3QgdGltZSBhIGNvbW1hbmQgaXMg
ZXhlY3V0ZWQuDQoNClRoZSBjaG9pY2Ugd2UgaGF2ZSB0byBtYWtlIGlzIHRvIGFkZCBtb3JlIHN0
YXRlIGluZm9ybWF0aW9uIGFuZCBjb21wbGljYXRlZA0KY29kZSBmb3Iga2VlcGluZyB0aGF0IHN0
YXRlIGluZm9ybWF0aW9uIHVwLXRvLWRhdGUgdG8gdGhlIFNDU0kgY29yZSBvciBtYWtpbmcgYQ0K
c21hbGwgYW5kIGVhc3kgdG8gbWFpbnRhaW4gY2hhbmdlIHRvIHRoZSBibG9jayBsYXllciBjb3Jl
IHRoYXQgZG9lcyBub3QgaW52b2x2ZQ0KYW55IG5ldyBzdGF0ZSBpbmZvcm1hdGlvbi4gVGhhdCdz
IHdoeSBJJ20gYXNraW5nIHlvdSB0byByZWNvbnNpZGVyIHRoZSBwYXRjaCBhdA0KdGhlIHN0YXJ0
IG9mIHRoaXMgZS1tYWlsIHRocmVhZC4NCg0KVGhhbmtzLA0KDQpCYXJ0Lg==

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Jens Axboe]

On 08/29/2017 02:57 PM, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 10:10 +0200, Christoph Hellwig wrote:
>> 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

Re: [PATCH] block: Call .initialize_rq_fn() also for filesystem requests

[Ming Lei]

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:
> > 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.

OK.

-- 
Ming