blk-mq: reimplement timeout handling
Subsystems (2)
PATCH v3 1/1 blk-mq: reimplement timeout handling
Hello,
Changes from [v2]
- Possible extended looping around seqcount and u64_stat_sync fixed.
- Misplaced MQ_RQ_IDLE state setting fixed.
- RQF_MQ_TIMEOUT_EXPIRED added to prevent firing the same timeout multiple times.
- s/queue_rq_src/srcu/ patch added.
- Other misc changes.
Changes from [v1]
- BLK_EH_RESET_TIMER handling fixed.
- s/request->gstate_seqc/request->gstate_seq/
- READ_ONCE() added to blk_mq_rq_udpate_state().
- Removed left over blk_clear_rq_complete() invocation from blk_mq_rq_timed_out().
Currently, blk-mq timeout path synchronizes against the usual issue/completion path using a complex scheme involving atomic bitflags, REQ_ATOM_*, memory barriers and subtle memory coherence rules. Unfortunatley, it contains quite a few holes.
It's pretty easy to make blk_mq_check_expired() terminate a later instance of a request. If we induce 5 sec delay before time_after_eq() test in blk_mq_check_expired(), shorten the timeout to 2s, and issue back-to-back large IOs, blk-mq starts timing out requests spuriously pretty quickly. Nothing actually timed out. It just made the call on a recycle instance of a request and then terminated a later instance long after the original instance finished. The scenario isn't theoretical either.
This patchset replaces the broken synchronization mechanism with a RCU and generation number based one. Please read the patch description of the second path for more details.
This patchset contains the following six patches.
0001-blk-mq-protect-completion-path-with-RCU.patch 0002-blk-mq-replace-timeout-synchronization-with-a-RCU-an.patch 0003-blk-mq-use-blk_mq_rq_state-instead-of-testing-REQ_AT.patch 0004-blk-mq-make-blk_abort_request-trigger-timeout-path.patch 0005-blk-mq-remove-REQ_ATOM_COMPLETE-usages-from-blk-mq.patch 0006-blk-mq-remove-REQ_ATOM_STARTED.patch 0007-blk-mq-rename-blk_mq_hw_ctx-queue_rq_srcu-to-srcu.patch
and is available in the following git branch.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git blk-mq-timeout-v3
diffstat follows. Thanks.
block/blk-core.c | 2 block/blk-mq-debugfs.c | 4 block/blk-mq.c | 285 +++++++++++++++++++++++++++++-------------------- block/blk-mq.h | 49 ++++++++ block/blk-timeout.c | 11 + block/blk.h | 7 - include/linux/blk-mq.h | 3 include/linux/blkdev.h | 25 ++++ 8 files changed, 257 insertions(+), 129 deletions(-)
--
tejun
[v2] http://lkml.kernel.org/r/[email protected]
[v1] http://lkml.kernel.org/r/[email protected]
PATCH 1/7 blk-mq: protect completion path with RCU
Currently, blk-mq protects only the issue path with RCU. This patch puts the completion path under the same RCU protection. This will be used to synchronize issue/completion against timeout by later patches, which will also add the comments.
--- block/blk-mq.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
block/blk-mq.c +14 -2
@@ -568,11 +568,23 @@ static void __blk_mq_complete_request(struct request *rq)void blk_mq_complete_request (struct request *rq) {struct request_queue *q = rq->q; +struct blk_mq_hw_ctx *hctx =blk_mq_map_queue (q, rq->mq_ctx->cpu); +int srcu_idx;if (unlikely (blk_should_fake_timeout (q)))return ; -if (!blk_mark_rq_complete (rq)) -__blk_mq_complete_request (rq); + +if (!(hctx->flags &BLK_MQ_F_BLOCKING )) { +rcu_read_lock (); +if (!blk_mark_rq_complete (rq)) +__blk_mq_complete_request (rq); +rcu_read_unlock (); + }else { + srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); +if (!blk_mark_rq_complete (rq)) +__blk_mq_complete_request (rq); +srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); + } }EXPORT_SYMBOL (blk_mq_complete_request);
Signature · 1 line
2.9.5
Re: [PATCH 1/7] blk-mq: protect completion path with RCU
Why do you need the srcu protection? The completion path can never sleep.
If there is a good reason to keep it please add commment, and make the srcu variant a separate function only used by drivers that need it instead of adding the conditional.
Re: [PATCH 1/7] blk-mq: protect completion path with RCU
Hello, Christoph.
Quoted 6 lines · Christoph Hellwig
Why do you need the srcu protection? The completion path can never sleep.
If there is a good reason to keep it please add commment, and make the srcu variant a separate function only used by drivers that need it instead of adding the conditional.
It's either that or adding protection against both srcu and rcu in the counterpart. It's easier / cleaner to just use single rcu construct for a given queue. Will add a comment. Jens already has a patch which factors out the conditionals.
Thanks.
Signature · 1 line
tejun
PATCH 2/7 blk-mq: replace timeout synchronization with a RCU and generation based scheme
In reply to Tejun HeoCurrently, blk-mq timeout path synchronizes against the usual issue/completion path using a complex scheme involving atomic bitflags, REQ_ATOM_*, memory barriers and subtle memory coherence rules. Unfortunately, it contains quite a few holes.
There's a complex dancing around REQ_ATOM_STARTED and REQ_ATOM_COMPLETE between issue/completion and timeout paths; however, they don't have a synchronization point across request recycle instances and it isn't clear what the barriers add. blk_mq_check_expired() can easily read STARTED from N-2'th iteration, deadline from N-1'th, blk_mark_rq_complete() against Nth instance.
In fact, it's pretty easy to make blk_mq_check_expired() terminate a later instance of a request. If we induce 5 sec delay before time_after_eq() test in blk_mq_check_expired(), shorten the timeout to 2s, and issue back-to-back large IOs, blk-mq starts timing out requests spuriously pretty quickly. Nothing actually timed out. It just made the call on a recycle instance of a request and then terminated a later instance long after the original instance finished. The scenario isn't theoretical either.
This patch replaces the broken synchronization mechanism with a RCU and generation number based one.
1. Each request has a u64 generation + state value, which can be updated only by the request owner. Whenever a request becomes in-flight, the generation number gets bumped up too. This provides the basis for the timeout path to distinguish different recycle instances of the request.
Also, marking a request in-flight and setting its deadline are protected with a seqcount so that the timeout path can fetch both values coherently.
2. The timeout path fetches the generation, state and deadline. If the verdict is timeout, it records the generation into a dedicated request abortion field and does RCU wait.
3. The completion path is also protected by RCU (from the previous patch) and checks whether the current generation number and state match the abortion field. If so, it skips completion.
4. The timeout path, after RCU wait, scans requests again and terminates the ones whose generation and state still match the ones requested for abortion.
By now, the timeout path knows that either the generation number and state changed if it lost the race or the completion will yield to it and can safely timeout the request.
While it's more lines of code, it's conceptually simpler, doesn't depend on direct use of subtle memory ordering or coherence, and hopefully doesn't terminate the wrong instance.
While this change makes REQ_ATOM_COMPLETE synchronization unnecessary between issue/complete and timeout paths, REQ_ATOM_COMPLETE isn't removed yet as it's still used in other places. Future patches will move all state tracking to the new mechanism and remove all bitops in the hot paths.
v2: - Fixed BLK_EH_RESET_TIMER handling as pointed out by Jianchao.
- s/request->gstate_seqc/request->gstate_seq/ as suggested by Peter.
- READ_ONCE() added in blk_mq_rq_update_state() as suggested by Peter.
v3: - Fixed possible extended seqcount / u64_stats_sync read looping
spotted by Peter.
- MQ_RQ_IDLE was incorrectly being set in complete_request instead of free_request. Fixed.
--- block/blk-core.c | 2 + block/blk-mq.c | 220 +++++++++++++++++++++++++++++++++---------------- block/blk-mq.h | 46 +++++++++++ block/blk-timeout.c | 2 +- block/blk.h | 6 -- include/linux/blk-mq.h | 1 + include/linux/blkdev.h | 23 ++++++ 7 files changed, 220 insertions(+), 80 deletions(-)
block/blk-core.c +2 -0
@@ -126,6 +126,8 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->start_time = jiffies;set_start_time_ns (rq); rq->part =NULL ; +seqcount_init (&rq->gstate_seq); +u64_stats_init (&rq->aborted_gstate_sync); }EXPORT_SYMBOL (blk_rq_init);
block/blk-mq.c +147 -73
341 lines not on this page · raw message
block/blk-mq.h +46 -0
58 lines not on this page · raw message
block/blk-timeout.c +1 -1
@@ -208,7 +208,7 @@ void blk_add_timer(struct request *req)if (!req->timeout) req->timeout = q->rq_timeout; -WRITE_ONCE (req->deadline, jiffies + req->timeout); + req->deadline = jiffies + req->timeout;/* * Only the non-mq case needs to add the request to a protected list.
block/blk.h +0 -6
@@ -123,12 +123,6 @@ void blk_account_io_done(struct request *req);* Internal atomic flags for request handling */ enum rq_atomic_flags { -/* -* Keep these two bits first - not because we depend on the -* value of them, but we do depend on them being in the same -* byte of storage to ensure ordering on writes. Keeping them -* first will achieve that nicely. -*/ REQ_ATOM_COMPLETE =0 ,REQ_ATOM_STARTED ,
include/linux/blk-mq.h +1 -0
@@ -51,6 +51,7 @@ struct blk_mq_hw_ctx {unsigned int queue_num;atomic_t nr_active; +unsigned int nr_expired;struct hlist_node cpuhp_dead;struct kobject kobj;
include/linux/blkdev.h +23 -0
@@ -27,6 +27,8 @@#include <linux/percpu-refcount.h> #include <linux/scatterlist.h> #include <linux/blkzoned.h> +#include <linux/seqlock.h> +#include <linux/u64_stats_sync.h> struct module ;struct scsi_ioctl_command ;
@@ -228,6 +230,27 @@ struct request {unsigned short write_hint; +/* +* On blk-mq, the lower bits of ->gstate carry the MQ_RQ_* state +* value and the upper bits the generation number which is +* monotonically incremented and used to distinguish the reuse +* instances. +* +* ->gstate_seq allows updates to ->gstate and other fields +* (currently ->deadline) during request start to be read +* atomically from the timeout path, so that it can operate on a +* coherent set of information. +*/ +seqcount_t gstate_seq; +u64 gstate; + +/* +* ->aborted_gstate is used by the timeout to claim a specific +* recycle instance of this request. See blk_mq_timeout_work(). +*/ +struct u64_stats_sync aborted_gstate_sync; +u64 aborted_gstate; +unsigned long deadline;struct list_head timeout_list;
Signature · 1 line
2.9.5
PATCH 3/7 blk-mq: use blk_mq_rq_state() instead of testing REQ_ATOM_COMPLETE
In reply to Tejun Heoblk_mq_check_inflight() and blk_mq_poll_hybrid_sleep() test REQ_ATOM_COMPLETE to determine the request state. Both uses are speculative and we can test REQ_ATOM_STARTED and blk_mq_rq_state() for equivalent results. Replace the tests. This will allow removing REQ_ATOM_COMPLETE usages from blk-mq.
--- block/blk-mq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
block/blk-mq.c +3 -3
@@ -95,8 +95,7 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, {struct mq_inflight *mi = priv; -if (test_bit (REQ_ATOM_STARTED , &rq->atomic_flags) && - !test_bit (REQ_ATOM_COMPLETE , &rq->atomic_flags)) { +if (blk_mq_rq_state (rq) ==MQ_RQ_IN_FLIGHT ) {/* * index[0] counts the specific partition that was asked * for. index[1] counts the ones that are active on the
@@ -2974,7 +2973,8 @@ static bool blk_mq_poll_hybrid_sleep(struct request_queue *q,hrtimer_init_sleeper (&hs, current); do { - if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags)) + if (test_bit(REQ_ATOM_STARTED, &rq->atomic_flags) && + blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT) break; set_current_state(TASK_UNINTERRUPTIBLE); hrtimer_start_expires(&hs.timer, mode);
Signature · 1 line
2.9.5
PATCH 4/7 blk-mq: make blk_abort_request() trigger timeout path
In reply to Tejun HeoWith issue/complete and timeout paths now using the generation number and state based synchronization, blk_abort_request() is the only one which depends on REQ_ATOM_COMPLETE for arbitrating completion.
There's no reason for blk_abort_request() to be a completely separate path. This patch makes blk_abort_request() piggyback on the timeout path instead of trying to terminate the request directly.
This removes the last dependency on REQ_ATOM_COMPLETE in blk-mq.
Note that this makes blk_abort_request() asynchronous - it initiates abortion but the actual termination will happen after a short while, even when the caller owns the request. AFAICS, SCSI and ATA should be fine with that and I think mtip32xx and dasd should be safe but not completely sure. It'd be great if people who know the drivers take a look.
--- block/blk-mq.c | 2 +- block/blk-mq.h | 2 -- block/blk-timeout.c | 8 ++++---- 3 files changed, 5 insertions(+), 7 deletions(-)
block/blk-mq.c +1 -1
@@ -800,7 +800,7 @@ struct blk_mq_timeout_data {unsigned int nr_expired; }; -void blk_mq_rq_timed_out (struct request *req,bool reserved) +static void blk_mq_rq_timed_out (struct request *req,bool reserved) {const struct blk_mq_ops *ops = req->q->mq_ops;enum blk_eh_timer_return ret =BLK_EH_RESET_TIMER ;
block/blk-mq.h +0 -2
@@ -94,8 +94,6 @@ extern int blk_mq_sysfs_register(struct request_queue *q);extern void blk_mq_sysfs_unregister (struct request_queue *q);extern void blk_mq_hctx_kobj_init (struct blk_mq_hw_ctx *hctx); -extern void blk_mq_rq_timed_out (struct request *req,bool reserved); -void blk_mq_release (struct request_queue *q);/**
block/blk-timeout.c +4 -4
@@ -156,12 +156,12 @@ void blk_timeout_work(struct work_struct *work)*/ void blk_abort_request (struct request *req) { -if (blk_mark_rq_complete (req)) -return ; -if (req->q->mq_ops) { -blk_mq_rq_timed_out (req, false); + req->deadline = jiffies; +mod_timer (&req->q->timeout,0 ); }else { +if (blk_mark_rq_complete (req)) +return ;blk_delete_timer (req);blk_rq_timed_out (req); }
Signature · 1 line
2.9.5
Re: [PATCH 4/7] blk-mq: make blk_abort_request() trigger timeout path
Quoted 6 lines · Tejun Heo
Note that this makes
blk_abort_request()asynchronous - it initiates abortion but the actual termination will happen after a short while, even when the caller owns the request. AFAICS, SCSI and ATA should be fine with that and I think mtip32xx and dasd should be safe but not completely sure. It'd be great if people who know the drivers take a look.
For that you'll need to CC linux-ide and linux-scsi, and for the SAS drivers some of the usual suspects that touch the SAS code.
PATCH 5/7 blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
In reply to Tejun HeoAfter the recent updates to use generation number and state based synchronization, blk-mq no longer depends on REQ_ATOM_COMPLETE except to avoid firing the same timeout multiple times.
Remove all REQ_ATOM_COMPLETE usages and use a new rq_flags flag RQF_MQ_TIMEOUT_EXPIRED to avoid firing the same timeout multiple times. This removes atomic bitops from hot paths too.
v2: Removed blk_clear_rq_complete() from blk_mq_rq_timed_out().
v3: Added RQF_MQ_TIMEOUT_EXPIRED flag.
--- block/blk-mq.c | 18 ++++++++---------- block/blk-timeout.c | 1 + include/linux/blkdev.h | 2 ++ 3 files changed, 11 insertions(+), 10 deletions(-)
block/blk-mq.c +8 -10
@@ -607,14 +607,12 @@ void blk_mq_complete_request(struct request *rq)*/ if (!(hctx->flags &BLK_MQ_F_BLOCKING )) {rcu_read_lock (); -if (blk_mq_rq_aborted_gstate (rq) != rq->gstate && - !blk_mark_rq_complete (rq)) +if (blk_mq_rq_aborted_gstate (rq) != rq->gstate)__blk_mq_complete_request (rq);rcu_read_unlock (); }else { srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); -if (blk_mq_rq_aborted_gstate (rq) != rq->gstate && - !blk_mark_rq_complete (rq)) +if (blk_mq_rq_aborted_gstate (rq) != rq->gstate)__blk_mq_complete_request (rq);srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); }
@@ -665,8 +663,6 @@ void blk_mq_start_request(struct request *rq)preempt_enable ();set_bit (REQ_ATOM_STARTED , &rq->atomic_flags); -if (test_bit (REQ_ATOM_COMPLETE , &rq->atomic_flags)) -clear_bit (REQ_ATOM_COMPLETE , &rq->atomic_flags);if (q->dma_drain_size &&blk_rq_bytes (rq)) {/*
@@ -817,6 +813,8 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)if (!test_bit (REQ_ATOM_STARTED , &req->atomic_flags))return ; + req->rq_flags |=RQF_MQ_TIMEOUT_EXPIRED ; +if (ops->timeout) ret = ops->timeout (req, reserved);
@@ -832,7 +830,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)*/ blk_mq_rq_update_aborted_gstate (req,0 );blk_add_timer (req); -blk_clear_rq_complete (req);break ;case BLK_EH_NOT_HANDLED :break ;
@@ -851,7 +848,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,might_sleep (); -if (!test_bit (REQ_ATOM_STARTED , &rq->atomic_flags)) +if ((rq->rq_flags &RQF_MQ_TIMEOUT_EXPIRED ) || + !test_bit (REQ_ATOM_STARTED , &rq->atomic_flags))return ;/* read coherent snapshots of @rq->state_gen and @rq->deadline */
@@ -886,8 +884,8 @@ static void blk_mq_terminate_expired(struct blk_mq_hw_ctx *hctx,* now guaranteed to see @rq->aborted_gstate and yield. If * @rq->aborted_gstate still matches @rq->gstate, @rq is ours. */ -if (READ_ONCE (rq->gstate) == rq->aborted_gstate && - !blk_mark_rq_complete (rq)) +if (!(rq->rq_flags &RQF_MQ_TIMEOUT_EXPIRED ) && +READ_ONCE (rq->gstate) == rq->aborted_gstate)blk_mq_rq_timed_out (rq, reserved); }
block/blk-timeout.c +1 -0
@@ -209,6 +209,7 @@ void blk_add_timer(struct request *req) req->timeout = q->rq_timeout; req->deadline = jiffies + req->timeout; + req->rq_flags &= ~RQF_MQ_TIMEOUT_EXPIRED ;/* * Only the non-mq case needs to add the request to a protected list.
include/linux/blkdev.h +2 -0
@@ -123,6 +123,8 @@ typedef __u32 __bitwise req_flags_t;/* Look at ->special_vec for the actual data payload instead of the bio chain. */ #define RQF_SPECIAL_PAYLOAD ((__force req_flags_t)(1 << 18)) +/* timeout is expired */ +#define RQF_MQ_TIMEOUT_EXPIRED ((__force req_flags_t)(1 << 19))/* flags that prevent us from merging requests: */ #define RQF_NOMERGE_FLAGS \
Signature · 1 line
2.9.5
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hi tejun
Quoted 31 lines · Tejun Heo
After the recent updates to use generation number and state based synchronization, blk-mq no longer depends on
REQ_ATOM_COMPLETEexcept to avoid firing the same timeout multiple times.Remove all
REQ_ATOM_COMPLETEusages and use a newrq_flagsflagRQF_MQ_TIMEOUT_EXPIREDto avoid firing the same timeout multiple times. This removes atomic bitops from hot paths too.v2: Removed
blk_clear_rq_complete()fromblk_mq_rq_timed_out().v3: Added
RQF_MQ_TIMEOUT_EXPIREDflag.Signed-off-by Tejun Heo <[email protected]>Cc "jianchao.wang" <[email protected]>--- block/blk-mq.c | 18 ++++++++---------- block/blk-timeout.c | 1 + include/linux/blkdev.h | 2 ++ 3 files changed, 11 insertions(+), 10 deletions(-)block/blk-mq.c +1
-2@@ -607,14 +607,12 @@ void blk_mq_complete_request(struct request *rq)*/ if (!(hctx->flags &BLK_MQ_F_BLOCKING )) {rcu_read_lock (); -if (blk_mq_rq_aborted_gstate (rq) != rq->gstate && - !blk_mark_rq_complete (rq)) +if (blk_mq_rq_aborted_gstate (rq) != rq->gstate)
block/blk-mq.c void blk_mq_complete_request(struct request *rq) · line 618
Quoted 10 lines
block/blk-mq.c +1
-2__blk_mq_complete_request (rq);rcu_read_unlock (); }else { srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); -if (blk_mq_rq_aborted_gstate (rq) != rq->gstate && - !blk_mark_rq_complete (rq)) +if (blk_mq_rq_aborted_gstate (rq) != rq->gstate)__blk_mq_complete_request (rq);srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); }
It's worrying that even though the blk_mark_rq_complete() here is intended to synchronize with timeout path, but it indeed give the blk_mq_complete_request() the capability to exclude with itself. Maybe this capability should be reserved.
Thanks
Jianchao
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hello,
Quoted 3 lines · jianchao.wang
It's worrying that even though the
blk_mark_rq_complete()here is intended to synchronize with timeout path, but it indeed give theblk_mq_complete_request()the capability to exclude with itself. Maybe this capability should be reserved.
Can you explain further how that'd help? The problem there is that if you have two competing completions, where one isn't a timeout, there's nothing synchronizing the reuse of the request. IOW, the losing one can easily complete the next recycle instance. The atomic bitops might feel like more protection but it's just feels.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Sorry for my non-detailed description.
Quoted 5 lines · Tejun Heo
Hello,
Quoted 2 lines · jianchao.wang
It's worrying that even though the
blk_mark_rq_complete()here is intended to synchronize with timeout path, but it indeed give theblk_mq_complete_request()the capability to exclude with
There could be scenario where the driver itself stop a request itself with blk_mq_complete_request() or some other interface that will invoke it, races with the normal completion path where a same request comes. For example:
a reset could be triggered through sysfs on nvme-rdma
Then the driver will cancel all the reqs, including in-flight ones. nvme_rdma_reset_ctrl_work()
nvme_rdma_shutdown_ctrl()
>>>>
if (ctrl->ctrl.queue_count > 1) {
nvme_stop_queues(&ctrl->ctrl); //quiesce the queue
blk_mq_tagset_busy_iter(&ctrl->tag_set,
nvme_cancel_request, &ctrl->ctrl); //invoke blk_mq_complete_request()
nvme_rdma_destroy_io_queues(ctrl, shutdown);
}
>>>>These operations could race with the normal completion path of in-flight ones. It should drain all the in-flight ones first here. But there maybe some other places similar with this.
Quoted 7 lines
Quoted 1 line
itself. Maybe this capability should be reserved.
Can you explain further how that'd help? The problem there is that if you have two competing completions, where one isn't a timeout, there's nothing synchronizing the reuse of the request. IOW, the losing on can easily complete the next recycle instance. The atomic bitops might feel like more protection but it's just feels.
In above case, the request may simultaneously enter requeue and end path.
Thanks
Jianchao
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hello, Jianchao.
Quoted 9 lines · jianchao.wang
Quoted 4 lines · jianchao.wang
Quoted 3 lines
It's worrying that even though the
blk_mark_rq_complete()here is intended to synchronize with timeout path, but it indeed give theblk_mq_complete_request()the capability to exclude withThere could be scenario where the driver itself stop a request itself with
blk_mq_complete_request()or some other interface that will invoke it, races with the normal completion path where a same request comes.
But what'd prevent the completion reinitializing the request and then the actual completion path coming in and completing the request again?
Quoted 14 lines
For example:
a reset could be triggered through sysfs on nvme-rdma Then the driver will cancel all the reqs, including in-flight ones.nvme_rdma_reset_ctrl_work()nvme_rdma_shutdown_ctrl() >>>> if (ctrl->ctrl.queue_count > 1) { nvme_stop_queues(&ctrl->ctrl); //quiesce the queue blk_mq_tagset_busy_iter(&ctrl->tag_set, nvme_cancel_request, &ctrl->ctrl); //invoke blk_mq_complete_request() nvme_rdma_destroy_io_queues(ctrl, shutdown); } >>>>
Quoted 3 lines
These operations could race with the normal completion path of in-flight ones. It should drain all the in-flight ones first here. But there maybe some other places similar with this.
If there are any such places, they should be using an interface which is propelry synchronized like blk_abort_request(), which btw is what libata already does. Otherwise, it's racy with or without these patches.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hi tejun
Many thanks for you kindly response.
Quoted 8 lines · Tejun Heo
Hello, Jianchao.
Quoted 5 lines · jianchao.wang
Quoted 4 lines · jianchao.wang
Quoted 3 lines
It's worrying that even though the
blk_mark_rq_complete()here is intended to synchronize with timeout path, but it indeed give theblk_mq_complete_request()the capability to exclude with
Quoted 8 lines
Quoted 4 lines
There could be scenario where the driver itself stop a request itself with
blk_mq_complete_request()or some other interface that will invoke it, races with the normal completion path where a same request comes.But what'd prevent the completion reinitializing the request and then the actual completion path coming in and completing the request again?
blk_mark_rq_complete() will gate and ensure there will be only one __blk_mq_complete_request() to be invoked.
Quoted 14 lines
Quoted 14 lines
For example:
a reset could be triggered through sysfs on nvme-rdma Then the driver will cancel all the reqs, including in-flight ones.nvme_rdma_reset_ctrl_work()nvme_rdma_shutdown_ctrl() >>>> if (ctrl->ctrl.queue_count > 1) { nvme_stop_queues(&ctrl->ctrl); //quiesce the queue blk_mq_tagset_busy_iter(&ctrl->tag_set, nvme_cancel_request, &ctrl->ctrl); //invoke blk_mq_complete_request() nvme_rdma_destroy_io_queues(ctrl, shutdown); } >>>>
Quoted 8 lines
Quoted 3 lines
These operations could race with the normal completion path of in-flight ones. It should drain all the in-flight ones first here. But there maybe some other places similar with this.
If there are any such places, they should be using an interface which is propelry synchronized like
blk_abort_request(), which btw is what libata already does. Otherwise, it's racy with or without these patches.
Yes, it is that.
Thanks for you kindly response again.
Jianchao
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hello,
Quoted 5 lines · jianchao.wang
Quoted 2 lines
But what'd prevent the completion reinitializing the request and then the actual completion path coming in and completing the request again?
blk_mark_rq_complete()will gate and ensure there will be only one__blk_mq_complete_request()to be invoked.
Yeah, but then the complete flag will be cleared once completion is done and the request is reinitialized.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 5/7] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq
Hi tejun
Many thanks for your kindly response.
Quoted 11 lines · Tejun Heo
Hello,
Quoted 5 lines · jianchao.wang
Quoted 2 lines
But what'd prevent the completion reinitializing the request and then the actual completion path coming in and completing the request again?
blk_mark_rq_complete()will gate and ensure there will be only one__blk_mq_complete_request()to be invoked.Yeah, but then the complete flag will be cleared once completion is done and the request is reinitialized.
Yes, it is. What I mean is not to reserve blk_mark_rq_complete and REQ_ATOM_COMPLETE usages in blk-mq but the side-effect after blk_mq_complete_request cannot exclude with itself. As you said, the scene is racy and should be modified. :)
Thanks
Jianchao
PATCH 6/7 blk-mq: remove REQ_ATOM_STARTED
In reply to Tejun HeoAfter the recent updates to use generation number and state based synchronization, we can easily replace REQ_ATOM_STARTED usages by adding an extra state to distinguish completed but not yet freed state.
Add MQ_RQ_COMPLETE and replace REQ_ATOM_STARTED usages with blk_mq_rq_state() tests. REQ_ATOM_STARTED no longer has any users left and is removed.
--- block/blk-mq-debugfs.c | 4 +--- block/blk-mq.c | 37 ++++++++----------------------------- block/blk-mq.h | 1 + block/blk.h | 1 - 4 files changed, 10 insertions(+), 33 deletions(-)
block/blk-mq-debugfs.c +1 -3
@@ -271,7 +271,6 @@ static const char *const cmd_flag_name[] = {#define RQF_NAME (name) [ilog2((__force u32)RQF_##name)] = #namestatic const char *const rqf_name[] = {RQF_NAME (SORTED ), -RQF_NAME (STARTED ),RQF_NAME (QUEUED ),RQF_NAME (SOFTBARRIER ),RQF_NAME (FLUSH_SEQ ),
@@ -295,7 +294,6 @@ static const char *const rqf_name[] = {#define RQAF_NAME (name) [REQ_ATOM_##name] = #namestatic const char *const rqaf_name[] = {RQAF_NAME (COMPLETE ), -RQAF_NAME (STARTED ),RQAF_NAME (POLL_SLEPT ), };#undef RQAF_NAME
@@ -409,7 +407,7 @@ static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)const struct show_busy_params *params = data;if (blk_mq_map_queue (rq->q, rq->mq_ctx->cpu) == params->hctx && -test_bit (REQ_ATOM_STARTED , &rq->atomic_flags)) +blk_mq_rq_state (rq) !=MQ_RQ_IDLE )__blk_mq_debugfs_rq_show (params->m,list_entry_rq (&rq->queuelist)); }
block/blk-mq.c +8 -29
@@ -483,7 +483,6 @@ void blk_mq_free_request(struct request *rq)blk_put_rl (blk_rq_rl (rq));blk_mq_rq_update_state (rq,MQ_RQ_IDLE ); -clear_bit (REQ_ATOM_STARTED , &rq->atomic_flags);clear_bit (REQ_ATOM_POLL_SLEPT , &rq->atomic_flags);if (rq->tag !=-1 )blk_mq_put_tag (hctx, hctx->tags, ctx, rq->tag);
@@ -531,6 +530,7 @@ static void __blk_mq_complete_request(struct request *rq)int cpu;WARN_ON_ONCE (blk_mq_rq_state (rq) !=MQ_RQ_IN_FLIGHT ); +blk_mq_rq_update_state (rq,MQ_RQ_COMPLETE );if (rq->internal_tag !=-1 )blk_mq_sched_completed_request (rq);
@@ -621,7 +621,7 @@ EXPORT_SYMBOL(blk_mq_complete_request);int blk_mq_request_started (struct request *rq) { -return test_bit (REQ_ATOM_STARTED , &rq->atomic_flags); +return blk_mq_rq_state (rq) !=MQ_RQ_IDLE ; }EXPORT_SYMBOL_GPL (blk_mq_request_started);
@@ -640,7 +640,6 @@ void blk_mq_start_request(struct request *rq) }WARN_ON_ONCE (blk_mq_rq_state (rq) !=MQ_RQ_IDLE ); -WARN_ON_ONCE (test_bit (REQ_ATOM_STARTED , &rq->atomic_flags));/* * Mark @rq in-flight which also advances the generation number,
@@ -662,8 +661,6 @@ void blk_mq_start_request(struct request *rq)write_seqcount_end (&rq->gstate_seq);preempt_enable (); -set_bit (REQ_ATOM_STARTED , &rq->atomic_flags); -if (q->dma_drain_size &&blk_rq_bytes (rq)) {/* * Make sure space for the drain appears. We know we can do
@@ -676,13 +673,9 @@ void blk_mq_start_request(struct request *rq)EXPORT_SYMBOL (blk_mq_start_request);/* -* When we reach here because queue is busy, REQ_ATOM_COMPLETE -* flag isn't set yet, so there may be race with timeout handler, -* but given rq->deadline is just set in .queue_rq() under -* this situation, the race won't be possible in reality because -* rq->timeout should be set as big enough to cover the window -* between blk_mq_start_request() called from .queue_rq() and -* clearing REQ_ATOM_STARTED here. +* When we reach here because queue is busy, it's safe to change the state +* to IDLE without checking @rq->aborted_gstate because we should still be +* holding the RCU read lock and thus protected against timeout. */ static void __blk_mq_requeue_request (struct request *rq) {
@@ -694,7 +687,7 @@ static void __blk_mq_requeue_request(struct request *rq)wbt_requeue (q->rq_wb, &rq->issue_stat);blk_mq_sched_requeue_request (rq); -if (test_and_clear_bit (REQ_ATOM_STARTED , &rq->atomic_flags)) { +if (blk_mq_rq_state (rq) !=MQ_RQ_IDLE ) {blk_mq_rq_update_state (rq,MQ_RQ_IDLE );if (q->dma_drain_size &&blk_rq_bytes (rq)) rq->nr_phys_segments--;
@@ -801,18 +794,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool reserved)const struct blk_mq_ops *ops = req->q->mq_ops;enum blk_eh_timer_return ret =BLK_EH_RESET_TIMER ; -/* -* We know that complete is set at this point. If STARTED isn't set -* anymore, then the request isn't active and the "timeout" should -* just be ignored. This can happen due to the bitflag ordering. -* Timeout first checks if STARTED is set, and if it is, assumes -* the request is active. But if we race with completion, then -* both flags will get cleared. So check here again, and ignore -* a timeout event with a request that isn't active. -*/ -if (!test_bit (REQ_ATOM_STARTED , &req->atomic_flags)) -return ; - req->rq_flags |=RQF_MQ_TIMEOUT_EXPIRED ;if (ops->timeout)
@@ -848,8 +829,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,might_sleep (); -if ((rq->rq_flags &RQF_MQ_TIMEOUT_EXPIRED ) || - !test_bit (REQ_ATOM_STARTED , &rq->atomic_flags)) +if (rq->rq_flags &RQF_MQ_TIMEOUT_EXPIRED )return ;/* read coherent snapshots of @rq->state_gen and @rq->deadline */
@@ -2971,8 +2951,7 @@ static bool blk_mq_poll_hybrid_sleep(struct request_queue *q,hrtimer_init_sleeper (&hs, current); do { - if (test_bit(REQ_ATOM_STARTED, &rq->atomic_flags) && - blk_mq_rq_state(rq) != MQ_RQ_IN_FLIGHT) + if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE) break; set_current_state(TASK_UNINTERRUPTIBLE); hrtimer_start_expires(&hs.timer, mode);
block/blk-mq.h +1 -0
@@ -34,6 +34,7 @@ struct blk_mq_ctx {enum mq_rq_state {MQ_RQ_IDLE =0 ,MQ_RQ_IN_FLIGHT =1 , +MQ_RQ_COMPLETE =2 ,MQ_RQ_STATE_BITS =2 ,MQ_RQ_STATE_MASK = (1 <<MQ_RQ_STATE_BITS ) -1 ,
block/blk.h +0 -1
@@ -124,7 +124,6 @@ void blk_account_io_done(struct request *req);*/ enum rq_atomic_flags {REQ_ATOM_COMPLETE =0 , -REQ_ATOM_STARTED ,REQ_ATOM_POLL_SLEPT , };
Signature · 1 line
2.9.5
PATCH 7/7 blk-mq: rename blk_mq_hw_ctx->queue_rq_srcu to ->srcu
In reply to Tejun HeoThe RCU protection has been expanded to cover both queueing and completion paths making ->queue_rq_srcu a misnomer. Rename it to ->srcu as suggested by Bart.
--- block/blk-mq.c | 22 +++++++++++----------- include/linux/blk-mq.h | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-)
block/blk-mq.c +11 -11
@@ -219,7 +219,7 @@ void blk_mq_quiesce_queue(struct request_queue *q)queue_for_each_hw_ctx (q, hctx, i) {if (hctx->flags &BLK_MQ_F_BLOCKING ) -synchronize_srcu (hctx->queue_rq_srcu); +synchronize_srcu (hctx->srcu);else rcu = true; }
@@ -611,10 +611,10 @@ void blk_mq_complete_request(struct request *rq)__blk_mq_complete_request (rq);rcu_read_unlock (); }else { - srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); + srcu_idx =srcu_read_lock (hctx->srcu);if (blk_mq_rq_aborted_gstate (rq) != rq->gstate)__blk_mq_complete_request (rq); -srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); +srcu_read_unlock (hctx->srcu, srcu_idx); } }EXPORT_SYMBOL (blk_mq_complete_request);
@@ -916,7 +916,7 @@ static void blk_mq_timeout_work(struct work_struct *work)if (!(hctx->flags &BLK_MQ_F_BLOCKING )) has_rcu = true;else -synchronize_srcu (hctx->queue_rq_srcu); +synchronize_srcu (hctx->srcu); hctx->nr_expired =0 ; }
@@ -1279,9 +1279,9 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx) }else {might_sleep (); - srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); + srcu_idx =srcu_read_lock (hctx->srcu);blk_mq_sched_dispatch_requests (hctx); -srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); +srcu_read_unlock (hctx->srcu, srcu_idx); } }
@@ -1718,9 +1718,9 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,might_sleep (); - srcu_idx =srcu_read_lock (hctx->queue_rq_srcu); + srcu_idx =srcu_read_lock (hctx->srcu);__blk_mq_try_issue_directly (hctx, rq, cookie, true); -srcu_read_unlock (hctx->queue_rq_srcu, srcu_idx); +srcu_read_unlock (hctx->srcu, srcu_idx); } }
@@ -2074,7 +2074,7 @@ static void blk_mq_exit_hctx(struct request_queue *q, set->ops->exit_hctx (hctx, hctx_idx);if (hctx->flags &BLK_MQ_F_BLOCKING ) -cleanup_srcu_struct (hctx->queue_rq_srcu); +cleanup_srcu_struct (hctx->srcu);blk_mq_remove_cpuhp (hctx);blk_free_flush_queue (hctx->fq);
@@ -2147,7 +2147,7 @@ static int blk_mq_init_hctx(struct request_queue *q, goto free_fq;if (hctx->flags &BLK_MQ_F_BLOCKING ) -init_srcu_struct (hctx->queue_rq_srcu); +init_srcu_struct (hctx->srcu);blk_mq_debugfs_register_hctx (q, hctx);
@@ -2436,7 +2436,7 @@ static int blk_mq_hw_ctx_size(struct blk_mq_tag_set *tag_set) {int hw_ctx_size =sizeof (struct blk_mq_hw_ctx ); -BUILD_BUG_ON (ALIGN (offsetof(struct blk_mq_hw_ctx , queue_rq_srcu), +BUILD_BUG_ON (ALIGN (offsetof(struct blk_mq_hw_ctx , srcu), __alignof__(struct blk_mq_hw_ctx )) !=sizeof (struct blk_mq_hw_ctx ));
include/linux/blk-mq.h +1 -1
@@ -66,7 +66,7 @@ struct blk_mq_hw_ctx {#endif /* Must be the last member - see also blk_mq_hw_ctx_size(). */ -struct srcu_struct queue_rq_srcu[0 ]; +struct srcu_struct srcu[0 ]; };struct blk_mq_tag_set {
Signature · 1 line
2.9.5
This seems to miss the linux-block list once again. Please include it in the next resend.
Quoted 2 lines · Christoph Hellwig
This seems to miss the linux-block list once again. Please include it in the next resend.
Sorry about that. Copy/pasted from the older thread without thinking.
Thanks.
Signature · 1 line
tejun