* [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare [not found] <CGME20210416072533epcas5p305e83206f2b3d947e9b5fef9fde1c969@epcas5p3.samsung.com> @ 2021-04-16 7:22 ` Gollu Appalanaidu [not found] ` <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com> ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Gollu Appalanaidu @ 2021-04-16 7:22 UTC (permalink / raw) To: qemu-devel Cc: fam, kwolf, qemu-block, Gollu Appalanaidu, mreitz, its, stefanha, kbusch Currently in compare command metadata aio read blk_aio_preadv return value ignored, consider it and complete the block accounting. Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> --- hw/block/nvme.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 624a1431d0..c2727540f1 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) uint32_t reftag = le32_to_cpu(rw->reftag); struct nvme_compare_ctx *ctx = req->opaque; g_autofree uint8_t *buf = NULL; + BlockBackend *blk = ns->blkconf.blk; + BlockAcctCookie *acct = &req->acct; + BlockAcctStats *stats = blk_get_stats(blk); uint16_t status = NVME_SUCCESS; trace_pci_nvme_compare_mdata_cb(nvme_cid(req)); + if (ret) { + block_acct_failed(stats, acct); + nvme_aio_err(req, ret); + goto out; + } + buf = g_malloc(ctx->mdata.iov.size); status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size, @@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) goto out; } + block_acct_done(stats, acct); + out: qemu_iovec_destroy(&ctx->data.iov); g_free(ctx->data.bounce); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com>]
* [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization [not found] ` <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com> @ 2021-04-16 7:22 ` Gollu Appalanaidu 2021-04-16 8:48 ` Klaus Jensen 0 siblings, 1 reply; 6+ messages in thread From: Gollu Appalanaidu @ 2021-04-16 7:22 UTC (permalink / raw) To: qemu-devel Cc: fam, kwolf, qemu-block, Gollu Appalanaidu, mreitz, its, stefanha, kbusch Currently LBAF formats are being intialized based on metadata size if and only if nvme-ns "ms" parameter is non-zero value. Since FormatNVM command being supported device parameter "ms" may not be the criteria to initialize the supported LBAFs. Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> --- hw/block/nvme-ns.c | 48 ++++++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c index 7bb618f182..573dbb5a9d 100644 --- a/hw/block/nvme-ns.c +++ b/hw/block/nvme-ns.c @@ -85,38 +85,28 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) ds = 31 - clz32(ns->blkconf.logical_block_size); ms = ns->params.ms; - if (ns->params.ms) { - id_ns->mc = 0x3; + id_ns->mc = 0x3; - if (ns->params.mset) { - id_ns->flbas |= 0x10; - } + if (ms && ns->params.mset) { + id_ns->flbas |= 0x10; + } - id_ns->dpc = 0x1f; - id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; - - NvmeLBAF lbaf[16] = { - [0] = { .ds = 9 }, - [1] = { .ds = 9, .ms = 8 }, - [2] = { .ds = 9, .ms = 16 }, - [3] = { .ds = 9, .ms = 64 }, - [4] = { .ds = 12 }, - [5] = { .ds = 12, .ms = 8 }, - [6] = { .ds = 12, .ms = 16 }, - [7] = { .ds = 12, .ms = 64 }, - }; - - memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); - id_ns->nlbaf = 7; - } else { - NvmeLBAF lbaf[16] = { - [0] = { .ds = 9 }, - [1] = { .ds = 12 }, - }; + id_ns->dpc = 0x1f; + id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; - memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); - id_ns->nlbaf = 1; - } + NvmeLBAF lbaf[16] = { + [0] = { .ds = 9 }, + [1] = { .ds = 9, .ms = 8 }, + [2] = { .ds = 9, .ms = 16 }, + [3] = { .ds = 9, .ms = 64 }, + [4] = { .ds = 12 }, + [5] = { .ds = 12, .ms = 8 }, + [6] = { .ds = 12, .ms = 16 }, + [7] = { .ds = 12, .ms = 64 }, + }; + + memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); + id_ns->nlbaf = 7; for (i = 0; i <= id_ns->nlbaf; i++) { NvmeLBAF *lbaf = &id_ns->lbaf[i]; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization 2021-04-16 7:22 ` [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization Gollu Appalanaidu @ 2021-04-16 8:48 ` Klaus Jensen 2021-04-16 10:50 ` Gollu Appalanaidu 0 siblings, 1 reply; 6+ messages in thread From: Klaus Jensen @ 2021-04-16 8:48 UTC (permalink / raw) To: Gollu Appalanaidu Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch [-- Attachment #1: Type: text/plain, Size: 2786 bytes --] On Apr 16 12:52, Gollu Appalanaidu wrote: >Currently LBAF formats are being intialized based on metadata >size if and only if nvme-ns "ms" parameter is non-zero value. >Since FormatNVM command being supported device parameter "ms" >may not be the criteria to initialize the supported LBAFs. > >Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> >--- > hw/block/nvme-ns.c | 48 ++++++++++++++++++---------------------------- > 1 file changed, 19 insertions(+), 29 deletions(-) > >diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c >index 7bb618f182..573dbb5a9d 100644 >--- a/hw/block/nvme-ns.c >+++ b/hw/block/nvme-ns.c >@@ -85,38 +85,28 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) > ds = 31 - clz32(ns->blkconf.logical_block_size); > ms = ns->params.ms; > >- if (ns->params.ms) { >- id_ns->mc = 0x3; >+ id_ns->mc = 0x3; > >- if (ns->params.mset) { >- id_ns->flbas |= 0x10; >- } >+ if (ms && ns->params.mset) { >+ id_ns->flbas |= 0x10; >+ } > >- id_ns->dpc = 0x1f; >- id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; >- >- NvmeLBAF lbaf[16] = { >- [0] = { .ds = 9 }, >- [1] = { .ds = 9, .ms = 8 }, >- [2] = { .ds = 9, .ms = 16 }, >- [3] = { .ds = 9, .ms = 64 }, >- [4] = { .ds = 12 }, >- [5] = { .ds = 12, .ms = 8 }, >- [6] = { .ds = 12, .ms = 16 }, >- [7] = { .ds = 12, .ms = 64 }, >- }; >- >- memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >- id_ns->nlbaf = 7; >- } else { >- NvmeLBAF lbaf[16] = { >- [0] = { .ds = 9 }, >- [1] = { .ds = 12 }, >- }; >+ id_ns->dpc = 0x1f; >+ id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; While nvme_ns_check_constraints() will error out if pi is set and the metadata bytes are insufficient, I don't think this should set bit 3 unless both metadata and pi is enabled. It's not against the spec, but it's just slightly weird. > >- memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >- id_ns->nlbaf = 1; >- } >+ NvmeLBAF lbaf[16] = { >+ [0] = { .ds = 9 }, >+ [1] = { .ds = 9, .ms = 8 }, >+ [2] = { .ds = 9, .ms = 16 }, >+ [3] = { .ds = 9, .ms = 64 }, >+ [4] = { .ds = 12 }, >+ [5] = { .ds = 12, .ms = 8 }, >+ [6] = { .ds = 12, .ms = 16 }, >+ [7] = { .ds = 12, .ms = 64 }, >+ }; >+ >+ memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >+ id_ns->nlbaf = 7; > > for (i = 0; i <= id_ns->nlbaf; i++) { > NvmeLBAF *lbaf = &id_ns->lbaf[i]; >-- >2.17.1 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization 2021-04-16 8:48 ` Klaus Jensen @ 2021-04-16 10:50 ` Gollu Appalanaidu 0 siblings, 0 replies; 6+ messages in thread From: Gollu Appalanaidu @ 2021-04-16 10:50 UTC (permalink / raw) To: Klaus Jensen; +Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch [-- Attachment #1: Type: text/plain, Size: 3319 bytes --] On Fri, Apr 16, 2021 at 10:48:16AM +0200, Klaus Jensen wrote: >On Apr 16 12:52, Gollu Appalanaidu wrote: >>Currently LBAF formats are being intialized based on metadata >>size if and only if nvme-ns "ms" parameter is non-zero value. >>Since FormatNVM command being supported device parameter "ms" >>may not be the criteria to initialize the supported LBAFs. >> >>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> >>--- >>hw/block/nvme-ns.c | 48 ++++++++++++++++++---------------------------- >>1 file changed, 19 insertions(+), 29 deletions(-) >> >>diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c >>index 7bb618f182..573dbb5a9d 100644 >>--- a/hw/block/nvme-ns.c >>+++ b/hw/block/nvme-ns.c >>@@ -85,38 +85,28 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) >> ds = 31 - clz32(ns->blkconf.logical_block_size); >> ms = ns->params.ms; >> >>- if (ns->params.ms) { >>- id_ns->mc = 0x3; >>+ id_ns->mc = 0x3; >> >>- if (ns->params.mset) { >>- id_ns->flbas |= 0x10; >>- } >>+ if (ms && ns->params.mset) { >>+ id_ns->flbas |= 0x10; >>+ } >> >>- id_ns->dpc = 0x1f; >>- id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; >>- >>- NvmeLBAF lbaf[16] = { >>- [0] = { .ds = 9 }, >>- [1] = { .ds = 9, .ms = 8 }, >>- [2] = { .ds = 9, .ms = 16 }, >>- [3] = { .ds = 9, .ms = 64 }, >>- [4] = { .ds = 12 }, >>- [5] = { .ds = 12, .ms = 8 }, >>- [6] = { .ds = 12, .ms = 16 }, >>- [7] = { .ds = 12, .ms = 64 }, >>- }; >>- >>- memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >>- id_ns->nlbaf = 7; >>- } else { >>- NvmeLBAF lbaf[16] = { >>- [0] = { .ds = 9 }, >>- [1] = { .ds = 12 }, >>- }; >>+ id_ns->dpc = 0x1f; >>+ id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi; > >While nvme_ns_check_constraints() will error out if pi is set and the >metadata bytes are insufficient, I don't think this should set bit 3 >unless both metadata and pi is enabled. It's not against the spec, but >it's just slightly weird. okay, will modify current "ms" and "pi" constraint check like this: if (ns->params.ms < 8) { if (ns->params.pi || ns->params.pil || ns->params.mset) { error_setg(errp, "at least 8 bytes of metadata required to enable " "protection information, protection information location and " "metadata settings"); return -1; } } and "mset" is set will set directly without checing "ms" in nvme_ns_init() > >> >>- memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >>- id_ns->nlbaf = 1; >>- } >>+ NvmeLBAF lbaf[16] = { >>+ [0] = { .ds = 9 }, >>+ [1] = { .ds = 9, .ms = 8 }, >>+ [2] = { .ds = 9, .ms = 16 }, >>+ [3] = { .ds = 9, .ms = 64 }, >>+ [4] = { .ds = 12 }, >>+ [5] = { .ds = 12, .ms = 8 }, >>+ [6] = { .ds = 12, .ms = 16 }, >>+ [7] = { .ds = 12, .ms = 64 }, >>+ }; >>+ >>+ memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf)); >>+ id_ns->nlbaf = 7; >> >> for (i = 0; i <= id_ns->nlbaf; i++) { >> NvmeLBAF *lbaf = &id_ns->lbaf[i]; >>-- >>2.17.1 >> >> > [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare 2021-04-16 7:22 ` [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare Gollu Appalanaidu [not found] ` <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com> @ 2021-04-16 12:06 ` Klaus Jensen 2021-04-20 19:58 ` Klaus Jensen 2 siblings, 0 replies; 6+ messages in thread From: Klaus Jensen @ 2021-04-16 12:06 UTC (permalink / raw) To: Gollu Appalanaidu Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch [-- Attachment #1: Type: text/plain, Size: 1695 bytes --] On Apr 16 12:52, Gollu Appalanaidu wrote: >Currently in compare command metadata aio read blk_aio_preadv return >value ignored, consider it and complete the block accounting. > >Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> >--- > hw/block/nvme.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > >diff --git a/hw/block/nvme.c b/hw/block/nvme.c >index 624a1431d0..c2727540f1 100644 >--- a/hw/block/nvme.c >+++ b/hw/block/nvme.c >@@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) > uint32_t reftag = le32_to_cpu(rw->reftag); > struct nvme_compare_ctx *ctx = req->opaque; > g_autofree uint8_t *buf = NULL; >+ BlockBackend *blk = ns->blkconf.blk; >+ BlockAcctCookie *acct = &req->acct; >+ BlockAcctStats *stats = blk_get_stats(blk); > uint16_t status = NVME_SUCCESS; > > trace_pci_nvme_compare_mdata_cb(nvme_cid(req)); > >+ if (ret) { >+ block_acct_failed(stats, acct); >+ nvme_aio_err(req, ret); >+ goto out; >+ } >+ > buf = g_malloc(ctx->mdata.iov.size); > > status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size, >@@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) > goto out; > } > >+ block_acct_done(stats, acct); >+ > out: > qemu_iovec_destroy(&ctx->data.iov); > g_free(ctx->data.bounce); >-- >2.17.1 > > Good fix, thanks! Since there is no crash, data corruption or other "bad" behavior, this isn't critical for v6.0. Might consider it for a potential stable release though, so I'll add a Fixes: tag and queue it up. Reviewed-by: Klaus Jensen <k.jensen@samsung.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare 2021-04-16 7:22 ` [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare Gollu Appalanaidu [not found] ` <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com> 2021-04-16 12:06 ` [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare Klaus Jensen @ 2021-04-20 19:58 ` Klaus Jensen 2 siblings, 0 replies; 6+ messages in thread From: Klaus Jensen @ 2021-04-20 19:58 UTC (permalink / raw) To: Gollu Appalanaidu Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] On Apr 16 12:52, Gollu Appalanaidu wrote: >Currently in compare command metadata aio read blk_aio_preadv return >value ignored, consider it and complete the block accounting. > >Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> >--- > hw/block/nvme.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > >diff --git a/hw/block/nvme.c b/hw/block/nvme.c >index 624a1431d0..c2727540f1 100644 >--- a/hw/block/nvme.c >+++ b/hw/block/nvme.c >@@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) > uint32_t reftag = le32_to_cpu(rw->reftag); > struct nvme_compare_ctx *ctx = req->opaque; > g_autofree uint8_t *buf = NULL; >+ BlockBackend *blk = ns->blkconf.blk; >+ BlockAcctCookie *acct = &req->acct; >+ BlockAcctStats *stats = blk_get_stats(blk); > uint16_t status = NVME_SUCCESS; > > trace_pci_nvme_compare_mdata_cb(nvme_cid(req)); > >+ if (ret) { >+ block_acct_failed(stats, acct); >+ nvme_aio_err(req, ret); >+ goto out; >+ } >+ > buf = g_malloc(ctx->mdata.iov.size); > > status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size, >@@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret) > goto out; > } > >+ block_acct_done(stats, acct); >+ > out: > qemu_iovec_destroy(&ctx->data.iov); > g_free(ctx->data.bounce); >-- >2.17.1 > Applied to nvme-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-20 20:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20210416072533epcas5p305e83206f2b3d947e9b5fef9fde1c969@epcas5p3.samsung.com>
2021-04-16 7:22 ` [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare Gollu Appalanaidu
[not found] ` <CGME20210416072544epcas5p26bf011c82ad4b60693cfaac32bc9e36f@epcas5p2.samsung.com>
2021-04-16 7:22 ` [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization Gollu Appalanaidu
2021-04-16 8:48 ` Klaus Jensen
2021-04-16 10:50 ` Gollu Appalanaidu
2021-04-16 12:06 ` [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare Klaus Jensen
2021-04-20 19:58 ` Klaus Jensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).