Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully
On Fri, Aug 21, 2020 at 03:36:47PM +0200, Philippe Mathieu-Daudé wrote:
> 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
> >> Signed-off-by: Philippe Mathieu-Daudé
> >> ---
> >> 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).
Cool, with that fixed the patch LGTM:
Reviewed-by: Stefano Garzarella
>
> >
> > 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(>lock);
> >> q->s = s;
> >> q->index = idx;
> >> qemu_co_queue_init(>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
> >>
> >>
> >
>
Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully
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
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> 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(>lock);
>> q->s = s;
>> q->index = idx;
>> qemu_co_queue_init(>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
>>
>>
>
Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully
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
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> 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?
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(>lock);
> q->s = s;
> q->index = idx;
> qemu_co_queue_init(>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
>
>
[PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully
As nvme_create_queue_pair() is allowed to fail, replace the
alloc() calls by try_alloc() to avoid aborting QEMU.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Philippe Mathieu-Daudé
---
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);
+if (!q->prp_list_pages) {
+goto fail;
+}
qemu_mutex_init(>lock);
q->s = s;
q->index = idx;
qemu_co_queue_init(>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
