diff for duplicates of <1504539634.3189.15.camel@wdc.com> diff --git a/a/1.txt b/N1/1.txt index b65f66f..6ebe982 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,43 +1,49 @@ -T24gTW9uLCAyMDE3LTA5LTA0IGF0IDE1OjE2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g -TW9uLCBTZXAgMDQsIDIwMTcgYXQgMDQ6MTM6MjZBTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy -b3RlOg0KPiA+IEFsbG93aW5nIGJsa19nZXRfcmVxdWVzdCgpIHRvIHN1Y2NlZWQgYWZ0ZXIgdGhl -IERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0IGlzDQo+ID4gY29tcGxldGVseSB3cm9uZyBiZWNhdXNl -IHRoYXQgY291bGQgcmVzdWx0IGluIGEgcmVxdWVzdCBiZWluZyBxdWV1ZWQgYWZ0ZXINCj4gPiB0 -aGUgREVBRCBmbGFnIGhhcyBiZWVuIHNldCwgcmVzdWx0aW5nIGluIGVpdGhlciBhIGhhbmdpbmcg -cmVxdWVzdCBvciBhIGtlcm5lbA0KPiA+IGNyYXNoLiBUaGlzIGlzIHdoeSBpdCdzIGNvbXBsZXRl -bHkgd3JvbmcgdG8gYWRkIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBjYWxsDQo+ID4gaW4gYmxr -X29sZF9nZXRfcmVxdWVzdCgpIG9yIGJsa19tcV9hbGxvY19yZXF1ZXN0KCkuIEhlbmNlIG15IE5B -SyBmb3IgYW55DQo+ID4gcGF0Y2ggdGhhdCBhZGRzIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBj -YWxsIHRvIGFueSBmdW5jdGlvbiBjYWxsZWQgZnJvbQ0KPiA+IGJsa19nZXRfcmVxdWVzdCgpLiBU -aGF0IGluY2x1ZGVzIHRoZSBwYXRjaCBhdCB0aGUgc3RhcnQgb2YgdGhpcyBlLW1haWwgdGhyZWFk -Lg0KPg0KPiBTZWUgYWJvdmUsIHRoaXMgcGF0Y2ggY2hhbmdlcyBub3RoaW5nIGFib3V0IHRoaXMg -ZmFjdCwgcGxlYXNlIGxvb2sgYXQNCj4gdGhlIHBhdGNoIGNhcmVmdWxseSBuZXh0IHRpbWUganVz -dCBiZWZvcmUgcG9zdGluZyB5b3VyIGxvbmcgY29tbWVudC4NCg0KQXJlIHlvdSByZWFsbHkgc3Vy -ZSB0aGF0IHlvdXIgcGF0Y2ggZG9lcyBub3QgYWxsb3cgYmxrX2dldF9yZXF1ZXN0KCkgdG8NCnN1 -Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0PyBibGtfbXFfYWxsb2NfcmVx -dWVzdCgpIGNhbGxzIGJvdGgNCmJsa19xdWV1ZV9pc19wcmVlbXB0X2Zyb3plbigpIGFuZCBibGtf -cXVldWVfZW50ZXJfbGl2ZSgpIHdpdGhvdXQgaG9sZGluZw0KYW55IGxvY2suIEEgdGhyZWFkIHRo -YXQgaXMgcnVubmluZyBjb25jdXJyZW50bHkgd2l0aCBibGtfbXFfZ2V0X3JlcXVlc3QoKQ0KY2Fu -IHVuZnJlZXplIHRoZSBxdWV1ZSBhZnRlciBibGtfcXVldWVfaXNfcHJlZW1wdF9mcm96ZW4oKSBy -ZXR1cm5lZCBhbmQNCmJlZm9yZSBibGtfcXVldWVfZW50ZXJfbGl2ZSgpIGlzIGNhbGxlZC4gVGhp -cyBtZWFucyB0aGF0IHdpdGggeW91ciBwYXRjaA0Kc2VyaWVzIGFwcGxpZWQgYmxrX2dldF9yZXF1 -ZXN0KCkgY2FuIHN1Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4NCnNldCwgd2hp -Y2ggaXMgc29tZXRoaW5nIHdlIGRvbid0IHdhbnQuIEFkZGl0aW9uYWxseSwgSSBkb24ndCB0aGlu -ayB3ZSB3YW50DQp0byBpbnRyb2R1Y2UgYW55IGtpbmQgb2YgbG9ja2luZyBpbiBibGtfbXFfZ2V0 -X3JlcXVlc3QoKSBiZWNhdXNlIHRoYXQgd291bGQNCmJlIGEgc2VyaWFsaXphdGlvbiBwb2ludC4N -Cg0KSGF2ZSB5b3UgY29uc2lkZXJlZCB0byB1c2UgdGhlIGJsay1tcSAicmVzZXJ2ZWQgcmVxdWVz -dCIgbWVjaGFuaXNtIHRvIGF2b2lkDQpzdGFydmF0aW9uIG9mIHBvd2VyIG1hbmFnZW1lbnQgcmVx -dWVzdHMgaW5zdGVhZCBvZiBtYWtpbmcgdGhlIGJsb2NrIGxheWVyDQpldmVuIG1vcmUgY29tcGxp -Y2F0ZWQgdGhhbiBpdCBhbHJlYWR5IGlzPw0KDQpOb3RlOiBleHRlbmRpbmcgYmxrX21xX2ZyZWV6 -ZS91bmZyZWV6ZV9xdWV1ZSgpIHRvIHRoZSBsZWdhY3kgYmxvY2sgbGF5ZXINCmNvdWxkIGJlIHVz -ZWZ1bCB0byBtYWtlIHNjc2lfd2FpdF9mb3JfcXVldWVjb21tYW5kKCkgbW9yZSBlbGVnYW50LiBI -b3dldmVyLA0KSSBkb24ndCB0aGluayB3ZSBzaG91bGQgc3BlbmQgb3VyIHRpbWUgb24gbGVnYWN5 -IGJsb2NrIGxheWVyIC8gU0NTSSBjb3JlDQpjaGFuZ2VzLiBUaGUgY29kZSBJJ20gcmVmZXJyaW5n -IHRvIGlzIHRoZSBmb2xsb3dpbmc6DQoNCi8qKg0KICogc2NzaV93YWl0X2Zvcl9xdWV1ZWNvbW1h -bmQoKSAtIHdhaXQgZm9yIG9uZ29pbmcgcXVldWVjb21tYW5kKCkgY2FsbHMNCiAqIEBzZGV2OiBT -Q1NJIGRldmljZSBwb2ludGVyLg0KICoNCiAqIFdhaXQgdW50aWwgdGhlIG9uZ29pbmcgc2hvc3Qt -Pmhvc3R0LT5xdWV1ZWNvbW1hbmQoKSBjYWxscyB0aGF0IGFyZQ0KICogaW52b2tlZCBmcm9tIHNj -c2lfcmVxdWVzdF9mbigpIGhhdmUgZmluaXNoZWQuDQogKi8NCnN0YXRpYyB2b2lkIHNjc2lfd2Fp -dF9mb3JfcXVldWVjb21tYW5kKHN0cnVjdCBzY3NpX2RldmljZSAqc2RldikNCnsNCglXQVJOX09O -X09OQ0Uoc2Rldi0+aG9zdC0+dXNlX2Jsa19tcSk7DQoNCgl3aGlsZSAoc2NzaV9yZXF1ZXN0X2Zu -X2FjdGl2ZShzZGV2KSkNCgkJbXNsZWVwKDIwKTsNCn0NCg0KQmFydC4= +On Mon, 2017-09-04 at 15:16 +0800, Ming Lei wrote: +> On Mon, Sep 04, 2017 at 04:13:26AM +0000, Bart Van Assche wrote: +> > Allowing blk_get_request() to succeed after the DYING flag has been set is +> > completely wrong because that could result in a request being queued after +> > the DEAD flag has been set, resulting in either a hanging request or a kernel +> > crash. This is why it's completely wrong to add a blk_queue_enter_live() call +> > in blk_old_get_request() or blk_mq_alloc_request(). Hence my NAK for any +> > patch that adds a blk_queue_enter_live() call to any function called from +> > blk_get_request(). That includes the patch at the start of this e-mail thread. +> +> See above, this patch changes nothing about this fact, please look at +> the patch carefully next time just before posting your long comment. + +Are you really sure that your patch does not allow blk_get_request() to +succeed after the DYING flag has been set? blk_mq_alloc_request() calls both +blk_queue_is_preempt_frozen() and blk_queue_enter_live() without holding +any lock. A thread that is running concurrently with blk_mq_get_request() +can unfreeze the queue after blk_queue_is_preempt_frozen() returned and +before blk_queue_enter_live() is called. This means that with your patch +series applied blk_get_request() can succeed after the DYING flag has been +set, which is something we don't want. Additionally, I don't think we want +to introduce any kind of locking in blk_mq_get_request() because that would +be a serialization point. + +Have you considered to use the blk-mq "reserved request" mechanism to avoid +starvation of power management requests instead of making the block layer +even more complicated than it already is? + +Note: extending blk_mq_freeze/unfreeze_queue() to the legacy block layer +could be useful to make scsi_wait_for_queuecommand() more elegant. However, +I don't think we should spend our time on legacy block layer / SCSI core +changes. The code I'm referring to is the following: + +/** + * scsi_wait_for_queuecommand() - wait for ongoing queuecommand() calls + * @sdev: SCSI device pointer. + * + * Wait until the ongoing shost->hostt->queuecommand() calls that are + * invoked from scsi_request_fn() have finished. + */ +static void scsi_wait_for_queuecommand(struct scsi_device *sdev) +{ + WARN_ON_ONCE(sdev->host->use_blk_mq); + + while (scsi_request_fn_active(sdev)) + msleep(20); +} + +Bart. diff --git a/a/content_digest b/N1/content_digest index 93538e4..83d88a8 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -18,48 +18,54 @@ " tj@kernel.org <tj@kernel.org>\0" "\00:1\0" "b\0" - "T24gTW9uLCAyMDE3LTA5LTA0IGF0IDE1OjE2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g\n" - "TW9uLCBTZXAgMDQsIDIwMTcgYXQgMDQ6MTM6MjZBTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy\n" - "b3RlOg0KPiA+IEFsbG93aW5nIGJsa19nZXRfcmVxdWVzdCgpIHRvIHN1Y2NlZWQgYWZ0ZXIgdGhl\n" - "IERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0IGlzDQo+ID4gY29tcGxldGVseSB3cm9uZyBiZWNhdXNl\n" - "IHRoYXQgY291bGQgcmVzdWx0IGluIGEgcmVxdWVzdCBiZWluZyBxdWV1ZWQgYWZ0ZXINCj4gPiB0\n" - "aGUgREVBRCBmbGFnIGhhcyBiZWVuIHNldCwgcmVzdWx0aW5nIGluIGVpdGhlciBhIGhhbmdpbmcg\n" - "cmVxdWVzdCBvciBhIGtlcm5lbA0KPiA+IGNyYXNoLiBUaGlzIGlzIHdoeSBpdCdzIGNvbXBsZXRl\n" - "bHkgd3JvbmcgdG8gYWRkIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBjYWxsDQo+ID4gaW4gYmxr\n" - "X29sZF9nZXRfcmVxdWVzdCgpIG9yIGJsa19tcV9hbGxvY19yZXF1ZXN0KCkuIEhlbmNlIG15IE5B\n" - "SyBmb3IgYW55DQo+ID4gcGF0Y2ggdGhhdCBhZGRzIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBj\n" - "YWxsIHRvIGFueSBmdW5jdGlvbiBjYWxsZWQgZnJvbQ0KPiA+IGJsa19nZXRfcmVxdWVzdCgpLiBU\n" - "aGF0IGluY2x1ZGVzIHRoZSBwYXRjaCBhdCB0aGUgc3RhcnQgb2YgdGhpcyBlLW1haWwgdGhyZWFk\n" - "Lg0KPg0KPiBTZWUgYWJvdmUsIHRoaXMgcGF0Y2ggY2hhbmdlcyBub3RoaW5nIGFib3V0IHRoaXMg\n" - "ZmFjdCwgcGxlYXNlIGxvb2sgYXQNCj4gdGhlIHBhdGNoIGNhcmVmdWxseSBuZXh0IHRpbWUganVz\n" - "dCBiZWZvcmUgcG9zdGluZyB5b3VyIGxvbmcgY29tbWVudC4NCg0KQXJlIHlvdSByZWFsbHkgc3Vy\n" - "ZSB0aGF0IHlvdXIgcGF0Y2ggZG9lcyBub3QgYWxsb3cgYmxrX2dldF9yZXF1ZXN0KCkgdG8NCnN1\n" - "Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0PyBibGtfbXFfYWxsb2NfcmVx\n" - "dWVzdCgpIGNhbGxzIGJvdGgNCmJsa19xdWV1ZV9pc19wcmVlbXB0X2Zyb3plbigpIGFuZCBibGtf\n" - "cXVldWVfZW50ZXJfbGl2ZSgpIHdpdGhvdXQgaG9sZGluZw0KYW55IGxvY2suIEEgdGhyZWFkIHRo\n" - "YXQgaXMgcnVubmluZyBjb25jdXJyZW50bHkgd2l0aCBibGtfbXFfZ2V0X3JlcXVlc3QoKQ0KY2Fu\n" - "IHVuZnJlZXplIHRoZSBxdWV1ZSBhZnRlciBibGtfcXVldWVfaXNfcHJlZW1wdF9mcm96ZW4oKSBy\n" - "ZXR1cm5lZCBhbmQNCmJlZm9yZSBibGtfcXVldWVfZW50ZXJfbGl2ZSgpIGlzIGNhbGxlZC4gVGhp\n" - "cyBtZWFucyB0aGF0IHdpdGggeW91ciBwYXRjaA0Kc2VyaWVzIGFwcGxpZWQgYmxrX2dldF9yZXF1\n" - "ZXN0KCkgY2FuIHN1Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4NCnNldCwgd2hp\n" - "Y2ggaXMgc29tZXRoaW5nIHdlIGRvbid0IHdhbnQuIEFkZGl0aW9uYWxseSwgSSBkb24ndCB0aGlu\n" - "ayB3ZSB3YW50DQp0byBpbnRyb2R1Y2UgYW55IGtpbmQgb2YgbG9ja2luZyBpbiBibGtfbXFfZ2V0\n" - "X3JlcXVlc3QoKSBiZWNhdXNlIHRoYXQgd291bGQNCmJlIGEgc2VyaWFsaXphdGlvbiBwb2ludC4N\n" - "Cg0KSGF2ZSB5b3UgY29uc2lkZXJlZCB0byB1c2UgdGhlIGJsay1tcSAicmVzZXJ2ZWQgcmVxdWVz\n" - "dCIgbWVjaGFuaXNtIHRvIGF2b2lkDQpzdGFydmF0aW9uIG9mIHBvd2VyIG1hbmFnZW1lbnQgcmVx\n" - "dWVzdHMgaW5zdGVhZCBvZiBtYWtpbmcgdGhlIGJsb2NrIGxheWVyDQpldmVuIG1vcmUgY29tcGxp\n" - "Y2F0ZWQgdGhhbiBpdCBhbHJlYWR5IGlzPw0KDQpOb3RlOiBleHRlbmRpbmcgYmxrX21xX2ZyZWV6\n" - "ZS91bmZyZWV6ZV9xdWV1ZSgpIHRvIHRoZSBsZWdhY3kgYmxvY2sgbGF5ZXINCmNvdWxkIGJlIHVz\n" - "ZWZ1bCB0byBtYWtlIHNjc2lfd2FpdF9mb3JfcXVldWVjb21tYW5kKCkgbW9yZSBlbGVnYW50LiBI\n" - "b3dldmVyLA0KSSBkb24ndCB0aGluayB3ZSBzaG91bGQgc3BlbmQgb3VyIHRpbWUgb24gbGVnYWN5\n" - "IGJsb2NrIGxheWVyIC8gU0NTSSBjb3JlDQpjaGFuZ2VzLiBUaGUgY29kZSBJJ20gcmVmZXJyaW5n\n" - "IHRvIGlzIHRoZSBmb2xsb3dpbmc6DQoNCi8qKg0KICogc2NzaV93YWl0X2Zvcl9xdWV1ZWNvbW1h\n" - "bmQoKSAtIHdhaXQgZm9yIG9uZ29pbmcgcXVldWVjb21tYW5kKCkgY2FsbHMNCiAqIEBzZGV2OiBT\n" - "Q1NJIGRldmljZSBwb2ludGVyLg0KICoNCiAqIFdhaXQgdW50aWwgdGhlIG9uZ29pbmcgc2hvc3Qt\n" - "Pmhvc3R0LT5xdWV1ZWNvbW1hbmQoKSBjYWxscyB0aGF0IGFyZQ0KICogaW52b2tlZCBmcm9tIHNj\n" - "c2lfcmVxdWVzdF9mbigpIGhhdmUgZmluaXNoZWQuDQogKi8NCnN0YXRpYyB2b2lkIHNjc2lfd2Fp\n" - "dF9mb3JfcXVldWVjb21tYW5kKHN0cnVjdCBzY3NpX2RldmljZSAqc2RldikNCnsNCglXQVJOX09O\n" - "X09OQ0Uoc2Rldi0+aG9zdC0+dXNlX2Jsa19tcSk7DQoNCgl3aGlsZSAoc2NzaV9yZXF1ZXN0X2Zu\n" - X2FjdGl2ZShzZGV2KSkNCgkJbXNsZWVwKDIwKTsNCn0NCg0KQmFydC4= + "On Mon, 2017-09-04 at 15:16 +0800, Ming Lei wrote:\n" + "> On Mon, Sep 04, 2017 at 04:13:26AM +0000, Bart Van Assche wrote:\n" + "> > Allowing blk_get_request() to succeed after the DYING flag has been set is\n" + "> > completely wrong because that could result in a request being queued after\n" + "> > the DEAD flag has been set, resulting in either a hanging request or a kernel\n" + "> > crash. This is why it's completely wrong to add a blk_queue_enter_live() call\n" + "> > in blk_old_get_request() or blk_mq_alloc_request(). Hence my NAK for any\n" + "> > patch that adds a blk_queue_enter_live() call to any function called from\n" + "> > blk_get_request(). That includes the patch at the start of this e-mail thread.\n" + ">\n" + "> See above, this patch changes nothing about this fact, please look at\n" + "> the patch carefully next time just before posting your long comment.\n" + "\n" + "Are you really sure that your patch does not allow blk_get_request() to\n" + "succeed after the DYING flag has been set? blk_mq_alloc_request() calls both\n" + "blk_queue_is_preempt_frozen() and blk_queue_enter_live() without holding\n" + "any lock. A thread that is running concurrently with blk_mq_get_request()\n" + "can unfreeze the queue after blk_queue_is_preempt_frozen() returned and\n" + "before blk_queue_enter_live() is called. This means that with your patch\n" + "series applied blk_get_request() can succeed after the DYING flag has been\n" + "set, which is something we don't want. Additionally, I don't think we want\n" + "to introduce any kind of locking in blk_mq_get_request() because that would\n" + "be a serialization point.\n" + "\n" + "Have you considered to use the blk-mq \"reserved request\" mechanism to avoid\n" + "starvation of power management requests instead of making the block layer\n" + "even more complicated than it already is?\n" + "\n" + "Note: extending blk_mq_freeze/unfreeze_queue() to the legacy block layer\n" + "could be useful to make scsi_wait_for_queuecommand() more elegant. However,\n" + "I don't think we should spend our time on legacy block layer / SCSI core\n" + "changes. The code I'm referring to is the following:\n" + "\n" + "/**\n" + " * scsi_wait_for_queuecommand() - wait for ongoing queuecommand() calls\n" + " * @sdev: SCSI device pointer.\n" + " *\n" + " * Wait until the ongoing shost->hostt->queuecommand() calls that are\n" + " * invoked from scsi_request_fn() have finished.\n" + " */\n" + "static void scsi_wait_for_queuecommand(struct scsi_device *sdev)\n" + "{\n" + "\tWARN_ON_ONCE(sdev->host->use_blk_mq);\n" + "\n" + "\twhile (scsi_request_fn_active(sdev))\n" + "\t\tmsleep(20);\n" + "}\n" + "\n" + Bart. -081c30dab8f23d0035f9dc5e3ad14b9974e67b255578afcdce67e9aef180dccb +4cf2b85046e3c945ec7ee46722a7a9e5ddacdf1e889bf8513acdef67c3ddbc64
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.