Re: Block layer use of __GFP flags
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
Re: Block layer use of __GFP flags
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
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
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
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
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
On Mon, 9 Apr 2018 04:46:22 + "Bart Van Assche" wrote: > On Sun, 2018-04-08 at 12:08 -0700, Matthew Wilcox wrote: > > On Sun, Apr 08, 2018 at 04:40:59PM +, 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
On Sun, 2018-04-08 at 12:08 -0700, Matthew Wilcox wrote: > On Sun, Apr 08, 2018 at 04:40:59PM +, 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
Re: Block layer use of __GFP flags
On Sun, Apr 08, 2018 at 04:40:59PM +, 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
On Sat, 2018-04-07 at 23:54 -0700, Matthew Wilcox wrote: > Please explain: > > commit 6a15674d1e90917f1723a814e2e8c949000440f7 > Author: Bart Van Assche > 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.
Block layer use of __GFP flags
Please explain: commit 6a15674d1e90917f1723a814e2e8c949000440f7 Author: Bart Van Assche 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.