[PATCH 1/3] nvme: fixup kato deadlock

[Hannes Reinecke]

On 3/3/21 9:42 AM, Christoph Hellwig wrote:
> On Tue, Mar 02, 2021 at 10:26:42AM +0100, Hannes Reinecke wrote:
>> +	if (test_and_set_bit(NVME_CTRL_KATO_RUNNING, &ctrl->flags))
>> +		return 0;
>> +
>>  	rq = nvme_alloc_request(ctrl->admin_q, &ctrl->ka_cmd,
>> -			BLK_MQ_REQ_RESERVED);
>> -	if (IS_ERR(rq))
>> +			BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
>> +	if (IS_ERR(rq)) {
>> +		clear_bit(NVME_CTRL_KATO_RUNNING, &ctrl->flags);
>>  		return PTR_ERR(rq);
>> +	}
> 
> Adding BLK_MQ_REQ_NOWAIT should be a separate prep patch, together with
> reducing the number of reserved tags.
> 
Okay.
But why would we need to reduce the number of tags? It's not that we're
changing anything with the allocation, we're just guaranteed to fail if
for some reason the stack messes up.

> Also why do we still need the extra test_and_set_bit with the
> NOWAIT allocation?
> 
This is essentially a safeguard; there is nothing in the current code
telling us if a KATO command is in flight or not.
And I really do hate doing pointless work (like allocating) commands if
we don't need to.

Plus it'll differentiate between legit callers of
nvme_start_keep_alive() (which can be called at any time, and hence
might be adding the ka_work element just after the previous one had
submitted the KATO command), and real failures like failing to allocate
the KATO command itself, which points to a real issue in the stack as
normally there should be enough reserved commands.

But yeah, I'll split if off in two patches.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare at suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer