Re: [PATCH 3/8] blk-mq: replace timeout synchronization with a RCU and generation based scheme
On Mon, Jan 08, 2018 at 11:29:11PM +, Bart Van Assche wrote: > Does "gstate" perhaps stand for "generation number and state"? If so, please > mention this in one of the above comments. Yeah, will do. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] blk-mq: replace timeout synchronization with a RCU and generation based scheme
On Mon, Jan 08, 2018 at 09:06:55PM +, Bart Van Assche wrote:
> On Mon, 2018-01-08 at 11:15 -0800, Tejun Heo wrote:
> > +static void blk_mq_rq_update_aborted_gstate(struct request *rq, u64 gstate)
> > +{
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + u64_stats_update_begin(&rq->aborted_gstate_sync);
> > + rq->aborted_gstate = gstate;
> > + u64_stats_update_end(&rq->aborted_gstate_sync);
> > + local_irq_restore(flags);
> > +}
>
> Please add a comment that explains the purpose of local_irq_save() and
> local_irq_restore(). Please also explain why you chose to disable interrupts
Will do.
> instead of disabling preemption. I think that disabling preemption should be
> sufficient since this is the only code that updates rq->aborted_gstate and
> since this function is always called from thread context.
blk_mq_complete_request() can read it from the irq context. If that
happens between update_begin and end, it'll end up looping infinitely.
> > @@ -801,6 +840,12 @@ void blk_mq_rq_timed_out(struct request *req, bool
> > reserved)
> > __blk_mq_complete_request(req);
> > break;
> > case BLK_EH_RESET_TIMER:
> > + /*
> > +* As nothing prevents from completion happening while
> > +* ->aborted_gstate is set, this may lead to ignored
> > +* completions and further spurious timeouts.
> > +*/
> > + blk_mq_rq_update_aborted_gstate(req, 0);
> > blk_add_timer(req);
> > blk_clear_rq_complete(req);
> > break;
>
> Is the race that the comment refers to addressed by one of the later patches?
No, but it's not a new race. It has always been there and I suppose
doesn't lead to practical problems - the race window is pretty small
and the effect isn't critical. I'm just documenting the existing race
condition. Will note that in the description.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] blk-mq: replace timeout synchronization with a RCU and generation based scheme
On Mon, 2018-01-08 at 11:15 -0800, Tejun Heo wrote:
> @@ -230,6 +232,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;
Does "gstate" perhaps stand for "generation number and state"? If so, please
mention this in one of the above comments.
Thanks,
Bart.N�r��yb�X��ǧv�^�){.n�+{�n�߲)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH 3/8] blk-mq: replace timeout synchronization with a RCU and generation based scheme
On Mon, 2018-01-08 at 11:15 -0800, Tejun Heo wrote:
> +static void blk_mq_rq_update_aborted_gstate(struct request *rq, u64 gstate)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + u64_stats_update_begin(&rq->aborted_gstate_sync);
> + rq->aborted_gstate = gstate;
> + u64_stats_update_end(&rq->aborted_gstate_sync);
> + local_irq_restore(flags);
> +}
Please add a comment that explains the purpose of local_irq_save() and
local_irq_restore(). Please also explain why you chose to disable interrupts
instead of disabling preemption. I think that disabling preemption should be
sufficient since this is the only code that updates rq->aborted_gstate and
since this function is always called from thread context.
> @@ -801,6 +840,12 @@ void blk_mq_rq_timed_out(struct request *req, bool
> reserved)
> __blk_mq_complete_request(req);
> break;
> case BLK_EH_RESET_TIMER:
> + /*
> + * As nothing prevents from completion happening while
> + * ->aborted_gstate is set, this may lead to ignored
> + * completions and further spurious timeouts.
> + */
> + blk_mq_rq_update_aborted_gstate(req, 0);
> blk_add_timer(req);
> blk_clear_rq_complete(req);
> break;
Is the race that the comment refers to addressed by one of the later patches?
Thanks,
Bart.N�r��yb�X��ǧv�^�){.n�+{�n�߲)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
