Re: [PATCH v3 12/19] block,nvme,scsi,dm: Add blk_status to pr_ops callouts

From: Mike Christie <michael.christie@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: bvanassche@acm.org, martin.petersen@oracle.com,
	linux-scsi@vger.kernel.org,
	james.bottomley@hansenpartnership.com,
	linux-block@vger.kernel.org, dm-devel@redhat.com,
	snitzer@kernel.org, axboe@kernel.dk,
	linux-nvme@lists.infradead.org, chaitanyak@nvidia.com,
	kbusch@kernel.org, target-devel@vger.kernel.org
Subject: Re: [PATCH v3 12/19] block,nvme,scsi,dm: Add blk_status to pr_ops callouts
Date: Sun, 30 Oct 2022 18:05:35 -0500	[thread overview]
Message-ID: <a7e447b5-b26e-7aa0-ab6a-3463b8497d26@oracle.com> (raw)
In-Reply-To: <20221030082020.GC4774@lst.de>

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;
 }
 
