Re: [PATCH 2/2] hw/block/nvme: drain namespaces on sq deletion

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On Apr  9 20:09, Minwoo Im wrote:
>On 21-04-08 21:37:09, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> For most commands, when issuing an AIO, the BlockAIOCB is stored in the
>> NvmeRequest aiocb pointer when the AIO is issued. The main use of this
>> is cancelling AIOs when deleting submission queues (it is currently not
>> used for Abort).
>>
>> However, some commands like Dataset Management Zone Management Send
>> (zone reset) may involve more than one AIO and here the AIOs are issued
>> without saving a reference to the BlockAIOCB. This is a problem since
>> nvme_del_sq() will attempt to cancel outstanding AIOs, potentially with
>> an invalid BlockAIOCB since the aiocb pointer is not NULL'ed when the
>> request structure is recycled.
>>
>> Fix this by
>>
>>   1. making sure the aiocb pointer is NULL'ed when requests are recycled
>>   2. only attempt to cancel the AIO if the aiocb is non-NULL
>>   3. if any AIOs could not be cancelled, drain all aio as a last resort.
>>
>> Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
>> Fixes: c94973288cd9 ("hw/block/nvme: add broadcast nsid support flush command")
>> Fixes: e4e430b3d6ba ("hw/block/nvme: add simple copy command")
>> Fixes: 5f5dc4c6a942 ("hw/block/nvme: zero out zones on reset")
>> Fixes: 2605257a26b8 ("hw/block/nvme: add the dataset management command")
>> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>> Cc: Minwoo Im <minwoo.im@samsung.com>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>  hw/block/nvme.c | 23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 94bc373260be..3c4297e38a52 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -470,6 +470,7 @@ static void nvme_req_clear(NvmeRequest *req)
>>  {
>>      req->ns = NULL;
>>      req->opaque = NULL;
>> +    req->aiocb = NULL;
>>      memset(&req->cqe, 0x0, sizeof(req->cqe));
>>      req->status = NVME_SUCCESS;
>>  }
>> @@ -3681,6 +3682,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
>>      NvmeSQueue *sq;
>>      NvmeCQueue *cq;
>>      uint16_t qid = le16_to_cpu(c->qid);
>> +    int nsid;
>
>Even we don't have fully supported number of namespaces in this device
>(0xFFFFFFFF), can we have this one with `uint32_t` ?
>

Sure!

>Otherwise, looks good to me.
>
>Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]