| Message ID | 20230224174502.321490-3-michael.christie@oracle.com |
|---|---|
| State | Superseded |
| 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>
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(-)