RE: [PATCH v7 10/11] hw/block/nvme: Separate read and write handlers

2020-10-20 Thread Dmitry Fomichev
> -Original Message-
> From: Keith Busch 
> Sent: Tuesday, October 20, 2020 8:36 AM
> To: Klaus Jensen 
> Cc: Dmitry Fomichev ; Klaus Jensen
> ; Kevin Wolf ; Philippe
> Mathieu-Daudé ; Maxim Levitsky
> ; Fam Zheng ; Niklas Cassel
> ; Damien Le Moal ;
> qemu-bl...@nongnu.org; qemu-devel@nongnu.org; Alistair Francis
> ; Matias Bjorling 
> Subject: Re: [PATCH v7 10/11] hw/block/nvme: Separate read and write
> handlers
> 
> On Tue, Oct 20, 2020 at 10:28:22AM +0200, Klaus Jensen wrote:
> > On Oct 19 11:17, Dmitry Fomichev wrote:
> > > With ZNS support in place, the majority of code in nvme_rw() has
> > > become read- or write-specific. Move these parts to two separate
> > > handlers, nvme_read() and nvme_write() to make the code more
> > > readable and to remove multiple is_write checks that so far existed
> > > in the i/o path.
> > >
> > > This is a refactoring patch, no change in functionality.
> > >
> >
> > This makes a lot of sense, totally Acked, but it might be better to move
> > it ahead as a preparation patch? It would make the zoned patch easier on
> > the eye.
> 
> I agree with the suggestion.

Ok, will move them to the front of the series.



Re: [PATCH v7 10/11] hw/block/nvme: Separate read and write handlers

2020-10-20 Thread Keith Busch
On Tue, Oct 20, 2020 at 10:28:22AM +0200, Klaus Jensen wrote:
> On Oct 19 11:17, Dmitry Fomichev wrote:
> > With ZNS support in place, the majority of code in nvme_rw() has
> > become read- or write-specific. Move these parts to two separate
> > handlers, nvme_read() and nvme_write() to make the code more
> > readable and to remove multiple is_write checks that so far existed
> > in the i/o path.
> > 
> > This is a refactoring patch, no change in functionality.
> > 
> 
> This makes a lot of sense, totally Acked, but it might be better to move
> it ahead as a preparation patch? It would make the zoned patch easier on
> the eye.

I agree with the suggestion.



Re: [PATCH v7 10/11] hw/block/nvme: Separate read and write handlers

2020-10-20 Thread Klaus Jensen
On Oct 19 11:17, Dmitry Fomichev wrote:
> With ZNS support in place, the majority of code in nvme_rw() has
> become read- or write-specific. Move these parts to two separate
> handlers, nvme_read() and nvme_write() to make the code more
> readable and to remove multiple is_write checks that so far existed
> in the i/o path.
> 
> This is a refactoring patch, no change in functionality.
> 

This makes a lot of sense, totally Acked, but it might be better to move
it ahead as a preparation patch? It would make the zoned patch easier on
the eye.


signature.asc
Description: PGP signature


[PATCH v7 10/11] hw/block/nvme: Separate read and write handlers

2020-10-18 Thread Dmitry Fomichev
With ZNS support in place, the majority of code in nvme_rw() has
become read- or write-specific. Move these parts to two separate
handlers, nvme_read() and nvme_write() to make the code more
readable and to remove multiple is_write checks that so far existed
in the i/o path.

This is a refactoring patch, no change in functionality.

Signed-off-by: Dmitry Fomichev 
---
 hw/block/nvme.c   | 191 +-
 hw/block/trace-events |   3 +-
 2 files changed, 114 insertions(+), 80 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 3b9ea326d7..5ec4ce5e28 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1162,10 +1162,10 @@ typedef struct NvmeReadFillCtx {
 uint32_t  post_rd_fill_nlb;
 } NvmeReadFillCtx;
 
-static uint16_t nvme_check_zone_read(NvmeNamespace *ns, NvmeZone *zone,
- uint64_t slba, uint32_t nlb,
- NvmeReadFillCtx *rfc)
+static uint16_t nvme_check_zone_read(NvmeNamespace *ns, uint64_t slba,
+ uint32_t nlb, NvmeReadFillCtx *rfc)
 {
+NvmeZone *zone = nvme_get_zone_by_slba(ns, slba);
 NvmeZone *next_zone;
 uint64_t bndry = nvme_zone_rd_boundary(ns, zone);
 uint64_t end = slba + nlb, wp1, wp2;
@@ -1449,6 +1449,86 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
 return NVME_NO_COMPLETE;
 }
 
+static uint16_t nvme_read(NvmeCtrl *n, NvmeRequest *req)
+{
+NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
+NvmeNamespace *ns = req->ns;
+uint64_t slba = le64_to_cpu(rw->slba);
+uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1;
+uint32_t fill_len;
+uint64_t data_size = nvme_l2b(ns, nlb);
+uint64_t data_offset, fill_ofs;
+NvmeReadFillCtx rfc;
+BlockBackend *blk = ns->blkconf.blk;
+uint16_t status;
+
+trace_pci_nvme_read(nvme_cid(req), nvme_nsid(ns), nlb, data_size, slba);
+
+status = nvme_check_mdts(n, data_size);
+if (status) {
+trace_pci_nvme_err_mdts(nvme_cid(req), data_size);
+goto invalid;
+}
+
+status = nvme_check_bounds(n, ns, slba, nlb);
+if (status) {
+trace_pci_nvme_err_invalid_lba_range(slba, nlb, ns->id_ns.nsze);
+goto invalid;
+}
+
+if (ns->params.zoned) {
+status = nvme_check_zone_read(ns, slba, nlb, );
+if (status != NVME_SUCCESS) {
+trace_pci_nvme_err_zone_read_not_ok(slba, nlb, status);
+goto invalid;
+}
+}
+
+status = nvme_map_dptr(n, data_size, req);
+if (status) {
+goto invalid;
+}
+
+if (ns->params.zoned) {
+if (rfc.pre_rd_fill_nlb) {
+fill_ofs = nvme_l2b(ns, rfc.pre_rd_fill_slba - slba);
+fill_len = nvme_l2b(ns, rfc.pre_rd_fill_nlb);
+nvme_fill_read_data(req, fill_ofs, fill_len,
+n->params.fill_pattern);
+}
+if (!rfc.read_nlb) {
+/* No backend I/O necessary, only needed to fill the buffer */
+req->status = NVME_SUCCESS;
+return NVME_SUCCESS;
+}
+if (rfc.post_rd_fill_nlb) {
+req->fill_ofs = nvme_l2b(ns, rfc.post_rd_fill_slba - slba);
+req->fill_len = nvme_l2b(ns, rfc.post_rd_fill_nlb);
+} else {
+req->fill_len = 0;
+}
+slba = rfc.read_slba;
+data_size = nvme_l2b(ns, rfc.read_nlb);
+}
+
+data_offset = nvme_l2b(ns, slba);
+
+block_acct_start(blk_get_stats(blk), >acct, data_size,
+ BLOCK_ACCT_READ);
+if (req->qsg.sg) {
+req->aiocb = dma_blk_read(blk, >qsg, data_offset,
+  BDRV_SECTOR_SIZE, nvme_rw_cb, req);
+} else {
+req->aiocb = blk_aio_preadv(blk, data_offset, >iov, 0,
+nvme_rw_cb, req);
+}
+return NVME_NO_COMPLETE;
+
+invalid:
+block_acct_invalid(blk_get_stats(blk), BLOCK_ACCT_READ);
+return status | NVME_DNR;
+}
+
 static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest *req)
 {
 NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
@@ -1495,25 +1575,20 @@ invalid:
 return status | NVME_DNR;
 }
 
-static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req, bool append)
+static uint16_t nvme_write(NvmeCtrl *n, NvmeRequest *req, bool append)
 {
 NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
 NvmeNamespace *ns = req->ns;
-uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1;
 uint64_t slba = le64_to_cpu(rw->slba);
+uint32_t nlb = (uint32_t)le16_to_cpu(rw->nlb) + 1;
 uint64_t data_size = nvme_l2b(ns, nlb);
-uint64_t data_offset, fill_ofs;
-
+uint64_t data_offset;
 NvmeZone *zone;
-uint32_t fill_len;
-NvmeReadFillCtx rfc;
-bool is_write = rw->opcode == NVME_CMD_WRITE || append;
-enum BlockAcctType acct = is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ;
 BlockBackend *blk = ns->blkconf.blk;
 uint16_t status;
 
-trace_pci_nvme_rw(nvme_cid(req),