| Message ID | 20230224174502.321490-3-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v4,01/18] block: Add PR callouts for read keys and reservation | expand |
On Fri, Feb 24, 2023 at 11:44:46AM -0600, Mike Christie wrote: > BLK_STS_NEXUS is used for NVMe/SCSI reservation conflicts or in dasd's > case something similar. This renames BLK_STS_NEXUS so it better reflects > this. I like this rename a lot. > diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c > index a9c2a8d76c45..a2899d9690d4 100644 > --- a/drivers/s390/block/dasd.c > +++ b/drivers/s390/block/dasd.c > @@ -2723,7 +2723,7 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr) > else if (status == 0) { > switch (cqr->intrc) { > case -EPERM: > - error = BLK_STS_NEXUS; > + error = BLK_STS_RESV_CONFLICT; > break; But is this really a reservation conflict? Or should the DASD code maybe use a different error code here?
Am 14.03.23 um 18:11 schrieb Christoph Hellwig: > On Fri, Feb 24, 2023 at 11:44:46AM -0600, Mike Christie wrote: >> BLK_STS_NEXUS is used for NVMe/SCSI reservation conflicts or in dasd's >> case something similar. This renames BLK_STS_NEXUS so it better reflects >> this. > I like this rename a lot. > >> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c >> index a9c2a8d76c45..a2899d9690d4 100644 >> --- a/drivers/s390/block/dasd.c >> +++ b/drivers/s390/block/dasd.c >> @@ -2723,7 +2723,7 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr) >> else if (status == 0) { >> switch (cqr->intrc) { >> case -EPERM: >> - error = BLK_STS_NEXUS; >> + error = BLK_STS_RESV_CONFLICT; >> break; > But is this really a reservation conflict? Or should the DASD code > maybe use a different error code here? > This also fits for the DASD case. We use this error code for a reservation/locking conflict of the DASD device when the lock we previously held was stolen. Acked-by: Stefan Haberland <sth@linux.ibm.com>
On Wed, Mar 15, 2023 at 11:04:22AM +0100, Stefan Haberland wrote: > This also fits for the DASD case. We use this error code for a > reservation/locking conflict of the DASD device when the lock we > previously held was stolen. But that's not really a reservation conflict in the sense of the reservation API. Given that DASD doesn't support it it might not matter. Do you have applications that checks for the translated errno value? We'll probably at least want a comment documenting this status code.
Am 15.03.23 um 14:30 schrieb Christoph Hellwig: > On Wed, Mar 15, 2023 at 11:04:22AM +0100, Stefan Haberland wrote: >> This also fits for the DASD case. We use this error code for a >> reservation/locking conflict of the DASD device when the lock we >> previously held was stolen. > But that's not really a reservation conflict in the sense > of the reservation API. Given that DASD doesn't support it it > might not matter. Do you have applications that checks for > the translated errno value? We'll probably at least want > a comment documenting this status code. Well, I might completely misunderstand the use case for this error code. Sorry if that is the case. Beside that I thought that the return codes are generic blocklayer return codes and not bound to a specific API. I am not familiar with the reservation API you are talking about. What I understood from the reservation in NVMe context is that a namespace might be reserved to a host. If there is a conflict with this reservation this error code is provided for the IO request. For DASDs we have the possibility to reserve a disk for a host. If there is a conflict with this platform specific reservation we would present this error for an IO request. This sounded quite similar for me. I am completely open to using another return code and I am not aware of an application checking for this specific return code. Is there any that would fit better from your point of view?
On 3/16/23 5:17 AM, Stefan Haberland wrote: > Am 15.03.23 um 14:30 schrieb Christoph Hellwig: >> On Wed, Mar 15, 2023 at 11:04:22AM +0100, Stefan Haberland wrote: >>> This also fits for the DASD case. We use this error code for a >>> reservation/locking conflict of the DASD device when the lock we >>> previously held was stolen. >> But that's not really a reservation conflict in the sense >> of the reservation API. Given that DASD doesn't support it it >> might not matter. Do you have applications that checks for >> the translated errno value? We'll probably at least want >> a comment documenting this status code. > > Well, I might completely misunderstand the use case for this error code. > Sorry if that is the case. > > Beside that I thought that the return codes are generic blocklayer return codes > and not bound to a specific API. I am not familiar with the reservation API you > are talking about. > > What I understood from the reservation in NVMe context is that a namespace > might be reserved to a host. If there is a conflict with this reservation > this error code is provided for the IO request. > > For DASDs we have the possibility to reserve a disk for a host. If there is a > conflict with this platform specific reservation we would present this error > for an IO request. > > This sounded quite similar for me. > > I am completely open to using another return code and I am not aware of an > application checking for this specific return code. > > Is there any that would fit better from your point of view? > I think we are ok for dasd using BLK_STS_RESV_CONFLICT. It thought it sounded similar to SCSI/NVMe and userspace will still see -EBADE because the blk_status_to_errno/errno_to_blk_status will handle this. There was no internal dasd code checking for BLK_STS_NEXUS. There is a pr_ops API, but dasd is not hooked into it so we don't have to worry about behavior changes.
On Thu, Mar 16, 2023 at 11:36:12AM -0500, Mike Christie wrote: > I think we are ok for dasd using BLK_STS_RESV_CONFLICT. > > It thought it sounded similar to SCSI/NVMe and userspace will still > see -EBADE because the blk_status_to_errno/errno_to_blk_status will > handle this. > > There was no internal dasd code checking for BLK_STS_NEXUS. > > There is a pr_ops API, but dasd is not hooked into it so we don't > have to worry about behavior changes. Yes, we don't have to worry about it. I just find a bit confusing to have a PR-related error in a driver that doesn't use PRs. Maybe add a little comment that it is used for some s390 or DASD specific locking instead.
On 3/20/23 8:06 AM, Christoph Hellwig wrote: > On Thu, Mar 16, 2023 at 11:36:12AM -0500, Mike Christie wrote: >> I think we are ok for dasd using BLK_STS_RESV_CONFLICT. >> >> It thought it sounded similar to SCSI/NVMe and userspace will still >> see -EBADE because the blk_status_to_errno/errno_to_blk_status will >> handle this. >> >> There was no internal dasd code checking for BLK_STS_NEXUS. >> >> There is a pr_ops API, but dasd is not hooked into it so we don't >> have to worry about behavior changes. > > Yes, we don't have to worry about it. I just find a bit confusing > to have a PR-related error in a driver that doesn't use PRs. > > Maybe add a little comment that it is used for some s390 or DASD > specific locking instead. Ok.
diff --git a/block/blk-core.c b/block/blk-core.c index 82b5b2c53f1e..4439e68064a2 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -155,7 +155,7 @@ static const struct { [BLK_STS_NOSPC] = { -ENOSPC, "critical space allocation" }, [BLK_STS_TRANSPORT] = { -ENOLINK, "recoverable transport" }, [BLK_STS_TARGET] = { -EREMOTEIO, "critical target" }, - [BLK_STS_NEXUS] = { -EBADE, "critical nexus" }, + [BLK_STS_RESV_CONFLICT] = { -EBADE, "reservation conflict" }, [BLK_STS_MEDIUM] = { -ENODATA, "critical medium" }, [BLK_STS_PROTECTION] = { -EILSEQ, "protection" }, [BLK_STS_RESOURCE] = { -ENOMEM, "kernel resource" }, diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 8698410aeb84..ba6f1911f7ea 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -278,7 +278,7 @@ static blk_status_t nvme_error_status(u16 status) case NVME_SC_INVALID_PI: return BLK_STS_PROTECTION; case NVME_SC_RESERVATION_CONFLICT: - return BLK_STS_NEXUS; + return BLK_STS_RESV_CONFLICT; case NVME_SC_HOST_PATH_ERROR: return BLK_STS_TRANSPORT; case NVME_SC_ZONE_TOO_MANY_ACTIVE: diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c index a9c2a8d76c45..a2899d9690d4 100644 --- a/drivers/s390/block/dasd.c +++ b/drivers/s390/block/dasd.c @@ -2723,7 +2723,7 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr) else if (status == 0) { switch (cqr->intrc) { case -EPERM: - error = BLK_STS_NEXUS; + error = BLK_STS_RESV_CONFLICT; break; case -ENOLINK: error = BLK_STS_TRANSPORT; diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index abe93ec8b7d0..7cc7fb2d3359 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -598,7 +598,7 @@ static blk_status_t scsi_result_to_blk_status(int result) case SCSIML_STAT_OK: break; case SCSIML_STAT_RESV_CONFLICT: - return BLK_STS_NEXUS; + return BLK_STS_RESV_CONFLICT; case SCSIML_STAT_NOSPC: return BLK_STS_NOSPC; case SCSIML_STAT_MED_ERROR: diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 99be590f952f..2b2452086a2f 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -96,7 +96,7 @@ typedef u16 blk_short_t; #define BLK_STS_NOSPC ((__force blk_status_t)3) #define BLK_STS_TRANSPORT ((__force blk_status_t)4) #define BLK_STS_TARGET ((__force blk_status_t)5) -#define BLK_STS_NEXUS ((__force blk_status_t)6) +#define BLK_STS_RESV_CONFLICT ((__force blk_status_t)6) #define BLK_STS_MEDIUM ((__force blk_status_t)7) #define BLK_STS_PROTECTION ((__force blk_status_t)8) #define BLK_STS_RESOURCE ((__force blk_status_t)9) @@ -184,7 +184,7 @@ static inline bool blk_path_error(blk_status_t error) case BLK_STS_NOTSUPP: case BLK_STS_NOSPC: case BLK_STS_TARGET: - case BLK_STS_NEXUS: + case BLK_STS_RESV_CONFLICT: case BLK_STS_MEDIUM: case BLK_STS_PROTECTION: return false;
BLK_STS_NEXUS is used for NVMe/SCSI reservation conflicts or in dasd's case something similar. This renames BLK_STS_NEXUS so it better reflects this. Signed-off-by: Mike Christie <michael.christie@oracle.com> Cc: Stefan Haberland <sth@linux.ibm.com> Cc: Jan Hoeppner <hoeppner@linux.ibm.com> --- block/blk-core.c | 2 +- drivers/nvme/host/core.c | 2 +- drivers/s390/block/dasd.c | 2 +- drivers/scsi/scsi_lib.c | 2 +- include/linux/blk_types.h | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-)