| Message ID | 20220603065536.5641-10-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use block pr_ops in LIO | expand |
On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote: > @@ -171,6 +171,7 @@ static const struct { > /* zone device specific errors */ > [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, > [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, > + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, You misspelled "reservation". :) And since you want a different error, why reuse EBADE for the errno? That is already used for BLK_STS_NEXUS that you're trying to differentiate from, right? At least for nvme, this error code is returned when the host lacks sufficient rights, so something like EACCESS might make sense. Looks good otherwise. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 6/3/22 2:45 PM, Keith Busch wrote: > On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote: >> @@ -171,6 +171,7 @@ static const struct { >> /* zone device specific errors */ >> [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, >> [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, >> + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, > > You misspelled "reservation". :) Will fix. > > And since you want a different error, why reuse EBADE for the errno? That is > already used for BLK_STS_NEXUS that you're trying to differentiate from, right? > At least for nvme, this error code is returned when the host lacks sufficient > rights, so something like EACCESS might make sense. > Ah ok I might have misuderstood the reason/usage of the -Exyz error. The patches in this set use the pr_ops in the kernel so I can see the BLK_STS value. We do bio based IO so we get that value in the end io callback. I thought the -Exyx error can get returned to userspace. Because scsi and nvme currently return -EBADE for reservation conflicts I thought I had to keep doing that. If that's not the case, then yeah -EACCESS is better and I'll definitely use it. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 6/3/22 21:45, Keith Busch wrote: > On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote: >> @@ -171,6 +171,7 @@ static const struct { >> /* zone device specific errors */ >> [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, >> [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, >> + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, > > You misspelled "reservation". :) > > And since you want a different error, why reuse EBADE for the errno? That is > already used for BLK_STS_NEXUS that you're trying to differentiate from, right? > At least for nvme, this error code is returned when the host lacks sufficient > rights, so something like EACCESS might make sense. > > Looks good otherwise. Welll ... BLK_STS_NEXUS _is_ the reservation error. I'd rather modify the existing code to return BLK_STS_RSV_CONFLICT instead of BLK_STS_NEXUS, but keep the 'EBADE' mapping. Userspace ABI and all that. Cheers, Hannes
On 6/4/22 2:38 AM, Hannes Reinecke wrote: > On 6/3/22 21:45, Keith Busch wrote: >> On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote: >>> @@ -171,6 +171,7 @@ static const struct { >>> /* zone device specific errors */ >>> [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, >>> [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, >>> + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, >> >> You misspelled "reservation". :) >> >> And since you want a different error, why reuse EBADE for the errno? That is >> already used for BLK_STS_NEXUS that you're trying to differentiate from, right? >> At least for nvme, this error code is returned when the host lacks sufficient >> rights, so something like EACCESS might make sense. >> >> Looks good otherwise. > > Welll ... BLK_STS_NEXUS _is_ the reservation error. I was not sure of xen/virtio scsi uses of BLK_STS_NEXUS/DID_NEXUS_FAILURE. The virtio spec's description for VIRTIO_SCSI_S_NEXUS_FAILURE: if the nexus is suffering a failure but retrying on other paths might yield a different result. looks like the description for DID_NEXUS_FAILURE in scsi_status.h. To me the the description sounded generic where it could used for other errors like the endpoint/port for the I_T is removed. However, the qemu code only uses VIRTIO_SCSI_S_NEXUS_FAILURE for reservation conflicts. If we are saying that is always the case in other virt implementations, I don't even need this patch :) and we can do what you requested and do more of a rename. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 6/2/22 23:55, Mike Christie wrote: > BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also > general nexus failures. For the pr_ops use we want to know if an IO failed > for specifically a reservation conflict so we can report that error upwards > to a VM. This patch adds a new error code for this case and converts nvme. > The next patch converts scsi because it's a little more complicated. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > --- > block/blk-core.c | 1 + > drivers/nvme/host/core.c | 2 +- > include/linux/blk_types.h | 4 ++++ > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index bc0506772152..3908ac4a70b6 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -171,6 +171,7 @@ static const struct { > /* zone device specific errors */ > [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, > [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, > + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, ^^^^^^^^^^ Please fix the spelling of "reservation". Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 6/4/22 19:13, michael.christie@oracle.com wrote: > On 6/4/22 2:38 AM, Hannes Reinecke wrote: >> On 6/3/22 21:45, Keith Busch wrote: >>> On Fri, Jun 03, 2022 at 01:55:34AM -0500, Mike Christie wrote: >>>> @@ -171,6 +171,7 @@ static const struct { >>>> /* zone device specific errors */ >>>> [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, >>>> [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, >>>> + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, >>> >>> You misspelled "reservation". :) >>> >>> And since you want a different error, why reuse EBADE for the errno? That is >>> already used for BLK_STS_NEXUS that you're trying to differentiate from, right? >>> At least for nvme, this error code is returned when the host lacks sufficient >>> rights, so something like EACCESS might make sense. >>> >>> Looks good otherwise. >> >> Welll ... BLK_STS_NEXUS _is_ the reservation error. > > I was not sure of xen/virtio scsi uses of BLK_STS_NEXUS/DID_NEXUS_FAILURE. > The virtio spec's description for VIRTIO_SCSI_S_NEXUS_FAILURE: > > if the nexus is suffering a failure but retrying on other paths might > yield a different result. > > looks like the description for DID_NEXUS_FAILURE in scsi_status.h. > To me the the description sounded generic where it could used for > other errors like the endpoint/port for the I_T is removed. > > However, the qemu code only uses VIRTIO_SCSI_S_NEXUS_FAILURE for > reservation conflicts. If we are saying that is always the case in > other virt implementations, I don't even need this patch :) and we > can do what you requested and do more of a rename. Well ... we tried to find a generic error for reservation failure, as we thought that reservation failure was too SCSI specific. And we wanted the error to describe what the resulting handling should be, not what the cause was. Hence we ended up with BLK_STS_NEXUS. But turns out that our initial assumption wasn't valid, and that reservations are a general concept. So by all means, rename BLK_STS_NEXUS to BLK_STS_RSV_CONFLICT to make it clear what this error is about. Cheers, Hannes
On Sun, Jun 05, 2022 at 11:42:11AM +0200, Hannes Reinecke wrote: > Well ... we tried to find a generic error for reservation failure, as we > thought that reservation failure was too SCSI specific. > And we wanted the error to describe what the resulting handling should be, > not what the cause was. Hence we ended up with BLK_STS_NEXUS. > > But turns out that our initial assumption wasn't valid, and that > reservations are a general concept. So by all means, rename BLK_STS_NEXUS > to BLK_STS_RSV_CONFLICT to make it clear what this error is about. I think think this is a good ida, but we'll need to involve the s390 dasd folks. Maybe do this as a separate prep patch? While thinking about DASD I think it would benefit from returning the blk_status_t from ->free_cp insted of the hand crafted conversion as well. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/block/blk-core.c b/block/blk-core.c index bc0506772152..3908ac4a70b6 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -171,6 +171,7 @@ static const struct { /* zone device specific errors */ [BLK_STS_ZONE_OPEN_RESOURCE] = { -ETOOMANYREFS, "open zones exceeded" }, [BLK_STS_ZONE_ACTIVE_RESOURCE] = { -EOVERFLOW, "active zones exceeded" }, + [BLK_STS_RSV_CONFLICT] = { -EBADE, "resevation conflict" }, /* everything else not covered above: */ [BLK_STS_IOERR] = { -EIO, "I/O" }, diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index e1846d04817f..9b77d8eb480c 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -268,7 +268,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_RSV_CONFLICT; case NVME_SC_HOST_PATH_ERROR: return BLK_STS_TRANSPORT; case NVME_SC_ZONE_TOO_MANY_ACTIVE: diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 1973ef9bd40f..927d9d30e1df 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -162,6 +162,9 @@ typedef u16 blk_short_t; */ #define BLK_STS_OFFLINE ((__force blk_status_t)17) +/* NVMe/SCSI reservation conflict. */ +#define BLK_STS_RSV_CONFLICT ((__force blk_status_t)18) + /** * blk_path_error - returns true if error may be path related * @error: status the request was completed with @@ -183,6 +186,7 @@ static inline bool blk_path_error(blk_status_t error) case BLK_STS_NEXUS: case BLK_STS_MEDIUM: case BLK_STS_PROTECTION: + case BLK_STS_RSV_CONFLICT: return false; }
BLK_STS_NEXUS is used for nvme/scsi reservation conflicts and also general nexus failures. For the pr_ops use we want to know if an IO failed for specifically a reservation conflict so we can report that error upwards to a VM. This patch adds a new error code for this case and converts nvme. The next patch converts scsi because it's a little more complicated. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- block/blk-core.c | 1 + drivers/nvme/host/core.c | 2 +- include/linux/blk_types.h | 4 ++++ 3 files changed, 6 insertions(+), 1 deletion(-)