Hello, Bart.

On Wed, Feb 07, 2018 at 06:14:13PM +0000, Bart Van Assche wrote:
> When I wrote my comment I was not sure whether or not non-reentrancy is
> guaranteed for work queue items. However, according to what I found in the
> workqueue implementation I think that is guaranteed. So it shouldn't be
> possible that the timer activated by blk_add_timer() gets handled before
> aborted_gstate is reset. But since the timeout handler and completion

Yeah, we're basically single threaded in the timeout path.

> handlers can be executed by different CPUs, shouldn't a memory barrier be
> inserted between the blk_add_timer() call and resetting aborted_gsync to
> guarantee that a completion cannot occur before blk_add_timer() has reset

Ah, you're right.  u64_stat_sync doesn't imply barriers, so we want
something like the following.

diff --git a/block/blk-mq.c b/block/blk-mq.c
index df93102..d6edf3b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -593,7 +593,7 @@ static void blk_mq_rq_update_aborted_gstate(struct request 
*rq, u64 gstate)
-       rq->aborted_gstate = gstate;
+       smp_store_release(&rq->aborted_gstate, gstate);
@@ -605,7 +605,7 @@ static u64 blk_mq_rq_aborted_gstate(struct request *rq)
        do {
                start = u64_stats_fetch_begin(&rq->aborted_gstate_sync);
-               aborted_gstate = rq->aborted_gstate;
+               aborted_gstate = smp_load_acquire(&rq->aborted_gstate);
        } while (u64_stats_fetch_retry(&rq->aborted_gstate_sync, start));
        return aborted_gstate;
@@ -836,8 +836,8 @@ static void blk_mq_rq_timed_out(struct request *req, bool 
                 * ->aborted_gstate is set, this may lead to ignored
                 * completions and further spurious timeouts.
-               blk_mq_rq_update_aborted_gstate(req, 0);
+               blk_mq_rq_update_aborted_gstate(req, 0);
        case BLK_EH_NOT_HANDLED:

Reply via email to