Block layer use of __GFP flags

Block layer use of __GFP flags

[Matthew Wilcox]

Please explain:

commit 6a15674d1e90917f1723a814e2e8c949000440f7
Author: Bart Van Assche <bart.vanassche@wdc.com>
Date:   Thu Nov 9 10:49:54 2017 -0800

    block: Introduce blk_get_request_flags()
    
    A side effect of this patch is that the GFP mask that is passed to
    several allocation functions in the legacy block layer is changed
    from GFP_KERNEL into __GFP_DIRECT_RECLAIM.

Why was this thought to be a good idea?  I think gfp.h is pretty clear:

 * Useful GFP flag combinations that are commonly used. It is recommended
 * that subsystems start with one of these combinations and then set/clear
 * __GFP_FOO flags as necessary.

Instead, the block layer now throws away all but one bit of the
information being passed in by the callers, and all it tells the allocator
is whether or not it can start doing direct reclaim.  I can see that
you may well be in a situation where you don't want to start more I/O,
but your caller knows that!  Why make the allocator work harder than
it has to?  In particular, why isn't the page allocator allowed to wake
up kswapd to do reclaim in non-atomic context, but is when the caller
is in atomic context?

This changelog is woefully short on detail.  It says what you're doing,
but not why you're doing it.  And now I have no idea and I have to ask you
what you were thinking at the time.  Please be more considerate in future.

Re: Block layer use of __GFP flags

[Bart Van Assche]

T24gU2F0LCAyMDE4LTA0LTA3IGF0IDIzOjU0IC0wNzAwLCBNYXR0aGV3IFdpbGNveCB3cm90ZToN
Cj4gUGxlYXNlIGV4cGxhaW46DQo+IA0KPiBjb21taXQgNmExNTY3NGQxZTkwOTE3ZjE3MjNhODE0
ZTJlOGM5NDkwMDA0NDBmNw0KPiBBdXRob3I6IEJhcnQgVmFuIEFzc2NoZSA8YmFydC52YW5hc3Nj
aGVAd2RjLmNvbT4NCj4gRGF0ZTogICBUaHUgTm92IDkgMTA6NDk6NTQgMjAxNyAtMDgwMA0KPiAN
Cj4gICAgIGJsb2NrOiBJbnRyb2R1Y2UgYmxrX2dldF9yZXF1ZXN0X2ZsYWdzKCkNCj4gICAgIA0K
PiAgICAgQSBzaWRlIGVmZmVjdCBvZiB0aGlzIHBhdGNoIGlzIHRoYXQgdGhlIEdGUCBtYXNrIHRo
YXQgaXMgcGFzc2VkIHRvDQo+ICAgICBzZXZlcmFsIGFsbG9jYXRpb24gZnVuY3Rpb25zIGluIHRo
ZSBsZWdhY3kgYmxvY2sgbGF5ZXIgaXMgY2hhbmdlZA0KPiAgICAgZnJvbSBHRlBfS0VSTkVMIGlu
dG8gX19HRlBfRElSRUNUX1JFQ0xBSU0uDQo+IA0KPiBXaHkgd2FzIHRoaXMgdGhvdWdodCB0byBi
ZSBhIGdvb2QgaWRlYT8gIEkgdGhpbmsgZ2ZwLmggaXMgcHJldHR5IGNsZWFyOg0KPiANCj4gICog
VXNlZnVsIEdGUCBmbGFnIGNvbWJpbmF0aW9ucyB0aGF0IGFyZSBjb21tb25seSB1c2VkLiBJdCBp
cyByZWNvbW1lbmRlZA0KPiAgKiB0aGF0IHN1YnN5c3RlbXMgc3RhcnQgd2l0aCBvbmUgb2YgdGhl
c2UgY29tYmluYXRpb25zIGFuZCB0aGVuIHNldC9jbGVhcg0KPiAgKiBfX0dGUF9GT08gZmxhZ3Mg
YXMgbmVjZXNzYXJ5Lg0KPiANCj4gSW5zdGVhZCwgdGhlIGJsb2NrIGxheWVyIG5vdyB0aHJvd3Mg
YXdheSBhbGwgYnV0IG9uZSBiaXQgb2YgdGhlDQo+IGluZm9ybWF0aW9uIGJlaW5nIHBhc3NlZCBp
biBieSB0aGUgY2FsbGVycywgYW5kIGFsbCBpdCB0ZWxscyB0aGUgYWxsb2NhdG9yDQo+IGlzIHdo
ZXRoZXIgb3Igbm90IGl0IGNhbiBzdGFydCBkb2luZyBkaXJlY3QgcmVjbGFpbS4gSSBjYW4gc2Vl
IHRoYXQNCj4geW91IG1heSB3ZWxsIGJlIGluIGEgc2l0dWF0aW9uIHdoZXJlIHlvdSBkb24ndCB3
YW50IHRvIHN0YXJ0IG1vcmUgSS9PLA0KPiBidXQgeW91ciBjYWxsZXIga25vd3MgdGhhdCEgIFdo
eSBtYWtlIHRoZSBhbGxvY2F0b3Igd29yayBoYXJkZXIgdGhhbg0KPiBpdCBoYXMgdG8/ICBJbiBw
YXJ0aWN1bGFyLCB3aHkgaXNuJ3QgdGhlIHBhZ2UgYWxsb2NhdG9yIGFsbG93ZWQgdG8gd2FrZQ0K
PiB1cCBrc3dhcGQgdG8gZG8gcmVjbGFpbSBpbiBub24tYXRvbWljIGNvbnRleHQsIGJ1dCBpcyB3
aGVuIHRoZSBjYWxsZXINCj4gaXMgaW4gYXRvbWljIGNvbnRleHQ/DQoNCl9fR0ZQX0tTV0FQRF9S
RUNMQUlNIHdhc24ndCBzdHJpcHBlZCBvZmYgb24gcHVycG9zZSBmb3Igbm9uLWF0b21pYw0KYWxs
b2NhdGlvbnMuIFRoYXQgd2FzIGFuIG92ZXJzaWdodC4gDQoNCkRvIHlvdSBwZXJoYXBzIHdhbnQg
bWUgdG8gcHJlcGFyZSBhIHBhdGNoIHRoYXQgbWFrZXMgYmxrX2dldF9yZXF1ZXN0KCkgYWdhaW4N
CnJlc3BlY3QgdGhlIGZ1bGwgZ2ZwIG1hc2sgcGFzc2VkIGFzIHRoaXJkIGFyZ3VtZW50IHRvIGJs
a19nZXRfcmVxdWVzdCgpPw0KDQpCYXJ0Lg0KDQoNCg0K

Re: Block layer use of __GFP flags

[Bart Van Assche]

On Sat, 2018-04-07 at 23:54 -0700, Matthew Wilcox wrote:
> Please explain:
> 
> commit 6a15674d1e90917f1723a814e2e8c949000440f7
> Author: Bart Van Assche <bart.vanassche@wdc.com>
> Date:   Thu Nov 9 10:49:54 2017 -0800
> 
>     block: Introduce blk_get_request_flags()
>     
>     A side effect of this patch is that the GFP mask that is passed to
>     several allocation functions in the legacy block layer is changed
>     from GFP_KERNEL into __GFP_DIRECT_RECLAIM.
> 
> Why was this thought to be a good idea?  I think gfp.h is pretty clear:
> 
>  * Useful GFP flag combinations that are commonly used. It is recommended
>  * that subsystems start with one of these combinations and then set/clear
>  * __GFP_FOO flags as necessary.
> 
> Instead, the block layer now throws away all but one bit of the
> information being passed in by the callers, and all it tells the allocator
> is whether or not it can start doing direct reclaim. I can see that
> you may well be in a situation where you don't want to start more I/O,
> but your caller knows that!  Why make the allocator work harder than
> it has to?  In particular, why isn't the page allocator allowed to wake
> up kswapd to do reclaim in non-atomic context, but is when the caller
> is in atomic context?

__GFP_KSWAPD_RECLAIM wasn't stripped off on purpose for non-atomic
allocations. That was an oversight. 

Do you perhaps want me to prepare a patch that makes blk_get_request() again
respect the full gfp mask passed as third argument to blk_get_request()?

Bart.

Re: Block layer use of __GFP flags

[Matthew Wilcox]

On Sun, Apr 08, 2018 at 04:40:59PM +0000, Bart Van Assche wrote:
> __GFP_KSWAPD_RECLAIM wasn't stripped off on purpose for non-atomic
> allocations. That was an oversight. 

OK, good.

> Do you perhaps want me to prepare a patch that makes blk_get_request() again
> respect the full gfp mask passed as third argument to blk_get_request()?

I think that would be a good idea.  If it's onerous to have extra arguments,
there are some bits in gfp_flags which could be used for your purposes.

Re: Block layer use of __GFP flags

[Bart Van Assche]

T24gU3VuLCAyMDE4LTA0LTA4IGF0IDEyOjA4IC0wNzAwLCBNYXR0aGV3IFdpbGNveCB3cm90ZToN
Cj4gT24gU3VuLCBBcHIgMDgsIDIwMTggYXQgMDQ6NDA6NTlQTSArMDAwMCwgQmFydCBWYW4gQXNz
Y2hlIHdyb3RlOg0KPiA+IERvIHlvdSBwZXJoYXBzIHdhbnQgbWUgdG8gcHJlcGFyZSBhIHBhdGNo
IHRoYXQgbWFrZXMgYmxrX2dldF9yZXF1ZXN0KCkgYWdhaW4NCj4gPiByZXNwZWN0IHRoZSBmdWxs
IGdmcCBtYXNrIHBhc3NlZCBhcyB0aGlyZCBhcmd1bWVudCB0byBibGtfZ2V0X3JlcXVlc3QoKT8N
Cj4gDQo+IEkgdGhpbmsgdGhhdCB3b3VsZCBiZSBhIGdvb2QgaWRlYS4gIElmIGl0J3Mgb25lcm91
cyB0byBoYXZlIGV4dHJhIGFyZ3VtZW50cywNCj4gdGhlcmUgYXJlIHNvbWUgYml0cyBpbiBnZnBf
ZmxhZ3Mgd2hpY2ggY291bGQgYmUgdXNlZCBmb3IgeW91ciBwdXJwb3Nlcy4NCg0KVGhhdCdzIGlu
ZGVlZCBzb21ldGhpbmcgd2UgY2FuIGNvbnNpZGVyLg0KDQpJdCB3b3VsZCBiZSBhcHByZWNpYXRl
ZCBpZiB5b3UgY291bGQgaGF2ZSBhIGxvb2sgYXQgdGhlIHBhdGNoIGJlbG93Lg0KDQpUaGFua3Ms
DQoNCkJhcnQuDQoNCg0KU3ViamVjdDogYmxvY2s6IE1ha2UgYmxrX2dldF9yZXF1ZXN0KCkgYWdh
aW4gcmVzcGVjdCB0aGUgZW50aXJlIGdmcF90IGFyZ3VtZW50DQoNCkNvbW1pdCA2YTE1Njc0ZDFl
OTAgKCJibG9jazogSW50cm9kdWNlIGJsa19nZXRfcmVxdWVzdF9mbGFncygpIikNCnRyYW5zbGF0
ZXMgdGhlIHRoaXJkIGJsa19nZXRfcmVxdWVzdCgpIGFyZ3VtZW50cyBHRlBfS0VSTkVMLCBHRlBf
Tk9JTw0KYW5kIF9fR0ZQX1JFQ0xBSU0gaW50byBfX0dGUF9ESVJFQ1RfUkVDTEFJTS4gTWFrZSBi
bGtfZ2V0X3JlcXVlc3QoKQ0KYWdhaW4gcGFzcyB0aGUgZnVsbCBnZnBfdCBhcmd1bWVudCB0byBi
bGtfb2xkX2dldF9yZXF1ZXN0KCkgc3VjaCB0aGF0DQprc3dhcGQgaXMgYWdhaW4gd29rZW4gdXAg
dW5kZXIgbG93IG1lbW9yeSBjb25kaXRpb25zIGlmIHRoZSBjYWxsZXINCnJlcXVlc3RlZCB0aGlz
Lg0KDQpOb3RlczoNCi0gVGhpcyBjaGFuZ2Ugb25seSBhZmZlY3RzIHRoZSBiZWhhdmlvciBvZiB0
aGUgbGVnYWN5IGJsb2NrIGxheWVyLiBTZWUNCiAgYWxzbyB0aGUgbWVtcG9vbF9hbGxvYygpIGNh
bGwgaW4gX19nZXRfcmVxdWVzdCgpLg0KLSBUaGUgX19HRlBfUkVDTEFJTSBmbGFnIGluIHRoZSBi
bGtfZ2V0X3JlcXVlc3RfZmxhZ3MoKSBjYWxscyBpbg0KICB0aGUgSURFIGFuZCBTQ1NJIGRyaXZl
cnMgd2FzIHJlbW92ZWQgYnkgY29tbWl0IDAzOWM2MzVmNGU2NiAoImlkZSwNCiAgc2NzaTogVGVs
bCB0aGUgYmxvY2sgbGF5ZXIgYXQgcmVxdWVzdCBhbGxvY2F0aW9uIHRpbWUgYWJvdXQgcHJlZW1w
dA0KICByZXF1ZXN0cyIpLg0KLS0tDQogYmxvY2svYmxrLWNvcmUuYyAgICAgICAgfCAyOCArKysr
KysrKysrKysrKystLS0tLS0tLS0tLS0tDQogZHJpdmVycy9pZGUvaWRlLXBtLmMgICAgfCAgMiAr
LQ0KIGRyaXZlcnMvc2NzaS9zY3NpX2xpYi5jIHwgIDMgKystDQogaW5jbHVkZS9saW51eC9ibGtk
ZXYuaCAgfCAgMyArKy0NCiA0IGZpbGVzIGNoYW5nZWQsIDIwIGluc2VydGlvbnMoKyksIDE2IGRl
bGV0aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvYmxvY2svYmxrLWNvcmUuYyBiL2Jsb2NrL2Jsay1j
b3JlLmMNCmluZGV4IDNhYzlkZDI1ZTA0ZS4uODNjN2E1OGU0ZmIzIDEwMDY0NA0KLS0tIGEvYmxv
Y2svYmxrLWNvcmUuYw0KKysrIGIvYmxvY2svYmxrLWNvcmUuYw0KQEAgLTEzMzMsNiArMTMzMyw3
IEBAIGludCBibGtfdXBkYXRlX25yX3JlcXVlc3RzKHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxLCB1
bnNpZ25lZCBpbnQgbnIpDQogICogQG9wOiBvcGVyYXRpb24gYW5kIGZsYWdzDQogICogQGJpbzog
YmlvIHRvIGFsbG9jYXRlIHJlcXVlc3QgZm9yIChjYW4gYmUgJU5VTEwpDQogICogQGZsYWdzOiBC
TFFfTVFfUkVRXyogZmxhZ3MNCisgKiBAZ2ZwX21hc2s6IGFsbG9jYXRpb24gbWFzaw0KICAqDQog
ICogR2V0IGEgZnJlZSByZXF1ZXN0IGZyb20gQHEuICBUaGlzIGZ1bmN0aW9uIG1heSBmYWlsIHVu
ZGVyIG1lbW9yeQ0KICAqIHByZXNzdXJlIG9yIGlmIEBxIGlzIGRlYWQuDQpAQCAtMTM0Miw3ICsx
MzQzLDggQEAgaW50IGJsa191cGRhdGVfbnJfcmVxdWVzdHMoc3RydWN0IHJlcXVlc3RfcXVldWUg
KnEsIHVuc2lnbmVkIGludCBucikNCiAgKiBSZXR1cm5zIHJlcXVlc3QgcG9pbnRlciBvbiBzdWNj
ZXNzLCB3aXRoIEBxLT5xdWV1ZV9sb2NrICpub3QgaGVsZCouDQogICovDQogc3RhdGljIHN0cnVj
dCByZXF1ZXN0ICpfX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X2xpc3QgKnJsLCB1bnNpZ25l
ZCBpbnQgb3AsDQotCQkJCSAgICAgc3RydWN0IGJpbyAqYmlvLCBibGtfbXFfcmVxX2ZsYWdzX3Qg
ZmxhZ3MpDQorCQkJCSAgICAgc3RydWN0IGJpbyAqYmlvLCBibGtfbXFfcmVxX2ZsYWdzX3QgZmxh
Z3MsDQorCQkJCSAgICAgZ2ZwX3QgZ2ZwX21hc2spDQogew0KIAlzdHJ1Y3QgcmVxdWVzdF9xdWV1
ZSAqcSA9IHJsLT5xOw0KIAlzdHJ1Y3QgcmVxdWVzdCAqcnE7DQpAQCAtMTM1MSw4ICsxMzUzLDYg
QEAgc3RhdGljIHN0cnVjdCByZXF1ZXN0ICpfX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X2xp
c3QgKnJsLCB1bnNpZ25lZCBpbnQgb3AsDQogCXN0cnVjdCBpb19jcSAqaWNxID0gTlVMTDsNCiAJ
Y29uc3QgYm9vbCBpc19zeW5jID0gb3BfaXNfc3luYyhvcCk7DQogCWludCBtYXlfcXVldWU7DQot
CWdmcF90IGdmcF9tYXNrID0gZmxhZ3MgJiBCTEtfTVFfUkVRX05PV0FJVCA/IEdGUF9BVE9NSUMg
Og0KLQkJCSBfX0dGUF9ESVJFQ1RfUkVDTEFJTTsNCiAJcmVxX2ZsYWdzX3QgcnFfZmxhZ3MgPSBS
UUZfQUxMT0NFRDsNCiANCiAJbG9ja2RlcF9hc3NlcnRfaGVsZChxLT5xdWV1ZV9sb2NrKTsNCkBA
IC0xNTE2LDYgKzE1MTYsNyBAQCBzdGF0aWMgc3RydWN0IHJlcXVlc3QgKl9fZ2V0X3JlcXVlc3Qo
c3RydWN0IHJlcXVlc3RfbGlzdCAqcmwsIHVuc2lnbmVkIGludCBvcCwNCiAgKiBAb3A6IG9wZXJh
dGlvbiBhbmQgZmxhZ3MNCiAgKiBAYmlvOiBiaW8gdG8gYWxsb2NhdGUgcmVxdWVzdCBmb3IgKGNh
biBiZSAlTlVMTCkNCiAgKiBAZmxhZ3M6IEJMS19NUV9SRVFfKiBmbGFncy4NCisgKiBAZ2ZwX21h
c2s6IGFsbG9jYXRpb24gbWFzaw0KICAqDQogICogR2V0IGEgZnJlZSByZXF1ZXN0IGZyb20gQHEu
ICBJZiAlX19HRlBfRElSRUNUX1JFQ0xBSU0gaXMgc2V0IGluIEBnZnBfbWFzaywNCiAgKiB0aGlz
IGZ1bmN0aW9uIGtlZXBzIHJldHJ5aW5nIHVuZGVyIG1lbW9yeSBwcmVzc3VyZSBhbmQgZmFpbHMg
aWZmIEBxIGlzIGRlYWQuDQpAQCAtMTUyNSw3ICsxNTI2LDggQEAgc3RhdGljIHN0cnVjdCByZXF1
ZXN0ICpfX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X2xpc3QgKnJsLCB1bnNpZ25lZCBpbnQg
b3AsDQogICogUmV0dXJucyByZXF1ZXN0IHBvaW50ZXIgb24gc3VjY2Vzcywgd2l0aCBAcS0+cXVl
dWVfbG9jayAqbm90IGhlbGQqLg0KICAqLw0KIHN0YXRpYyBzdHJ1Y3QgcmVxdWVzdCAqZ2V0X3Jl
cXVlc3Qoc3RydWN0IHJlcXVlc3RfcXVldWUgKnEsIHVuc2lnbmVkIGludCBvcCwNCi0JCQkJICAg
c3RydWN0IGJpbyAqYmlvLCBibGtfbXFfcmVxX2ZsYWdzX3QgZmxhZ3MpDQorCQkJCSAgIHN0cnVj
dCBiaW8gKmJpbywgYmxrX21xX3JlcV9mbGFnc190IGZsYWdzLA0KKwkJCQkgICBnZnBfdCBnZnBf
bWFzaykNCiB7DQogCWNvbnN0IGJvb2wgaXNfc3luYyA9IG9wX2lzX3N5bmMob3ApOw0KIAlERUZJ
TkVfV0FJVCh3YWl0KTsNCkBAIC0xNTM3LDcgKzE1MzksNyBAQCBzdGF0aWMgc3RydWN0IHJlcXVl
c3QgKmdldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxLCB1bnNpZ25lZCBpbnQgb3As
DQogDQogCXJsID0gYmxrX2dldF9ybChxLCBiaW8pOwkvKiB0cmFuc2ZlcnJlZCB0byBAcnEgb24g
c3VjY2VzcyAqLw0KIHJldHJ5Og0KLQlycSA9IF9fZ2V0X3JlcXVlc3QocmwsIG9wLCBiaW8sIGZs
YWdzKTsNCisJcnEgPSBfX2dldF9yZXF1ZXN0KHJsLCBvcCwgYmlvLCBmbGFncywgZ2ZwX21hc2sp
Ow0KIAlpZiAoIUlTX0VSUihycSkpDQogCQlyZXR1cm4gcnE7DQogDQpAQCAtMTU3NSwxMSArMTU3
NywxMCBAQCBzdGF0aWMgc3RydWN0IHJlcXVlc3QgKmdldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0
X3F1ZXVlICpxLCB1bnNpZ25lZCBpbnQgb3AsDQogDQogLyogZmxhZ3M6IEJMS19NUV9SRVFfUFJF
RU1QVCBhbmQvb3IgQkxLX01RX1JFUV9OT1dBSVQuICovDQogc3RhdGljIHN0cnVjdCByZXF1ZXN0
ICpibGtfb2xkX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxLA0KLQkJCQl1bnNp
Z25lZCBpbnQgb3AsIGJsa19tcV9yZXFfZmxhZ3NfdCBmbGFncykNCisJCQkJdW5zaWduZWQgaW50
IG9wLCBibGtfbXFfcmVxX2ZsYWdzX3QgZmxhZ3MsDQorCQkJCWdmcF90IGdmcF9tYXNrKQ0KIHsN
CiAJc3RydWN0IHJlcXVlc3QgKnJxOw0KLQlnZnBfdCBnZnBfbWFzayA9IGZsYWdzICYgQkxLX01R
X1JFUV9OT1dBSVQgPyBHRlBfQVRPTUlDIDoNCi0JCQkgX19HRlBfRElSRUNUX1JFQ0xBSU07DQog
CWludCByZXQgPSAwOw0KIA0KIAlXQVJOX09OX09OQ0UocS0+bXFfb3BzKTsNCkBAIC0xNTkxLDcg
KzE1OTIsNyBAQCBzdGF0aWMgc3RydWN0IHJlcXVlc3QgKmJsa19vbGRfZ2V0X3JlcXVlc3Qoc3Ry
dWN0IHJlcXVlc3RfcXVldWUgKnEsDQogCWlmIChyZXQpDQogCQlyZXR1cm4gRVJSX1BUUihyZXQp
Ow0KIAlzcGluX2xvY2tfaXJxKHEtPnF1ZXVlX2xvY2spOw0KLQlycSA9IGdldF9yZXF1ZXN0KHEs
IG9wLCBOVUxMLCBmbGFncyk7DQorCXJxID0gZ2V0X3JlcXVlc3QocSwgb3AsIE5VTEwsIGZsYWdz
LCBnZnBfbWFzayk7DQogCWlmIChJU19FUlIocnEpKSB7DQogCQlzcGluX3VubG9ja19pcnEocS0+
cXVldWVfbG9jayk7DQogCQlibGtfcXVldWVfZXhpdChxKTsNCkBAIC0xNjEwLDkgKzE2MTEsMTAg
QEAgc3RhdGljIHN0cnVjdCByZXF1ZXN0ICpibGtfb2xkX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1
ZXN0X3F1ZXVlICpxLA0KICAqIEBxOiByZXF1ZXN0IHF1ZXVlIHRvIGFsbG9jYXRlIGEgcmVxdWVz
dCBmb3INCiAgKiBAb3A6IG9wZXJhdGlvbiAoUkVRX09QXyopIGFuZCBSRVFfKiBmbGFncywgZS5n
LiBSRVFfU1lOQy4NCiAgKiBAZmxhZ3M6IEJMS19NUV9SRVFfKiBmbGFncywgZS5nLiBCTEtfTVFf
UkVRX05PV0FJVC4NCisgKiBAZ2ZwX21hc2s6IGFsbG9jYXRpb24gbWFzaw0KICAqLw0KIHN0cnVj
dCByZXF1ZXN0ICpibGtfZ2V0X3JlcXVlc3RfZmxhZ3Moc3RydWN0IHJlcXVlc3RfcXVldWUgKnEs
IHVuc2lnbmVkIGludCBvcCwNCi0JCQkJICAgICAgYmxrX21xX3JlcV9mbGFnc190IGZsYWdzKQ0K
KwkJCQkgICAgICBibGtfbXFfcmVxX2ZsYWdzX3QgZmxhZ3MsIGdmcF90IGdmcF9tYXNrKQ0KIHsN
CiAJc3RydWN0IHJlcXVlc3QgKnJlcTsNCiANCkBAIC0xNjI0LDcgKzE2MjYsNyBAQCBzdHJ1Y3Qg
cmVxdWVzdCAqYmxrX2dldF9yZXF1ZXN0X2ZsYWdzKHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxLCB1
bnNpZ25lZCBpbnQgb3AsDQogCQlpZiAoIUlTX0VSUihyZXEpICYmIHEtPm1xX29wcy0+aW5pdGlh
bGl6ZV9ycV9mbikNCiAJCQlxLT5tcV9vcHMtPmluaXRpYWxpemVfcnFfZm4ocmVxKTsNCiAJfSBl
bHNlIHsNCi0JCXJlcSA9IGJsa19vbGRfZ2V0X3JlcXVlc3QocSwgb3AsIGZsYWdzKTsNCisJCXJl
cSA9IGJsa19vbGRfZ2V0X3JlcXVlc3QocSwgb3AsIGZsYWdzLCBnZnBfbWFzayk7DQogCQlpZiAo
IUlTX0VSUihyZXEpICYmIHEtPmluaXRpYWxpemVfcnFfZm4pDQogCQkJcS0+aW5pdGlhbGl6ZV9y
cV9mbihyZXEpOw0KIAl9DQpAQCAtMTYzNyw3ICsxNjM5LDcgQEAgc3RydWN0IHJlcXVlc3QgKmJs
a19nZXRfcmVxdWVzdChzdHJ1Y3QgcmVxdWVzdF9xdWV1ZSAqcSwgdW5zaWduZWQgaW50IG9wLA0K
IAkJCQlnZnBfdCBnZnBfbWFzaykNCiB7DQogCXJldHVybiBibGtfZ2V0X3JlcXVlc3RfZmxhZ3Mo
cSwgb3AsIGdmcF9tYXNrICYgX19HRlBfRElSRUNUX1JFQ0xBSU0gPw0KLQkJCQkgICAgIDAgOiBC
TEtfTVFfUkVRX05PV0FJVCk7DQorCQkJCSAgICAgMCA6IEJMS19NUV9SRVFfTk9XQUlULCBnZnBf
bWFzayk7DQogfQ0KIEVYUE9SVF9TWU1CT0woYmxrX2dldF9yZXF1ZXN0KTsNCiANCkBAIC0yMDY1
LDcgKzIwNjcsNyBAQCBzdGF0aWMgYmxrX3FjX3QgYmxrX3F1ZXVlX2JpbyhzdHJ1Y3QgcmVxdWVz
dF9xdWV1ZSAqcSwgc3RydWN0IGJpbyAqYmlvKQ0KIAkgKiBSZXR1cm5zIHdpdGggdGhlIHF1ZXVl
IHVubG9ja2VkLg0KIAkgKi8NCiAJYmxrX3F1ZXVlX2VudGVyX2xpdmUocSk7DQotCXJlcSA9IGdl
dF9yZXF1ZXN0KHEsIGJpby0+Ymlfb3BmLCBiaW8sIDApOw0KKwlyZXEgPSBnZXRfcmVxdWVzdChx
LCBiaW8tPmJpX29wZiwgYmlvLCAwLCBHRlBfTk9JTyk7DQogCWlmIChJU19FUlIocmVxKSkgew0K
IAkJYmxrX3F1ZXVlX2V4aXQocSk7DQogCQlfX3didF9kb25lKHEtPnJxX3diLCB3Yl9hY2N0KTsN
CmRpZmYgLS1naXQgYS9kcml2ZXJzL2lkZS9pZGUtcG0uYyBiL2RyaXZlcnMvaWRlL2lkZS1wbS5j
DQppbmRleCBhZDhhMTI1ZGVmZGQuLjNkZGI0NjRiNzJlNiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMv
aWRlL2lkZS1wbS5jDQorKysgYi9kcml2ZXJzL2lkZS9pZGUtcG0uYw0KQEAgLTkxLDcgKzkxLDcg
QEAgaW50IGdlbmVyaWNfaWRlX3Jlc3VtZShzdHJ1Y3QgZGV2aWNlICpkZXYpDQogDQogCW1lbXNl
dCgmcnFwbSwgMCwgc2l6ZW9mKHJxcG0pKTsNCiAJcnEgPSBibGtfZ2V0X3JlcXVlc3RfZmxhZ3Mo
ZHJpdmUtPnF1ZXVlLCBSRVFfT1BfRFJWX0lOLA0KLQkJCQkgICBCTEtfTVFfUkVRX1BSRUVNUFQp
Ow0KKwkJCQkgICBCTEtfTVFfUkVRX1BSRUVNUFQsIF9fR0ZQX1JFQ0xBSU0pOw0KIAlpZGVfcmVx
KHJxKS0+dHlwZSA9IEFUQV9QUklWX1BNX1JFU1VNRTsNCiAJcnEtPnNwZWNpYWwgPSAmcnFwbTsN
CiAJcnFwbS5wbV9zdGVwID0gSURFX1BNX1NUQVJUX1JFU1VNRTsNCmRpZmYgLS1naXQgYS9kcml2
ZXJzL3Njc2kvc2NzaV9saWIuYyBiL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jDQppbmRleCAwZGZl
YzBkZWRkNWUuLjYyZDMxNDAzNzY3YSAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvc2NzaS9zY3NpX2xp
Yi5jDQorKysgYi9kcml2ZXJzL3Njc2kvc2NzaV9saWIuYw0KQEAgLTI2Nyw3ICsyNjcsOCBAQCBp
bnQgc2NzaV9leGVjdXRlKHN0cnVjdCBzY3NpX2RldmljZSAqc2RldiwgY29uc3QgdW5zaWduZWQg
Y2hhciAqY21kLA0KIA0KIAlyZXEgPSBibGtfZ2V0X3JlcXVlc3RfZmxhZ3Moc2Rldi0+cmVxdWVz
dF9xdWV1ZSwNCiAJCQlkYXRhX2RpcmVjdGlvbiA9PSBETUFfVE9fREVWSUNFID8NCi0JCQlSRVFf
T1BfU0NTSV9PVVQgOiBSRVFfT1BfU0NTSV9JTiwgQkxLX01RX1JFUV9QUkVFTVBUKTsNCisJCQlS
RVFfT1BfU0NTSV9PVVQgOiBSRVFfT1BfU0NTSV9JTiwgQkxLX01RX1JFUV9QUkVFTVBULA0KKwkJ
CV9fR0ZQX1JFQ0xBSU0pOw0KIAlpZiAoSVNfRVJSKHJlcSkpDQogCQlyZXR1cm4gcmV0Ow0KIAly
cSA9IHNjc2lfcmVxKHJlcSk7DQpkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9ibGtkZXYuaCBi
L2luY2x1ZGUvbGludXgvYmxrZGV2LmgNCmluZGV4IGYyY2RmMjYzNjk3NC4uZTBhNmE3NDFhZmQw
IDEwMDY0NA0KLS0tIGEvaW5jbHVkZS9saW51eC9ibGtkZXYuaA0KKysrIGIvaW5jbHVkZS9saW51
eC9ibGtkZXYuaA0KQEAgLTk0Myw3ICs5NDMsOCBAQCBleHRlcm4gdm9pZCBibGtfcHV0X3JlcXVl
c3Qoc3RydWN0IHJlcXVlc3QgKik7DQogZXh0ZXJuIHZvaWQgX19ibGtfcHV0X3JlcXVlc3Qoc3Ry
dWN0IHJlcXVlc3RfcXVldWUgKiwgc3RydWN0IHJlcXVlc3QgKik7DQogZXh0ZXJuIHN0cnVjdCBy
ZXF1ZXN0ICpibGtfZ2V0X3JlcXVlc3RfZmxhZ3Moc3RydWN0IHJlcXVlc3RfcXVldWUgKiwNCiAJ
CQkJCSAgICAgdW5zaWduZWQgaW50IG9wLA0KLQkJCQkJICAgICBibGtfbXFfcmVxX2ZsYWdzX3Qg
ZmxhZ3MpOw0KKwkJCQkJICAgICBibGtfbXFfcmVxX2ZsYWdzX3QgZmxhZ3MsDQorCQkJCQkgICAg
IGdmcF90IGdmcF9tYXNrKTsNCiBleHRlcm4gc3RydWN0IHJlcXVlc3QgKmJsa19nZXRfcmVxdWVz
dChzdHJ1Y3QgcmVxdWVzdF9xdWV1ZSAqLCB1bnNpZ25lZCBpbnQgb3AsDQogCQkJCSAgICAgICBn
ZnBfdCBnZnBfbWFzayk7DQogZXh0ZXJuIHZvaWQgYmxrX3JlcXVldWVfcmVxdWVzdChzdHJ1Y3Qg
cmVxdWVzdF9xdWV1ZSAqLCBzdHJ1Y3QgcmVxdWVzdCAqKTsNCi0tIA0KMi4xNi4yDQo=

Re: Block layer use of __GFP flags

[Bart Van Assche]

On Sun, 2018-04-08 at 12:08 -0700, Matthew Wilcox wrote:
> On Sun, Apr 08, 2018 at 04:40:59PM +0000, Bart Van Assche wrote:
> > Do you perhaps want me to prepare a patch that makes blk_get_request() again
> > respect the full gfp mask passed as third argument to blk_get_request()?
> 
> I think that would be a good idea.  If it's onerous to have extra arguments,
> there are some bits in gfp_flags which could be used for your purposes.

That's indeed something we can consider.

It would be appreciated if you could have a look at the patch below.

Thanks,

Bart.


Subject: block: Make blk_get_request() again respect the entire gfp_t argument

Commit 6a15674d1e90 ("block: Introduce blk_get_request_flags()")
translates the third blk_get_request() arguments GFP_KERNEL, GFP_NOIO
and __GFP_RECLAIM into __GFP_DIRECT_RECLAIM. Make blk_get_request()
again pass the full gfp_t argument to blk_old_get_request() such that
kswapd is again woken up under low memory conditions if the caller
requested this.

Notes:
- This change only affects the behavior of the legacy block layer. See
  also the mempool_alloc() call in __get_request().
- The __GFP_RECLAIM flag in the blk_get_request_flags() calls in
  the IDE and SCSI drivers was removed by commit 039c635f4e66 ("ide,
  scsi: Tell the block layer at request allocation time about preempt
  requests").
---
 block/blk-core.c        | 28 +++++++++++++++-------------
 drivers/ide/ide-pm.c    |  2 +-
 drivers/scsi/scsi_lib.c |  3 ++-
 include/linux/blkdev.h  |  3 ++-
 4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3ac9dd25e04e..83c7a58e4fb3 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1333,6 +1333,7 @@ int blk_update_nr_requests(struct request_queue *q, unsigned int nr)
  * @op: operation and flags
  * @bio: bio to allocate request for (can be %NULL)
  * @flags: BLQ_MQ_REQ_* flags
+ * @gfp_mask: allocation mask
  *
  * Get a free request from @q.  This function may fail under memory
  * pressure or if @q is dead.
@@ -1342,7 +1343,8 @@ int blk_update_nr_requests(struct request_queue *q, unsigned int nr)
  * Returns request pointer on success, with @q->queue_lock *not held*.
  */
 static struct request *__get_request(struct request_list *rl, unsigned int op,
-				     struct bio *bio, blk_mq_req_flags_t flags)
+				     struct bio *bio, blk_mq_req_flags_t flags,
+				     gfp_t gfp_mask)
 {
 	struct request_queue *q = rl->q;
 	struct request *rq;
@@ -1351,8 +1353,6 @@ static struct request *__get_request(struct request_list *rl, unsigned int op,
 	struct io_cq *icq = NULL;
 	const bool is_sync = op_is_sync(op);
 	int may_queue;
-	gfp_t gfp_mask = flags & BLK_MQ_REQ_NOWAIT ? GFP_ATOMIC :
-			 __GFP_DIRECT_RECLAIM;
 	req_flags_t rq_flags = RQF_ALLOCED;
 
 	lockdep_assert_held(q->queue_lock);
@@ -1516,6 +1516,7 @@ static struct request *__get_request(struct request_list *rl, unsigned int op,
  * @op: operation and flags
  * @bio: bio to allocate request for (can be %NULL)
  * @flags: BLK_MQ_REQ_* flags.
+ * @gfp_mask: allocation mask
  *
  * Get a free request from @q.  If %__GFP_DIRECT_RECLAIM is set in @gfp_mask,
  * this function keeps retrying under memory pressure and fails iff @q is dead.
@@ -1525,7 +1526,8 @@ static struct request *__get_request(struct request_list *rl, unsigned int op,
  * Returns request pointer on success, with @q->queue_lock *not held*.
  */
 static struct request *get_request(struct request_queue *q, unsigned int op,
-				   struct bio *bio, blk_mq_req_flags_t flags)
+				   struct bio *bio, blk_mq_req_flags_t flags,
+				   gfp_t gfp_mask)
 {
 	const bool is_sync = op_is_sync(op);
 	DEFINE_WAIT(wait);
@@ -1537,7 +1539,7 @@ static struct request *get_request(struct request_queue *q, unsigned int op,
 
 	rl = blk_get_rl(q, bio);	/* transferred to @rq on success */
 retry:
-	rq = __get_request(rl, op, bio, flags);
+	rq = __get_request(rl, op, bio, flags, gfp_mask);
 	if (!IS_ERR(rq))
 		return rq;
 
@@ -1575,11 +1577,10 @@ static struct request *get_request(struct request_queue *q, unsigned int op,
 
 /* flags: BLK_MQ_REQ_PREEMPT and/or BLK_MQ_REQ_NOWAIT. */
 static struct request *blk_old_get_request(struct request_queue *q,
-				unsigned int op, blk_mq_req_flags_t flags)
+				unsigned int op, blk_mq_req_flags_t flags,
+				gfp_t gfp_mask)
 {
 	struct request *rq;
-	gfp_t gfp_mask = flags & BLK_MQ_REQ_NOWAIT ? GFP_ATOMIC :
-			 __GFP_DIRECT_RECLAIM;
 	int ret = 0;
 
 	WARN_ON_ONCE(q->mq_ops);
@@ -1591,7 +1592,7 @@ static struct request *blk_old_get_request(struct request_queue *q,
 	if (ret)
 		return ERR_PTR(ret);
 	spin_lock_irq(q->queue_lock);
-	rq = get_request(q, op, NULL, flags);
+	rq = get_request(q, op, NULL, flags, gfp_mask);
 	if (IS_ERR(rq)) {
 		spin_unlock_irq(q->queue_lock);
 		blk_queue_exit(q);
@@ -1610,9 +1611,10 @@ static struct request *blk_old_get_request(struct request_queue *q,
  * @q: request queue to allocate a request for
  * @op: operation (REQ_OP_*) and REQ_* flags, e.g. REQ_SYNC.
  * @flags: BLK_MQ_REQ_* flags, e.g. BLK_MQ_REQ_NOWAIT.
+ * @gfp_mask: allocation mask
  */
 struct request *blk_get_request_flags(struct request_queue *q, unsigned int op,
-				      blk_mq_req_flags_t flags)
+				      blk_mq_req_flags_t flags, gfp_t gfp_mask)
 {
 	struct request *req;
 
@@ -1624,7 +1626,7 @@ struct request *blk_get_request_flags(struct request_queue *q, unsigned int op,
 		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, flags);
+		req = blk_old_get_request(q, op, flags, gfp_mask);
 		if (!IS_ERR(req) && q->initialize_rq_fn)
 			q->initialize_rq_fn(req);
 	}
@@ -1637,7 +1639,7 @@ struct request *blk_get_request(struct request_queue *q, unsigned int op,
 				gfp_t gfp_mask)
 {
 	return blk_get_request_flags(q, op, gfp_mask & __GFP_DIRECT_RECLAIM ?
-				     0 : BLK_MQ_REQ_NOWAIT);
+				     0 : BLK_MQ_REQ_NOWAIT, gfp_mask);
 }
 EXPORT_SYMBOL(blk_get_request);
 
@@ -2065,7 +2067,7 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 	 * Returns with the queue unlocked.
 	 */
 	blk_queue_enter_live(q);
-	req = get_request(q, bio->bi_opf, bio, 0);
+	req = get_request(q, bio->bi_opf, bio, 0, GFP_NOIO);
 	if (IS_ERR(req)) {
 		blk_queue_exit(q);
 		__wbt_done(q->rq_wb, wb_acct);
diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index ad8a125defdd..3ddb464b72e6 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -91,7 +91,7 @@ int generic_ide_resume(struct device *dev)
 
 	memset(&rqpm, 0, sizeof(rqpm));
 	rq = blk_get_request_flags(drive->queue, REQ_OP_DRV_IN,
-				   BLK_MQ_REQ_PREEMPT);
+				   BLK_MQ_REQ_PREEMPT, __GFP_RECLAIM);
 	ide_req(rq)->type = ATA_PRIV_PM_RESUME;
 	rq->special = &rqpm;
 	rqpm.pm_step = IDE_PM_START_RESUME;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0dfec0dedd5e..62d31403767a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -267,7 +267,8 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 
 	req = blk_get_request_flags(sdev->request_queue,
 			data_direction == DMA_TO_DEVICE ?
-			REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
+			REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT,
+			__GFP_RECLAIM);
 	if (IS_ERR(req))
 		return ret;
 	rq = scsi_req(req);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f2cdf2636974..e0a6a741afd0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -943,7 +943,8 @@ extern void blk_put_request(struct request *);
 extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request_flags(struct request_queue *,
 					     unsigned int op,
-					     blk_mq_req_flags_t flags);
+					     blk_mq_req_flags_t flags,
+					     gfp_t gfp_mask);
 extern struct request *blk_get_request(struct request_queue *, unsigned int op,
 				       gfp_t gfp_mask);
 extern void blk_requeue_request(struct request_queue *, struct request *);
-- 
2.16.2

Re: Block layer use of __GFP flags

[Hannes Reinecke]

On Mon, 9 Apr 2018 04:46:22 +0000
"Bart Van Assche" <Bart.VanAssche@wdc.com> wrote:

> On Sun, 2018-04-08 at 12:08 -0700, Matthew Wilcox wrote:
> > On Sun, Apr 08, 2018 at 04:40:59PM +0000, Bart Van Assche wrote:  
> > > Do you perhaps want me to prepare a patch that makes
> > > blk_get_request() again respect the full gfp mask passed as third
> > > argument to blk_get_request()?  
> > 
> > I think that would be a good idea.  If it's onerous to have extra
> > arguments, there are some bits in gfp_flags which could be used for
> > your purposes.  
> 
> That's indeed something we can consider.
> 
> It would be appreciated if you could have a look at the patch below.
> 
> Thanks,
> 
> Bart.
> 
> 

Why don't you fold the 'flags' argument into the 'gfp_flags', and drop
the 'flags' argument completely?
Looks a bit pointless to me, having two arguments denoting basically
the same ...

Cheers,

Hannes

Re: Block layer use of __GFP flags

[Christoph Hellwig]

On Mon, Apr 09, 2018 at 08:53:49AM +0200, Hannes Reinecke wrote:
> Why don't you fold the 'flags' argument into the 'gfp_flags', and drop
> the 'flags' argument completely?
> Looks a bit pointless to me, having two arguments denoting basically
> the same ...

Wrong way around.  gfp_flags doesn't really make much sense in this
context.  We just want the plain flags argument, including a non-block
flag for it.

Re: Block layer use of __GFP flags

[Michal Hocko]

On Mon 09-04-18 04:46:22, Bart Van Assche wrote:
[...]
[...]
> diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
> index ad8a125defdd..3ddb464b72e6 100644
> --- a/drivers/ide/ide-pm.c
> +++ b/drivers/ide/ide-pm.c
> @@ -91,7 +91,7 @@ int generic_ide_resume(struct device *dev)
>  
>  	memset(&rqpm, 0, sizeof(rqpm));
>  	rq = blk_get_request_flags(drive->queue, REQ_OP_DRV_IN,
> -				   BLK_MQ_REQ_PREEMPT);
> +				   BLK_MQ_REQ_PREEMPT, __GFP_RECLAIM);

Is there any reason to use __GFP_RECLAIM directly. I guess you wanted to
have GFP_NOIO semantic, right? So why not be explicit about that. Same
for other instances of this flag in the patch
-- 
Michal Hocko
SUSE Labs

Re: Block layer use of __GFP flags

[Bart Van Assche]

T24gTW9uLCAyMDE4LTA0LTA5IGF0IDExOjAwICswMjAwLCBNaWNoYWwgSG9ja28gd3JvdGU6DQo+
IE9uIE1vbiAwOS0wNC0xOCAwNDo0NjoyMiwgQmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiBbLi4u
XQ0KPiBbLi4uXQ0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lkZS9pZGUtcG0uYyBiL2RyaXZl
cnMvaWRlL2lkZS1wbS5jDQo+ID4gaW5kZXggYWQ4YTEyNWRlZmRkLi4zZGRiNDY0YjcyZTYgMTAw
NjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9pZGUvaWRlLXBtLmMNCj4gPiArKysgYi9kcml2ZXJzL2lk
ZS9pZGUtcG0uYw0KPiA+IEBAIC05MSw3ICs5MSw3IEBAIGludCBnZW5lcmljX2lkZV9yZXN1bWUo
c3RydWN0IGRldmljZSAqZGV2KQ0KPiA+ICANCj4gPiAgCW1lbXNldCgmcnFwbSwgMCwgc2l6ZW9m
KHJxcG0pKTsNCj4gPiAgCXJxID0gYmxrX2dldF9yZXF1ZXN0X2ZsYWdzKGRyaXZlLT5xdWV1ZSwg
UkVRX09QX0RSVl9JTiwNCj4gPiAtCQkJCSAgIEJMS19NUV9SRVFfUFJFRU1QVCk7DQo+ID4gKwkJ
CQkgICBCTEtfTVFfUkVRX1BSRUVNUFQsIF9fR0ZQX1JFQ0xBSU0pOw0KPiANCj4gSXMgdGhlcmUg
YW55IHJlYXNvbiB0byB1c2UgX19HRlBfUkVDTEFJTSBkaXJlY3RseS4gSSBndWVzcyB5b3Ugd2Fu
dGVkIHRvDQo+IGhhdmUgR0ZQX05PSU8gc2VtYW50aWMsIHJpZ2h0PyBTbyB3aHkgbm90IGJlIGV4
cGxpY2l0IGFib3V0IHRoYXQuIFNhbWUNCj4gZm9yIG90aGVyIGluc3RhbmNlcyBvZiB0aGlzIGZs
YWcgaW4gdGhlIHBhdGNoDQoNCkhlbGxvIE1pY2hhbCwNCg0KVGhhbmtzIGZvciB0aGUgcmV2aWV3
LiBUaGUgdXNlIG9mIF9fR0ZQX1JFQ0xBSU0gaW4gdGhpcyBjb2RlICh3aGljaCB3YXMNCmNhbGxl
ZCBfX0dGUF9XQUlUIGluIHRoZSBwYXN0KSBwcmVkYXRlcyB0aGUgZ2l0IGhpc3RvcnkuIEluIG90
aGVyIHdvcmRzLCBpdA0Kd2FzIGludHJvZHVjZWQgYmVmb3JlIGtlcm5lbCB2ZXJzaW9uIDIuNi4x
MiAoMjAwNSkuIFNvIEknbSByZWx1Y3RhbnQgdG8gbWFrZQ0Kc3VjaCBhIGNoYW5nZSBpbiB0aGUg
SURFIGNvZGUuIEJ1dCBJIHdpbGwgbWFrZSB0aGF0IGNoYW5nZSBpbiB0aGUgU0NTSSBjb2RlLg0K
DQpCYXJ0Lg0KDQoNCg0K

Re: Block layer use of __GFP flags

[Bart Van Assche]

On Mon, 2018-04-09 at 11:00 +0200, Michal Hocko wrote:
> On Mon 09-04-18 04:46:22, Bart Van Assche wrote:
> [...]
> [...]
> > diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
> > index ad8a125defdd..3ddb464b72e6 100644
> > --- a/drivers/ide/ide-pm.c
> > +++ b/drivers/ide/ide-pm.c
> > @@ -91,7 +91,7 @@ int generic_ide_resume(struct device *dev)
> >  
> >  	memset(&rqpm, 0, sizeof(rqpm));
> >  	rq = blk_get_request_flags(drive->queue, REQ_OP_DRV_IN,
> > -				   BLK_MQ_REQ_PREEMPT);
> > +				   BLK_MQ_REQ_PREEMPT, __GFP_RECLAIM);
> 
> Is there any reason to use __GFP_RECLAIM directly. I guess you wanted to
> have GFP_NOIO semantic, right? So why not be explicit about that. Same
> for other instances of this flag in the patch

Hello Michal,

Thanks for the review. The use of __GFP_RECLAIM in this code (which was
called __GFP_WAIT in the past) predates the git history. In other words, it
was introduced before kernel version 2.6.12 (2005). So I'm reluctant to make
such a change in the IDE code. But I will make that change in the SCSI code.

Bart.

Re: Block layer use of __GFP flags

[Matthew Wilcox]

On Mon, Apr 09, 2018 at 01:26:50AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 09, 2018 at 08:53:49AM +0200, Hannes Reinecke wrote:
> > Why don't you fold the 'flags' argument into the 'gfp_flags', and drop
> > the 'flags' argument completely?
> > Looks a bit pointless to me, having two arguments denoting basically
> > the same ...
> 
> Wrong way around.  gfp_flags doesn't really make much sense in this
> context.  We just want the plain flags argument, including a non-block
> flag for it.

Look at this sequence from scsi_ioctl.c:

        if (bytes) {
                buffer = kzalloc(bytes, q->bounce_gfp | GFP_USER| __GFP_NOWARN);
                if (!buffer)
                        return -ENOMEM;

        }

        rq = blk_get_request(q, in_len ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN,
                        __GFP_RECLAIM);

That makes no damn sense.  If the buffer can be allocated using GFP_USER,
then the request should also be allocatable using GFP_USER.  In the current
tree, that (wrongly) gets translated into __GFP_DIRECT_RECLAIM.

Re: Block layer use of __GFP flags

[Bart Van Assche]

T24gTW9uLCAyMDE4LTA0LTA5IGF0IDAxOjI2IC0wNzAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gT24gTW9uLCBBcHIgMDksIDIwMTggYXQgMDg6NTM6NDlBTSArMDIwMCwgSGFubmVzIFJl
aW5lY2tlIHdyb3RlOg0KPiA+IFdoeSBkb24ndCB5b3UgZm9sZCB0aGUgJ2ZsYWdzJyBhcmd1bWVu
dCBpbnRvIHRoZSAnZ2ZwX2ZsYWdzJywgYW5kIGRyb3ANCj4gPiB0aGUgJ2ZsYWdzJyBhcmd1bWVu
dCBjb21wbGV0ZWx5Pw0KPiA+IExvb2tzIGEgYml0IHBvaW50bGVzcyB0byBtZSwgaGF2aW5nIHR3
byBhcmd1bWVudHMgZGVub3RpbmcgYmFzaWNhbGx5DQo+ID4gdGhlIHNhbWUgLi4uDQo+IA0KPiBX
cm9uZyB3YXkgYXJvdW5kLiAgZ2ZwX2ZsYWdzIGRvZXNuJ3QgcmVhbGx5IG1ha2UgbXVjaCBzZW5z
ZSBpbiB0aGlzDQo+IGNvbnRleHQuICBXZSBqdXN0IHdhbnQgdGhlIHBsYWluIGZsYWdzIGFyZ3Vt
ZW50LCBpbmNsdWRpbmcgYSBub24tYmxvY2sNCj4gZmxhZyBmb3IgaXQuDQoNCkhlbGxvIENocmlz
dG9waCBhbmQgSGFubmVzLA0KDQpUb2RheSBzcGFyc2UgdmVyaWZpZXMgd2hldGhlciBvciBub3Qg
Z2ZwX3QgYW5kIGJsa19tcV9yZXFfdCBmbGFncyBhcmUgbm90DQptaXhlZCB3aXRoIG90aGVyIGZs
YWdzLiBDb21iaW5pbmcgdGhlc2UgdHdvIHR5cGVzIG9mIGZsYWdzIGludG8gYSBzaW5nbGUNCmJp
dCBwYXR0ZXJuIHdvdWxkIHJlcXVpcmUgc29tZSB1Z2x5IGNhc3RzIHRvIGF2b2lkIHRoYXQgc3Bh
cnNlIGNvbXBsYWlucw0KYWJvdXQgY29tYmluaW5nIHRoZXNlIGZsYWdzIGludG8gYSBzaW5nbGUg
Yml0IHBhdHRlcm4uDQoNCkJhcnQuDQoNCg0KDQo=

Re: Block layer use of __GFP flags

[Bart Van Assche]

On Mon, 2018-04-09 at 01:26 -0700, Christoph Hellwig wrote:
> On Mon, Apr 09, 2018 at 08:53:49AM +0200, Hannes Reinecke wrote:
> > Why don't you fold the 'flags' argument into the 'gfp_flags', and drop
> > the 'flags' argument completely?
> > Looks a bit pointless to me, having two arguments denoting basically
> > the same ...
> 
> Wrong way around.  gfp_flags doesn't really make much sense in this
> context.  We just want the plain flags argument, including a non-block
> flag for it.

Hello Christoph and Hannes,

Today sparse verifies whether or not gfp_t and blk_mq_req_t flags are not
mixed with other flags. Combining these two types of flags into a single
bit pattern would require some ugly casts to avoid that sparse complains
about combining these flags into a single bit pattern.

Bart.

Re: Block layer use of __GFP flags

[Michal Hocko]

On Mon 09-04-18 15:03:45, Bart Van Assche wrote:
> On Mon, 2018-04-09 at 11:00 +0200, Michal Hocko wrote:
> > On Mon 09-04-18 04:46:22, Bart Van Assche wrote:
> > [...]
> > [...]
> > > diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
> > > index ad8a125defdd..3ddb464b72e6 100644
> > > --- a/drivers/ide/ide-pm.c
> > > +++ b/drivers/ide/ide-pm.c
> > > @@ -91,7 +91,7 @@ int generic_ide_resume(struct device *dev)
> > >  
> > >  	memset(&rqpm, 0, sizeof(rqpm));
> > >  	rq = blk_get_request_flags(drive->queue, REQ_OP_DRV_IN,
> > > -				   BLK_MQ_REQ_PREEMPT);
> > > +				   BLK_MQ_REQ_PREEMPT, __GFP_RECLAIM);
> > 
> > Is there any reason to use __GFP_RECLAIM directly. I guess you wanted to
> > have GFP_NOIO semantic, right? So why not be explicit about that. Same
> > for other instances of this flag in the patch
> 
> Hello Michal,
> 
> Thanks for the review. The use of __GFP_RECLAIM in this code (which was
> called __GFP_WAIT in the past) predates the git history.

Yeah, __GFP_WAIT -> __GFP_RECLAIM was a pseudo automated change IIRC.
Anyway GFP_NOIO should be pretty much equivalent and self explanatory.
__GFP_RECLAIM is more of an internal thing than something be for used as
a plain gfp mask.

Sure, there is no real need to change that but if you want to make the
code more neat and self explanatory I would go with GFP_NOIO.

Just my 2c
-- 
Michal Hocko
SUSE Labs