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

["Singh, Balbir" <sblbir@amazon.com>]

On Tue, 2019-09-17 at 10:55 -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 any request is sent down, from what I understand the following
happens in nvme_set_queue_dying()

	if (!ns->disk || test_and_set_bit(NVME_NS_DEAD, &ns->flags))
		return;

In nvme_revalidate_disk(), we do the following

	if (test_bit(NVME_NS_DEAD, &ns->flags)) {
		set_capacity(disk, 0);
		return -ENODEV;
	}

I don't think we ever send the request down, we just set capacity to 0.
and then revalidate_disk() calls check_disk_size_change().

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