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 12:56 -0700, Bart Van Assche wrote:
> On 9/16/19 12:38 PM, Singh, Balbir wrote:
> > 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?
> 
> I don't think so. Running the hardware queues is not sufficient to 
> guarantee that requests that had been started before the DYING was set 
> have finished.

May be I am missing something, but in this particular scenario, there
should be no requests pending with the controller - we've frozen the
queues and disabled the device (nvme_dev_disable()). A combination of
blk_set_queue_dying() and blk_mq_unquiesce_queue() should flush any
pending bits out.

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

Cheers,
Balbir Singh.

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