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(, 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

[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

[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 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(, 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

[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(, 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

[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

[Hannes Reinecke]

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

[Bart Van Assche]

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

[Matthew Wilcox]

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

[Bart Van Assche]

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.