diff mbox series

[v4,1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

Message ID 20210628095210.26249-2-mwilck@suse.com (mailing list archive)
State Superseded
Headers show
Series scsi/dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath | expand

Commit Message

Martin Wilck June 28, 2021, 9:52 a.m. UTC 
From: Martin Wilck <mwilck@suse.com>

This makes it possible to use scsi_result_to_blk_status() from
code that shouldn't depend on scsi_mod (e.g. device mapper).

scsi_ioctl.c is selected by CONFIG_BLK_SCSI_REQUEST, which is automatically
selected by CONFIG_SCSI.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c      | 47 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_lib.c | 24 +--------------------
 include/linux/blkdev.h  |  1 +
 3 files changed, 49 insertions(+), 23 deletions(-)

Comments

hch@lst.de June 28, 2021, 9:53 a.m. UTC | #1 
On Mon, Jun 28, 2021 at 11:52:08AM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This makes it possible to use scsi_result_to_blk_status() from
> code that shouldn't depend on scsi_mod (e.g. device mapper).

This really has no business being used outside of low-level SCSI code.
Martin Wilck June 28, 2021, 2:57 p.m. UTC | #2 
Hello Christoph,

On Mo, 2021-06-28 at 11:53 +0200, Christoph Hellwig wrote:
> On Mon, Jun 28, 2021 at 11:52:08AM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > This makes it possible to use scsi_result_to_blk_status() from
> > code that shouldn't depend on scsi_mod (e.g. device mapper).
> 
> This really has no business being used outside of low-level SCSI
> code.

And this is where my patch set uses it. Can you recommend a better
way how to access this algorithm, without making dm_mod.ko or dm-
mpath.ko depend on scsi_mod.ko, and without open-coding it
independently in a different code path?

The sg_io-on-multipath code needs to answer the question "is this a
path or a target error?". Therefore it calls blk_path_error(), which
requires obtaining a blk_status_t first. But that's not available in
the sg_io code path. So how should I deal with this situation?

The first approach - inlining scsi_result_to_blk_status() - has been
rejected before.

Regards,
Martin
hch@lst.de June 29, 2021, 12:59 p.m. UTC | #3 
On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> Hello Christoph,
> 
> On Mo, 2021-06-28 at 11:53 +0200, Christoph Hellwig wrote:
> > On Mon, Jun 28, 2021 at 11:52:08AM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > This makes it possible to use scsi_result_to_blk_status() from
> > > code that shouldn't depend on scsi_mod (e.g. device mapper).
> > 
> > This really has no business being used outside of low-level SCSI
> > code.
> 
> And this is where my patch set uses it. Can you recommend a better
> way how to access this algorithm, without making dm_mod.ko or dm-
> mpath.ko depend on scsi_mod.ko, and without open-coding it
> independently in a different code path?

> The sg_io-on-multipath code needs to answer the question "is this a
> path or a target error?". Therefore it calls blk_path_error(), which
> requires obtaining a blk_status_t first. But that's not available in
> the sg_io code path. So how should I deal with this situation?

Not by exporting random crap from the SCSI code.
Martin Wilck June 29, 2021, 7:23 p.m. UTC | #4 
On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:
> On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> 
> > The sg_io-on-multipath code needs to answer the question "is this a
> > path or a target error?". Therefore it calls blk_path_error(),
> > which
> > requires obtaining a blk_status_t first. But that's not available
> > in
> > the sg_io code path. So how should I deal with this situation?
> 
> Not by exporting random crap from the SCSI code.

So, you'd prefer inlining scsi_result_to_blk_status()?

Thanks,
Martin
Keith Busch June 29, 2021, 9:23 p.m. UTC | #5 
On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:
> On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:
> > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> > 
> > > The sg_io-on-multipath code needs to answer the question "is this a
> > > path or a target error?". Therefore it calls blk_path_error(),
> > > which
> > > requires obtaining a blk_status_t first. But that's not available
> > > in
> > > the sg_io code path. So how should I deal with this situation?
> > 
> > Not by exporting random crap from the SCSI code.
> 
> So, you'd prefer inlining scsi_result_to_blk_status()?

I don't think you need to. The only scsi_result_to_blk_status() caller
is sg_io_to_blk_status(). That's already in the same file as
scsi_result_to_blk_status() so no need to export it. You could even make
it static.
Martin Wilck June 30, 2021, 8:12 a.m. UTC | #6 
On Di, 2021-06-29 at 14:23 -0700, Keith Busch wrote:
> On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:
> > On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:
> > > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> > > 
> > > > The sg_io-on-multipath code needs to answer the question "is
> > > > this a
> > > > path or a target error?". Therefore it calls blk_path_error(),
> > > > which
> > > > requires obtaining a blk_status_t first. But that's not
> > > > available
> > > > in
> > > > the sg_io code path. So how should I deal with this situation?
> > > 
> > > Not by exporting random crap from the SCSI code.
> > 
> > So, you'd prefer inlining scsi_result_to_blk_status()?
> 
> I don't think you need to. The only scsi_result_to_blk_status()
> caller
> is sg_io_to_blk_status(). That's already in the same file as
> scsi_result_to_blk_status() so no need to export it. You could even
> make
> it static.

Thanks for your suggestion. I'd be lucky if this was true. But the most
important users of scsi_result_to_blk_status() are in scsi_lib.c
(scsi_io_completion_action(), scsi_io_completion_nz_result()).

If I move scsi_result_to_blk_status() to vmlinux without exporting it,
it won't be available in the SCSI core any more, at least not with
CONFIG_SCSI=m.

Regards,
Martin
Mike Snitzer June 30, 2021, 4:01 p.m. UTC | #7 
On Wed, Jun 30 2021 at  4:12P -0400,
Martin Wilck <mwilck@suse.com> wrote:

> On Di, 2021-06-29 at 14:23 -0700, Keith Busch wrote:
> > On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:
> > > On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:
> > > > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> > > > 
> > > > > The sg_io-on-multipath code needs to answer the question "is
> > > > > this a
> > > > > path or a target error?". Therefore it calls blk_path_error(),
> > > > > which
> > > > > requires obtaining a blk_status_t first. But that's not
> > > > > available
> > > > > in
> > > > > the sg_io code path. So how should I deal with this situation?
> > > > 
> > > > Not by exporting random crap from the SCSI code.

Helpful as always Christoph... ;)

> > > So, you'd prefer inlining scsi_result_to_blk_status()?
> > 
> > I don't think you need to. The only scsi_result_to_blk_status()
> > caller
> > is sg_io_to_blk_status(). That's already in the same file as
> > scsi_result_to_blk_status() so no need to export it. You could even
> > make
> > it static.
> 
> Thanks for your suggestion. I'd be lucky if this was true. But the most
> important users of scsi_result_to_blk_status() are in scsi_lib.c
> (scsi_io_completion_action(), scsi_io_completion_nz_result()).
> 
> If I move scsi_result_to_blk_status() to vmlinux without exporting it,
> it won't be available in the SCSI core any more, at least not with
> CONFIG_SCSI=m.

For what you're trying to accomplish with this patchset, you only need
sg_io_to_blk_status() exported.

So check with Martin and/or Bart on the best way to give
sg_io_to_blk_status() access to the equivalent of your
__scsi_result_to_blk_status() without exporting it.

I'd start by just folding patches 1 and 2, fixing up the logic Dan
Carpenter pointed ouit, and only exporting sg_io_to_blk_status().

Mike
Martin Wilck June 30, 2021, 4:54 p.m. UTC | #8 
On Mi, 2021-06-30 at 12:01 -0400, Mike Snitzer wrote:
> On Wed, Jun 30 2021 at  4:12P -0400,
> Martin Wilck <mwilck@suse.com> wrote:
> > 
> > Thanks for your suggestion. I'd be lucky if this was true. But the
> > most
> > important users of scsi_result_to_blk_status() are in scsi_lib.c
> > (scsi_io_completion_action(), scsi_io_completion_nz_result()).
> > 
> > If I move scsi_result_to_blk_status() to vmlinux without exporting
> > it,
> > it won't be available in the SCSI core any more, at least not with
> > CONFIG_SCSI=m.
> 
> For what you're trying to accomplish with this patchset, you only
> need
> sg_io_to_blk_status() exported.
> 
> So check with Martin and/or Bart on the best way to give
> sg_io_to_blk_status() access to the equivalent of your
> __scsi_result_to_blk_status() without exporting it.
> 
> I'd start by just folding patches 1 and 2, fixing up the logic Dan
> Carpenter pointed ouit, and only exporting sg_io_to_blk_status().

Thank you! FTR, the issue Dan Carpenter reported was already fixed in
v5 of this patch set.

@Martin, @Bart, do you have a suggestion for me?

Thanks,
Martin
Bart Van Assche June 30, 2021, 10:08 p.m. UTC | #9 
On 6/30/21 9:54 AM, Martin Wilck wrote:
> @Martin, @Bart, do you have a suggestion for me?

The code in block/scsi_ioctl.c exists in the block layer since until
recently most of it was used by both the IDE and SCSI code. Now that
drivers/ide is gone (thanks Christoph!), how about moving block/bsg.c
and block/scsi_ioctl.c to drivers/scsi/? As far as I can see the BSG
code is only used by SCSI drivers:

$ git grep -nH BLK_DEV_BSG | grep /Kconfig
block/Kconfig:38:config BLK_DEV_BSG
block/Kconfig:57:config BLK_DEV_BSGLIB
block/Kconfig:59:	select BLK_DEV_BSG
drivers/scsi/Kconfig:231:	select BLK_DEV_BSGLIB
drivers/scsi/Kconfig:241:	select BLK_DEV_BSGLIB
drivers/scsi/Kconfig:250:	select BLK_DEV_BSGLIB
drivers/scsi/libsas/Kconfig:13:	select BLK_DEV_BSGLIB
drivers/scsi/ufs/Kconfig:150:	select BLK_DEV_BSGLIB

The block/scsi_ioctl.c code is used more widely however:

$ git grep -nH BLK_SCSI_REQUEST | grep /Kconfig
block/Kconfig:32:config BLK_SCSI_REQUEST
block/Kconfig:41:	select BLK_SCSI_REQUEST
block/Kconfig:60:	select BLK_SCSI_REQUEST
drivers/block/Kconfig:77:	select BLK_SCSI_REQUEST
drivers/block/Kconfig:309:	select BLK_SCSI_REQUEST
drivers/block/paride/Kconfig:30:	select BLK_SCSI_REQUEST
drivers/scsi/Kconfig:22:	select BLK_SCSI_REQUEST
drivers/target/Kconfig:8:	select BLK_SCSI_REQUEST
fs/nfsd/Kconfig:112:	select BLK_SCSI_REQUEST

Bart.
hch@lst.de July 1, 2021, 6:19 a.m. UTC | #10 
On Wed, Jun 30, 2021 at 03:08:11PM -0700, Bart Van Assche wrote:
> On 6/30/21 9:54 AM, Martin Wilck wrote:
> > @Martin, @Bart, do you have a suggestion for me?
> 
> The code in block/scsi_ioctl.c exists in the block layer since until
> recently most of it was used by both the IDE and SCSI code. Now that
> drivers/ide is gone (thanks Christoph!), how about moving block/bsg.c
> and block/scsi_ioctl.c to drivers/scsi/? As far as I can see the BSG
> code is only used by SCSI drivers:

I have a series to clean this mess up:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/scsi-ioctl

But more importantly, dm has no business interpreting the errors.  Just
like how SG_IO processing generally does not look at the error and
just returns it to userspace and leaves error handling to the caller
so should be the decision to resubmit it in a multi-path setup.
Paolo Bonzini July 6, 2021, 4:40 p.m. UTC | #11 
On 01/07/21 08:19, Christoph Hellwig wrote:
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/scsi-ioctl
> 
> But more importantly, dm has no business interpreting the errors.  Just
> like how SG_IO processing generally does not look at the error and
> just returns it to userspace and leaves error handling to the caller
> so should be the decision to resubmit it in a multi-path setup.

The problem is that userspace does not have a way to direct the command 
to a different path in the resubmission.  It may not even have 
permission to issue DM_TABLE_STATUS, or to access the /dev nodes for the 
underlying paths, so without Martin's patches SG_IO on dm-mpath is 
basically unreliable by design.

Paolo
diff mbox series

Patch

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index fa6df11b8bdd..19b63b64ecbc 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -882,6 +882,53 @@  void scsi_req_init(struct scsi_request *req)
 }
 EXPORT_SYMBOL(scsi_req_init);
 
+/* See set_host_byte() in include/scsi/scsi_cmnd.h */
+static void __set_host_byte(int *result, char status)
+{
+	*result = (*result & 0xff00ffff) | (status << 16);
+}
+
+/**
+ * __scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
+ * @cmd:	SCSI command
+ * @cmd_result: pointer to scsi cmnd result code to be possibly changed
+ *
+ * Translate a SCSI result code into a blk_status_t value. May reset the host
+ * byte of @cmd_result.
+ */
+blk_status_t __scsi_result_to_blk_status(int *cmd_result, int result)
+{
+	switch (host_byte(result)) {
+	case DID_OK:
+		/*
+		 * Also check the other bytes than the status byte in result
+		 * to handle the case when a SCSI LLD sets result to
+		 * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
+		 */
+		if (scsi_status_is_good(result))
+			return BLK_STS_OK;
+		return BLK_STS_IOERR;
+	case DID_TRANSPORT_FAILFAST:
+	case DID_TRANSPORT_MARGINAL:
+		return BLK_STS_TRANSPORT;
+	case DID_TARGET_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_TARGET;
+	case DID_NEXUS_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_NEXUS;
+	case DID_ALLOC_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_NOSPC;
+	case DID_MEDIUM_ERROR:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_MEDIUM;
+	default:
+		return BLK_STS_IOERR;
+	}
+}
+EXPORT_SYMBOL(__scsi_result_to_blk_status);
+
 static int __init blk_scsi_ioctl_init(void)
 {
 	blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6b994baf87c2..2c0eca3693af 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -589,29 +589,7 @@  static bool scsi_end_request(struct request *req, blk_status_t error,
  */
 static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 {
-	switch (host_byte(result)) {
-	case DID_OK:
-		if (scsi_status_is_good(result))
-			return BLK_STS_OK;
-		return BLK_STS_IOERR;
-	case DID_TRANSPORT_FAILFAST:
-	case DID_TRANSPORT_MARGINAL:
-		return BLK_STS_TRANSPORT;
-	case DID_TARGET_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_TARGET;
-	case DID_NEXUS_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NEXUS;
-	case DID_ALLOC_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NOSPC;
-	case DID_MEDIUM_ERROR:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_MEDIUM;
-	default:
-		return BLK_STS_IOERR;
-	}
+	return __scsi_result_to_blk_status(&cmd->result, result);
 }
 
 /* Helper for scsi_io_completion() when "reprep" action required. */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1255823b2bc0..48497a77428d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -2021,4 +2021,5 @@  int fsync_bdev(struct block_device *bdev);
 int freeze_bdev(struct block_device *bdev);
 int thaw_bdev(struct block_device *bdev);
 
+blk_status_t __scsi_result_to_blk_status(int *cmd_result, int result);
 #endif /* _LINUX_BLKDEV_H */