Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Philippe Mathieu-Daudé]

On 8/21/20 11:44 AM, Stefano Garzarella wrote:
> On Thu, Aug 20, 2020 at 06:58:49PM +0200, Philippe Mathieu-Daudé wrote:
>> As nvme_create_queue_pair() is allowed to fail, replace the
>> alloc() calls by try_alloc() to avoid aborting QEMU.
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 8c30a5fee28..e1893b4e792 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -213,14 +213,22 @@ static NVMeQueuePair 
>> *nvme_create_queue_pair(BlockDriverState *bs,
>>      int i, r;
>>      BDRVNVMeState *s = bs->opaque;
>>      Error *local_err = NULL;
>> -    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
>> +    NVMeQueuePair *q;
>>      uint64_t prp_list_iova;
>>  
>> +    q = g_try_new0(NVMeQueuePair, 1);
>> +    if (!q) {
>> +        return NULL;
>> +    }
>> +    q->prp_list_pages = qemu_try_blockalign0(bs,
>> +                                          s->page_size * NVME_QUEUE_SIZE);
> 
> Here you use NVME_QUEUE_SIZE instead of NVME_NUM_REQS, is that an
> intentional change?

No... Thanks for spotting this, I missed it because git didn't
emit any warning while rebasing on top of "block/nvme: support nested
aio_poll".
This value has been changed in 1086e95da17 ("block/nvme: switch to a
NVMeRequest freelist").

Good catch!
I'll respin (after reviewing the 'nested aio_poll' changes).

> 
> Maybe is not an issue, sice NVME_QUEUE_SIZE is bigger than
> NVME_NUM_REQS, but we should mention in the commit message.
> 
> Thanks,
> Stefano
> 
>> +    if (!q->prp_list_pages) {
>> +        goto fail;
>> +    }
>>      qemu_mutex_init(&q->lock);
>>      q->s = s;
>>      q->index = idx;
>>      qemu_co_queue_init(&q->free_req_queue);
>> -    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
>>      q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
>>                                    nvme_process_completion_bh, q);
>>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>> -- 
>> 2.26.2
>>
>>
>