Re: [PATCH 2/2] nvme-pci: don't wait freeze during resetting

[Ming Lei <ming.lei@redhat.com>]

On Thu, Aug 25, 2022 at 07:42:44AM -0600, Keith Busch wrote:
> On Thu, Aug 25, 2022 at 07:34:30PM +0800, Ming Lei wrote:
> > On Thu, Aug 25, 2022 at 06:05:30PM +0800, Chao Leng wrote:
> > > 
> > > 
> > > On 2022/8/21 16:47, Ming Lei wrote:
> > > > First it isn't necessary to call nvme_wait_freeze in nvme_reset_work().
> > > > If tagset isn't allocated, there can't be any inflight IOs; otherwise
> > > > blk_mq_update_nr_hw_queues can freeze & wait queues.
> > > > 
> > > > Second, since commit bdd6316094e0 ("block: Allow unfreezing of a queue
> > > > while requests are in progress"), it is fine to unfreeze queue without
> > > > draining inflight IOs.
> > > > 
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >   drivers/nvme/host/pci.c | 1 -
> > > >   1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > > > index 3a1c37f32f30..91b2903fcc24 100644
> > > > --- a/drivers/nvme/host/pci.c
> > > > +++ b/drivers/nvme/host/pci.c
> > > > @@ -2910,7 +2910,6 @@ static void nvme_reset_work(struct work_struct *work)
> > > >   		nvme_free_tagset(dev);
> > > >   	} else {
> > > >   		nvme_start_queues(&dev->ctrl);
> > > > -		nvme_wait_freeze(&dev->ctrl);
> > > It is not safe.
> > > nvme_dev_add may call blk_mq_update_nr_hw_queues, blk_mq_update_nr_hw_queues
> > > require the state is frozen.
> > 
> > blk_mq_update_nr_hw_queues() calls blk_freeze_queue() for every queue in
> > this tagset, so it needn't nvme's freeze wait.
> 
> Right, the blk-mq realloc should be fine, and it's really a very uncommon event
> that nvme needs to realloc the tag set.
> 
> I'm trying to remember why I added the wait freeze here in the first place, and
> I don't recall a good reason for it. I think I had a controller that always
> timed out IO, and the resets would just continue indefinitely. Waiting for
> freeze within a reset causes the next IO timeout to trigger a driver detach.
> That doesn't sound like a good idea, though, since it tears down the admin
> queue too.

The wait becomes not necessary since commit bdd6316094e0 ("block: Allow unfreezing
of a queue while requests are in progress").


Thanks,
Ming