| Message ID | 63d16891d115de25ac2776088571d7e90dab867a.camel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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.
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.
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
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 *);