diff for duplicates of <1504541911.3189.18.camel@wdc.com> diff --git a/a/1.txt b/N1/1.txt index c79a9e4..825dee3 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,41 +1,38 @@ -T24gVHVlLCAyMDE3LTA5LTA1IGF0IDAwOjA4ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g -TW9uLCBTZXAgMDQsIDIwMTcgYXQgMDM6NDA6MzVQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy -b3RlOg0KPiA+IE9uIE1vbiwgMjAxNy0wOS0wNCBhdCAxNToxNiArMDgwMCwgTWluZyBMZWkgd3Jv -dGU6DQo+ID4gPiBPbiBNb24sIFNlcCAwNCwgMjAxNyBhdCAwNDoxMzoyNkFNICswMDAwLCBCYXJ0 -IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+IEFsbG93aW5nIGJsa19nZXRfcmVxdWVzdCgpIHRv -IHN1Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0IGlzDQo+ID4gPiA+IGNv -bXBsZXRlbHkgd3JvbmcgYmVjYXVzZSB0aGF0IGNvdWxkIHJlc3VsdCBpbiBhIHJlcXVlc3QgYmVp -bmcgcXVldWVkIGFmdGVyDQo+ID4gPiA+IHRoZSBERUFEIGZsYWcgaGFzIGJlZW4gc2V0LCByZXN1 -bHRpbmcgaW4gZWl0aGVyIGEgaGFuZ2luZyByZXF1ZXN0IG9yIGEga2VybmVsDQo+ID4gPiA+IGNy -YXNoLiBUaGlzIGlzIHdoeSBpdCdzIGNvbXBsZXRlbHkgd3JvbmcgdG8gYWRkIGEgYmxrX3F1ZXVl -X2VudGVyX2xpdmUoKSBjYWxsDQo+ID4gPiA+IGluIGJsa19vbGRfZ2V0X3JlcXVlc3QoKSBvciBi -bGtfbXFfYWxsb2NfcmVxdWVzdCgpLiBIZW5jZSBteSBOQUsgZm9yIGFueQ0KPiA+ID4gPiBwYXRj -aCB0aGF0IGFkZHMgYSBibGtfcXVldWVfZW50ZXJfbGl2ZSgpIGNhbGwgdG8gYW55IGZ1bmN0aW9u -IGNhbGxlZCBmcm9tDQo+ID4gPiA+IGJsa19nZXRfcmVxdWVzdCgpLiBUaGF0IGluY2x1ZGVzIHRo -ZSBwYXRjaCBhdCB0aGUgc3RhcnQgb2YgdGhpcyBlLW1haWwgdGhyZWFkLg0KPiA+ID4gDQo+ID4g -PiBTZWUgYWJvdmUsIHRoaXMgcGF0Y2ggY2hhbmdlcyBub3RoaW5nIGFib3V0IHRoaXMgZmFjdCwg -cGxlYXNlIGxvb2sgYXQNCj4gPiA+IHRoZSBwYXRjaCBjYXJlZnVsbHkgbmV4dCB0aW1lIGp1c3Qg -YmVmb3JlIHBvc3RpbmcgeW91ciBsb25nIGNvbW1lbnQuDQo+ID4gDQo+ID4gQXJlIHlvdSByZWFs -bHkgc3VyZSB0aGF0IHlvdXIgcGF0Y2ggZG9lcyBub3QgYWxsb3cgYmxrX2dldF9yZXF1ZXN0KCkg -dG8NCj4gPiBzdWNjZWVkIGFmdGVyIHRoZSBEWUlORyBmbGFnIGhhcyBiZWVuIHNldD8gYmxrX21x -X2FsbG9jX3JlcXVlc3QoKSBjYWxscyBib3RoDQo+ID4gYmxrX3F1ZXVlX2lzX3ByZWVtcHRfZnJv -emVuKCkgYW5kIGJsa19xdWV1ZV9lbnRlcl9saXZlKCkgd2l0aG91dCBob2xkaW5nDQo+ID4gYW55 -IGxvY2suIEEgdGhyZWFkIHRoYXQgaXMgcnVubmluZyBjb25jdXJyZW50bHkgd2l0aCBibGtfbXFf -Z2V0X3JlcXVlc3QoKQ0KPiA+IGNhbiB1bmZyZWV6ZSB0aGUgcXVldWUgYWZ0ZXIgYmxrX3F1ZXVl -X2lzX3ByZWVtcHRfZnJvemVuKCkgcmV0dXJuZWQgYW5kDQo+ID4gYmVmb3JlIGJsa19xdWV1ZV9l -bnRlcl9saXZlKCkgaXMgY2FsbGVkLiBUaGlzIG1lYW5zIHRoYXQgd2l0aCB5b3VyIHBhdGNoDQo+ -ID4gc2VyaWVzIGFwcGxpZWQgYmxrX2dldF9yZXF1ZXN0KCkgY2FuIHN1Y2NlZWQgYWZ0ZXIgdGhl -IERZSU5HIGZsYWcgaGFzIGJlZW4NCj4gPiBzZXQsIHdoaWNoIGlzIHNvbWV0aGluZyB3ZSBkb24n -dCB3YW50LiBBZGRpdGlvbmFsbHksIEkgZG9uJ3QgdGhpbmsgd2Ugd2FudA0KPiA+IHRvIGludHJv -ZHVjZSBhbnkga2luZCBvZiBsb2NraW5nIGluIGJsa19tcV9nZXRfcmVxdWVzdCgpIGJlY2F1c2Ug -dGhhdCB3b3VsZA0KPiA+IGJlIGEgc2VyaWFsaXphdGlvbiBwb2ludC4NCj4NCj4gWWVhaCwgSSBh -bSBwcmV0dHkgc3VyZS4NCj4gDQo+IEZpcnN0bHkgYmxrX3F1ZXVlX2ZyZWV6ZV9wcmVlbXB0KCkg -aXMgZXhjbHVzaXZlLCB0aGF0IG1lYW5zIGl0IHdpbGwgd2FpdA0KPiBmb3IgY29tcGxldGlvbiBv -ZiBhbGwgcGVuZGluZyBmcmVlemluZyhib3RoIG5vcm1hbCBhbmQgcHJlZW1wdCksIGFuZCBvdGhl -cg0KPiBmcmVlemluZyBjYW4ndCBiZSBzdGFydGVkIHRvbyBpZiB0aGVyZSBpcyBpbi1wcm9ncmVz -cyBwcmVlbXB0DQo+IGZyZWV6aW5nLCBhY3R1YWxseSBpdCBpcyBhIHR5cGljYWwgcmVhZC93cml0 -ZSBsb2NrIHVzZSBjYXNlLCBidXQNCj4gd2UgbmVlZCB0byBzdXBwb3J0IG5lc3RlZCBub3JtYWwg -ZnJlZXppbmcsIHNvIHdlIGNhbid0IHVzZSByd3NlbS4gDQoNCllvdSBzZWVtIHRvIG92ZXJsb29r -IHRoYXQgYmxrX2dldF9yZXF1ZXN0KCkgY2FuIGJlIGNhbGxlZCBmcm9tIGFub3RoZXIgdGhyZWFk -DQp0aGFuIHRoZSB0aHJlYWQgdGhhdCBpcyBwZXJmb3JtaW5nIHRoZSBmcmVlemluZyBhbmQgdW5m -cmVlemluZy4NCg0KQmFydC4= +On Tue, 2017-09-05 at 00:08 +0800, Ming Lei wrote: +> On Mon, Sep 04, 2017 at 03:40:35PM +0000, Bart Van Assche wrote: +> > 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. +> +> Yeah, I am pretty sure. +> +> Firstly blk_queue_freeze_preempt() is exclusive, that means it will wait +> for completion of all pending freezing(both normal and preempt), and other +> freezing can't be started too if there is in-progress preempt +> freezing, actually it is a typical read/write lock use case, but +> we need to support nested normal freezing, so we can't use rwsem. + +You seem to overlook that blk_get_request() can be called from another thread +than the thread that is performing the freezing and unfreezing. + +Bart. diff --git a/a/content_digest b/N1/content_digest index a5bdbf6..3268f03 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -20,46 +20,43 @@ " tj@kernel.org <tj@kernel.org>\0" "\00:1\0" "b\0" - "T24gVHVlLCAyMDE3LTA5LTA1IGF0IDAwOjA4ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g\n" - "TW9uLCBTZXAgMDQsIDIwMTcgYXQgMDM6NDA6MzVQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy\n" - "b3RlOg0KPiA+IE9uIE1vbiwgMjAxNy0wOS0wNCBhdCAxNToxNiArMDgwMCwgTWluZyBMZWkgd3Jv\n" - "dGU6DQo+ID4gPiBPbiBNb24sIFNlcCAwNCwgMjAxNyBhdCAwNDoxMzoyNkFNICswMDAwLCBCYXJ0\n" - "IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+IEFsbG93aW5nIGJsa19nZXRfcmVxdWVzdCgpIHRv\n" - "IHN1Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0IGlzDQo+ID4gPiA+IGNv\n" - "bXBsZXRlbHkgd3JvbmcgYmVjYXVzZSB0aGF0IGNvdWxkIHJlc3VsdCBpbiBhIHJlcXVlc3QgYmVp\n" - "bmcgcXVldWVkIGFmdGVyDQo+ID4gPiA+IHRoZSBERUFEIGZsYWcgaGFzIGJlZW4gc2V0LCByZXN1\n" - "bHRpbmcgaW4gZWl0aGVyIGEgaGFuZ2luZyByZXF1ZXN0IG9yIGEga2VybmVsDQo+ID4gPiA+IGNy\n" - "YXNoLiBUaGlzIGlzIHdoeSBpdCdzIGNvbXBsZXRlbHkgd3JvbmcgdG8gYWRkIGEgYmxrX3F1ZXVl\n" - "X2VudGVyX2xpdmUoKSBjYWxsDQo+ID4gPiA+IGluIGJsa19vbGRfZ2V0X3JlcXVlc3QoKSBvciBi\n" - "bGtfbXFfYWxsb2NfcmVxdWVzdCgpLiBIZW5jZSBteSBOQUsgZm9yIGFueQ0KPiA+ID4gPiBwYXRj\n" - "aCB0aGF0IGFkZHMgYSBibGtfcXVldWVfZW50ZXJfbGl2ZSgpIGNhbGwgdG8gYW55IGZ1bmN0aW9u\n" - "IGNhbGxlZCBmcm9tDQo+ID4gPiA+IGJsa19nZXRfcmVxdWVzdCgpLiBUaGF0IGluY2x1ZGVzIHRo\n" - "ZSBwYXRjaCBhdCB0aGUgc3RhcnQgb2YgdGhpcyBlLW1haWwgdGhyZWFkLg0KPiA+ID4gDQo+ID4g\n" - "PiBTZWUgYWJvdmUsIHRoaXMgcGF0Y2ggY2hhbmdlcyBub3RoaW5nIGFib3V0IHRoaXMgZmFjdCwg\n" - "cGxlYXNlIGxvb2sgYXQNCj4gPiA+IHRoZSBwYXRjaCBjYXJlZnVsbHkgbmV4dCB0aW1lIGp1c3Qg\n" - "YmVmb3JlIHBvc3RpbmcgeW91ciBsb25nIGNvbW1lbnQuDQo+ID4gDQo+ID4gQXJlIHlvdSByZWFs\n" - "bHkgc3VyZSB0aGF0IHlvdXIgcGF0Y2ggZG9lcyBub3QgYWxsb3cgYmxrX2dldF9yZXF1ZXN0KCkg\n" - "dG8NCj4gPiBzdWNjZWVkIGFmdGVyIHRoZSBEWUlORyBmbGFnIGhhcyBiZWVuIHNldD8gYmxrX21x\n" - "X2FsbG9jX3JlcXVlc3QoKSBjYWxscyBib3RoDQo+ID4gYmxrX3F1ZXVlX2lzX3ByZWVtcHRfZnJv\n" - "emVuKCkgYW5kIGJsa19xdWV1ZV9lbnRlcl9saXZlKCkgd2l0aG91dCBob2xkaW5nDQo+ID4gYW55\n" - "IGxvY2suIEEgdGhyZWFkIHRoYXQgaXMgcnVubmluZyBjb25jdXJyZW50bHkgd2l0aCBibGtfbXFf\n" - "Z2V0X3JlcXVlc3QoKQ0KPiA+IGNhbiB1bmZyZWV6ZSB0aGUgcXVldWUgYWZ0ZXIgYmxrX3F1ZXVl\n" - "X2lzX3ByZWVtcHRfZnJvemVuKCkgcmV0dXJuZWQgYW5kDQo+ID4gYmVmb3JlIGJsa19xdWV1ZV9l\n" - "bnRlcl9saXZlKCkgaXMgY2FsbGVkLiBUaGlzIG1lYW5zIHRoYXQgd2l0aCB5b3VyIHBhdGNoDQo+\n" - "ID4gc2VyaWVzIGFwcGxpZWQgYmxrX2dldF9yZXF1ZXN0KCkgY2FuIHN1Y2NlZWQgYWZ0ZXIgdGhl\n" - "IERZSU5HIGZsYWcgaGFzIGJlZW4NCj4gPiBzZXQsIHdoaWNoIGlzIHNvbWV0aGluZyB3ZSBkb24n\n" - "dCB3YW50LiBBZGRpdGlvbmFsbHksIEkgZG9uJ3QgdGhpbmsgd2Ugd2FudA0KPiA+IHRvIGludHJv\n" - "ZHVjZSBhbnkga2luZCBvZiBsb2NraW5nIGluIGJsa19tcV9nZXRfcmVxdWVzdCgpIGJlY2F1c2Ug\n" - "dGhhdCB3b3VsZA0KPiA+IGJlIGEgc2VyaWFsaXphdGlvbiBwb2ludC4NCj4NCj4gWWVhaCwgSSBh\n" - "bSBwcmV0dHkgc3VyZS4NCj4gDQo+IEZpcnN0bHkgYmxrX3F1ZXVlX2ZyZWV6ZV9wcmVlbXB0KCkg\n" - "aXMgZXhjbHVzaXZlLCB0aGF0IG1lYW5zIGl0IHdpbGwgd2FpdA0KPiBmb3IgY29tcGxldGlvbiBv\n" - "ZiBhbGwgcGVuZGluZyBmcmVlemluZyhib3RoIG5vcm1hbCBhbmQgcHJlZW1wdCksIGFuZCBvdGhl\n" - "cg0KPiBmcmVlemluZyBjYW4ndCBiZSBzdGFydGVkIHRvbyBpZiB0aGVyZSBpcyBpbi1wcm9ncmVz\n" - "cyBwcmVlbXB0DQo+IGZyZWV6aW5nLCBhY3R1YWxseSBpdCBpcyBhIHR5cGljYWwgcmVhZC93cml0\n" - "ZSBsb2NrIHVzZSBjYXNlLCBidXQNCj4gd2UgbmVlZCB0byBzdXBwb3J0IG5lc3RlZCBub3JtYWwg\n" - "ZnJlZXppbmcsIHNvIHdlIGNhbid0IHVzZSByd3NlbS4gDQoNCllvdSBzZWVtIHRvIG92ZXJsb29r\n" - "IHRoYXQgYmxrX2dldF9yZXF1ZXN0KCkgY2FuIGJlIGNhbGxlZCBmcm9tIGFub3RoZXIgdGhyZWFk\n" - "DQp0aGFuIHRoZSB0aHJlYWQgdGhhdCBpcyBwZXJmb3JtaW5nIHRoZSBmcmVlemluZyBhbmQgdW5m\n" - cmVlemluZy4NCg0KQmFydC4= + "On Tue, 2017-09-05 at 00:08 +0800, Ming Lei wrote:\n" + "> On Mon, Sep 04, 2017 at 03:40:35PM +0000, Bart Van Assche wrote:\n" + "> > 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" + "> Yeah, I am pretty sure.\n" + "> \n" + "> Firstly blk_queue_freeze_preempt() is exclusive, that means it will wait\n" + "> for completion of all pending freezing(both normal and preempt), and other\n" + "> freezing can't be started too if there is in-progress preempt\n" + "> freezing, actually it is a typical read/write lock use case, but\n" + "> we need to support nested normal freezing, so we can't use rwsem. \n" + "\n" + "You seem to overlook that blk_get_request() can be called from another thread\n" + "than the thread that is performing the freezing and unfreezing.\n" + "\n" + Bart. -0e2bf3b1db88cf3876f2903807a586d658ac412cc08ba9be42b2291432e17df0 +7b3bfb92c07ef8f574361c7c3d5f04b783da27a9cc45fbfdab2d554b2b6b68d0
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.