On Wed, May 27, 2020 at 04:09:19PM -0700, Bart Van Assche wrote:
> On 2020-05-27 11:06, Christoph Hellwig wrote:
> > --- a/block/blk-mq-tag.c
> > +++ b/block/blk-mq-tag.c
> > @@ -180,6 +180,14 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
> > sbitmap_finish_wait(bt, ws, &wait);
> >
> > found_tag:
> > + /*
> > + * Give up this allocation if the hctx is inactive. The caller will
> > + * retry on an active hctx.
> > + */
> > + if (unlikely(test_bit(BLK_MQ_S_INACTIVE, &data->hctx->state))) {
> > + blk_mq_put_tag(tags, data->ctx, tag + tag_offset);
> > + return -1;
> > + }
> > return tag + tag_offset;
> > }
>
> The code that has been added in blk_mq_hctx_notify_offline() will only
> work correctly if blk_mq_get_tag() tests BLK_MQ_S_INACTIVE after the
> store instructions involved in the tag allocation happened. Does this
> mean that a memory barrier should be added in the above function before
> the test_bit() call?
Please see comment in blk_mq_hctx_notify_offline():
+ /*
+ * Prevent new request from being allocated on the current hctx.
+ *
+ * The smp_mb__after_atomic() Pairs with the implied barrier in
+ * test_and_set_bit_lock in sbitmap_get(). Ensures the inactive flag is
+ * seen once we return from the tag allocator.
+ */
+ set_bit(BLK_MQ_S_INACTIVE, &hctx->state);
Thanks,
Ming
