diff for duplicates of <1509991480.2409.54.camel@wdc.com> diff --git a/a/1.txt b/N1/1.txt index 27a90e3..a618aaa 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,48 +1,56 @@ -T24gU2F0LCAyMDE3LTExLTA0IGF0IDA5OjU1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gSXQg -aXMgdmVyeSBleHBlbnNpdmUgdG8gYXRvbWljX2luYy9hdG9taWNfZGVjIHRoZSBob3N0IHdpZGUg -Y291bnRlciBvZg0KPiBob3N0LT5idXN5X2NvdW50LCBhbmQgaXQgc2hvdWxkIGhhdmUgYmVlbiBh -dm9pZGVkIHZpYSBibGstbXEncyBtZWNoYW5pc20NCj4gb2YgZ2V0dGluZyBkcml2ZXIgdGFnLCB3 -aGljaCB1c2VzIHRoZSBtb3JlIGVmZmljaWVudCB3YXkgb2Ygc2JpdG1hcCBxdWV1ZS4NCg0KRGlk -IHlvdSBwZXJoYXBzIG1lYW4gdGhlIGhvc3QtPmhvc3RfYnVzeSBjb3VudGVyPyBVbnJlbGF0ZWQg -dG8gdGhpcyBwYXRjaDoNCkkgdGhpbmsgdGhhdCBjb21taXQgNjRkNTEzYWMzMWJkICgic2NzaTog -dXNlIGhvc3Qgd2lkZSB0YWdzIGJ5IGRlZmF1bHQiKSBtYWRlDQp0aGF0IGNvdW50ZXIgc3VwZXJm -bHVvdXMuDQoNCj4gQWxzbyB3ZSBkb24ndCBjaGVjayBhdG9taWNfcmVhZCgmc2Rldi0+ZGV2aWNl -X2J1c3kpIGluIHNjc2lfbXFfZ2V0X2J1ZGdldCgpDQo+IGFuZCBkb24ndCBydW4gcXVldWUgaWYg -dGhlIGNvdW50ZXIgYmVjb21lcyB6ZXJvLCBzbyBJTyBoYW5nIG1heSBiZSBjYXVzZWQNCj4gaWYg -YWxsIHJlcXVlc3RzIGFyZSBjb21wbGV0ZWQganVzdCBiZWZvcmUgdGhlIGN1cnJlbnQgU0NTSSBk -ZXZpY2UNCj4gaXMgYWRkZWQgdG8gc2hvc3QtPnN0YXJ2ZWRfbGlzdC4NCg0KV2hhdCBpcyB0aGUg -cmVsYXRpb25zaGlwIGJldHdlZW4gdGhlIGFib3ZlIGRlc2NyaXB0aW9uIGFuZCB0aGUgY29kZSBj -aGFuZ2VzDQpiZWxvdz8gVGhlIGFib3ZlIGRlc2NyaXB0aW9uIGRvZXMgbm90IGV4cGxhaW4gd2hl -dGhlciB0aGUgc2NzaV9tcV9nZXQvcHV0X2J1ZGdldCgpDQpjaGFuZ2VzIGJlbG93IHByZXZlbnQg -dGhhdCBhbGwgb3V0c3RhbmRpbmcgU0NTSSByZXF1ZXN0cyBjb21wbGV0ZSBiZWZvcmUNCmFub3Ro -ZXIgcXVldWUgaXMgYWRkZWQgdG8gdGhlIHN0YXJ2ZWQgbGlzdC4NCg0KSXMgdGhpcyBwZXJoYXBz -IGFuIGF0dGVtcHQgdG8gbW92ZSB0aGUgY29kZSB0aGF0IGNhbiBhZGQgYSByZXF1ZXN0IHF1ZXVl -IHRvDQoic3RhcnZlZF9saXN0IiBmcm9tIGluc2lkZSBzY3NpX21xX2dldF9idWRnZXQoKSBpbnRv -IHNjc2lfcXVldWVfcnEoKT8gRG9lcw0KdGhpcyBwYXRjaCBtb3JlIHRoYW4gcmVkdWNpbmcgdGhl -IHByb2JhYmlsaXR5IHRoYXQgdGhlIHJhY2UgaXMgZW5jb3VudGVyZWQNCnRoYXQgYSBxdWV1ZSBp -cyBhZGRlZCB0byAic3RhcnZlZF9saXN0IiBhZnRlciBhbGwgcmVxdWVzdHMgaGF2ZSBmaW5pc2hl -ZD8NCg0KPiBGaXhlczogMGRmMjFjODZiZGJmKHNjc2k6IGltcGxlbWVudCAuZ2V0X2J1ZGdldCBh -bmQgLnB1dF9idWRnZXQgZm9yIGJsay1tcSkNCj4gUmVwb3J0ZWQtYnk6IEJhcnQgVmFuIEFzc2No -ZSA8YmFydC52YW5hc3NjaGVAd2RjLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTogTWluZyBMZWkgPG1p -bmcubGVpQHJlZGhhdC5jb20+DQoNClNpbmNlIHRoaXMgaXMgYSBTQ1NJIHBhdGNoIHRoZSBTQ1NJ -IG1haW50YWluZXIsIE1hcnRpbiBQZXRlcnNlbiwgc2hvdWxkIGhhdmUNCmJlZW4gQ2MtZWQgZm9y -IHRoaXMgcGF0Y2guIEFkZGl0aW9uYWxseSwgSSB0aGluayB0aGF0IHRoaXMgcGF0Y2ggc2hvdWxk -IG5vdA0KaGF2ZSBiZWVuIHF1ZXVlZCBieSBKZW5zIGJlZm9yZSBNYXJ0aW4gaGFkIGFwcHJvdmVk -IHRoaXMgcGF0Y2guDQoNCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jIGIv -ZHJpdmVycy9zY3NpL3Njc2lfbGliLmMNCj4gaW5kZXggYTA5OGFmMzA3MGExLi43ZjIxOGVmNjE5 -MDAgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jDQo+ICsrKyBiL2RyaXZl -cnMvc2NzaS9zY3NpX2xpYi5jDQo+IEBAIC0xOTQ0LDExICsxOTQ0LDcgQEAgc3RhdGljIHZvaWQg -c2NzaV9tcV9wdXRfYnVkZ2V0KHN0cnVjdCBibGtfbXFfaHdfY3R4ICpoY3R4KQ0KPiAgew0KPiAg -CXN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxID0gaGN0eC0+cXVldWU7DQo+ICAJc3RydWN0IHNjc2lf -ZGV2aWNlICpzZGV2ID0gcS0+cXVldWVkYXRhOw0KPiAtCXN0cnVjdCBTY3NpX0hvc3QgKnNob3N0 -ID0gc2Rldi0+aG9zdDsNCj4gIA0KPiAtCWF0b21pY19kZWMoJnNob3N0LT5ob3N0X2J1c3kpOw0K -PiAtCWlmIChzY3NpX3RhcmdldChzZGV2KS0+Y2FuX3F1ZXVlID4gMCkNCj4gLQkJYXRvbWljX2Rl -Yygmc2NzaV90YXJnZXQoc2RldiktPnRhcmdldF9idXN5KTsNCj4gIAlhdG9taWNfZGVjKCZzZGV2 -LT5kZXZpY2VfYnVzeSk7DQo+ICAJcHV0X2RldmljZSgmc2Rldi0+c2Rldl9nZW5kZXYpOw0KPiAg -fQ0KDQpzY3NpX21xX2dldC9wdXRfYnVkZ2V0KCkgd2VyZSBpbnRyb2R1Y2VkIHRvIGltcHJvdmUg -c2VxdWVudGlhbCBJL08NCnBlcmZvcm1hbmNlLiBEb2VzIHJlbW92aW5nIHRoZSBTQ1NJIHRhcmdl -dCBidXN5IGNoZWNrIGhhdmUgYSBuZWdhdGl2ZQ0KcGVyZm9ybWFuY2Ugb24gc2VxdWVudGlhbCBJ -L08gZm9yIGUuZy4gdGhlIGlTRVIgaW5pdGlhdG9yIGRyaXZlcj8gVGhlIGlTQ1NJDQpvdmVyIFRD -UCBpbml0aWF0b3IgZHJpdmVyIGlzIG5vdCBhcHByb3ByaWF0ZSBmb3IgdGVzdGluZyBwZXJmb3Jt -YW5jZQ0KcmVncmVzc2lvbnMgYmVjYXVzZSBpdCBsaW1pdHMgdGhlIGlTQ1NJIHBhcmFtZXRlciBN -YXhPdXRzdGFuZGluZ1IyVCB0byBvbmUuDQoNCkJhcnQu +On Sat, 2017-11-04 at 09:55 +0800, Ming Lei wrote: +> It is very expensive to atomic_inc/atomic_dec the host wide counter of +> host->busy_count, and it should have been avoided via blk-mq's mechanism +> of getting driver tag, which uses the more efficient way of sbitmap queue. + +Did you perhaps mean the host->host_busy counter? Unrelated to this patch: +I think that commit 64d513ac31bd ("scsi: use host wide tags by default") made +that counter superfluous. + +> Also we don't check atomic_read(&sdev->device_busy) in scsi_mq_get_budget() +> and don't run queue if the counter becomes zero, so IO hang may be caused +> if all requests are completed just before the current SCSI device +> is added to shost->starved_list. + +What is the relationship between the above description and the code changes +below? The above description does not explain whether the scsi_mq_get/put_budget() +changes below prevent that all outstanding SCSI requests complete before +another queue is added to the starved list. + +Is this perhaps an attempt to move the code that can add a request queue to +"starved_list" from inside scsi_mq_get_budget() into scsi_queue_rq()? Does +this patch more than reducing the probability that the race is encountered +that a queue is added to "starved_list" after all requests have finished? + +> Fixes: 0df21c86bdbf(scsi: implement .get_budget and .put_budget for blk-mq) +> Reported-by: Bart Van Assche <bart.vanassche@wdc.com> +> Signed-off-by: Ming Lei <ming.lei@redhat.com> + +Since this is a SCSI patch the SCSI maintainer, Martin Petersen, should have +been Cc-ed for this patch. Additionally, I think that this patch should not +have been queued by Jens before Martin had approved this patch. + +> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c +> index a098af3070a1..7f218ef61900 100644 +> --- a/drivers/scsi/scsi_lib.c +> +++ b/drivers/scsi/scsi_lib.c +> @@ -1944,11 +1944,7 @@ static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx) +> { +> struct request_queue *q = hctx->queue; +> struct scsi_device *sdev = q->queuedata; +> - struct Scsi_Host *shost = sdev->host; +> +> - atomic_dec(&shost->host_busy); +> - if (scsi_target(sdev)->can_queue > 0) +> - atomic_dec(&scsi_target(sdev)->target_busy); +> atomic_dec(&sdev->device_busy); +> put_device(&sdev->sdev_gendev); +> } + +scsi_mq_get/put_budget() were introduced to improve sequential I/O +performance. Does removing the SCSI target busy check have a negative +performance on sequential I/O for e.g. the iSER initiator driver? The iSCSI +over TCP initiator driver is not appropriate for testing performance +regressions because it limits the iSCSI parameter MaxOutstandingR2T to one. + +Bart. diff --git a/a/content_digest b/N1/content_digest index 533d322..ea63d1d 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -12,53 +12,61 @@ " loberman@redhat.com <loberman@redhat.com>\0" "\00:1\0" "b\0" - "T24gU2F0LCAyMDE3LTExLTA0IGF0IDA5OjU1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gSXQg\n" - "aXMgdmVyeSBleHBlbnNpdmUgdG8gYXRvbWljX2luYy9hdG9taWNfZGVjIHRoZSBob3N0IHdpZGUg\n" - "Y291bnRlciBvZg0KPiBob3N0LT5idXN5X2NvdW50LCBhbmQgaXQgc2hvdWxkIGhhdmUgYmVlbiBh\n" - "dm9pZGVkIHZpYSBibGstbXEncyBtZWNoYW5pc20NCj4gb2YgZ2V0dGluZyBkcml2ZXIgdGFnLCB3\n" - "aGljaCB1c2VzIHRoZSBtb3JlIGVmZmljaWVudCB3YXkgb2Ygc2JpdG1hcCBxdWV1ZS4NCg0KRGlk\n" - "IHlvdSBwZXJoYXBzIG1lYW4gdGhlIGhvc3QtPmhvc3RfYnVzeSBjb3VudGVyPyBVbnJlbGF0ZWQg\n" - "dG8gdGhpcyBwYXRjaDoNCkkgdGhpbmsgdGhhdCBjb21taXQgNjRkNTEzYWMzMWJkICgic2NzaTog\n" - "dXNlIGhvc3Qgd2lkZSB0YWdzIGJ5IGRlZmF1bHQiKSBtYWRlDQp0aGF0IGNvdW50ZXIgc3VwZXJm\n" - "bHVvdXMuDQoNCj4gQWxzbyB3ZSBkb24ndCBjaGVjayBhdG9taWNfcmVhZCgmc2Rldi0+ZGV2aWNl\n" - "X2J1c3kpIGluIHNjc2lfbXFfZ2V0X2J1ZGdldCgpDQo+IGFuZCBkb24ndCBydW4gcXVldWUgaWYg\n" - "dGhlIGNvdW50ZXIgYmVjb21lcyB6ZXJvLCBzbyBJTyBoYW5nIG1heSBiZSBjYXVzZWQNCj4gaWYg\n" - "YWxsIHJlcXVlc3RzIGFyZSBjb21wbGV0ZWQganVzdCBiZWZvcmUgdGhlIGN1cnJlbnQgU0NTSSBk\n" - "ZXZpY2UNCj4gaXMgYWRkZWQgdG8gc2hvc3QtPnN0YXJ2ZWRfbGlzdC4NCg0KV2hhdCBpcyB0aGUg\n" - "cmVsYXRpb25zaGlwIGJldHdlZW4gdGhlIGFib3ZlIGRlc2NyaXB0aW9uIGFuZCB0aGUgY29kZSBj\n" - "aGFuZ2VzDQpiZWxvdz8gVGhlIGFib3ZlIGRlc2NyaXB0aW9uIGRvZXMgbm90IGV4cGxhaW4gd2hl\n" - "dGhlciB0aGUgc2NzaV9tcV9nZXQvcHV0X2J1ZGdldCgpDQpjaGFuZ2VzIGJlbG93IHByZXZlbnQg\n" - "dGhhdCBhbGwgb3V0c3RhbmRpbmcgU0NTSSByZXF1ZXN0cyBjb21wbGV0ZSBiZWZvcmUNCmFub3Ro\n" - "ZXIgcXVldWUgaXMgYWRkZWQgdG8gdGhlIHN0YXJ2ZWQgbGlzdC4NCg0KSXMgdGhpcyBwZXJoYXBz\n" - "IGFuIGF0dGVtcHQgdG8gbW92ZSB0aGUgY29kZSB0aGF0IGNhbiBhZGQgYSByZXF1ZXN0IHF1ZXVl\n" - "IHRvDQoic3RhcnZlZF9saXN0IiBmcm9tIGluc2lkZSBzY3NpX21xX2dldF9idWRnZXQoKSBpbnRv\n" - "IHNjc2lfcXVldWVfcnEoKT8gRG9lcw0KdGhpcyBwYXRjaCBtb3JlIHRoYW4gcmVkdWNpbmcgdGhl\n" - "IHByb2JhYmlsaXR5IHRoYXQgdGhlIHJhY2UgaXMgZW5jb3VudGVyZWQNCnRoYXQgYSBxdWV1ZSBp\n" - "cyBhZGRlZCB0byAic3RhcnZlZF9saXN0IiBhZnRlciBhbGwgcmVxdWVzdHMgaGF2ZSBmaW5pc2hl\n" - "ZD8NCg0KPiBGaXhlczogMGRmMjFjODZiZGJmKHNjc2k6IGltcGxlbWVudCAuZ2V0X2J1ZGdldCBh\n" - "bmQgLnB1dF9idWRnZXQgZm9yIGJsay1tcSkNCj4gUmVwb3J0ZWQtYnk6IEJhcnQgVmFuIEFzc2No\n" - "ZSA8YmFydC52YW5hc3NjaGVAd2RjLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTogTWluZyBMZWkgPG1p\n" - "bmcubGVpQHJlZGhhdC5jb20+DQoNClNpbmNlIHRoaXMgaXMgYSBTQ1NJIHBhdGNoIHRoZSBTQ1NJ\n" - "IG1haW50YWluZXIsIE1hcnRpbiBQZXRlcnNlbiwgc2hvdWxkIGhhdmUNCmJlZW4gQ2MtZWQgZm9y\n" - "IHRoaXMgcGF0Y2guIEFkZGl0aW9uYWxseSwgSSB0aGluayB0aGF0IHRoaXMgcGF0Y2ggc2hvdWxk\n" - "IG5vdA0KaGF2ZSBiZWVuIHF1ZXVlZCBieSBKZW5zIGJlZm9yZSBNYXJ0aW4gaGFkIGFwcHJvdmVk\n" - "IHRoaXMgcGF0Y2guDQoNCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jIGIv\n" - "ZHJpdmVycy9zY3NpL3Njc2lfbGliLmMNCj4gaW5kZXggYTA5OGFmMzA3MGExLi43ZjIxOGVmNjE5\n" - "MDAgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jDQo+ICsrKyBiL2RyaXZl\n" - "cnMvc2NzaS9zY3NpX2xpYi5jDQo+IEBAIC0xOTQ0LDExICsxOTQ0LDcgQEAgc3RhdGljIHZvaWQg\n" - "c2NzaV9tcV9wdXRfYnVkZ2V0KHN0cnVjdCBibGtfbXFfaHdfY3R4ICpoY3R4KQ0KPiAgew0KPiAg\n" - "CXN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxID0gaGN0eC0+cXVldWU7DQo+ICAJc3RydWN0IHNjc2lf\n" - "ZGV2aWNlICpzZGV2ID0gcS0+cXVldWVkYXRhOw0KPiAtCXN0cnVjdCBTY3NpX0hvc3QgKnNob3N0\n" - "ID0gc2Rldi0+aG9zdDsNCj4gIA0KPiAtCWF0b21pY19kZWMoJnNob3N0LT5ob3N0X2J1c3kpOw0K\n" - "PiAtCWlmIChzY3NpX3RhcmdldChzZGV2KS0+Y2FuX3F1ZXVlID4gMCkNCj4gLQkJYXRvbWljX2Rl\n" - "Yygmc2NzaV90YXJnZXQoc2RldiktPnRhcmdldF9idXN5KTsNCj4gIAlhdG9taWNfZGVjKCZzZGV2\n" - "LT5kZXZpY2VfYnVzeSk7DQo+ICAJcHV0X2RldmljZSgmc2Rldi0+c2Rldl9nZW5kZXYpOw0KPiAg\n" - "fQ0KDQpzY3NpX21xX2dldC9wdXRfYnVkZ2V0KCkgd2VyZSBpbnRyb2R1Y2VkIHRvIGltcHJvdmUg\n" - "c2VxdWVudGlhbCBJL08NCnBlcmZvcm1hbmNlLiBEb2VzIHJlbW92aW5nIHRoZSBTQ1NJIHRhcmdl\n" - "dCBidXN5IGNoZWNrIGhhdmUgYSBuZWdhdGl2ZQ0KcGVyZm9ybWFuY2Ugb24gc2VxdWVudGlhbCBJ\n" - "L08gZm9yIGUuZy4gdGhlIGlTRVIgaW5pdGlhdG9yIGRyaXZlcj8gVGhlIGlTQ1NJDQpvdmVyIFRD\n" - "UCBpbml0aWF0b3IgZHJpdmVyIGlzIG5vdCBhcHByb3ByaWF0ZSBmb3IgdGVzdGluZyBwZXJmb3Jt\n" - "YW5jZQ0KcmVncmVzc2lvbnMgYmVjYXVzZSBpdCBsaW1pdHMgdGhlIGlTQ1NJIHBhcmFtZXRlciBN\n" - YXhPdXRzdGFuZGluZ1IyVCB0byBvbmUuDQoNCkJhcnQu + "On Sat, 2017-11-04 at 09:55 +0800, Ming Lei wrote:\n" + "> It is very expensive to atomic_inc/atomic_dec the host wide counter of\n" + "> host->busy_count, and it should have been avoided via blk-mq's mechanism\n" + "> of getting driver tag, which uses the more efficient way of sbitmap queue.\n" + "\n" + "Did you perhaps mean the host->host_busy counter? Unrelated to this patch:\n" + "I think that commit 64d513ac31bd (\"scsi: use host wide tags by default\") made\n" + "that counter superfluous.\n" + "\n" + "> Also we don't check atomic_read(&sdev->device_busy) in scsi_mq_get_budget()\n" + "> and don't run queue if the counter becomes zero, so IO hang may be caused\n" + "> if all requests are completed just before the current SCSI device\n" + "> is added to shost->starved_list.\n" + "\n" + "What is the relationship between the above description and the code changes\n" + "below? The above description does not explain whether the scsi_mq_get/put_budget()\n" + "changes below prevent that all outstanding SCSI requests complete before\n" + "another queue is added to the starved list.\n" + "\n" + "Is this perhaps an attempt to move the code that can add a request queue to\n" + "\"starved_list\" from inside scsi_mq_get_budget() into scsi_queue_rq()? Does\n" + "this patch more than reducing the probability that the race is encountered\n" + "that a queue is added to \"starved_list\" after all requests have finished?\n" + "\n" + "> Fixes: 0df21c86bdbf(scsi: implement .get_budget and .put_budget for blk-mq)\n" + "> Reported-by: Bart Van Assche <bart.vanassche@wdc.com>\n" + "> Signed-off-by: Ming Lei <ming.lei@redhat.com>\n" + "\n" + "Since this is a SCSI patch the SCSI maintainer, Martin Petersen, should have\n" + "been Cc-ed for this patch. Additionally, I think that this patch should not\n" + "have been queued by Jens before Martin had approved this patch.\n" + "\n" + "> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c\n" + "> index a098af3070a1..7f218ef61900 100644\n" + "> --- a/drivers/scsi/scsi_lib.c\n" + "> +++ b/drivers/scsi/scsi_lib.c\n" + "> @@ -1944,11 +1944,7 @@ static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)\n" + "> {\n" + "> \tstruct request_queue *q = hctx->queue;\n" + "> \tstruct scsi_device *sdev = q->queuedata;\n" + "> -\tstruct Scsi_Host *shost = sdev->host;\n" + "> \n" + "> -\tatomic_dec(&shost->host_busy);\n" + "> -\tif (scsi_target(sdev)->can_queue > 0)\n" + "> -\t\tatomic_dec(&scsi_target(sdev)->target_busy);\n" + "> \tatomic_dec(&sdev->device_busy);\n" + "> \tput_device(&sdev->sdev_gendev);\n" + "> }\n" + "\n" + "scsi_mq_get/put_budget() were introduced to improve sequential I/O\n" + "performance. Does removing the SCSI target busy check have a negative\n" + "performance on sequential I/O for e.g. the iSER initiator driver? The iSCSI\n" + "over TCP initiator driver is not appropriate for testing performance\n" + "regressions because it limits the iSCSI parameter MaxOutstandingR2T to one.\n" + "\n" + Bart. -fa636e7d5afdff0b1788ec1047f4130d02ca12c677c901a6ee9bd507beebaf72 +214ec207eb4d9fffd8d2d4460f3de6cfb5fe43896b80a311a23a413f592a1b7f
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.