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

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

On Mon, 2019-09-16 at 08:40 -0700, Bart Van Assche wrote:
> On 9/13/19 4:36 PM, Balbir Singh wrote:
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index b45f82d58be8..f6ddb58a7013 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -103,10 +103,16 @@ static void nvme_set_queue_dying(struct
> > nvme_ns *ns)
> >   	 */
> >   	if (!ns->disk || test_and_set_bit(NVME_NS_DEAD, &ns->flags))
> >   		return;
> > -	revalidate_disk(ns->disk);
> >   	blk_set_queue_dying(ns->queue);
> >   	/* Forcibly unquiesce queues to avoid blocking dispatch */
> >   	blk_mq_unquiesce_queue(ns->queue);
> > +	/*
> > +	 * revalidate_disk, after all pending IO is cleaned up
> > +	 * by blk_set_queue_dying, largely any races with blk parittion
> > +	 * reads that might come in after freezing the queues,
> > otherwise
> > +	 * we'll end up waiting up on bd_mutex, creating a deadlock.
> > +	 */
> > +	revalidate_disk(ns->disk);
> >   }
> 
> The comment above revalidate_disk() looks wrong to me. I don't think 
> that blk_set_queue_dying() guarantees that ongoing commands have 
> finished by the time that function returns. All
> blk_set_queue_dying() 
> does is to set the DYING flag, to kill q->q_usage_counter and to wake
> up 
> threads that are waiting inside a request allocation function. It
> does 
> not wait for pending commands to finish.

I was referring to the combined effect of blk_set_queue_dying() and
blk_mq_unquiesce_queue() which should invoke blk_mq_run_hw_queues().
I can see how that might be misleading. I can reword it to say

/*
 * revalidate_disk, after all pending IO is cleaned up
 * largely any races with block partition
 * reads that might come in after freezing the queues, otherwise
 * we'll end up waiting up on bd_mutex, creating a deadlock
 */

Would that work?
Balbir Singh.

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