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

[Bart Van Assche <bvanassche@acm.org>]

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.

Bart.

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