Re: [PATCH V2 1/5] block: introduce blk_quiesce_timeout() and blk_unquiesce_timeout()
Hi Ming On 05/02/2018 11:33 AM, Ming Lei wrote: > No, there isn't such race, the 'mod_timer' doesn't make a difference > because 'q->timeout_off' will be visible in new work func after > cancel_work_sync() returns. So even the timer is expired, work func > still returns immediately. Yes, you are right. even if timer is setup , but the timeout work will return. Thanks Jianchao
Re: [PATCH V2 1/5] block: introduce blk_quiesce_timeout() and blk_unquiesce_timeout()
On Wed, May 02, 2018 at 10:23:35AM +0800, jianchao.wang wrote:
> Hi ming
>
> On 04/29/2018 11:41 PM, Ming Lei wrote:
> >
> > +static void __blk_unquiesce_timeout(struct request_queue *q)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + q->timeout_off = false;
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +}
> > +
> > +void blk_unquiesce_timeout(struct request_queue *q)
> > +{
> > + __blk_unquiesce_timeout(q);
> > + mod_timer(>timeout, jiffies + q->rq_timeout);
> > +}
> > +EXPORT_SYMBOL(blk_unquiesce_timeout);
> > +
> > +void blk_quiesce_timeout(struct request_queue *q)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + q->timeout_off = true;
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +
> > + del_timer_sync(>timeout);
> > + cancel_work_sync(>timeout_work);
> > +}
> > +EXPORT_SYMBOL(blk_quiesce_timeout);
> > +
> > /**
> > * blk_sync_queue - cancel any pending callbacks on a queue
> > * @q: the queue
> > @@ -408,8 +437,7 @@ EXPORT_SYMBOL(blk_stop_queue);
> > */
> > void blk_sync_queue(struct request_queue *q)
> > {
> > - del_timer_sync(>timeout);
> > - cancel_work_sync(>timeout_work);
> > + blk_quiesce_timeout(q);
> >
> > if (q->mq_ops) {
> > struct blk_mq_hw_ctx *hctx;
> > @@ -421,6 +449,8 @@ void blk_sync_queue(struct request_queue *q)
> > } else {
> > cancel_delayed_work_sync(>delay_work);
> > }
> > +
> > + __blk_unquiesce_timeout(q);
> > }
> > EXPORT_SYMBOL(blk_sync_queue);
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 0dc9e341c2a7..890dd3b3e3e1 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -901,6 +901,15 @@ static void blk_mq_timeout_work(struct work_struct
> > *work)
> > };
> > struct blk_mq_hw_ctx *hctx;
> > int i;
> > + bool timeout_off;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + timeout_off = q->timeout_off;
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +
> > + if (timeout_off)
> > + return;
>
> Looks like there is still race as following:
>
> blk_quiesce_timeout blk_mq_timeout_work
>-> timeout_off =
> q->timeout_off; //still a false
> -> q->timeout_off = true;
> -> del_timer_sync(>timeout);
>-> mod_timer
> -> cancel_work_sync(>timeout_work);
No, there isn't such race, the 'mod_timer' doesn't make a difference
because 'q->timeout_off' will be visible in new work func after
cancel_work_sync() returns. So even the timer is expired, work func
still returns immediately.
That means del_timer_sync() isn't absolutely necessary too.
Thanks,
Ming
Re: [PATCH V2 1/5] block: introduce blk_quiesce_timeout() and blk_unquiesce_timeout()
Hi ming
On 04/29/2018 11:41 PM, Ming Lei wrote:
>
> +static void __blk_unquiesce_timeout(struct request_queue *q)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + q->timeout_off = false;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +}
> +
> +void blk_unquiesce_timeout(struct request_queue *q)
> +{
> + __blk_unquiesce_timeout(q);
> + mod_timer(>timeout, jiffies + q->rq_timeout);
> +}
> +EXPORT_SYMBOL(blk_unquiesce_timeout);
> +
> +void blk_quiesce_timeout(struct request_queue *q)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + q->timeout_off = true;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + del_timer_sync(>timeout);
> + cancel_work_sync(>timeout_work);
> +}
> +EXPORT_SYMBOL(blk_quiesce_timeout);
> +
> /**
> * blk_sync_queue - cancel any pending callbacks on a queue
> * @q: the queue
> @@ -408,8 +437,7 @@ EXPORT_SYMBOL(blk_stop_queue);
> */
> void blk_sync_queue(struct request_queue *q)
> {
> - del_timer_sync(>timeout);
> - cancel_work_sync(>timeout_work);
> + blk_quiesce_timeout(q);
>
> if (q->mq_ops) {
> struct blk_mq_hw_ctx *hctx;
> @@ -421,6 +449,8 @@ void blk_sync_queue(struct request_queue *q)
> } else {
> cancel_delayed_work_sync(>delay_work);
> }
> +
> + __blk_unquiesce_timeout(q);
> }
> EXPORT_SYMBOL(blk_sync_queue);
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 0dc9e341c2a7..890dd3b3e3e1 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -901,6 +901,15 @@ static void blk_mq_timeout_work(struct work_struct *work)
> };
> struct blk_mq_hw_ctx *hctx;
> int i;
> + bool timeout_off;
> + unsigned long flags;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + timeout_off = q->timeout_off;
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + if (timeout_off)
> + return;
Looks like there is still race as following:
blk_quiesce_timeout blk_mq_timeout_work
-> timeout_off = q->timeout_off;
//still a false
-> q->timeout_off = true;
-> del_timer_sync(>timeout);
-> mod_timer
-> cancel_work_sync(>timeout_work);
How about this:
Introduce a new request queue flag QUEUE_FLAG_TIMEOUT_QUIESCED
void blk_quiesce_queue_timeout(struct request_queue *q)
{
unsigned long flags;
spin_lock_irqsave(q->queue_lock, flags);
queue_flag_set(QUEUE_FLAG_TIMEOUT_QUIESCED, q);
spin_unlock_irqrestore(q->queue_lock, flags);
synchronize_rcu_bh();
cancel_work_sync(>timeout_work);
/*
* The timeout work will not be invoked anymore.
*/
del_timer_sync(>timeout);
}
void blk_set_queue_dying(struct request_queue *q)
{
spin_lock_irq(q->queue_lock);
@@ -867,6 +905,9 @@ static void blk_rq_timed_out_timer(struct timer_list *t)
{
struct request_queue *q = from_timer(q, t, timeout);
+ if (blk_queue_timeout_quiesced(q))
+ return;
kblockd_schedule_work(>timeout_work);
}
Thanks
Jianchao
