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]

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]

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]

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]

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