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.
