Re: [PATCH V3 7/8] block: allow to allocate req with REQF_PREEMPT when queue is preempt frozen

Bart Van Assche
2017-09-04T16:18:32Z

On Tue, 2017-09-05 at 00:08 +0800, Ming Lei wrote:
> On Mon, Sep 04, 2017 at 03:40:35PM +0000, Bart Van Assche wrote:
> > On Mon, 2017-09-04 at 15:16 +0800, Ming Lei wrote:
> > > On Mon, Sep 04, 2017 at 04:13:26AM +0000, Bart Van Assche wrote:
> > > > Allowing blk_get_request() to succeed after the DYING flag has been set is
> > > > completely wrong because that could result in a request being queued after
> > > > the DEAD flag has been set, resulting in either a hanging request or a kernel
> > > > crash. This is why it's completely wrong to add a blk_queue_enter_live() call
> > > > in blk_old_get_request() or blk_mq_alloc_request(). Hence my NAK for any
> > > > patch that adds a blk_queue_enter_live() call to any function called from
> > > > blk_get_request(). That includes the patch at the start of this e-mail thread.
> > >
> > > See above, this patch changes nothing about this fact, please look at
> > > the patch carefully next time just before posting your long comment.
> >
> > Are you really sure that your patch does not allow blk_get_request() to
> > succeed after the DYING flag has been set? blk_mq_alloc_request() calls both
> > blk_queue_is_preempt_frozen() and blk_queue_enter_live() without holding
> > any lock. A thread that is running concurrently with blk_mq_get_request()
> > can unfreeze the queue after blk_queue_is_preempt_frozen() returned and
> > before blk_queue_enter_live() is called. This means that with your patch
> > series applied blk_get_request() can succeed after the DYING flag has been
> > set, which is something we don't want. Additionally, I don't think we want
> > to introduce any kind of locking in blk_mq_get_request() because that would
> > be a serialization point.
>
> Yeah, I am pretty sure.
>
> Firstly blk_queue_freeze_preempt() is exclusive, that means it will wait
> for completion of all pending freezing(both normal and preempt), and other
> freezing can't be started too if there is in-progress preempt
> freezing, actually it is a typical read/write lock use case, but
> we need to support nested normal freezing, so we can't use rwsem.

You seem to overlook that blk_get_request() can be called from another thread
than the thread that is performing the freezing and unfreezing.

Bart.