| Message ID | 20200930220414.562527-4-kbusch@kernel.org |
|---|---|
| State | Accepted |
| Commit | 2fbbecc5cd90ec00027a155f7044f2f70ed84f30 |
| Headers | show |
| Series | nvme qemu cleanups and fixes | expand |
On Sep 30 15:04, Keith Busch wrote: > Let the user specify a specific namespace if they want to get access > stats for a specific namespace. > I don't think this makes sense for v1.3+. NVM Express v1.3d, Section 5.14.1.2: "There is no namespace specific information defined in the SMART / Health log page in this revision of the specification. therefore the controller log page and namespace specific log page contain identical information". I have no idea why the TWG decided this, but that's the way it is ;) > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > hw/block/nvme.c | 66 +++++++++++++++++++++++++++----------------- > include/block/nvme.h | 1 + > 2 files changed, 41 insertions(+), 26 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 8d2b5be567..41389b2b09 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1164,48 +1164,62 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req) > return NVME_SUCCESS; > } > > +struct nvme_stats { > + uint64_t units_read; > + uint64_t units_written; > + uint64_t read_commands; > + uint64_t write_commands; > +}; > + > +static void nvme_set_blk_stats(NvmeNamespace *ns, struct nvme_stats *stats) > +{ > + BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); > + > + stats->units_read += s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS; > + stats->units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS; > + stats->read_commands += s->nr_ops[BLOCK_ACCT_READ]; > + stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; > +} > + > static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len, > uint64_t off, NvmeRequest *req) > { > uint32_t nsid = le32_to_cpu(req->cmd.nsid); > - > + struct nvme_stats stats = { 0 }; > + NvmeSmartLog smart = { 0 }; > uint32_t trans_len; > + NvmeNamespace *ns; > time_t current_ms; > - uint64_t units_read = 0, units_written = 0; > - uint64_t read_commands = 0, write_commands = 0; > - NvmeSmartLog smart; > - > - if (nsid && nsid != 0xffffffff) { > - return NVME_INVALID_FIELD | NVME_DNR; > - } > > if (off >= sizeof(smart)) { > return NVME_INVALID_FIELD | NVME_DNR; > } > > - for (int i = 1; i <= n->num_namespaces; i++) { > - NvmeNamespace *ns = nvme_ns(n, i); > - if (!ns) { > - continue; > - } > - > - BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); > + if (nsid != 0xffffffff) { > + ns = nvme_ns(n, nsid); > + if (!ns) > + return NVME_INVALID_NSID | NVME_DNR; > + nvme_set_blk_stats(ns, &stats); > + } else { > + int i; > > - units_read += s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS; > - units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS; > - read_commands += s->nr_ops[BLOCK_ACCT_READ]; > - write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; > + for (i = 1; i <= n->num_namespaces; i++) { > + ns = nvme_ns(n, i); > + if (!ns) { > + continue; > + } > + nvme_set_blk_stats(ns, &stats); > + } > } > > trans_len = MIN(sizeof(smart) - off, buf_len); > > - memset(&smart, 0x0, sizeof(smart)); > - > - smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(units_read, 1000)); > - smart.data_units_written[0] = cpu_to_le64(DIV_ROUND_UP(units_written, > + smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read, > + 1000)); > + smart.data_units_written[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_written, > 1000)); > - smart.host_read_commands[0] = cpu_to_le64(read_commands); > - smart.host_write_commands[0] = cpu_to_le64(write_commands); > + smart.host_read_commands[0] = cpu_to_le64(stats.read_commands); > + smart.host_write_commands[0] = cpu_to_le64(stats.write_commands); > > smart.temperature = cpu_to_le16(n->temperature); > > @@ -2708,7 +2722,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) > id->acl = 3; > id->aerl = n->params.aerl; > id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO; > - id->lpa = NVME_LPA_EXTENDED; > + id->lpa = NVME_LPA_NS_SMART | NVME_LPA_EXTENDED; > > /* recommended default value (~70 C) */ > id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING); > diff --git a/include/block/nvme.h b/include/block/nvme.h > index 58647bcdad..868cf53f0b 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -849,6 +849,7 @@ enum NvmeIdCtrlFrmw { > }; > > enum NvmeIdCtrlLpa { > + NVME_LPA_NS_SMART = 1 << 0, > NVME_LPA_EXTENDED = 1 << 2, > }; > > -- > 2.24.1 > >
On Thu, Oct 01, 2020 at 06:10:57AM +0200, Klaus Jensen wrote: > On Sep 30 15:04, Keith Busch wrote: > > Let the user specify a specific namespace if they want to get access > > stats for a specific namespace. > > > > I don't think this makes sense for v1.3+. > > NVM Express v1.3d, Section 5.14.1.2: "There is no namespace specific > information defined in the SMART / Health log page in this revision of > the specification. therefore the controller log page and namespace > specific log page contain identical information". > > I have no idea why the TWG decided this, but that's the way it is ;) I don't think they did that. The behavior you're referring to is specific to controllers that operate a particular way: "If the log page is not supported on a per namespace basis ...". They were trying to provide a spec compliant way for controllers to return a success status if you supplied a valid NSID when the controller doesn't support per-namespace smart data.. The previous paragraph is more clear on this: "The controller may also support requesting the log page on a per namespace basis, as indicated by bit 0 of the LPA field in the Identify Controller data structure".
On Oct 1 09:20, Keith Busch wrote: > On Thu, Oct 01, 2020 at 06:10:57AM +0200, Klaus Jensen wrote: > > On Sep 30 15:04, Keith Busch wrote: > > > Let the user specify a specific namespace if they want to get access > > > stats for a specific namespace. > > > > > > > I don't think this makes sense for v1.3+. > > > > NVM Express v1.3d, Section 5.14.1.2: "There is no namespace specific > > information defined in the SMART / Health log page in this revision of > > the specification. therefore the controller log page and namespace > > specific log page contain identical information". > > > > I have no idea why the TWG decided this, but that's the way it is ;) > > I don't think they did that. The behavior you're referring to is specific to > controllers that operate a particular way: "If the log page is not supported on > a per namespace basis ...". They were trying to provide a spec compliant way > for controllers to return a success status if you supplied a valid NSID when > the controller doesn't support per-namespace smart data.. > > The previous paragraph is more clear on this: "The controller may also support > requesting the log page on a per namespace basis, as indicated by bit 0 of the > LPA field in the Identify Controller data structure". OK, so I agree that it makes sense for it to be supported on a per namespace basis, but I think the spec is just keeping the door open for future namespace specific stuff in the log page - currently there is none. Figure 94 (the actual SMART log page) says that the Data Units Read/Written are controller wide, so there really is no namespace specific information. Maybe this could be in the context of shared namespaces? How would a controller know how much data has been read/written from it without asking the other controllers? What if a controller is detached from the namespace - you'd lose those numbers.
On Thu, Oct 01, 2020 at 07:18:37PM +0200, Klaus Jensen wrote: > OK, so I agree that it makes sense for it to be supported on a per > namespace basis, but I think the spec is just keeping the door open for > future namespace specific stuff in the log page - currently there is > none. > > Figure 94 (the actual SMART log page) says that the Data Units > Read/Written are controller wide, so there really is no namespace > specific information. Maybe this could be in the context of shared > namespaces? How would a controller know how much data has been > read/written from it without asking the other controllers? What if a > controller is detached from the namespace - you'd lose those numbers. That text is wrong. There is no "controller" scope to the smart log. Figure 191 says the smart scope is to the subsystem or the namespace. It doesn't matter which controller performed an IO to a particular namespace; the log needs to report the same information regardless of which controller you query. How that is coordinated within the subsystem is a detail not defined by spec. Not that that particular detail matters here, as we don't support multi-controller subsystems (yet!). But the smart log text has missed an update to reflect this, so it looks like trivial ECN material to me.
On Oct 1 10:30, Keith Busch wrote: > On Thu, Oct 01, 2020 at 07:18:37PM +0200, Klaus Jensen wrote: > > OK, so I agree that it makes sense for it to be supported on a per > > namespace basis, but I think the spec is just keeping the door open for > > future namespace specific stuff in the log page - currently there is > > none. > > > > Figure 94 (the actual SMART log page) says that the Data Units > > Read/Written are controller wide, so there really is no namespace > > specific information. Maybe this could be in the context of shared > > namespaces? How would a controller know how much data has been > > read/written from it without asking the other controllers? What if a > > controller is detached from the namespace - you'd lose those numbers. > > That text is wrong. There is no "controller" scope to the smart log. > Figure 191 says the smart scope is to the subsystem or the namespace. It > doesn't matter which controller performed an IO to a particular > namespace; the log needs to report the same information regardless of > which controller you query. How that is coordinated within the subsystem > is a detail not defined by spec. > Oh! Thats new in v1.4. So they updated that, but forgot figure 194. In v1.3 it is controller and namespace scope. Anyway, fair enough. In that case, Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
On Sep 30 15:04, Keith Busch wrote: > Let the user specify a specific namespace if they want to get access > stats for a specific namespace. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > hw/block/nvme.c | 66 +++++++++++++++++++++++++++----------------- > include/block/nvme.h | 1 + > 2 files changed, 41 insertions(+), 26 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 8d2b5be567..41389b2b09 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len, > uint64_t off, NvmeRequest *req) > { > uint32_t nsid = le32_to_cpu(req->cmd.nsid); > - > + struct nvme_stats stats = { 0 }; > + NvmeSmartLog smart = { 0 }; > uint32_t trans_len; > + NvmeNamespace *ns; > time_t current_ms; > - uint64_t units_read = 0, units_written = 0; > - uint64_t read_commands = 0, write_commands = 0; > - NvmeSmartLog smart; > - > - if (nsid && nsid != 0xffffffff) { > - return NVME_INVALID_FIELD | NVME_DNR; > - } > > if (off >= sizeof(smart)) { > return NVME_INVALID_FIELD | NVME_DNR; > } > > - for (int i = 1; i <= n->num_namespaces; i++) { > - NvmeNamespace *ns = nvme_ns(n, i); > - if (!ns) { > - continue; > - } > - > - BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); > + if (nsid != 0xffffffff) { > + ns = nvme_ns(n, nsid); > + if (!ns) > + return NVME_INVALID_NSID | NVME_DNR; Btw, this is failing style check (missing braces).
> -----Original Message----- > From: Keith Busch <kbusch@kernel.org> > Sent: Wednesday, September 30, 2020 6:04 PM > To: qemu-block@nongnu.org; qemu-devel@nongnu.org; Klaus Jensen > <k.jensen@samsung.com> > Cc: Niklas Cassel <Niklas.Cassel@wdc.com>; Dmitry Fomichev > <Dmitry.Fomichev@wdc.com>; Kevin Wolf <kwolf@redhat.com>; Philippe > Mathieu-Daudé <philmd@redhat.com>; Keith Busch <kbusch@kernel.org> > Subject: [PATCH 3/9] hw/block/nvme: support per-namespace smart log > > Let the user specify a specific namespace if they want to get access > stats for a specific namespace. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > hw/block/nvme.c | 66 +++++++++++++++++++++++++++----------------- > include/block/nvme.h | 1 + > 2 files changed, 41 insertions(+), 26 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 8d2b5be567..41389b2b09 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1164,48 +1164,62 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, > NvmeRequest *req) > return NVME_SUCCESS; > } > > +struct nvme_stats { > + uint64_t units_read; > + uint64_t units_written; > + uint64_t read_commands; > + uint64_t write_commands; > +}; > + > +static void nvme_set_blk_stats(NvmeNamespace *ns, struct nvme_stats > *stats) > +{ > + BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); > + > + stats->units_read += s->nr_bytes[BLOCK_ACCT_READ] >> > BDRV_SECTOR_BITS; > + stats->units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> > BDRV_SECTOR_BITS; > + stats->read_commands += s->nr_ops[BLOCK_ACCT_READ]; > + stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; > +} > + > static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t > buf_len, > uint64_t off, NvmeRequest *req) > { > uint32_t nsid = le32_to_cpu(req->cmd.nsid); > - > + struct nvme_stats stats = { 0 }; > + NvmeSmartLog smart = { 0 }; > uint32_t trans_len; > + NvmeNamespace *ns; > time_t current_ms; > - uint64_t units_read = 0, units_written = 0; > - uint64_t read_commands = 0, write_commands = 0; > - NvmeSmartLog smart; > - > - if (nsid && nsid != 0xffffffff) { > - return NVME_INVALID_FIELD | NVME_DNR; > - } > > if (off >= sizeof(smart)) { > return NVME_INVALID_FIELD | NVME_DNR; > } > > - for (int i = 1; i <= n->num_namespaces; i++) { > - NvmeNamespace *ns = nvme_ns(n, i); > - if (!ns) { > - continue; > - } > - > - BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); > + if (nsid != 0xffffffff) { > + ns = nvme_ns(n, nsid); > + if (!ns) > + return NVME_INVALID_NSID | NVME_DNR; checkpatch will complain about missing curly braces here > + nvme_set_blk_stats(ns, &stats); > + } else { > + int i; > > - units_read += s->nr_bytes[BLOCK_ACCT_READ] >> > BDRV_SECTOR_BITS; > - units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> > BDRV_SECTOR_BITS; > - read_commands += s->nr_ops[BLOCK_ACCT_READ]; > - write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; > + for (i = 1; i <= n->num_namespaces; i++) { > + ns = nvme_ns(n, i); > + if (!ns) { > + continue; > + } > + nvme_set_blk_stats(ns, &stats); > + } > } > > trans_len = MIN(sizeof(smart) - off, buf_len); > > - memset(&smart, 0x0, sizeof(smart)); > - > - smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(units_read, > 1000)); > - smart.data_units_written[0] = > cpu_to_le64(DIV_ROUND_UP(units_written, > + smart.data_units_read[0] = > cpu_to_le64(DIV_ROUND_UP(stats.units_read, > + 1000)); > + smart.data_units_written[0] = > cpu_to_le64(DIV_ROUND_UP(stats.units_written, > 1000)); > - smart.host_read_commands[0] = cpu_to_le64(read_commands); > - smart.host_write_commands[0] = cpu_to_le64(write_commands); > + smart.host_read_commands[0] = cpu_to_le64(stats.read_commands); > + smart.host_write_commands[0] = cpu_to_le64(stats.write_commands); > > smart.temperature = cpu_to_le16(n->temperature); > > @@ -2708,7 +2722,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice > *pci_dev) > id->acl = 3; > id->aerl = n->params.aerl; > id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO; > - id->lpa = NVME_LPA_EXTENDED; > + id->lpa = NVME_LPA_NS_SMART | NVME_LPA_EXTENDED; > > /* recommended default value (~70 C) */ > id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING); > diff --git a/include/block/nvme.h b/include/block/nvme.h > index 58647bcdad..868cf53f0b 100644 > --- a/include/block/nvme.h > +++ b/include/block/nvme.h > @@ -849,6 +849,7 @@ enum NvmeIdCtrlFrmw { > }; > > enum NvmeIdCtrlLpa { > + NVME_LPA_NS_SMART = 1 << 0, > NVME_LPA_EXTENDED = 1 << 2, > }; > > -- > 2.24.1
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 8d2b5be567..41389b2b09 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1164,48 +1164,62 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req) return NVME_SUCCESS; } +struct nvme_stats { + uint64_t units_read; + uint64_t units_written; + uint64_t read_commands; + uint64_t write_commands; +}; + +static void nvme_set_blk_stats(NvmeNamespace *ns, struct nvme_stats *stats) +{ + BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); + + stats->units_read += s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS; + stats->units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS; + stats->read_commands += s->nr_ops[BLOCK_ACCT_READ]; + stats->write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; +} + static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len, uint64_t off, NvmeRequest *req) { uint32_t nsid = le32_to_cpu(req->cmd.nsid); - + struct nvme_stats stats = { 0 }; + NvmeSmartLog smart = { 0 }; uint32_t trans_len; + NvmeNamespace *ns; time_t current_ms; - uint64_t units_read = 0, units_written = 0; - uint64_t read_commands = 0, write_commands = 0; - NvmeSmartLog smart; - - if (nsid && nsid != 0xffffffff) { - return NVME_INVALID_FIELD | NVME_DNR; - } if (off >= sizeof(smart)) { return NVME_INVALID_FIELD | NVME_DNR; } - for (int i = 1; i <= n->num_namespaces; i++) { - NvmeNamespace *ns = nvme_ns(n, i); - if (!ns) { - continue; - } - - BlockAcctStats *s = blk_get_stats(ns->blkconf.blk); + if (nsid != 0xffffffff) { + ns = nvme_ns(n, nsid); + if (!ns) + return NVME_INVALID_NSID | NVME_DNR; + nvme_set_blk_stats(ns, &stats); + } else { + int i; - units_read += s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS; - units_written += s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS; - read_commands += s->nr_ops[BLOCK_ACCT_READ]; - write_commands += s->nr_ops[BLOCK_ACCT_WRITE]; + for (i = 1; i <= n->num_namespaces; i++) { + ns = nvme_ns(n, i); + if (!ns) { + continue; + } + nvme_set_blk_stats(ns, &stats); + } } trans_len = MIN(sizeof(smart) - off, buf_len); - memset(&smart, 0x0, sizeof(smart)); - - smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(units_read, 1000)); - smart.data_units_written[0] = cpu_to_le64(DIV_ROUND_UP(units_written, + smart.data_units_read[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_read, + 1000)); + smart.data_units_written[0] = cpu_to_le64(DIV_ROUND_UP(stats.units_written, 1000)); - smart.host_read_commands[0] = cpu_to_le64(read_commands); - smart.host_write_commands[0] = cpu_to_le64(write_commands); + smart.host_read_commands[0] = cpu_to_le64(stats.read_commands); + smart.host_write_commands[0] = cpu_to_le64(stats.write_commands); smart.temperature = cpu_to_le16(n->temperature); @@ -2708,7 +2722,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) id->acl = 3; id->aerl = n->params.aerl; id->frmw = (NVME_NUM_FW_SLOTS << 1) | NVME_FRMW_SLOT1_RO; - id->lpa = NVME_LPA_EXTENDED; + id->lpa = NVME_LPA_NS_SMART | NVME_LPA_EXTENDED; /* recommended default value (~70 C) */ id->wctemp = cpu_to_le16(NVME_TEMPERATURE_WARNING); diff --git a/include/block/nvme.h b/include/block/nvme.h index 58647bcdad..868cf53f0b 100644 --- a/include/block/nvme.h +++ b/include/block/nvme.h @@ -849,6 +849,7 @@ enum NvmeIdCtrlFrmw { }; enum NvmeIdCtrlLpa { + NVME_LPA_NS_SMART = 1 << 0, NVME_LPA_EXTENDED = 1 << 2, };
Let the user specify a specific namespace if they want to get access stats for a specific namespace. Signed-off-by: Keith Busch <kbusch@kernel.org> --- hw/block/nvme.c | 66 +++++++++++++++++++++++++++----------------- include/block/nvme.h | 1 + 2 files changed, 41 insertions(+), 26 deletions(-)