* [PATCH 0/7] Improve I/O priority handling
@ 2018-11-19 3:51 Damien Le Moal
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
` (6 more replies)
0 siblings, 7 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
This small series based on for-4.21/block brings improvements to I/O priority
hanlding. The main fixes are in patches 5, 6 and 7. These fix BIO and request
I/O priority initialization for both the synchronous and asynchronous pathes.
Of note is that patch 3 is what I beleive a clear bug fix but can result in
userland applications behavior change since a different I/O priority values
will be returned for processes that have *not* executed ioprio_set(). This
needs careful review.
Damien Le Moal (7):
aio: Comment use of IOCB_FLAG_IOPRIO aio flag
block: Remove bio->bi_ioc
block: Fix get_task_ioprio() default return value
block: Introduce get_current_ioprio()
aio: Fix fallback I/O priority value
block: prevent merging of requests with different priorities
block: Initialize BIO I/O priority early
block/bio.c | 4 ----
block/blk-core.c | 12 +-----------
block/blk-merge.c | 14 ++++++++++++--
block/blk-mq-sched.c | 4 ++--
block/blk-mq-sched.h | 2 +-
block/blk-mq.c | 4 ++--
block/blk.h | 16 ----------------
block/ioprio.c | 2 +-
fs/aio.c | 2 +-
include/linux/blk_types.h | 3 +--
include/linux/fs.h | 2 +-
include/linux/ioprio.h | 13 +++++++++++++
include/uapi/linux/aio_abi.h | 2 ++
13 files changed, 37 insertions(+), 43 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:12 ` Christoph Hellwig
2018-11-19 8:15 ` Johannes Thumshirn
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
` (5 subsequent siblings)
6 siblings, 2 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Comment the use of the IOCB_FLAG_IOPRIO aio flag similarly to the
IOCB_FLAG_RESFD flag.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
include/uapi/linux/aio_abi.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index ce43d340f010..8387e0af0f76 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -50,6 +50,8 @@ enum {
*
* IOCB_FLAG_RESFD - Set if the "aio_resfd" member of the "struct iocb"
* is valid.
+ * IOCB_FLAG_IOPRIO - Set if the "aio_reqprio" member of the "struct iocb"
+ * is valid.
*/
#define IOCB_FLAG_RESFD (1 << 0)
#define IOCB_FLAG_IOPRIO (1 << 1)
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:13 ` Christoph Hellwig
` (3 more replies)
2018-11-19 3:51 ` [PATCH 3/7] block: Fix get_task_ioprio() default return value Damien Le Moal
` (4 subsequent siblings)
6 siblings, 4 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
bio->bi_ioc is never set so always NULL. Remove references to it in
bio_disassociate_task() and in rq_ioc() and delete this field from
struct bio. With this change, rq_ioc() always returns
current->io_context without the need for a bio argument. Further
simplify the code and make it more readable by also removing this
helper, which also allows to simplify blk_mq_sched_assign_ioc() by
removing its bio argument.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/bio.c | 4 ----
block/blk-core.c | 2 +-
block/blk-mq-sched.c | 4 ++--
block/blk-mq-sched.h | 2 +-
block/blk-mq.c | 4 ++--
block/blk.h | 16 ----------------
include/linux/blk_types.h | 3 +--
7 files changed, 7 insertions(+), 28 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 4f4d9884443b..03895cc0d74a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -2027,10 +2027,6 @@ int bio_associate_blkg(struct bio *bio, struct blkcg_gq *blkg)
*/
void bio_disassociate_task(struct bio *bio)
{
- if (bio->bi_ioc) {
- put_io_context(bio->bi_ioc);
- bio->bi_ioc = NULL;
- }
if (bio->bi_css) {
css_put(bio->bi_css);
bio->bi_css = NULL;
diff --git a/block/blk-core.c b/block/blk-core.c
index d6e8ab9ca99d..492648c96992 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -813,7 +813,7 @@ unsigned int blk_plug_queued_count(struct request_queue *q)
void blk_init_request_from_bio(struct request *req, struct bio *bio)
{
- struct io_context *ioc = rq_ioc(bio);
+ struct io_context *ioc = current->io_context;
if (bio->bi_opf & REQ_RAHEAD)
req->cmd_flags |= REQ_FAILFAST_MASK;
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index d084f731d104..13b8dc332541 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -31,10 +31,10 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
}
EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
-void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio)
+void blk_mq_sched_assign_ioc(struct request *rq)
{
struct request_queue *q = rq->q;
- struct io_context *ioc = rq_ioc(bio);
+ struct io_context *ioc = current->io_context;
struct io_cq *icq;
spin_lock_irq(&q->queue_lock);
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 7ff5671bf128..0f719c8532ae 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -8,7 +8,7 @@
void blk_mq_sched_free_hctx_data(struct request_queue *q,
void (*exit)(struct blk_mq_hw_ctx *));
-void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio);
+void blk_mq_sched_assign_ioc(struct request *rq);
void blk_mq_sched_request_inserted(struct request *rq);
bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 32b246ed44c0..636f80b96fa6 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -389,8 +389,8 @@ static struct request *blk_mq_get_request(struct request_queue *q,
if (!op_is_flush(data->cmd_flags)) {
rq->elv.icq = NULL;
if (e && e->type->ops.prepare_request) {
- if (e->type->icq_cache && rq_ioc(bio))
- blk_mq_sched_assign_ioc(rq, bio);
+ if (e->type->icq_cache)
+ blk_mq_sched_assign_ioc(rq);
e->type->ops.prepare_request(rq, bio);
rq->rq_flags |= RQF_ELVPRIV;
diff --git a/block/blk.h b/block/blk.h
index 816a9abb87cd..610948157a5b 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -254,22 +254,6 @@ void ioc_clear_queue(struct request_queue *q);
int create_task_io_context(struct task_struct *task, gfp_t gfp_mask, int node);
-/**
- * rq_ioc - determine io_context for request allocation
- * @bio: request being allocated is for this bio (can be %NULL)
- *
- * Determine io_context to use for request allocation for @bio. May return
- * %NULL if %current->io_context doesn't exist.
- */
-static inline struct io_context *rq_ioc(struct bio *bio)
-{
-#ifdef CONFIG_BLK_CGROUP
- if (bio && bio->bi_ioc)
- return bio->bi_ioc;
-#endif
- return current->io_context;
-}
-
/**
* create_io_context - try to create task->io_context
* @gfp_mask: allocation mask
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index dbdbfbd6a987..c0ba1a038ff3 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -174,10 +174,9 @@ struct bio {
void *bi_private;
#ifdef CONFIG_BLK_CGROUP
/*
- * Optional ioc and css associated with this bio. Put on bio
+ * Optional css associated with this bio. Put on bio
* release. Read comment on top of bio_associate_current().
*/
- struct io_context *bi_ioc;
struct cgroup_subsys_state *bi_css;
struct blkcg_gq *bi_blkg;
struct bio_issue bi_issue;
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 3/7] block: Fix get_task_ioprio() default return value
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:16 ` Christoph Hellwig
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
` (3 subsequent siblings)
6 siblings, 1 reply; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
As explained in ioprio_get() and ionice man pages, the default I/O
priority class for processes which have not set an I/O priority using
ioprio_set() is IOPRIO_CLASS_BE and not IOPRIO_CLASS_NONE.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/ioprio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index f9821080c92c..ea5be206eb26 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -163,7 +163,7 @@ static int get_task_ioprio(struct task_struct *p)
ret = security_task_getioprio(p);
if (ret)
goto out;
- ret = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, IOPRIO_NORM);
+ ret = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM);
task_lock(p);
if (p->io_context)
ret = p->io_context->ioprio;
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 4/7] block: Introduce get_current_ioprio()
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
` (2 preceding siblings ...)
2018-11-19 3:51 ` [PATCH 3/7] block: Fix get_task_ioprio() default return value Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:17 ` Christoph Hellwig
` (2 more replies)
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
` (2 subsequent siblings)
6 siblings, 3 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Define get_current_ioprio() as an inline helper to obtain the caller
I/O priority from its task I/O context. Use this helper in
blk_init_request_from_bio() to set a request ioprio.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/blk-core.c | 6 +-----
include/linux/ioprio.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 492648c96992..4450d3c08f25 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct request_queue *q)
void blk_init_request_from_bio(struct request *req, struct bio *bio)
{
- struct io_context *ioc = current->io_context;
-
if (bio->bi_opf & REQ_RAHEAD)
req->cmd_flags |= REQ_FAILFAST_MASK;
req->__sector = bio->bi_iter.bi_sector;
if (ioprio_valid(bio_prio(bio)))
req->ioprio = bio_prio(bio);
- else if (ioc)
- req->ioprio = ioc->ioprio;
else
- req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+ req->ioprio = get_current_ioprio();
req->write_hint = bio->bi_write_hint;
blk_rq_bio_prep(req->q, req, bio);
}
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 9e30ed6443db..e9bfe6972aed 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct task_struct *task)
return IOPRIO_CLASS_BE;
}
+/*
+ * If the calling process has set an I/O priority, use that. Otherwise, return
+ * the default I/O priority.
+ */
+static inline int get_current_ioprio(void)
+{
+ struct io_context *ioc = current->io_context;
+
+ if (ioc)
+ return ioc->ioprio;
+ return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+}
+
/*
* For inheritance, return the highest of the two given priorities
*/
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 5/7] aio: Fix fallback I/O priority value
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
` (3 preceding siblings ...)
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:18 ` Christoph Hellwig
` (2 more replies)
2018-11-19 3:51 ` [PATCH 6/7] block: prevent merging of requests with different priorities Damien Le Moal
2018-11-19 3:51 ` [PATCH 7/7] block: Initialize BIO I/O priority early Damien Le Moal
6 siblings, 3 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
For cases when the application does not specify aio_reqprio for an aio,
fallback to use get_current_ioprio() to obtain the task I/O priority
last set using ioprio_set() rather than the hardcoded IOPRIO_CLASS_NONE
value.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
fs/aio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/aio.c b/fs/aio.c
index 301e6314183b..b984918be4b7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1441,7 +1441,7 @@ static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
req->ki_ioprio = iocb->aio_reqprio;
} else
- req->ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+ req->ki_ioprio = get_current_ioprio();
ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
if (unlikely(ret))
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 6/7] block: prevent merging of requests with different priorities
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
` (4 preceding siblings ...)
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 8:31 ` Johannes Thumshirn
2018-11-19 3:51 ` [PATCH 7/7] block: Initialize BIO I/O priority early Damien Le Moal
6 siblings, 2 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Growing in size a high priority request by merging it with a lower
priority BIO or request will increase the request execution time. This
is the opposite result of the desired effect of high I/O priorities,
namely getting low I/O latencies. Prevent merging of requests and BIOs
that have different I/O priorities to fix this.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/blk-core.c | 3 ---
block/blk-merge.c | 14 ++++++++++++--
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 4450d3c08f25..dde30b08aa14 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -662,7 +662,6 @@ bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
req->biotail->bi_next = bio;
req->biotail = bio;
req->__data_len += bio->bi_iter.bi_size;
- req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
blk_account_io_start(req, false);
return true;
@@ -686,7 +685,6 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
req->__sector = bio->bi_iter.bi_sector;
req->__data_len += bio->bi_iter.bi_size;
- req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
blk_account_io_start(req, false);
return true;
@@ -706,7 +704,6 @@ bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
req->biotail->bi_next = bio;
req->biotail = bio;
req->__data_len += bio->bi_iter.bi_size;
- req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
req->nr_phys_segments = segments + 1;
blk_account_io_start(req, false);
diff --git a/block/blk-merge.c b/block/blk-merge.c
index b1df622cbd85..129f554d250d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -752,6 +752,12 @@ static struct request *attempt_merge(struct request_queue *q,
if (req->write_hint != next->write_hint)
return NULL;
+ /*
+ * Don't allow merge of different I/O priorities.
+ */
+ if (req->ioprio != next->ioprio)
+ return NULL;
+
/*
* If we are allowed to merge, then append bio list
* from next to rq and release next. merge_requests_fn
@@ -807,8 +813,6 @@ static struct request *attempt_merge(struct request_queue *q,
*/
blk_account_io_merge(next);
- req->ioprio = ioprio_best(req->ioprio, next->ioprio);
-
/*
* ownership of bio passed from next to req, return 'next' for
* the caller to free
@@ -883,6 +887,12 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
if (rq->write_hint != bio->bi_write_hint)
return false;
+ /*
+ * Don't allow merge of different I/O priorities.
+ */
+ if (rq->ioprio != bio_prio(bio))
+ return false;
+
return true;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 7/7] block: Initialize BIO I/O priority early
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
` (5 preceding siblings ...)
2018-11-19 3:51 ` [PATCH 6/7] block: prevent merging of requests with different priorities Damien Le Moal
@ 2018-11-19 3:51 ` Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 19:11 ` Adam Manzanares
6 siblings, 2 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 3:51 UTC (permalink / raw)
To: linux-block, Jens Axboe; +Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
For the synchronous I/O path case (read(), write() etc system calls), a
BIO I/O priority is not initialized until the execution of
blk_init_request_from_bio() when the BIO is submitted and a request
initialized for the BIO execution. This is due to the ki_ioprio field of
the struct kiocb defined on stack being always initialized to
IOPRIO_CLASS_NONE, regardless of the calling process I/O context ioprio
value set with ioprio_set(). This late initialization can result in the
BIO being merged to pending requests even when the I/O priorities
differ.
Fix this by initializing the ki_iopriority field of on stack struct
kiocb using the get_current_ioprio() helper, ensuring that all BIOs
allocated and submitted for the system call execution see the correct
intended I/O priority early. With this, since a BIO I/O priority is
always set to the intended effective value for both the sync and async
path, blk_init_request_from_bio() can be simplified.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
block/blk-core.c | 5 +----
include/linux/fs.h | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index dde30b08aa14..04f5be473638 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -814,10 +814,7 @@ void blk_init_request_from_bio(struct request *req, struct bio *bio)
req->cmd_flags |= REQ_FAILFAST_MASK;
req->__sector = bio->bi_iter.bi_sector;
- if (ioprio_valid(bio_prio(bio)))
- req->ioprio = bio_prio(bio);
- else
- req->ioprio = get_current_ioprio();
+ req->ioprio = bio_prio(bio);
req->write_hint = bio->bi_write_hint;
blk_rq_bio_prep(req->q, req, bio);
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c95c0807471f..a1ab233e6469 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2021,7 +2021,7 @@ static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
.ki_filp = filp,
.ki_flags = iocb_flags(filp),
.ki_hint = ki_hint_validate(file_write_hint(filp)),
- .ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0),
+ .ki_ioprio = get_current_ioprio(),
};
}
--
2.19.1
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
@ 2018-11-19 8:12 ` Christoph Hellwig
2018-11-19 8:15 ` Johannes Thumshirn
1 sibling, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:12 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:25PM +0900, Damien Le Moal wrote:
> Comment the use of the IOCB_FLAG_IOPRIO aio flag similarly to the
> IOCB_FLAG_RESFD flag.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
@ 2018-11-19 8:13 ` Christoph Hellwig
2018-11-19 8:16 ` Johannes Thumshirn
` (2 subsequent siblings)
3 siblings, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:13 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:26PM +0900, Damien Le Moal wrote:
> bio->bi_ioc is never set so always NULL. Remove references to it in
> bio_disassociate_task() and in rq_ioc() and delete this field from
> struct bio. With this change, rq_ioc() always returns
> current->io_context without the need for a bio argument. Further
> simplify the code and make it more readable by also removing this
> helper, which also allows to simplify blk_mq_sched_assign_ioc() by
> removing its bio argument.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
2018-11-19 8:12 ` Christoph Hellwig
@ 2018-11-19 8:15 ` Johannes Thumshirn
1 sibling, 0 replies; 37+ messages in thread
From: Johannes Thumshirn @ 2018-11-19 8:15 UTC (permalink / raw)
To: Damien Le Moal, linux-block, Jens Axboe
Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn SUSE Labs
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
2018-11-19 8:13 ` Christoph Hellwig
@ 2018-11-19 8:16 ` Johannes Thumshirn
2018-11-19 19:07 ` Adam Manzanares
2018-11-20 17:21 ` Ming Lei
3 siblings, 0 replies; 37+ messages in thread
From: Johannes Thumshirn @ 2018-11-19 8:16 UTC (permalink / raw)
To: Damien Le Moal, linux-block, Jens Axboe
Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn SUSE Labs
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/7] block: Fix get_task_ioprio() default return value
2018-11-19 3:51 ` [PATCH 3/7] block: Fix get_task_ioprio() default return value Damien Le Moal
@ 2018-11-19 8:16 ` Christoph Hellwig
2018-11-20 1:47 ` Damien Le Moal
0 siblings, 1 reply; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:16 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:27PM +0900, Damien Le Moal wrote:
> As explained in ioprio_get() and ionice man pages, the default I/O
> priority class for processes which have not set an I/O priority using
> ioprio_set() is IOPRIO_CLASS_BE and not IOPRIO_CLASS_NONE.
While this matches the documentation, we've returned the current
values for 10 years, and before that plain 0 without any ioprio
magic. It might be safer to just fix the documentation.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/7] block: Introduce get_current_ioprio()
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
@ 2018-11-19 8:17 ` Christoph Hellwig
2018-11-19 8:26 ` Johannes Thumshirn
2018-11-19 18:17 ` Adam Manzanares
2 siblings, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:17 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:28PM +0900, Damien Le Moal wrote:
> Define get_current_ioprio() as an inline helper to obtain the caller
> I/O priority from its task I/O context. Use this helper in
> blk_init_request_from_bio() to set a request ioprio.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/7] aio: Fix fallback I/O priority value
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
@ 2018-11-19 8:18 ` Christoph Hellwig
2018-11-19 8:27 ` Johannes Thumshirn
2018-11-19 19:08 ` Adam Manzanares
2 siblings, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:18 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:29PM +0900, Damien Le Moal wrote:
> For cases when the application does not specify aio_reqprio for an aio,
> fallback to use get_current_ioprio() to obtain the task I/O priority
> last set using ioprio_set() rather than the hardcoded IOPRIO_CLASS_NONE
> value.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 6/7] block: prevent merging of requests with different priorities
2018-11-19 3:51 ` [PATCH 6/7] block: prevent merging of requests with different priorities Damien Le Moal
@ 2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 8:31 ` Johannes Thumshirn
1 sibling, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:19 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
> + /*
> + * Don't allow merge of different I/O priorities.
> + */
> + if (req->ioprio != next->ioprio)
> + return NULL;
> + /*
> + * Don't allow merge of different I/O priorities.
> + */
> + if (rq->ioprio != bio_prio(bio))
> + return false;
> +
These comments just restate what is obvious in the code. I would
recommend to drop them.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 7/7] block: Initialize BIO I/O priority early
2018-11-19 3:51 ` [PATCH 7/7] block: Initialize BIO I/O priority early Damien Le Moal
@ 2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 19:11 ` Adam Manzanares
1 sibling, 0 replies; 37+ messages in thread
From: Christoph Hellwig @ 2018-11-19 8:19 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:31PM +0900, Damien Le Moal wrote:
> For the synchronous I/O path case (read(), write() etc system calls), a
> BIO I/O priority is not initialized until the execution of
> blk_init_request_from_bio() when the BIO is submitted and a request
> initialized for the BIO execution. This is due to the ki_ioprio field of
> the struct kiocb defined on stack being always initialized to
> IOPRIO_CLASS_NONE, regardless of the calling process I/O context ioprio
> value set with ioprio_set(). This late initialization can result in the
> BIO being merged to pending requests even when the I/O priorities
> differ.
>
> Fix this by initializing the ki_iopriority field of on stack struct
> kiocb using the get_current_ioprio() helper, ensuring that all BIOs
> allocated and submitted for the system call execution see the correct
> intended I/O priority early. With this, since a BIO I/O priority is
> always set to the intended effective value for both the sync and async
> path, blk_init_request_from_bio() can be simplified.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/7] block: Introduce get_current_ioprio()
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
2018-11-19 8:17 ` Christoph Hellwig
@ 2018-11-19 8:26 ` Johannes Thumshirn
2018-11-19 18:17 ` Adam Manzanares
2 siblings, 0 replies; 37+ messages in thread
From: Johannes Thumshirn @ 2018-11-19 8:26 UTC (permalink / raw)
To: Damien Le Moal, linux-block, Jens Axboe
Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn SUSE Labs
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/7] aio: Fix fallback I/O priority value
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
2018-11-19 8:18 ` Christoph Hellwig
@ 2018-11-19 8:27 ` Johannes Thumshirn
2018-11-19 19:08 ` Adam Manzanares
2 siblings, 0 replies; 37+ messages in thread
From: Johannes Thumshirn @ 2018-11-19 8:27 UTC (permalink / raw)
To: Damien Le Moal, linux-block, Jens Axboe
Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Looks good,
Reiewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn SUSE Labs
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 6/7] block: prevent merging of requests with different priorities
2018-11-19 3:51 ` [PATCH 6/7] block: prevent merging of requests with different priorities Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
@ 2018-11-19 8:31 ` Johannes Thumshirn
1 sibling, 0 replies; 37+ messages in thread
From: Johannes Thumshirn @ 2018-11-19 8:31 UTC (permalink / raw)
To: Damien Le Moal, linux-block, Jens Axboe
Cc: Adam Manzanares, Alexander Viro, linux-fsdevel
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
Johannes Thumshirn SUSE Labs
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/7] block: Introduce get_current_ioprio()
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
2018-11-19 8:17 ` Christoph Hellwig
2018-11-19 8:26 ` Johannes Thumshirn
@ 2018-11-19 18:17 ` Adam Manzanares
2018-11-19 23:46 ` Damien Le Moal
2 siblings, 1 reply; 37+ messages in thread
From: Adam Manzanares @ 2018-11-19 18:17 UTC (permalink / raw)
To: linux-block, Damien Le Moal, axboe; +Cc: viro, linux-fsdevel
On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote:
> Define get_current_ioprio() as an inline helper to obtain the caller
> I/O priority from its task I/O context. Use this helper in
> blk_init_request_from_bio() to set a request ioprio.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> block/blk-core.c | 6 +-----
> include/linux/ioprio.h | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 492648c96992..4450d3c08f25 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct
> request_queue *q)
>
> void blk_init_request_from_bio(struct request *req, struct bio *bio)
> {
> - struct io_context *ioc = current->io_context;
> -
> if (bio->bi_opf & REQ_RAHEAD)
> req->cmd_flags |= REQ_FAILFAST_MASK;
>
> req->__sector = bio->bi_iter.bi_sector;
> if (ioprio_valid(bio_prio(bio)))
> req->ioprio = bio_prio(bio);
> - else if (ioc)
> - req->ioprio = ioc->ioprio;
> else
> - req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
> + req->ioprio = get_current_ioprio();
> req->write_hint = bio->bi_write_hint;
> blk_rq_bio_prep(req->q, req, bio);
> }
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 9e30ed6443db..e9bfe6972aed 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct
> task_struct *task)
> return IOPRIO_CLASS_BE;
> }
>
> +/*
> + * If the calling process has set an I/O priority, use that.
> Otherwise, return
> + * the default I/O priority.
> + */
> +static inline int get_current_ioprio(void)
> +{
> + struct io_context *ioc = current->io_context;
> +
> + if (ioc)
> + return ioc->ioprio;
> + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
Shouldn't this be IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) to be
consistent with patch 3?
> +}
> +
> /*
> * For inheritance, return the highest of the two given priorities
> */
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
2018-11-19 8:13 ` Christoph Hellwig
2018-11-19 8:16 ` Johannes Thumshirn
@ 2018-11-19 19:07 ` Adam Manzanares
2018-11-20 17:21 ` Ming Lei
3 siblings, 0 replies; 37+ messages in thread
From: Adam Manzanares @ 2018-11-19 19:07 UTC (permalink / raw)
To: linux-block, Damien Le Moal, axboe; +Cc: viro, linux-fsdevel
On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote:
> bio->bi_ioc is never set so always NULL. Remove references to it in
> bio_disassociate_task() and in rq_ioc() and delete this field from
> struct bio. With this change, rq_ioc() always returns
> current->io_context without the need for a bio argument. Further
> simplify the code and make it more readable by also removing this
> helper, which also allows to simplify blk_mq_sched_assign_ioc() by
> removing its bio argument.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed by:
> ---
> block/bio.c | 4 ----
> block/blk-core.c | 2 +-
> block/blk-mq-sched.c | 4 ++--
> block/blk-mq-sched.h | 2 +-
> block/blk-mq.c | 4 ++--
> block/blk.h | 16 ----------------
> include/linux/blk_types.h | 3 +--
> 7 files changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 4f4d9884443b..03895cc0d74a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -2027,10 +2027,6 @@ int bio_associate_blkg(struct bio *bio, struct
> blkcg_gq *blkg)
> */
> void bio_disassociate_task(struct bio *bio)
> {
> - if (bio->bi_ioc) {
> - put_io_context(bio->bi_ioc);
> - bio->bi_ioc = NULL;
> - }
> if (bio->bi_css) {
> css_put(bio->bi_css);
> bio->bi_css = NULL;
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d6e8ab9ca99d..492648c96992 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -813,7 +813,7 @@ unsigned int blk_plug_queued_count(struct
> request_queue *q)
>
> void blk_init_request_from_bio(struct request *req, struct bio *bio)
> {
> - struct io_context *ioc = rq_ioc(bio);
> + struct io_context *ioc = current->io_context;
>
> if (bio->bi_opf & REQ_RAHEAD)
> req->cmd_flags |= REQ_FAILFAST_MASK;
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index d084f731d104..13b8dc332541 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -31,10 +31,10 @@ void blk_mq_sched_free_hctx_data(struct
> request_queue *q,
> }
> EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>
> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio)
> +void blk_mq_sched_assign_ioc(struct request *rq)
> {
> struct request_queue *q = rq->q;
> - struct io_context *ioc = rq_ioc(bio);
> + struct io_context *ioc = current->io_context;
> struct io_cq *icq;
>
> spin_lock_irq(&q->queue_lock);
> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> index 7ff5671bf128..0f719c8532ae 100644
> --- a/block/blk-mq-sched.h
> +++ b/block/blk-mq-sched.h
> @@ -8,7 +8,7 @@
> void blk_mq_sched_free_hctx_data(struct request_queue *q,
> void (*exit)(struct blk_mq_hw_ctx *));
>
> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio);
> +void blk_mq_sched_assign_ioc(struct request *rq);
>
> void blk_mq_sched_request_inserted(struct request *rq);
> bool blk_mq_sched_try_merge(struct request_queue *q, struct bio
> *bio,
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 32b246ed44c0..636f80b96fa6 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -389,8 +389,8 @@ static struct request *blk_mq_get_request(struct
> request_queue *q,
> if (!op_is_flush(data->cmd_flags)) {
> rq->elv.icq = NULL;
> if (e && e->type->ops.prepare_request) {
> - if (e->type->icq_cache && rq_ioc(bio))
> - blk_mq_sched_assign_ioc(rq, bio);
> + if (e->type->icq_cache)
> + blk_mq_sched_assign_ioc(rq);
>
> e->type->ops.prepare_request(rq, bio);
> rq->rq_flags |= RQF_ELVPRIV;
> diff --git a/block/blk.h b/block/blk.h
> index 816a9abb87cd..610948157a5b 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -254,22 +254,6 @@ void ioc_clear_queue(struct request_queue *q);
>
> int create_task_io_context(struct task_struct *task, gfp_t gfp_mask,
> int node);
>
> -/**
> - * rq_ioc - determine io_context for request allocation
> - * @bio: request being allocated is for this bio (can be %NULL)
> - *
> - * Determine io_context to use for request allocation for @bio. May
> return
> - * %NULL if %current->io_context doesn't exist.
> - */
> -static inline struct io_context *rq_ioc(struct bio *bio)
> -{
> -#ifdef CONFIG_BLK_CGROUP
> - if (bio && bio->bi_ioc)
> - return bio->bi_ioc;
> -#endif
> - return current->io_context;
> -}
> -
> /**
> * create_io_context - try to create task->io_context
> * @gfp_mask: allocation mask
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index dbdbfbd6a987..c0ba1a038ff3 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -174,10 +174,9 @@ struct bio {
> void *bi_private;
> #ifdef CONFIG_BLK_CGROUP
> /*
> - * Optional ioc and css associated with this bio. Put on bio
> + * Optional css associated with this bio. Put on bio
> * release. Read comment on top of bio_associate_current().
> */
> - struct io_context *bi_ioc;
> struct cgroup_subsys_state *bi_css;
> struct blkcg_gq *bi_blkg;
> struct bio_issue bi_issue;
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/7] aio: Fix fallback I/O priority value
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
2018-11-19 8:18 ` Christoph Hellwig
2018-11-19 8:27 ` Johannes Thumshirn
@ 2018-11-19 19:08 ` Adam Manzanares
2 siblings, 0 replies; 37+ messages in thread
From: Adam Manzanares @ 2018-11-19 19:08 UTC (permalink / raw)
To: linux-block, Damien Le Moal, axboe; +Cc: viro, linux-fsdevel
On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote:
> For cases when the application does not specify aio_reqprio for an
> aio,
> fallback to use get_current_ioprio() to obtain the task I/O priority
> last set using ioprio_set() rather than the hardcoded
> IOPRIO_CLASS_NONE
> value.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Looks good,
Reviewed-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
> fs/aio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 301e6314183b..b984918be4b7 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1441,7 +1441,7 @@ static int aio_prep_rw(struct kiocb *req,
> struct iocb *iocb)
>
> req->ki_ioprio = iocb->aio_reqprio;
> } else
> - req->ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE,
> 0);
> + req->ki_ioprio = get_current_ioprio();
>
> ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
> if (unlikely(ret))
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 7/7] block: Initialize BIO I/O priority early
2018-11-19 3:51 ` [PATCH 7/7] block: Initialize BIO I/O priority early Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
@ 2018-11-19 19:11 ` Adam Manzanares
1 sibling, 0 replies; 37+ messages in thread
From: Adam Manzanares @ 2018-11-19 19:11 UTC (permalink / raw)
To: linux-block, Damien Le Moal, axboe; +Cc: viro, linux-fsdevel
On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote:
> For the synchronous I/O path case (read(), write() etc system calls),
> a
> BIO I/O priority is not initialized until the execution of
> blk_init_request_from_bio() when the BIO is submitted and a request
> initialized for the BIO execution. This is due to the ki_ioprio field
> of
> the struct kiocb defined on stack being always initialized to
> IOPRIO_CLASS_NONE, regardless of the calling process I/O context
> ioprio
> value set with ioprio_set(). This late initialization can result in
> the
> BIO being merged to pending requests even when the I/O priorities
> differ.
>
> Fix this by initializing the ki_iopriority field of on stack struct
> kiocb using the get_current_ioprio() helper, ensuring that all BIOs
> allocated and submitted for the system call execution see the correct
> intended I/O priority early. With this, since a BIO I/O priority is
> always set to the intended effective value for both the sync and
> async
> path, blk_init_request_from_bio() can be simplified.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Nice cleanup, looks good. Thanks Damien.
Reviewed-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
> block/blk-core.c | 5 +----
> include/linux/fs.h | 2 +-
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index dde30b08aa14..04f5be473638 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -814,10 +814,7 @@ void blk_init_request_from_bio(struct request
> *req, struct bio *bio)
> req->cmd_flags |= REQ_FAILFAST_MASK;
>
> req->__sector = bio->bi_iter.bi_sector;
> - if (ioprio_valid(bio_prio(bio)))
> - req->ioprio = bio_prio(bio);
> - else
> - req->ioprio = get_current_ioprio();
> + req->ioprio = bio_prio(bio);
> req->write_hint = bio->bi_write_hint;
> blk_rq_bio_prep(req->q, req, bio);
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c95c0807471f..a1ab233e6469 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2021,7 +2021,7 @@ static inline void init_sync_kiocb(struct kiocb
> *kiocb, struct file *filp)
> .ki_filp = filp,
> .ki_flags = iocb_flags(filp),
> .ki_hint = ki_hint_validate(file_write_hint(filp)),
> - .ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0),
> + .ki_ioprio = get_current_ioprio(),
> };
> }
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/7] block: Introduce get_current_ioprio()
2018-11-19 18:17 ` Adam Manzanares
@ 2018-11-19 23:46 ` Damien Le Moal
0 siblings, 0 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-19 23:46 UTC (permalink / raw)
To: Adam Manzanares, linux-block, axboe; +Cc: viro, linux-fsdevel
Adam,
On 2018/11/20 3:18, Adam Manzanares wrote:
> On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote:
>> Define get_current_ioprio() as an inline helper to obtain the caller
>> I/O priority from its task I/O context. Use this helper in
>> blk_init_request_from_bio() to set a request ioprio.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>> block/blk-core.c | 6 +-----
>> include/linux/ioprio.h | 13 +++++++++++++
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 492648c96992..4450d3c08f25 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct
>> request_queue *q)
>>
>> void blk_init_request_from_bio(struct request *req, struct bio *bio)
>> {
>> - struct io_context *ioc = current->io_context;
>> -
>> if (bio->bi_opf & REQ_RAHEAD)
>> req->cmd_flags |= REQ_FAILFAST_MASK;
>>
>> req->__sector = bio->bi_iter.bi_sector;
>> if (ioprio_valid(bio_prio(bio)))
>> req->ioprio = bio_prio(bio);
>> - else if (ioc)
>> - req->ioprio = ioc->ioprio;
>> else
>> - req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
>> + req->ioprio = get_current_ioprio();
>> req->write_hint = bio->bi_write_hint;
>> blk_rq_bio_prep(req->q, req, bio);
>> }
>> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
>> index 9e30ed6443db..e9bfe6972aed 100644
>> --- a/include/linux/ioprio.h
>> +++ b/include/linux/ioprio.h
>> @@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct
>> task_struct *task)
>> return IOPRIO_CLASS_BE;
>> }
>>
>> +/*
>> + * If the calling process has set an I/O priority, use that.
>> Otherwise, return
>> + * the default I/O priority.
>> + */
>> +static inline int get_current_ioprio(void)
>> +{
>> + struct io_context *ioc = current->io_context;
>> +
>> + if (ioc)
>> + return ioc->ioprio;
>> + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
>
> Shouldn't this be IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) to be
> consistent with patch 3?
I do not think so. The effective I/O priority used in the case of NONE/0 is
determined by the scheduler. See the use of task_nice_ioprio() and
task_nice_ioclass() in cfq and bfq. So using NONE/0 is correct here I think.
Using BE/NORM would render all ioprio_valid() tests useless as BE/NORM is a
valid I/O priority.
Thinking more of it now, I think patch 3 should actually return by default
IOPRIO_PRIO_VALUE(task_nice_ioprio(), task_nice_ioclass()) rather than BE/NORM
as this value is only valid if (1) cfq or bfq are in use AND (2) the task
scheduling priority and nice values are the default.
In any case, I am dropping patch 3 as suggested by Christoph. We can try to
revisit this later making sure in the process that user land does not break (if
that is at all possible).
Best regards.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/7] block: Fix get_task_ioprio() default return value
2018-11-19 8:16 ` Christoph Hellwig
@ 2018-11-20 1:47 ` Damien Le Moal
0 siblings, 0 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-20 1:47 UTC (permalink / raw)
To: hch; +Cc: Adam Manzanares, viro, linux-block, axboe, linux-fsdevel
On Mon, 2018-11-19 at 00:16 -0800, Christoph Hellwig wrote:
> On Mon, Nov 19, 2018 at 12:51:27PM +0900, Damien Le Moal wrote:
> > As explained in ioprio_get() and ionice man pages, the default I/O
> > priority class for processes which have not set an I/O priority
> > using
> > ioprio_set() is IOPRIO_CLASS_BE and not IOPRIO_CLASS_NONE.
>
> While this matches the documentation, we've returned the current
> values for 10 years, and before that plain 0 without any ioprio
> magic. It might be safer to just fix the documentation.
OK. Will send a patch to fix the man pages.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
` (2 preceding siblings ...)
2018-11-19 19:07 ` Adam Manzanares
@ 2018-11-20 17:21 ` Ming Lei
2018-11-20 17:31 ` Jens Axboe
3 siblings, 1 reply; 37+ messages in thread
From: Ming Lei @ 2018-11-20 17:21 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-block, Jens Axboe, Adam Manzanares, Alexander Viro, linux-fsdevel
On Mon, Nov 19, 2018 at 12:51:26PM +0900, Damien Le Moal wrote:
> bio->bi_ioc is never set so always NULL. Remove references to it in
> bio_disassociate_task() and in rq_ioc() and delete this field from
> struct bio. With this change, rq_ioc() always returns
> current->io_context without the need for a bio argument. Further
> simplify the code and make it more readable by also removing this
> helper, which also allows to simplify blk_mq_sched_assign_ioc() by
> removing its bio argument.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> block/bio.c | 4 ----
> block/blk-core.c | 2 +-
> block/blk-mq-sched.c | 4 ++--
> block/blk-mq-sched.h | 2 +-
> block/blk-mq.c | 4 ++--
> block/blk.h | 16 ----------------
> include/linux/blk_types.h | 3 +--
> 7 files changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 4f4d9884443b..03895cc0d74a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -2027,10 +2027,6 @@ int bio_associate_blkg(struct bio *bio, struct blkcg_gq *blkg)
> */
> void bio_disassociate_task(struct bio *bio)
> {
> - if (bio->bi_ioc) {
> - put_io_context(bio->bi_ioc);
> - bio->bi_ioc = NULL;
> - }
> if (bio->bi_css) {
> css_put(bio->bi_css);
> bio->bi_css = NULL;
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d6e8ab9ca99d..492648c96992 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -813,7 +813,7 @@ unsigned int blk_plug_queued_count(struct request_queue *q)
>
> void blk_init_request_from_bio(struct request *req, struct bio *bio)
> {
> - struct io_context *ioc = rq_ioc(bio);
> + struct io_context *ioc = current->io_context;
>
> if (bio->bi_opf & REQ_RAHEAD)
> req->cmd_flags |= REQ_FAILFAST_MASK;
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index d084f731d104..13b8dc332541 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -31,10 +31,10 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
> }
> EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>
> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio)
> +void blk_mq_sched_assign_ioc(struct request *rq)
> {
> struct request_queue *q = rq->q;
> - struct io_context *ioc = rq_ioc(bio);
> + struct io_context *ioc = current->io_context;
> struct io_cq *icq;
>
> spin_lock_irq(&q->queue_lock);
> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> index 7ff5671bf128..0f719c8532ae 100644
> --- a/block/blk-mq-sched.h
> +++ b/block/blk-mq-sched.h
> @@ -8,7 +8,7 @@
> void blk_mq_sched_free_hctx_data(struct request_queue *q,
> void (*exit)(struct blk_mq_hw_ctx *));
>
> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio);
> +void blk_mq_sched_assign_ioc(struct request *rq);
>
> void blk_mq_sched_request_inserted(struct request *rq);
> bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 32b246ed44c0..636f80b96fa6 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -389,8 +389,8 @@ static struct request *blk_mq_get_request(struct request_queue *q,
> if (!op_is_flush(data->cmd_flags)) {
> rq->elv.icq = NULL;
> if (e && e->type->ops.prepare_request) {
> - if (e->type->icq_cache && rq_ioc(bio))
> - blk_mq_sched_assign_ioc(rq, bio);
> + if (e->type->icq_cache)
> + blk_mq_sched_assign_ioc(rq);
>
> e->type->ops.prepare_request(rq, bio);
> rq->rq_flags |= RQF_ELVPRIV;
> diff --git a/block/blk.h b/block/blk.h
> index 816a9abb87cd..610948157a5b 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -254,22 +254,6 @@ void ioc_clear_queue(struct request_queue *q);
>
> int create_task_io_context(struct task_struct *task, gfp_t gfp_mask, int node);
>
> -/**
> - * rq_ioc - determine io_context for request allocation
> - * @bio: request being allocated is for this bio (can be %NULL)
> - *
> - * Determine io_context to use for request allocation for @bio. May return
> - * %NULL if %current->io_context doesn't exist.
> - */
> -static inline struct io_context *rq_ioc(struct bio *bio)
> -{
> -#ifdef CONFIG_BLK_CGROUP
> - if (bio && bio->bi_ioc)
> - return bio->bi_ioc;
> -#endif
> - return current->io_context;
> -}
> -
> /**
> * create_io_context - try to create task->io_context
> * @gfp_mask: allocation mask
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index dbdbfbd6a987..c0ba1a038ff3 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -174,10 +174,9 @@ struct bio {
> void *bi_private;
> #ifdef CONFIG_BLK_CGROUP
> /*
> - * Optional ioc and css associated with this bio. Put on bio
> + * Optional css associated with this bio. Put on bio
> * release. Read comment on top of bio_associate_current().
> */
> - struct io_context *bi_ioc;
> struct cgroup_subsys_state *bi_css;
> struct blkcg_gq *bi_blkg;
> struct bio_issue bi_issue;
Hi,
Just found the following kernel oops, seems it is likely related with this
patch.
[ 391.981012] BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
[ 391.982506] PGD 0 P4D 0
[ 391.982975] Oops: 0000 [#1] PREEMPT SMP PTI
[ 391.983769] CPU: 1 PID: 1790 Comm: scsi_id Not tainted 4.20.0-rc3_72abead3bf43_for-4.21-block-mp-bvec-V11+ #1
[ 391.985563] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-2.fc27 04/01/2014
[ 391.987107] RIP: 0010:ioc_lookup_icq+0x13/0x54
[ 391.987936] Code: f6 48 8b 3d 1c 78 5b 01 5b 5d 41 5c 41 5d 41 5e 41 5f e9 68 bd eb ff 0f 1f 44 00 00 55 53 48 89 fb 51 48 89 f5 e8 e3 82 da ff <48> 8b 43 38 48 85 c0 74 05 48 39 28 74 22 48 63 b5 8c 00 00 00 48
[ 391.991318] RSP: 0018:ffffc90001467bb0 EFLAGS: 00010002
[ 391.992292] RAX: ffff888266c85ac0 RBX: 0000000000000000 RCX: 0000000000000100
[ 391.993615] RDX: 0000000000000001 RSI: ffff88826601f230 RDI: 0000000000000000
[ 391.994917] RBP: ffff88826601f230 R08: 00000000f461df07 R09: 0000000000000006
[ 391.996242] R10: ffffc90001467b10 R11: 0000000000000020 R12: ffff888269df4000
[ 391.997572] R13: 0000000000000000 R14: ffff88826601f2c4 R15: 0000000000000001
[ 391.998905] FS: 00007fa6923b7940(0000) GS:ffff888277a80000(0000) knlGS:0000000000000000
[ 392.000389] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 392.001468] CR2: 0000000000000038 CR3: 0000000106dd8005 CR4: 0000000000760ee0
[ 392.002783] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 392.004108] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 392.005394] PKRU: 55555554
[ 392.005897] Call Trace:
[ 392.006384] blk_mq_sched_assign_ioc+0x3d/0x7f
[ 392.007216] blk_mq_get_request+0x321/0x354
[ 392.008008] blk_mq_alloc_request+0x4e/0xbf
[ 392.008802] blk_get_request+0x24/0x4c
[ 392.009518] sg_io+0x93/0x371
[ 392.010074] ? bd_acquire+0xa6/0xa6
[ 392.010707] ? dput+0x29/0xfd
[ 392.011232] ? mntput_no_expire+0x11/0x185
[ 392.011987] scsi_cmd_ioctl+0x1d3/0x386
[ 392.012707] sd_ioctl+0xbb/0xde [sd_mod]
[ 392.013449] blkdev_ioctl+0x893/0x8bf
[ 392.014132] block_ioctl+0x3c/0x3f
[ 392.014781] vfs_ioctl+0x1e/0x2b
[ 392.015378] do_vfs_ioctl+0x531/0x559
[ 392.016059] ksys_ioctl+0x3e/0x5d
[ 392.016681] __x64_sys_ioctl+0x16/0x19
[ 392.017361] do_syscall_64+0x84/0x13f
[ 392.018060] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 392.018995] RIP: 0033:0x7fa691edf267
[ 392.019663] Code: b3 66 90 48 8b 05 19 3c 2c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e9 3b 2c 00 f7 d8 64 89 01 48
[ 392.023072] RSP: 002b:00007ffe463e8de8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 392.024457] RAX: ffffffffffffffda RBX: 00007ffe463e8e20 RCX: 00007fa691edf267
[ 392.025767] RDX: 00007ffe463e8e20 RSI: 0000000000002285 RDI: 0000000000000004
[ 392.027068] RBP: 00007ffe463e9470 R08: 0000000000002006 R09: 00000000fffffe00
[ 392.028413] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe463e9920
[ 392.029722] R13: 00007ffe463e8e20 R14: 00007ffe463e8e2a R15: 00007ffe463e9920
[ 392.031031] Modules linked in: scsi_debug null_blk isofs iTCO_wdt iTCO_vendor_support i2c_i801 i2c_core lpc_ich mfd_core ip_tables sr_mod cdrom usb_storage sd_mod ahci libahci libata crc32c_intel virtio_scsi qemu_fw_cfg dm_mirror dm_region_hash dm_log dm_mod [last unloaded: null_blk]
[ 392.035573] Dumping ftrace buffer:
[ 392.036203] (ftrace buffer empty)
[ 392.036871] CR2: 0000000000000038
[ 392.037503] ---[ end trace fa20a1088b068790 ]---
Thanks,
Ming
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-20 17:21 ` Ming Lei
@ 2018-11-20 17:31 ` Jens Axboe
2018-11-20 23:58 ` Damien Le Moal
2018-11-21 1:21 ` Ming Lei
0 siblings, 2 replies; 37+ messages in thread
From: Jens Axboe @ 2018-11-20 17:31 UTC (permalink / raw)
To: Ming Lei, Damien Le Moal
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 11/20/18 10:21 AM, Ming Lei wrote:
> On Mon, Nov 19, 2018 at 12:51:26PM +0900, Damien Le Moal wrote:
>> bio->bi_ioc is never set so always NULL. Remove references to it in
>> bio_disassociate_task() and in rq_ioc() and delete this field from
>> struct bio. With this change, rq_ioc() always returns
>> current->io_context without the need for a bio argument. Further
>> simplify the code and make it more readable by also removing this
>> helper, which also allows to simplify blk_mq_sched_assign_ioc() by
>> removing its bio argument.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>> block/bio.c | 4 ----
>> block/blk-core.c | 2 +-
>> block/blk-mq-sched.c | 4 ++--
>> block/blk-mq-sched.h | 2 +-
>> block/blk-mq.c | 4 ++--
>> block/blk.h | 16 ----------------
>> include/linux/blk_types.h | 3 +--
>> 7 files changed, 7 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 4f4d9884443b..03895cc0d74a 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -2027,10 +2027,6 @@ int bio_associate_blkg(struct bio *bio, struct blkcg_gq *blkg)
>> */
>> void bio_disassociate_task(struct bio *bio)
>> {
>> - if (bio->bi_ioc) {
>> - put_io_context(bio->bi_ioc);
>> - bio->bi_ioc = NULL;
>> - }
>> if (bio->bi_css) {
>> css_put(bio->bi_css);
>> bio->bi_css = NULL;
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index d6e8ab9ca99d..492648c96992 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -813,7 +813,7 @@ unsigned int blk_plug_queued_count(struct request_queue *q)
>>
>> void blk_init_request_from_bio(struct request *req, struct bio *bio)
>> {
>> - struct io_context *ioc = rq_ioc(bio);
>> + struct io_context *ioc = current->io_context;
>>
>> if (bio->bi_opf & REQ_RAHEAD)
>> req->cmd_flags |= REQ_FAILFAST_MASK;
>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>> index d084f731d104..13b8dc332541 100644
>> --- a/block/blk-mq-sched.c
>> +++ b/block/blk-mq-sched.c
>> @@ -31,10 +31,10 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
>> }
>> EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>
>> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio)
>> +void blk_mq_sched_assign_ioc(struct request *rq)
>> {
>> struct request_queue *q = rq->q;
>> - struct io_context *ioc = rq_ioc(bio);
>> + struct io_context *ioc = current->io_context;
>> struct io_cq *icq;
>>
>> spin_lock_irq(&q->queue_lock);
>> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
>> index 7ff5671bf128..0f719c8532ae 100644
>> --- a/block/blk-mq-sched.h
>> +++ b/block/blk-mq-sched.h
>> @@ -8,7 +8,7 @@
>> void blk_mq_sched_free_hctx_data(struct request_queue *q,
>> void (*exit)(struct blk_mq_hw_ctx *));
>>
>> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio);
>> +void blk_mq_sched_assign_ioc(struct request *rq);
>>
>> void blk_mq_sched_request_inserted(struct request *rq);
>> bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 32b246ed44c0..636f80b96fa6 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -389,8 +389,8 @@ static struct request *blk_mq_get_request(struct request_queue *q,
>> if (!op_is_flush(data->cmd_flags)) {
>> rq->elv.icq = NULL;
>> if (e && e->type->ops.prepare_request) {
>> - if (e->type->icq_cache && rq_ioc(bio))
>> - blk_mq_sched_assign_ioc(rq, bio);
>> + if (e->type->icq_cache)
>> + blk_mq_sched_assign_ioc(rq);
>>
>> e->type->ops.prepare_request(rq, bio);
>> rq->rq_flags |= RQF_ELVPRIV;
>> diff --git a/block/blk.h b/block/blk.h
>> index 816a9abb87cd..610948157a5b 100644
>> --- a/block/blk.h
>> +++ b/block/blk.h
>> @@ -254,22 +254,6 @@ void ioc_clear_queue(struct request_queue *q);
>>
>> int create_task_io_context(struct task_struct *task, gfp_t gfp_mask, int node);
>>
>> -/**
>> - * rq_ioc - determine io_context for request allocation
>> - * @bio: request being allocated is for this bio (can be %NULL)
>> - *
>> - * Determine io_context to use for request allocation for @bio. May return
>> - * %NULL if %current->io_context doesn't exist.
>> - */
>> -static inline struct io_context *rq_ioc(struct bio *bio)
>> -{
>> -#ifdef CONFIG_BLK_CGROUP
>> - if (bio && bio->bi_ioc)
>> - return bio->bi_ioc;
>> -#endif
>> - return current->io_context;
>> -}
>> -
>> /**
>> * create_io_context - try to create task->io_context
>> * @gfp_mask: allocation mask
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index dbdbfbd6a987..c0ba1a038ff3 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -174,10 +174,9 @@ struct bio {
>> void *bi_private;
>> #ifdef CONFIG_BLK_CGROUP
>> /*
>> - * Optional ioc and css associated with this bio. Put on bio
>> + * Optional css associated with this bio. Put on bio
>> * release. Read comment on top of bio_associate_current().
>> */
>> - struct io_context *bi_ioc;
>> struct cgroup_subsys_state *bi_css;
>> struct blkcg_gq *bi_blkg;
>> struct bio_issue bi_issue;
>
> Hi,
>
> Just found the following kernel oops, seems it is likely related with this
> patch.
>
> [ 391.981012] BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
> [ 391.982506] PGD 0 P4D 0
> [ 391.982975] Oops: 0000 [#1] PREEMPT SMP PTI
> [ 391.983769] CPU: 1 PID: 1790 Comm: scsi_id Not tainted 4.20.0-rc3_72abead3bf43_for-4.21-block-mp-bvec-V11+ #1
> [ 391.985563] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-2.fc27 04/01/2014
> [ 391.987107] RIP: 0010:ioc_lookup_icq+0x13/0x54
> [ 391.987936] Code: f6 48 8b 3d 1c 78 5b 01 5b 5d 41 5c 41 5d 41 5e 41 5f e9 68 bd eb ff 0f 1f 44 00 00 55 53 48 89 fb 51 48 89 f5 e8 e3 82 da ff <48> 8b 43 38 48 85 c0 74 05 48 39 28 74 22 48 63 b5 8c 00 00 00 48
> [ 391.991318] RSP: 0018:ffffc90001467bb0 EFLAGS: 00010002
> [ 391.992292] RAX: ffff888266c85ac0 RBX: 0000000000000000 RCX: 0000000000000100
> [ 391.993615] RDX: 0000000000000001 RSI: ffff88826601f230 RDI: 0000000000000000
> [ 391.994917] RBP: ffff88826601f230 R08: 00000000f461df07 R09: 0000000000000006
> [ 391.996242] R10: ffffc90001467b10 R11: 0000000000000020 R12: ffff888269df4000
> [ 391.997572] R13: 0000000000000000 R14: ffff88826601f2c4 R15: 0000000000000001
> [ 391.998905] FS: 00007fa6923b7940(0000) GS:ffff888277a80000(0000) knlGS:0000000000000000
> [ 392.000389] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 392.001468] CR2: 0000000000000038 CR3: 0000000106dd8005 CR4: 0000000000760ee0
> [ 392.002783] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 392.004108] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 392.005394] PKRU: 55555554
> [ 392.005897] Call Trace:
> [ 392.006384] blk_mq_sched_assign_ioc+0x3d/0x7f
> [ 392.007216] blk_mq_get_request+0x321/0x354
> [ 392.008008] blk_mq_alloc_request+0x4e/0xbf
> [ 392.008802] blk_get_request+0x24/0x4c
> [ 392.009518] sg_io+0x93/0x371
> [ 392.010074] ? bd_acquire+0xa6/0xa6
> [ 392.010707] ? dput+0x29/0xfd
> [ 392.011232] ? mntput_no_expire+0x11/0x185
> [ 392.011987] scsi_cmd_ioctl+0x1d3/0x386
> [ 392.012707] sd_ioctl+0xbb/0xde [sd_mod]
> [ 392.013449] blkdev_ioctl+0x893/0x8bf
> [ 392.014132] block_ioctl+0x3c/0x3f
> [ 392.014781] vfs_ioctl+0x1e/0x2b
> [ 392.015378] do_vfs_ioctl+0x531/0x559
> [ 392.016059] ksys_ioctl+0x3e/0x5d
> [ 392.016681] __x64_sys_ioctl+0x16/0x19
> [ 392.017361] do_syscall_64+0x84/0x13f
> [ 392.018060] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 392.018995] RIP: 0033:0x7fa691edf267
> [ 392.019663] Code: b3 66 90 48 8b 05 19 3c 2c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e9 3b 2c 00 f7 d8 64 89 01 48
> [ 392.023072] RSP: 002b:00007ffe463e8de8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [ 392.024457] RAX: ffffffffffffffda RBX: 00007ffe463e8e20 RCX: 00007fa691edf267
> [ 392.025767] RDX: 00007ffe463e8e20 RSI: 0000000000002285 RDI: 0000000000000004
> [ 392.027068] RBP: 00007ffe463e9470 R08: 0000000000002006 R09: 00000000fffffe00
> [ 392.028413] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe463e9920
> [ 392.029722] R13: 00007ffe463e8e20 R14: 00007ffe463e8e2a R15: 00007ffe463e9920
> [ 392.031031] Modules linked in: scsi_debug null_blk isofs iTCO_wdt iTCO_vendor_support i2c_i801 i2c_core lpc_ich mfd_core ip_tables sr_mod cdrom usb_storage sd_mod ahci libahci libata crc32c_intel virtio_scsi qemu_fw_cfg dm_mirror dm_region_hash dm_log dm_mod [last unloaded: null_blk]
> [ 392.035573] Dumping ftrace buffer:
> [ 392.036203] (ftrace buffer empty)
> [ 392.036871] CR2: 0000000000000038
> [ 392.037503] ---[ end trace fa20a1088b068790 ]---
I think the below should fix it, we haven't necessarily setup an
ioc if we're just doing as passthrough request.
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 13b8dc332541..f096d8989773 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
void blk_mq_sched_assign_ioc(struct request *rq)
{
struct request_queue *q = rq->q;
- struct io_context *ioc = current->io_context;
+ struct io_context *ioc;
struct io_cq *icq;
+ /*
+ * May not have an IO context if it's a passthrough request
+ */
+ ioc = current->io_context;
+ if (!ioc)
+ return;
+
spin_lock_irq(&q->queue_lock);
icq = ioc_lookup_icq(ioc, q);
spin_unlock_irq(&q->queue_lock);
--
Jens Axboe
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-20 17:31 ` Jens Axboe
@ 2018-11-20 23:58 ` Damien Le Moal
2018-11-21 1:24 ` Ming Lei
2018-11-21 2:10 ` Jens Axboe
2018-11-21 1:21 ` Ming Lei
1 sibling, 2 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-20 23:58 UTC (permalink / raw)
To: Jens Axboe, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 2018/11/21 2:31, Jens Axboe wrote:
> I think the below should fix it, we haven't necessarily setup an
> ioc if we're just doing as passthrough request.
>
>
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 13b8dc332541..f096d8989773 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
> void blk_mq_sched_assign_ioc(struct request *rq)
> {
> struct request_queue *q = rq->q;
> - struct io_context *ioc = current->io_context;
> + struct io_context *ioc;
> struct io_cq *icq;
>
> + /*
> + * May not have an IO context if it's a passthrough request
> + */
> + ioc = current->io_context;
> + if (!ioc)
> + return;
> +
> spin_lock_irq(&q->queue_lock);
> icq = ioc_lookup_icq(ioc, q);
> spin_unlock_irq(&q->queue_lock);
This seems reasonable to me, but I wonder why this problem was not triggering
before. The previous code getting the ioc with the rq_ioc(bio) call was
essentially the same and there was no "if (!ioc) return;" in
blk_mq_sched_assign_ioc() before the patch.
Any idea why this is popping up now ?
Ming,
Is this a new test your are running ? If this same problem triggers on stable
kernels, Jens patch needs to go to stable too.
Best regards.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-20 17:31 ` Jens Axboe
2018-11-20 23:58 ` Damien Le Moal
@ 2018-11-21 1:21 ` Ming Lei
1 sibling, 0 replies; 37+ messages in thread
From: Ming Lei @ 2018-11-21 1:21 UTC (permalink / raw)
To: Jens Axboe
Cc: Damien Le Moal, linux-block, Adam Manzanares, Alexander Viro,
linux-fsdevel
On Tue, Nov 20, 2018 at 10:31:19AM -0700, Jens Axboe wrote:
> On 11/20/18 10:21 AM, Ming Lei wrote:
> > On Mon, Nov 19, 2018 at 12:51:26PM +0900, Damien Le Moal wrote:
> >> bio->bi_ioc is never set so always NULL. Remove references to it in
> >> bio_disassociate_task() and in rq_ioc() and delete this field from
> >> struct bio. With this change, rq_ioc() always returns
> >> current->io_context without the need for a bio argument. Further
> >> simplify the code and make it more readable by also removing this
> >> helper, which also allows to simplify blk_mq_sched_assign_ioc() by
> >> removing its bio argument.
> >>
> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> >> ---
> >> block/bio.c | 4 ----
> >> block/blk-core.c | 2 +-
> >> block/blk-mq-sched.c | 4 ++--
> >> block/blk-mq-sched.h | 2 +-
> >> block/blk-mq.c | 4 ++--
> >> block/blk.h | 16 ----------------
> >> include/linux/blk_types.h | 3 +--
> >> 7 files changed, 7 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index 4f4d9884443b..03895cc0d74a 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -2027,10 +2027,6 @@ int bio_associate_blkg(struct bio *bio, struct blkcg_gq *blkg)
> >> */
> >> void bio_disassociate_task(struct bio *bio)
> >> {
> >> - if (bio->bi_ioc) {
> >> - put_io_context(bio->bi_ioc);
> >> - bio->bi_ioc = NULL;
> >> - }
> >> if (bio->bi_css) {
> >> css_put(bio->bi_css);
> >> bio->bi_css = NULL;
> >> diff --git a/block/blk-core.c b/block/blk-core.c
> >> index d6e8ab9ca99d..492648c96992 100644
> >> --- a/block/blk-core.c
> >> +++ b/block/blk-core.c
> >> @@ -813,7 +813,7 @@ unsigned int blk_plug_queued_count(struct request_queue *q)
> >>
> >> void blk_init_request_from_bio(struct request *req, struct bio *bio)
> >> {
> >> - struct io_context *ioc = rq_ioc(bio);
> >> + struct io_context *ioc = current->io_context;
> >>
> >> if (bio->bi_opf & REQ_RAHEAD)
> >> req->cmd_flags |= REQ_FAILFAST_MASK;
> >> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> >> index d084f731d104..13b8dc332541 100644
> >> --- a/block/blk-mq-sched.c
> >> +++ b/block/blk-mq-sched.c
> >> @@ -31,10 +31,10 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
> >> }
> >> EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
> >>
> >> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio)
> >> +void blk_mq_sched_assign_ioc(struct request *rq)
> >> {
> >> struct request_queue *q = rq->q;
> >> - struct io_context *ioc = rq_ioc(bio);
> >> + struct io_context *ioc = current->io_context;
> >> struct io_cq *icq;
> >>
> >> spin_lock_irq(&q->queue_lock);
> >> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> >> index 7ff5671bf128..0f719c8532ae 100644
> >> --- a/block/blk-mq-sched.h
> >> +++ b/block/blk-mq-sched.h
> >> @@ -8,7 +8,7 @@
> >> void blk_mq_sched_free_hctx_data(struct request_queue *q,
> >> void (*exit)(struct blk_mq_hw_ctx *));
> >>
> >> -void blk_mq_sched_assign_ioc(struct request *rq, struct bio *bio);
> >> +void blk_mq_sched_assign_ioc(struct request *rq);
> >>
> >> void blk_mq_sched_request_inserted(struct request *rq);
> >> bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
> >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >> index 32b246ed44c0..636f80b96fa6 100644
> >> --- a/block/blk-mq.c
> >> +++ b/block/blk-mq.c
> >> @@ -389,8 +389,8 @@ static struct request *blk_mq_get_request(struct request_queue *q,
> >> if (!op_is_flush(data->cmd_flags)) {
> >> rq->elv.icq = NULL;
> >> if (e && e->type->ops.prepare_request) {
> >> - if (e->type->icq_cache && rq_ioc(bio))
> >> - blk_mq_sched_assign_ioc(rq, bio);
> >> + if (e->type->icq_cache)
> >> + blk_mq_sched_assign_ioc(rq);
> >>
> >> e->type->ops.prepare_request(rq, bio);
> >> rq->rq_flags |= RQF_ELVPRIV;
> >> diff --git a/block/blk.h b/block/blk.h
> >> index 816a9abb87cd..610948157a5b 100644
> >> --- a/block/blk.h
> >> +++ b/block/blk.h
> >> @@ -254,22 +254,6 @@ void ioc_clear_queue(struct request_queue *q);
> >>
> >> int create_task_io_context(struct task_struct *task, gfp_t gfp_mask, int node);
> >>
> >> -/**
> >> - * rq_ioc - determine io_context for request allocation
> >> - * @bio: request being allocated is for this bio (can be %NULL)
> >> - *
> >> - * Determine io_context to use for request allocation for @bio. May return
> >> - * %NULL if %current->io_context doesn't exist.
> >> - */
> >> -static inline struct io_context *rq_ioc(struct bio *bio)
> >> -{
> >> -#ifdef CONFIG_BLK_CGROUP
> >> - if (bio && bio->bi_ioc)
> >> - return bio->bi_ioc;
> >> -#endif
> >> - return current->io_context;
> >> -}
> >> -
> >> /**
> >> * create_io_context - try to create task->io_context
> >> * @gfp_mask: allocation mask
> >> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> >> index dbdbfbd6a987..c0ba1a038ff3 100644
> >> --- a/include/linux/blk_types.h
> >> +++ b/include/linux/blk_types.h
> >> @@ -174,10 +174,9 @@ struct bio {
> >> void *bi_private;
> >> #ifdef CONFIG_BLK_CGROUP
> >> /*
> >> - * Optional ioc and css associated with this bio. Put on bio
> >> + * Optional css associated with this bio. Put on bio
> >> * release. Read comment on top of bio_associate_current().
> >> */
> >> - struct io_context *bi_ioc;
> >> struct cgroup_subsys_state *bi_css;
> >> struct blkcg_gq *bi_blkg;
> >> struct bio_issue bi_issue;
> >
> > Hi,
> >
> > Just found the following kernel oops, seems it is likely related with this
> > patch.
> >
> > [ 391.981012] BUG: unable to handle kernel NULL pointer dereference at 0000000000000038
> > [ 391.982506] PGD 0 P4D 0
> > [ 391.982975] Oops: 0000 [#1] PREEMPT SMP PTI
> > [ 391.983769] CPU: 1 PID: 1790 Comm: scsi_id Not tainted 4.20.0-rc3_72abead3bf43_for-4.21-block-mp-bvec-V11+ #1
> > [ 391.985563] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-2.fc27 04/01/2014
> > [ 391.987107] RIP: 0010:ioc_lookup_icq+0x13/0x54
> > [ 391.987936] Code: f6 48 8b 3d 1c 78 5b 01 5b 5d 41 5c 41 5d 41 5e 41 5f e9 68 bd eb ff 0f 1f 44 00 00 55 53 48 89 fb 51 48 89 f5 e8 e3 82 da ff <48> 8b 43 38 48 85 c0 74 05 48 39 28 74 22 48 63 b5 8c 00 00 00 48
> > [ 391.991318] RSP: 0018:ffffc90001467bb0 EFLAGS: 00010002
> > [ 391.992292] RAX: ffff888266c85ac0 RBX: 0000000000000000 RCX: 0000000000000100
> > [ 391.993615] RDX: 0000000000000001 RSI: ffff88826601f230 RDI: 0000000000000000
> > [ 391.994917] RBP: ffff88826601f230 R08: 00000000f461df07 R09: 0000000000000006
> > [ 391.996242] R10: ffffc90001467b10 R11: 0000000000000020 R12: ffff888269df4000
> > [ 391.997572] R13: 0000000000000000 R14: ffff88826601f2c4 R15: 0000000000000001
> > [ 391.998905] FS: 00007fa6923b7940(0000) GS:ffff888277a80000(0000) knlGS:0000000000000000
> > [ 392.000389] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 392.001468] CR2: 0000000000000038 CR3: 0000000106dd8005 CR4: 0000000000760ee0
> > [ 392.002783] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 392.004108] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 392.005394] PKRU: 55555554
> > [ 392.005897] Call Trace:
> > [ 392.006384] blk_mq_sched_assign_ioc+0x3d/0x7f
> > [ 392.007216] blk_mq_get_request+0x321/0x354
> > [ 392.008008] blk_mq_alloc_request+0x4e/0xbf
> > [ 392.008802] blk_get_request+0x24/0x4c
> > [ 392.009518] sg_io+0x93/0x371
> > [ 392.010074] ? bd_acquire+0xa6/0xa6
> > [ 392.010707] ? dput+0x29/0xfd
> > [ 392.011232] ? mntput_no_expire+0x11/0x185
> > [ 392.011987] scsi_cmd_ioctl+0x1d3/0x386
> > [ 392.012707] sd_ioctl+0xbb/0xde [sd_mod]
> > [ 392.013449] blkdev_ioctl+0x893/0x8bf
> > [ 392.014132] block_ioctl+0x3c/0x3f
> > [ 392.014781] vfs_ioctl+0x1e/0x2b
> > [ 392.015378] do_vfs_ioctl+0x531/0x559
> > [ 392.016059] ksys_ioctl+0x3e/0x5d
> > [ 392.016681] __x64_sys_ioctl+0x16/0x19
> > [ 392.017361] do_syscall_64+0x84/0x13f
> > [ 392.018060] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 392.018995] RIP: 0033:0x7fa691edf267
> > [ 392.019663] Code: b3 66 90 48 8b 05 19 3c 2c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e9 3b 2c 00 f7 d8 64 89 01 48
> > [ 392.023072] RSP: 002b:00007ffe463e8de8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > [ 392.024457] RAX: ffffffffffffffda RBX: 00007ffe463e8e20 RCX: 00007fa691edf267
> > [ 392.025767] RDX: 00007ffe463e8e20 RSI: 0000000000002285 RDI: 0000000000000004
> > [ 392.027068] RBP: 00007ffe463e9470 R08: 0000000000002006 R09: 00000000fffffe00
> > [ 392.028413] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe463e9920
> > [ 392.029722] R13: 00007ffe463e8e20 R14: 00007ffe463e8e2a R15: 00007ffe463e9920
> > [ 392.031031] Modules linked in: scsi_debug null_blk isofs iTCO_wdt iTCO_vendor_support i2c_i801 i2c_core lpc_ich mfd_core ip_tables sr_mod cdrom usb_storage sd_mod ahci libahci libata crc32c_intel virtio_scsi qemu_fw_cfg dm_mirror dm_region_hash dm_log dm_mod [last unloaded: null_blk]
> > [ 392.035573] Dumping ftrace buffer:
> > [ 392.036203] (ftrace buffer empty)
> > [ 392.036871] CR2: 0000000000000038
> > [ 392.037503] ---[ end trace fa20a1088b068790 ]---
>
> I think the below should fix it, we haven't necessarily setup an
> ioc if we're just doing as passthrough request.
>
>
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 13b8dc332541..f096d8989773 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
> void blk_mq_sched_assign_ioc(struct request *rq)
> {
> struct request_queue *q = rq->q;
> - struct io_context *ioc = current->io_context;
> + struct io_context *ioc;
> struct io_cq *icq;
>
> + /*
> + * May not have an IO context if it's a passthrough request
> + */
> + ioc = current->io_context;
> + if (!ioc)
> + return;
> +
> spin_lock_irq(&q->queue_lock);
> icq = ioc_lookup_icq(ioc, q);
> spin_unlock_irq(&q->queue_lock);
Looks this patch fixes the kernel panic in elevator stress switch test.
Tested-by: Ming Lei <ming.lei@redhat.com>
Thanks,
Ming
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-20 23:58 ` Damien Le Moal
@ 2018-11-21 1:24 ` Ming Lei
2018-11-21 1:31 ` Damien Le Moal
2018-11-21 2:10 ` Jens Axboe
1 sibling, 1 reply; 37+ messages in thread
From: Ming Lei @ 2018-11-21 1:24 UTC (permalink / raw)
To: Damien Le Moal
Cc: Jens Axboe, linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On Tue, Nov 20, 2018 at 11:58:09PM +0000, Damien Le Moal wrote:
> On 2018/11/21 2:31, Jens Axboe wrote:
> > I think the below should fix it, we haven't necessarily setup an
> > ioc if we're just doing as passthrough request.
> >
> >
> > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> > index 13b8dc332541..f096d8989773 100644
> > --- a/block/blk-mq-sched.c
> > +++ b/block/blk-mq-sched.c
> > @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
> > void blk_mq_sched_assign_ioc(struct request *rq)
> > {
> > struct request_queue *q = rq->q;
> > - struct io_context *ioc = current->io_context;
> > + struct io_context *ioc;
> > struct io_cq *icq;
> >
> > + /*
> > + * May not have an IO context if it's a passthrough request
> > + */
> > + ioc = current->io_context;
> > + if (!ioc)
> > + return;
> > +
> > spin_lock_irq(&q->queue_lock);
> > icq = ioc_lookup_icq(ioc, q);
> > spin_unlock_irq(&q->queue_lock);
>
> This seems reasonable to me, but I wonder why this problem was not triggering
> before. The previous code getting the ioc with the rq_ioc(bio) call was
> essentially the same and there was no "if (!ioc) return;" in
> blk_mq_sched_assign_ioc() before the patch.
> Any idea why this is popping up now ?
>
> Ming,
>
> Is this a new test your are running ? If this same problem triggers on stable
> kernels, Jens patch needs to go to stable too.
No, I run daily block related tests on block for-next, and this issue is
just triggered when your patches landed.
You may find the test script:
https://people.redhat.com/minlei/tests/tools/elv-switch
Thanks,
Ming
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-21 1:24 ` Ming Lei
@ 2018-11-21 1:31 ` Damien Le Moal
0 siblings, 0 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-21 1:31 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 2018/11/21 10:24, Ming Lei wrote:
> On Tue, Nov 20, 2018 at 11:58:09PM +0000, Damien Le Moal wrote:
>> On 2018/11/21 2:31, Jens Axboe wrote:
>>> I think the below should fix it, we haven't necessarily setup an
>>> ioc if we're just doing as passthrough request.
>>>
>>>
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index 13b8dc332541..f096d8989773 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>> void blk_mq_sched_assign_ioc(struct request *rq)
>>> {
>>> struct request_queue *q = rq->q;
>>> - struct io_context *ioc = current->io_context;
>>> + struct io_context *ioc;
>>> struct io_cq *icq;
>>>
>>> + /*
>>> + * May not have an IO context if it's a passthrough request
>>> + */
>>> + ioc = current->io_context;
>>> + if (!ioc)
>>> + return;
>>> +
>>> spin_lock_irq(&q->queue_lock);
>>> icq = ioc_lookup_icq(ioc, q);
>>> spin_unlock_irq(&q->queue_lock);
>>
>> This seems reasonable to me, but I wonder why this problem was not triggering
>> before. The previous code getting the ioc with the rq_ioc(bio) call was
>> essentially the same and there was no "if (!ioc) return;" in
>> blk_mq_sched_assign_ioc() before the patch.
>> Any idea why this is popping up now ?
>>
>> Ming,
>>
>> Is this a new test your are running ? If this same problem triggers on stable
>> kernels, Jens patch needs to go to stable too.
>
> No, I run daily block related tests on block for-next, and this issue is
> just triggered when your patches landed.
>
> You may find the test script:
>
> https://people.redhat.com/minlei/tests/tools/elv-switch
Thanks for the information. I will look into what else caused the behavior change.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-20 23:58 ` Damien Le Moal
2018-11-21 1:24 ` Ming Lei
@ 2018-11-21 2:10 ` Jens Axboe
2018-11-21 2:14 ` Damien Le Moal
2018-11-21 2:45 ` Damien Le Moal
1 sibling, 2 replies; 37+ messages in thread
From: Jens Axboe @ 2018-11-21 2:10 UTC (permalink / raw)
To: Damien Le Moal, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 11/20/18 4:58 PM, Damien Le Moal wrote:
> On 2018/11/21 2:31, Jens Axboe wrote:
>> I think the below should fix it, we haven't necessarily setup an
>> ioc if we're just doing as passthrough request.
>>
>>
>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>> index 13b8dc332541..f096d8989773 100644
>> --- a/block/blk-mq-sched.c
>> +++ b/block/blk-mq-sched.c
>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>> void blk_mq_sched_assign_ioc(struct request *rq)
>> {
>> struct request_queue *q = rq->q;
>> - struct io_context *ioc = current->io_context;
>> + struct io_context *ioc;
>> struct io_cq *icq;
>>
>> + /*
>> + * May not have an IO context if it's a passthrough request
>> + */
>> + ioc = current->io_context;
>> + if (!ioc)
>> + return;
>> +
>> spin_lock_irq(&q->queue_lock);
>> icq = ioc_lookup_icq(ioc, q);
>> spin_unlock_irq(&q->queue_lock);
>
> This seems reasonable to me, but I wonder why this problem was not triggering
> before. The previous code getting the ioc with the rq_ioc(bio) call was
> essentially the same and there was no "if (!ioc) return;" in
> blk_mq_sched_assign_ioc() before the patch.
> Any idea why this is popping up now ?
>
> Ming,
>
> Is this a new test your are running ? If this same problem triggers on stable
> kernels, Jens patch needs to go to stable too.
No, it's definitely introduced in your patches:
- if (e->type->icq_cache && rq_ioc(bio))
- blk_mq_sched_assign_ioc(rq, bio);
+ if (e->type->icq_cache)
+ blk_mq_sched_assign_ioc(rq);
Please run blktests on a series. Always. There's no excuse not to.
--
Jens Axboe
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-21 2:10 ` Jens Axboe
@ 2018-11-21 2:14 ` Damien Le Moal
2018-11-21 2:45 ` Damien Le Moal
1 sibling, 0 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-21 2:14 UTC (permalink / raw)
To: Jens Axboe, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 2018/11/21 11:11, Jens Axboe wrote:
> On 11/20/18 4:58 PM, Damien Le Moal wrote:
>> On 2018/11/21 2:31, Jens Axboe wrote:
>>> I think the below should fix it, we haven't necessarily setup an
>>> ioc if we're just doing as passthrough request.
>>>
>>>
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index 13b8dc332541..f096d8989773 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>> void blk_mq_sched_assign_ioc(struct request *rq)
>>> {
>>> struct request_queue *q = rq->q;
>>> - struct io_context *ioc = current->io_context;
>>> + struct io_context *ioc;
>>> struct io_cq *icq;
>>>
>>> + /*
>>> + * May not have an IO context if it's a passthrough request
>>> + */
>>> + ioc = current->io_context;
>>> + if (!ioc)
>>> + return;
>>> +
>>> spin_lock_irq(&q->queue_lock);
>>> icq = ioc_lookup_icq(ioc, q);
>>> spin_unlock_irq(&q->queue_lock);
>>
>> This seems reasonable to me, but I wonder why this problem was not triggering
>> before. The previous code getting the ioc with the rq_ioc(bio) call was
>> essentially the same and there was no "if (!ioc) return;" in
>> blk_mq_sched_assign_ioc() before the patch.
>> Any idea why this is popping up now ?
>>
>> Ming,
>>
>> Is this a new test your are running ? If this same problem triggers on stable
>> kernels, Jens patch needs to go to stable too.
>
> No, it's definitely introduced in your patches:
>
> - if (e->type->icq_cache && rq_ioc(bio))
> - blk_mq_sched_assign_ioc(rq, bio);
> + if (e->type->icq_cache)
> + blk_mq_sched_assign_ioc(rq);
Arg ! Yes, I missed this. My apologies.
> Please run blktests on a series. Always. There's no excuse not to.
I did run my usual tests exercising drives with various fio workloads. But I did
not run blktests itself. I will fix my workflow to include it.
Thanks.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-21 2:10 ` Jens Axboe
2018-11-21 2:14 ` Damien Le Moal
@ 2018-11-21 2:45 ` Damien Le Moal
2018-11-21 2:48 ` Jens Axboe
1 sibling, 1 reply; 37+ messages in thread
From: Damien Le Moal @ 2018-11-21 2:45 UTC (permalink / raw)
To: Jens Axboe, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 2018/11/21 11:11, Jens Axboe wrote:
> On 11/20/18 4:58 PM, Damien Le Moal wrote:
>> On 2018/11/21 2:31, Jens Axboe wrote:
>>> I think the below should fix it, we haven't necessarily setup an
>>> ioc if we're just doing as passthrough request.
>>>
>>>
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index 13b8dc332541..f096d8989773 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>> void blk_mq_sched_assign_ioc(struct request *rq)
>>> {
>>> struct request_queue *q = rq->q;
>>> - struct io_context *ioc = current->io_context;
>>> + struct io_context *ioc;
>>> struct io_cq *icq;
>>>
>>> + /*
>>> + * May not have an IO context if it's a passthrough request
>>> + */
>>> + ioc = current->io_context;
>>> + if (!ioc)
>>> + return;
>>> +
>>> spin_lock_irq(&q->queue_lock);
>>> icq = ioc_lookup_icq(ioc, q);
>>> spin_unlock_irq(&q->queue_lock);
>>
>> This seems reasonable to me, but I wonder why this problem was not triggering
>> before. The previous code getting the ioc with the rq_ioc(bio) call was
>> essentially the same and there was no "if (!ioc) return;" in
>> blk_mq_sched_assign_ioc() before the patch.
>> Any idea why this is popping up now ?
>>
>> Ming,
>>
>> Is this a new test your are running ? If this same problem triggers on stable
>> kernels, Jens patch needs to go to stable too.
>
> No, it's definitely introduced in your patches:
>
> - if (e->type->icq_cache && rq_ioc(bio))
> - blk_mq_sched_assign_ioc(rq, bio);
> + if (e->type->icq_cache)
> + blk_mq_sched_assign_ioc(rq);
>
> Please run blktests on a series. Always. There's no excuse not to.
By the way, should I send an updated patch 2 to include your fix ?
Or will you add an incremental fix ?
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-21 2:45 ` Damien Le Moal
@ 2018-11-21 2:48 ` Jens Axboe
2018-11-21 2:50 ` Damien Le Moal
0 siblings, 1 reply; 37+ messages in thread
From: Jens Axboe @ 2018-11-21 2:48 UTC (permalink / raw)
To: Damien Le Moal, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 11/20/18 7:45 PM, Damien Le Moal wrote:
> On 2018/11/21 11:11, Jens Axboe wrote:
>> On 11/20/18 4:58 PM, Damien Le Moal wrote:
>>> On 2018/11/21 2:31, Jens Axboe wrote:
>>>> I think the below should fix it, we haven't necessarily setup an
>>>> ioc if we're just doing as passthrough request.
>>>>
>>>>
>>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>>> index 13b8dc332541..f096d8989773 100644
>>>> --- a/block/blk-mq-sched.c
>>>> +++ b/block/blk-mq-sched.c
>>>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>>> void blk_mq_sched_assign_ioc(struct request *rq)
>>>> {
>>>> struct request_queue *q = rq->q;
>>>> - struct io_context *ioc = current->io_context;
>>>> + struct io_context *ioc;
>>>> struct io_cq *icq;
>>>>
>>>> + /*
>>>> + * May not have an IO context if it's a passthrough request
>>>> + */
>>>> + ioc = current->io_context;
>>>> + if (!ioc)
>>>> + return;
>>>> +
>>>> spin_lock_irq(&q->queue_lock);
>>>> icq = ioc_lookup_icq(ioc, q);
>>>> spin_unlock_irq(&q->queue_lock);
>>>
>>> This seems reasonable to me, but I wonder why this problem was not triggering
>>> before. The previous code getting the ioc with the rq_ioc(bio) call was
>>> essentially the same and there was no "if (!ioc) return;" in
>>> blk_mq_sched_assign_ioc() before the patch.
>>> Any idea why this is popping up now ?
>>>
>>> Ming,
>>>
>>> Is this a new test your are running ? If this same problem triggers on stable
>>> kernels, Jens patch needs to go to stable too.
>>
>> No, it's definitely introduced in your patches:
>>
>> - if (e->type->icq_cache && rq_ioc(bio))
>> - blk_mq_sched_assign_ioc(rq, bio);
>> + if (e->type->icq_cache)
>> + blk_mq_sched_assign_ioc(rq);
>>
>> Please run blktests on a series. Always. There's no excuse not to.
>
> By the way, should I send an updated patch 2 to include your fix ?
> Or will you add an incremental fix ?
I had to add the incremental fix, I already merged your patches
earlier. It's all pushed out now.
--
Jens Axboe
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/7] block: Remove bio->bi_ioc
2018-11-21 2:48 ` Jens Axboe
@ 2018-11-21 2:50 ` Damien Le Moal
0 siblings, 0 replies; 37+ messages in thread
From: Damien Le Moal @ 2018-11-21 2:50 UTC (permalink / raw)
To: Jens Axboe, Ming Lei
Cc: linux-block, Adam Manzanares, Alexander Viro, linux-fsdevel
On 2018/11/21 11:49, Jens Axboe wrote:
> On 11/20/18 7:45 PM, Damien Le Moal wrote:
>> On 2018/11/21 11:11, Jens Axboe wrote:
>>> On 11/20/18 4:58 PM, Damien Le Moal wrote:
>>>> On 2018/11/21 2:31, Jens Axboe wrote:
>>>>> I think the below should fix it, we haven't necessarily setup an
>>>>> ioc if we're just doing as passthrough request.
>>>>>
>>>>>
>>>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>>>> index 13b8dc332541..f096d8989773 100644
>>>>> --- a/block/blk-mq-sched.c
>>>>> +++ b/block/blk-mq-sched.c
>>>>> @@ -34,9 +34,16 @@ EXPORT_SYMBOL_GPL(blk_mq_sched_free_hctx_data);
>>>>> void blk_mq_sched_assign_ioc(struct request *rq)
>>>>> {
>>>>> struct request_queue *q = rq->q;
>>>>> - struct io_context *ioc = current->io_context;
>>>>> + struct io_context *ioc;
>>>>> struct io_cq *icq;
>>>>>
>>>>> + /*
>>>>> + * May not have an IO context if it's a passthrough request
>>>>> + */
>>>>> + ioc = current->io_context;
>>>>> + if (!ioc)
>>>>> + return;
>>>>> +
>>>>> spin_lock_irq(&q->queue_lock);
>>>>> icq = ioc_lookup_icq(ioc, q);
>>>>> spin_unlock_irq(&q->queue_lock);
>>>>
>>>> This seems reasonable to me, but I wonder why this problem was not triggering
>>>> before. The previous code getting the ioc with the rq_ioc(bio) call was
>>>> essentially the same and there was no "if (!ioc) return;" in
>>>> blk_mq_sched_assign_ioc() before the patch.
>>>> Any idea why this is popping up now ?
>>>>
>>>> Ming,
>>>>
>>>> Is this a new test your are running ? If this same problem triggers on stable
>>>> kernels, Jens patch needs to go to stable too.
>>>
>>> No, it's definitely introduced in your patches:
>>>
>>> - if (e->type->icq_cache && rq_ioc(bio))
>>> - blk_mq_sched_assign_ioc(rq, bio);
>>> + if (e->type->icq_cache)
>>> + blk_mq_sched_assign_ioc(rq);
>>>
>>> Please run blktests on a series. Always. There's no excuse not to.
>>
>> By the way, should I send an updated patch 2 to include your fix ?
>> Or will you add an incremental fix ?
>
> I had to add the incremental fix, I already merged your patches
> earlier. It's all pushed out now.
Thank you.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2018-11-21 2:50 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 3:51 [PATCH 0/7] Improve I/O priority handling Damien Le Moal
2018-11-19 3:51 ` [PATCH 1/7] aio: Comment use of IOCB_FLAG_IOPRIO aio flag Damien Le Moal
2018-11-19 8:12 ` Christoph Hellwig
2018-11-19 8:15 ` Johannes Thumshirn
2018-11-19 3:51 ` [PATCH 2/7] block: Remove bio->bi_ioc Damien Le Moal
2018-11-19 8:13 ` Christoph Hellwig
2018-11-19 8:16 ` Johannes Thumshirn
2018-11-19 19:07 ` Adam Manzanares
2018-11-20 17:21 ` Ming Lei
2018-11-20 17:31 ` Jens Axboe
2018-11-20 23:58 ` Damien Le Moal
2018-11-21 1:24 ` Ming Lei
2018-11-21 1:31 ` Damien Le Moal
2018-11-21 2:10 ` Jens Axboe
2018-11-21 2:14 ` Damien Le Moal
2018-11-21 2:45 ` Damien Le Moal
2018-11-21 2:48 ` Jens Axboe
2018-11-21 2:50 ` Damien Le Moal
2018-11-21 1:21 ` Ming Lei
2018-11-19 3:51 ` [PATCH 3/7] block: Fix get_task_ioprio() default return value Damien Le Moal
2018-11-19 8:16 ` Christoph Hellwig
2018-11-20 1:47 ` Damien Le Moal
2018-11-19 3:51 ` [PATCH 4/7] block: Introduce get_current_ioprio() Damien Le Moal
2018-11-19 8:17 ` Christoph Hellwig
2018-11-19 8:26 ` Johannes Thumshirn
2018-11-19 18:17 ` Adam Manzanares
2018-11-19 23:46 ` Damien Le Moal
2018-11-19 3:51 ` [PATCH 5/7] aio: Fix fallback I/O priority value Damien Le Moal
2018-11-19 8:18 ` Christoph Hellwig
2018-11-19 8:27 ` Johannes Thumshirn
2018-11-19 19:08 ` Adam Manzanares
2018-11-19 3:51 ` [PATCH 6/7] block: prevent merging of requests with different priorities Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 8:31 ` Johannes Thumshirn
2018-11-19 3:51 ` [PATCH 7/7] block: Initialize BIO I/O priority early Damien Le Moal
2018-11-19 8:19 ` Christoph Hellwig
2018-11-19 19:11 ` Adam Manzanares
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.