| Message ID | 20241008145503.987195-1-m@bjorling.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | nvme: add rotational support | expand |
On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote: > From: Matias Bjørling <matias.bjorling@wdc.com> > > Enable support for NVMe devices that identifies as rotational. Thanks, this looks good to me. I still hope to see nvmet report this. It would be great to test this with HDD backed nvme-loop target.
Matias, > Enable support for NVMe devices that identifies as rotational. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote: > I still hope to see nvmet report this. It would be great to test this > with HDD backed nvme-loop target. I took the liberty to write one up. Looks like everything is reporting as expected. --- diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index 954d4c0747704..e167c9a2ff995 100644 --- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c @@ -685,6 +685,35 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req) nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm))); } +static void nvmet_execute_id_cs_indep(struct nvmet_req *req) +{ + struct nvme_id_ns_cs_indep *id; + u16 status; + + status = nvmet_req_find_ns(req); + if (status) + goto out; + + id = kzalloc(sizeof(*id), GFP_KERNEL); + if (!id) { + status = NVME_SC_INTERNAL; + goto out; + } + + id->nstat = NVME_NSTAT_NRDY; + id->anagrpid = req->ns->anagrpid; + id->nmic = NVME_NS_NMIC_SHARED; + if (req->ns->readonly) + id->nsattr |= NVME_NS_ATTR_RO; + if (req->ns->bdev && !bdev_nonrot(req->ns->bdev)) + id->nsfeat |= NVME_NS_ROTATIONAL; + + status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id)); + kfree(id); +out: + nvmet_req_complete(req, status); +} + static void nvmet_execute_identify(struct nvmet_req *req) { if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE)) @@ -729,6 +758,9 @@ static void nvmet_execute_identify(struct nvmet_req *req) break; } break; + case NVME_ID_CNS_NS_CS_INDEP: + nvmet_execute_id_cs_indep(req); + return; } pr_debug("unhandled identify cns %d on qid %d\n", --
On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote: > From: Matias Bjørling <matias.bjorling@wdc.com> > > Enable support for NVMe devices that identifies as rotational. > > Thanks to Keith, Damien, and Niklas for their feedback on the patchset. Hmm, the only previous version I've seen was the the RFCs from Wang Yugui, last seen in August. What the improvement over that version? Note that it also came with basic nvmet support which is kinda nice.
On 09-10-2024 00:04, Keith Busch wrote: > On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote: >> I still hope to see nvmet report this. It would be great to test this >> with HDD backed nvme-loop target. > > I took the liberty to write one up. Looks like everything is reporting > as expected. > > --- > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c > index 954d4c0747704..e167c9a2ff995 100644 > --- a/drivers/nvme/target/admin-cmd.c > +++ b/drivers/nvme/target/admin-cmd.c > @@ -685,6 +685,35 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req) > nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm))); > } > > +static void nvmet_execute_id_cs_indep(struct nvmet_req *req) > +{ > + struct nvme_id_ns_cs_indep *id; > + u16 status; > + > + status = nvmet_req_find_ns(req); > + if (status) > + goto out; > + > + id = kzalloc(sizeof(*id), GFP_KERNEL); > + if (!id) { > + status = NVME_SC_INTERNAL; > + goto out; > + } > + > + id->nstat = NVME_NSTAT_NRDY; > + id->anagrpid = req->ns->anagrpid; > + id->nmic = NVME_NS_NMIC_SHARED; > + if (req->ns->readonly) > + id->nsattr |= NVME_NS_ATTR_RO; > + if (req->ns->bdev && !bdev_nonrot(req->ns->bdev)) > + id->nsfeat |= NVME_NS_ROTATIONAL; > + > + status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id)); > + kfree(id); > +out: > + nvmet_req_complete(req, status); > +} > + > static void nvmet_execute_identify(struct nvmet_req *req) > { > if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE)) > @@ -729,6 +758,9 @@ static void nvmet_execute_identify(struct nvmet_req *req) > break; > } > break; > + case NVME_ID_CNS_NS_CS_INDEP: > + nvmet_execute_id_cs_indep(req); > + return; > } > > pr_debug("unhandled identify cns %d on qid %d\n", > -- That was quick! Nice. Would you like me to pack it up in the serie and resend?
On 09-10-2024 09:43, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote: >> From: Matias Bjørling <matias.bjorling@wdc.com> >> >> Enable support for NVMe devices that identifies as rotational. >> >> Thanks to Keith, Damien, and Niklas for their feedback on the patchset. > > Hmm, the only previous version I've seen was the the RFCs from > Wang Yugui, last seen in August. > > What the improvement over that version? Note that it also came > with basic nvmet support which is kinda nice. Ah, I made the patches without awareness of the earlier efforts. This version, together with Keith's nvmet patch, does not rely on setting/checking the CRIMS flag.
On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote: > > From: Matias Bjørling <matias.bjorling@wdc.com> > > > > Enable support for NVMe devices that identifies as rotational. > > > > Thanks to Keith, Damien, and Niklas for their feedback on the patchset. > > Hmm, the only previous version I've seen was the the RFCs from > Wang Yugui, last seen in August. Oops, that slipped by me as well. I think the right thing to do is bring that one forward and retain the original credit. I agree with Matias that we ought to be able to query the independent identification without relying on CRWMS, though.
On 09-10-2024 17:39, Keith Busch wrote: > On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote: >> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote: >>> From: Matias Bjørling <matias.bjorling@wdc.com> >>> >>> Enable support for NVMe devices that identifies as rotational. >>> >>> Thanks to Keith, Damien, and Niklas for their feedback on the patchset. >> >> Hmm, the only previous version I've seen was the the RFCs from >> Wang Yugui, last seen in August. > > Oops, that slipped by me as well. I think the right thing to do is bring > that one forward and retain the original credit. I agree with Matias > that we ought to be able to query the independent identification without > relying on CRWMS, though. Works for me. Yugui, are you okay with me posting the updated patch serie with your patch?
From: Matias Bjørling <matias.bjorling@wdc.com> Enable support for NVMe devices that identifies as rotational. Thanks to Keith, Damien, and Niklas for their feedback on the patchset. Matias Bjørling (2): nvme: make independent ns identify default nvme: add rotational support drivers/nvme/host/core.c | 12 ++++++++---- include/linux/nvme.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)