| Message ID | 20221109031106.201324-4-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/scsi/nvme: Add error codes for PR ops | expand |
On Tue, Nov 08, 2022 at 09:11:06PM -0600, Mike Christie wrote: > + case NVME_SC_ONCS_NOT_SUPPORTED: > + sts = PR_STS_OP_NOT_SUPP; > + break; > + case NVME_SC_BAD_ATTRIBUTES: > + case NVME_SC_INVALID_OPCODE: > + case NVME_SC_INVALID_FIELD: > + case NVME_SC_INVALID_NS: > + sts = PR_STS_OP_INVALID; > + break; Second thoughts on these: shouldn't we just return negative Linux errnos here?
On 2022/11/9 11:11, Mike Christie wrote: > This converts the NVMe errors we could see during PR handling to PT_STS > errors, so pr_ops callers can handle scsi and nvme errors without knowing > the device types. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > --- > drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index dc4220600585..8f0177045a2f 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c, > return nvme_submit_sync_cmd(ns->queue, c, data, 16); > } > > +static enum pr_status nvme_sc_to_pr_status(int nvme_sc) > +{ > + enum pr_status sts; > + > + switch (nvme_sc) { > + case NVME_SC_SUCCESS: > + sts = PR_STS_SUCCESS; > + break; > + case NVME_SC_RESERVATION_CONFLICT: > + sts = PR_STS_RESERVATION_CONFLICT; > + break; > + case NVME_SC_HOST_PATH_ERROR: > + sts = PR_STS_PATH_FAILED; All path-related errors should be considered.
On 11/9/22 12:53 AM, Christoph Hellwig wrote: > On Tue, Nov 08, 2022 at 09:11:06PM -0600, Mike Christie wrote: >> + case NVME_SC_ONCS_NOT_SUPPORTED: >> + sts = PR_STS_OP_NOT_SUPP; >> + break; >> + case NVME_SC_BAD_ATTRIBUTES: >> + case NVME_SC_INVALID_OPCODE: >> + case NVME_SC_INVALID_FIELD: >> + case NVME_SC_INVALID_NS: >> + sts = PR_STS_OP_INVALID; >> + break; > > Second thoughts on these: shouldn't we just return negative Linux > errnos here? I wasn't sure. I might have over thought it. I added the PR_STS error codes for those cases so a user could distinguish if the command was sent to the device and it reported it didn't support the command or the device determined it had an invalid field set. -EINVAL/-EOPNOTSUP would continue to work like it does now where we can get those errors if the drivers determined it didn't support a operation or field or it thought we had an invalid setting. There is no specific error case I was hitting. I was just thinking it's nice for userspace to be able to do a PR op and if it got -EOPNOTSUP the driver didn't support the command and if it got PR_STS_OP_NOT_SUPP then the device didn't support it.
On 11/9/22 2:28 AM, Chao Leng wrote: > > > On 2022/11/9 11:11, Mike Christie wrote: >> This converts the NVMe errors we could see during PR handling to PT_STS >> errors, so pr_ops callers can handle scsi and nvme errors without knowing >> the device types. >> >> Signed-off-by: Mike Christie <michael.christie@oracle.com> >> --- >> drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 40 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index dc4220600585..8f0177045a2f 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c, >> return nvme_submit_sync_cmd(ns->queue, c, data, 16); >> } >> +static enum pr_status nvme_sc_to_pr_status(int nvme_sc) >> +{ >> + enum pr_status sts; >> + >> + switch (nvme_sc) { >> + case NVME_SC_SUCCESS: >> + sts = PR_STS_SUCCESS; >> + break; >> + case NVME_SC_RESERVATION_CONFLICT: >> + sts = PR_STS_RESERVATION_CONFLICT; >> + break; >> + case NVME_SC_HOST_PATH_ERROR: >> + sts = PR_STS_PATH_FAILED; > All path-related errors should be considered. Will do. Just one question. I didn't see NVME_SC_CTRL_PATH_ERROR and NVME_SC_INTERNAL_PATH_ERROR being used. Are they retryable errors?
On 2022/11/10 1:35, Mike Christie wrote: > On 11/9/22 2:28 AM, Chao Leng wrote: >> >> >> On 2022/11/9 11:11, Mike Christie wrote: >>> This converts the NVMe errors we could see during PR handling to PT_STS >>> errors, so pr_ops callers can handle scsi and nvme errors without knowing >>> the device types. >>> >>> Signed-off-by: Mike Christie <michael.christie@oracle.com> >>> --- >>> drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 40 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>> index dc4220600585..8f0177045a2f 100644 >>> --- a/drivers/nvme/host/core.c >>> +++ b/drivers/nvme/host/core.c >>> @@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c, >>> return nvme_submit_sync_cmd(ns->queue, c, data, 16); >>> } >>> +static enum pr_status nvme_sc_to_pr_status(int nvme_sc) >>> +{ >>> + enum pr_status sts; >>> + >>> + switch (nvme_sc) { >>> + case NVME_SC_SUCCESS: >>> + sts = PR_STS_SUCCESS; >>> + break; >>> + case NVME_SC_RESERVATION_CONFLICT: >>> + sts = PR_STS_RESERVATION_CONFLICT; >>> + break; >>> + case NVME_SC_HOST_PATH_ERROR: >>> + sts = PR_STS_PATH_FAILED; >> All path-related errors should be considered. > > Will do. Just one question. > > I didn't see NVME_SC_CTRL_PATH_ERROR and NVME_SC_INTERNAL_PATH_ERROR > being used. Are they retryable errors? These two types of errors depend on the implementation of the target. All in all, the request with path-related error should fail over to retry successfully.
On Wed, Nov 09, 2022 at 11:20:07AM -0600, Mike Christie wrote: > >> + case NVME_SC_BAD_ATTRIBUTES: > >> + case NVME_SC_INVALID_OPCODE: > >> + case NVME_SC_INVALID_FIELD: > >> + case NVME_SC_INVALID_NS: > >> + sts = PR_STS_OP_INVALID; > >> + break; > > > > Second thoughts on these: shouldn't we just return negative Linux > > errnos here? > > I wasn't sure. I might have over thought it. > > I added the PR_STS error codes for those cases so a user could > distinguish if the command was sent to the device and it > reported it didn't support the command or the device determined it > had an invalid field set. But does it matter if the device or the kernel doesn't support them? The result for the users is very much the same.
On 11/15/22 3:14 AM, Christoph Hellwig wrote: > On Wed, Nov 09, 2022 at 11:20:07AM -0600, Mike Christie wrote: >>>> + case NVME_SC_BAD_ATTRIBUTES: >>>> + case NVME_SC_INVALID_OPCODE: >>>> + case NVME_SC_INVALID_FIELD: >>>> + case NVME_SC_INVALID_NS: >>>> + sts = PR_STS_OP_INVALID; >>>> + break; >>> >>> Second thoughts on these: shouldn't we just return negative Linux >>> errnos here? >> >> I wasn't sure. I might have over thought it. >> >> I added the PR_STS error codes for those cases so a user could >> distinguish if the command was sent to the device and it >> reported it didn't support the command or the device determined it >> had an invalid field set. > > But does it matter if the device or the kernel doesn't support > them? The result for the users is very much the same. Yeah, makes sense.
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index dc4220600585..8f0177045a2f 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2104,11 +2104,43 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c, return nvme_submit_sync_cmd(ns->queue, c, data, 16); } +static enum pr_status nvme_sc_to_pr_status(int nvme_sc) +{ + enum pr_status sts; + + switch (nvme_sc) { + case NVME_SC_SUCCESS: + sts = PR_STS_SUCCESS; + break; + case NVME_SC_RESERVATION_CONFLICT: + sts = PR_STS_RESERVATION_CONFLICT; + break; + case NVME_SC_HOST_PATH_ERROR: + sts = PR_STS_PATH_FAILED; + break; + case NVME_SC_ONCS_NOT_SUPPORTED: + sts = PR_STS_OP_NOT_SUPP; + break; + case NVME_SC_BAD_ATTRIBUTES: + case NVME_SC_INVALID_OPCODE: + case NVME_SC_INVALID_FIELD: + case NVME_SC_INVALID_NS: + sts = PR_STS_OP_INVALID; + break; + default: + sts = PR_STS_IOERR; + break; + } + + return sts; +} + static int nvme_pr_command(struct block_device *bdev, u32 cdw10, u64 key, u64 sa_key, u8 op) { struct nvme_command c = { }; u8 data[16] = { 0, }; + int ret; put_unaligned_le64(key, &data[0]); put_unaligned_le64(sa_key, &data[8]); @@ -2118,8 +2150,14 @@ static int nvme_pr_command(struct block_device *bdev, u32 cdw10, if (IS_ENABLED(CONFIG_NVME_MULTIPATH) && bdev->bd_disk->fops == &nvme_ns_head_ops) - return nvme_send_ns_head_pr_command(bdev, &c, data); - return nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c, data); + ret = nvme_send_ns_head_pr_command(bdev, &c, data); + else + ret = nvme_send_ns_pr_command(bdev->bd_disk->private_data, &c, + data); + if (ret < 0) + return ret; + + return nvme_sc_to_pr_status(ret); } static int nvme_pr_register(struct block_device *bdev, u64 old,
This converts the NVMe errors we could see during PR handling to PT_STS errors, so pr_ops callers can handle scsi and nvme errors without knowing the device types. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/nvme/host/core.c | 42 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)