Re: [PATCH v2 1/2] nvme/host/pci: Fix a race in controller removal

[Keith Busch <kbusch@kernel.org>]

On Tue, Sep 17, 2019 at 10:55:45AM -0700, Bart Van Assche wrote:
> On 9/16/19 1:40 PM, Singh, Balbir wrote:
> > Is your concern largely with the comment or the patch?
> > 
> > Keith just recommend added
> > 
> >   /* Revalidate after unblocking dispatchers that may be holding bd_butex */
> > 
> > That sounds quite reasonable to me
> 
> Hi Balbir,
> 
> Keith's comment suggestion sounds great to me.
> 
> Another concern I have is that your patch depends on implementation details
> of the block layer. Calling revalidate_disk() after the DYING flag has been
> set means that requests will be allocated, started and completed after the
> DYING flag has been set. I think that works with the current implementation
> of the block layer because blk_queue_enter() does not check the DYING flag
> if percpu_ref_tryget_live() succeeds. If your patch gets accepted and if
> blk_queue_enter() would be modified such that the DYING flag is checked for
> every blk_queue_enter() call then that would break the NVMe driver.

I don't think it should matter. Once the call blk_set_queue_dying()
returns, blk_queue_enter() will already never succeed again: in addition
to setting the DYING flag in the queue, it also sets the percpu_ref to
__PERCPU_REF_DEAD, which causes percpu_ref_tryget_live() to fail, at
which point it will observe the DYING flag and abort entering the queue.

In the short window in which a request may be allocated on a DYING queue
but before we've killed the percpu_ref, the nvme driver handles those
requests by unquiescing to get them flushed out through its .queue_rq().
I don't really like that hack, but blk_mq_queue_tag_busy_iter() isn't
exported for a driver to directly end requests on a quiesced queue.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme