On 10/30/22 3:20 AM, Christoph Hellwig wrote:
> On Wed, Oct 26, 2022 at 06:19:38PM -0500, Mike Christie wrote:
>> To handle both cases and keep userspace compatibility, this patch adds a
>> blk_status_t arg to the pr_ops callouts. The lower levels will convert
>> their device specific error to the blk_status_t then the upper levels
>> can easily check that code without knowing the device type. Adding the
>> extra return value will then allow us to not break userspace which expects
>> a negative -Exyz error code if the command fails before it's sent to the
>> device or a device/driver specific value if the error is > 0.
>
> I really hate this double error return. What -E* statuses that matter
> can be returned without a BLK_STS_* equivalent that we couldn't convert
> to and from?
Hey,
I didn't fully understand the question. The specific issue I'm hitting is
that if a scsi/nvme device returns SAM_STAT_RESERVATION_CONFLICT/
NVME_SC_RESERVATION_CONFLICT then in lio I need to be able to detect that
and return SAM_STAT_RESERVATION_CONFLICT. So I only care about
-EBADE/BLK_STS_NEXUS right now. So are you asking about -EBADE?
Do you mean I could have sd_pr_out_command return -EBADE when it gets
a SAM_STAT_RESERVATION_CONFLICT (nvme would do the equivalent). Then in
lio do:
ret = ops->pr_register()
if (ret == -EBADE)
return SAM_STAT_RESERVATION_CONFLICT;
The problem I hit is that in the ioctl code I then have to do:
@@ -269,7 +270,14 @@ static int blkdev_pr_register(struct block_device *bdev,
if (reg.flags & ~PR_FL_IGNORE_KEY)
return -EOPNOTSUPP;
- return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+ ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+ if (ret == -EBADE) {
+ if (bdev_is_nvme(bdev))
+ ret = NVME_SC_RESERVATION_CONFLICT;
+ else if (bdev_is_scsi(bdev)
+ ret = SAM_STAT_RESERVATION_CONFLICT;
+ }
+ return ret;
}
or I could convert the scsi/nvme or code to always use BLK_STS errors.
In LIO I can easily check for BLK_STS_NEXUS like with the -EBADE example. In
the ioctl code then for common errors I can go from BLK_STS using the
blk_status_to_errno helper. However, for some scsi/nvme specific errors we
would want to do:
@@ -269,7 +270,36 @@ static int blkdev_pr_register(struct block_device *bdev,
if (reg.flags & ~PR_FL_IGNORE_KEY)
return -EOPNOTSUPP;
- return ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+ ret = ops->pr_register(bdev, reg.old_key, reg.new_key, reg.flags);
+ switch (ret) {
+ /* there could be nvme/scsi helper functions for this which would
+ * be the reverse of nvme_error_status/ */
+ case BLK_STS_NEXUS:
+ if (bdev_is_nvme(bdev))
+ ret = NVME_SC_RESERVATION_CONFLICT;
+ else if (bdev_is_scsi(bdev)
+ ret = SAM_STAT_RESERVATION_CONFLICT;
+ break;
+ case BLK_STS_TRANSPORT:
+ if (bdev_is_nvme(bdev))
+ ret = NVME_SC_HOST_PATH_ERROR;
+ else if (bdev_is_scsi(bdev)
+ ret = DID_TRANSPORT_FAILFAST or DID_TRANSPORT_MARGINAL;
+ break;
+ case BLK_STS_NOTSUPP:
+ if (bdev_is_nvme(bdev))
+ ret = NVME_SC_BAD_ATTRIBUTES or
+ NVME_SC_ONCS_NOT_SUPPORTED or
+ NVME_SC_INVALID_OPCODE or
+ NVME_SC_INVALID_FIELD or
+ NVME_SC_INVALID_NS
+ else if (bdev_is_scsi(bdev)
+ ret = We don't have a way to support this in SCSI yet
because it would be in the sense/asc/ascq.
+ break;
+ default:
+ ret = blk_status_to_errno(ret);
+ }
+ return ret;
}
