[PATCH v4 0/3] scsi/dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath

[PATCH v4 0/3] scsi/dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hello Mike, hello Martin,

here is v4 of my attempt to add retry logic to SG_IO on dm-multipath devices.

Regards
Martin

Changes v3->v4 (thanks to Mike Snitzer):

 - Added an additional helper function sg_io_to_blk_status() to
   scsi_ioctl.c, in order to avoid open-coding handling of the SCSI result
   code in device-mapper.

 - Added a new method dm_sg_io_ioctl_fn() in struct target_type, define
   only by the multipath target. This allows moving the bulk of the new
   code to dm-mpath.c, and avoids the wrong limitation of the code to
   request-based multipath.

Changes v2->v3:

 - un-inlined scsi_result_to_blk_status again, and move the helper
   __scsi_result_to_blk_status to block/scsi_ioctl.c instead
   (Bart v. Assche)
 - open-coded the status/msg/host/driver-byte -> result conversion
   where the standard SCSI helpers aren't usable (Bart v. Assche)
    
Changes v1->v2:

 - applied modifications from Mike Snitzer
 - moved SG_IO dependent code to a separate file, no scsi includes in
   dm.c any more
 - made the new code depend on a configuration option 
 - separated out scsi changes, made scsi_result_to_blk_status()
   inline to avoid dependency of dm_mod from scsi_mod (Paolo Bonzini)

Martin Wilck (3):
  scsi: scsi_ioctl: export __scsi_result_to_blk_status()
  scsi: scsi_ioctl: add sg_io_to_blk_status()
  dm mpath: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO

 block/scsi_ioctl.c            |  72 ++++++++++++++++++++++-
 drivers/md/Kconfig            |  11 ++++
 drivers/md/dm-core.h          |   5 ++
 drivers/md/dm-mpath.c         | 105 ++++++++++++++++++++++++++++++++++
 drivers/md/dm.c               |  26 ++++++++-
 drivers/scsi/scsi_lib.c       |  24 +-------
 include/linux/blkdev.h        |   4 ++
 include/linux/device-mapper.h |   8 ++-
 8 files changed, 226 insertions(+), 29 deletions(-)

-- 
2.32.0

[dm-devel] [PATCH v4 0/3] scsi/dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hello Mike, hello Martin,

here is v4 of my attempt to add retry logic to SG_IO on dm-multipath devices.

Regards
Martin

Changes v3->v4 (thanks to Mike Snitzer):

 - Added an additional helper function sg_io_to_blk_status() to
   scsi_ioctl.c, in order to avoid open-coding handling of the SCSI result
   code in device-mapper.

 - Added a new method dm_sg_io_ioctl_fn() in struct target_type, define
   only by the multipath target. This allows moving the bulk of the new
   code to dm-mpath.c, and avoids the wrong limitation of the code to
   request-based multipath.

Changes v2->v3:

 - un-inlined scsi_result_to_blk_status again, and move the helper
   __scsi_result_to_blk_status to block/scsi_ioctl.c instead
   (Bart v. Assche)
 - open-coded the status/msg/host/driver-byte -> result conversion
   where the standard SCSI helpers aren't usable (Bart v. Assche)
    
Changes v1->v2:

 - applied modifications from Mike Snitzer
 - moved SG_IO dependent code to a separate file, no scsi includes in
   dm.c any more
 - made the new code depend on a configuration option 
 - separated out scsi changes, made scsi_result_to_blk_status()
   inline to avoid dependency of dm_mod from scsi_mod (Paolo Bonzini)

Martin Wilck (3):
  scsi: scsi_ioctl: export __scsi_result_to_blk_status()
  scsi: scsi_ioctl: add sg_io_to_blk_status()
  dm mpath: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO

 block/scsi_ioctl.c            |  72 ++++++++++++++++++++++-
 drivers/md/Kconfig            |  11 ++++
 drivers/md/dm-core.h          |   5 ++
 drivers/md/dm-mpath.c         | 105 ++++++++++++++++++++++++++++++++++
 drivers/md/dm.c               |  26 ++++++++-
 drivers/scsi/scsi_lib.c       |  24 +-------
 include/linux/blkdev.h        |   4 ++
 include/linux/device-mapper.h |   8 ++-
 8 files changed, 226 insertions(+), 29 deletions(-)

-- 
2.32.0


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[mwilck]

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(-)

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 */
-- 
2.32.0

[dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[mwilck]

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(-)

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 */
-- 
2.32.0


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Christoph Hellwig]

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.

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Christoph Hellwig]

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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Martin Wilck]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Martin Wilck]

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



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Christoph Hellwig]

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.

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Christoph Hellwig]

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.


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Martin Wilck]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Martin Wilck]

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



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Keith Busch]

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.

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Keith Busch]

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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Martin Wilck]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Martin Wilck]

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



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Mike Snitzer]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Mike Snitzer]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Martin Wilck]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Martin Wilck]

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



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Bart Van Assche]

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.

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Bart Van Assche]

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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Christoph Hellwig]

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.

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Christoph Hellwig]

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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[Paolo Bonzini]

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

Re: [dm-devel] [PATCH v4 1/3] scsi: scsi_ioctl: export __scsi_result_to_blk_status()

[Paolo Bonzini]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This helper converts the SCSI result in a sg_io_hdr struct to a blk_status_t.
It will be used in the SG_IO code path for dm-multipath. Putting it into
scsi_ioctl.c avoids open-coding SCSI specific code in the dm layer.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c     | 20 ++++++++++++++++++++
 include/linux/blkdev.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 19b63b64ecbc..f226cac02e88 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -929,6 +929,26 @@ blk_status_t __scsi_result_to_blk_status(int *cmd_result, int result)
 }
 EXPORT_SYMBOL(__scsi_result_to_blk_status);
 
+blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
+{
+	int result;
+	blk_status_t sts;
+
+	if (!hdr->info & SG_INFO_CHECK)
+		return BLK_STS_OK;
+
+	result = hdr->status |
+		(hdr->msg_status << 8) |
+		(hdr->host_status << 16) |
+		(hdr->driver_status << 24);
+
+	sts = __scsi_result_to_blk_status(&result, result);
+	hdr->host_status = host_byte(result);
+
+	return sts;
+}
+EXPORT_SYMBOL(sg_io_to_blk_status);
+
 static int __init blk_scsi_ioctl_init(void)
 {
 	blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 48497a77428d..5da03edf125c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -2022,4 +2022,5 @@ 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);
+blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr);
 #endif /* _LINUX_BLKDEV_H */
-- 
2.32.0

[dm-devel] [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This helper converts the SCSI result in a sg_io_hdr struct to a blk_status_t.
It will be used in the SG_IO code path for dm-multipath. Putting it into
scsi_ioctl.c avoids open-coding SCSI specific code in the dm layer.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c     | 20 ++++++++++++++++++++
 include/linux/blkdev.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 19b63b64ecbc..f226cac02e88 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -929,6 +929,26 @@ blk_status_t __scsi_result_to_blk_status(int *cmd_result, int result)
 }
 EXPORT_SYMBOL(__scsi_result_to_blk_status);
 
+blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
+{
+	int result;
+	blk_status_t sts;
+
+	if (!hdr->info & SG_INFO_CHECK)
+		return BLK_STS_OK;
+
+	result = hdr->status |
+		(hdr->msg_status << 8) |
+		(hdr->host_status << 16) |
+		(hdr->driver_status << 24);
+
+	sts = __scsi_result_to_blk_status(&result, result);
+	hdr->host_status = host_byte(result);
+
+	return sts;
+}
+EXPORT_SYMBOL(sg_io_to_blk_status);
+
 static int __init blk_scsi_ioctl_init(void)
 {
 	blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 48497a77428d..5da03edf125c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -2022,4 +2022,5 @@ 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);
+blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr);
 #endif /* _LINUX_BLKDEV_H */
-- 
2.32.0


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5229 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next next-20210628]
[cannot apply to dm/for-next block/for-next song-md/md-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-r032-20210628 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 4c92e31dd0f1bd152eda883af20ff7fbcaa14945)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                      ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                               ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                                        ^~~~
   include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
           (cond) ?                                        \
            ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
   3 warnings generated.


vim +937 block/scsi_ioctl.c

   931	
   932	blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
   933	{
   934		int result;
   935		blk_status_t sts;
   936	
 > 937		if (!hdr->info & SG_INFO_CHECK)
   938			return BLK_STS_OK;
   939	
   940		result = hdr->status |
   941			(hdr->msg_status << 8) |
   942			(hdr->host_status << 16) |
   943			(hdr->driver_status << 24);
   944	
   945		sts = __scsi_result_to_blk_status(&result, result);
   946		hdr->host_status = host_byte(result);
   947	
   948		return sts;
   949	}
   950	EXPORT_SYMBOL(sg_io_to_blk_status);
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39520 bytes --]

Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5329 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next next-20210628]
[cannot apply to dm/for-next block/for-next song-md/md-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-r032-20210628 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 4c92e31dd0f1bd152eda883af20ff7fbcaa14945)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                      ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                               ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                                        ^~~~
   include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
           (cond) ?                                        \
            ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
   3 warnings generated.


vim +937 block/scsi_ioctl.c

   931	
   932	blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
   933	{
   934		int result;
   935		blk_status_t sts;
   936	
 > 937		if (!hdr->info & SG_INFO_CHECK)
   938			return BLK_STS_OK;
   939	
   940		result = hdr->status |
   941			(hdr->msg_status << 8) |
   942			(hdr->host_status << 16) |
   943			(hdr->driver_status << 24);
   944	
   945		sts = __scsi_result_to_blk_status(&result, result);
   946		hdr->host_status = host_byte(result);
   947	
   948		return sts;
   949	}
   950	EXPORT_SYMBOL(sg_io_to_blk_status);
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39520 bytes --]

Re: [dm-devel] [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5229 bytes --]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next next-20210628]
[cannot apply to dm/for-next block/for-next song-md/md-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-r032-20210628 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 4c92e31dd0f1bd152eda883af20ff7fbcaa14945)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                      ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                               ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
>> block/scsi_ioctl.c:937:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
           if (!hdr->info & SG_INFO_CHECK)
               ^          ~
   include/linux/compiler.h:56:47: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                                                 ^~~~
   include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
   #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                                        ^~~~
   include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
           (cond) ?                                        \
            ^~~~
   block/scsi_ioctl.c:937:6: note: add parentheses after the '!' to evaluate the bitwise operator first
   block/scsi_ioctl.c:937:6: note: add parentheses around left hand side expression to silence this warning
   3 warnings generated.


vim +937 block/scsi_ioctl.c

   931	
   932	blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
   933	{
   934		int result;
   935		blk_status_t sts;
   936	
 > 937		if (!hdr->info & SG_INFO_CHECK)
   938			return BLK_STS_OK;
   939	
   940		result = hdr->status |
   941			(hdr->msg_status << 8) |
   942			(hdr->host_status << 16) |
   943			(hdr->driver_status << 24);
   944	
   945		sts = __scsi_result_to_blk_status(&result, result);
   946		hdr->host_status = host_byte(result);
   947	
   948		return sts;
   949	}
   950	EXPORT_SYMBOL(sg_io_to_blk_status);
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39520 bytes --]

[-- Attachment #3: Type: text/plain, Size: 97 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH v4 3/3] dm mpath: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In virtual deployments, SCSI passthrough over dm-multipath devices is a
common setup. The qemu "pr-helper" was specifically invented for it. I
believe that this is the most important real-world scenario for sending
SG_IO ioctls to device-mapper devices.

In this configuration, guests send SCSI IO to the hypervisor in the form of
SG_IO ioctls issued by qemu. But on the device-mapper level, these SCSI
ioctls aren't treated like regular IO. Until commit 2361ae595352 ("dm mpath:
switch paths in dm_blk_ioctl() code path"), no path switching was done at
all. Worse though, if an SG_IO call fails because of a path error,
dm-multipath doesn't retry the IO on a another path; rather, the failure is
passed back to the guest, and paths are not marked as faulty.  This is in
stark contrast with regular block IO of guests on dm-multipath devices, and
certainly comes as a surprise to users who switch to SCSI passthrough in
qemu. In general, users of dm-multipath devices would probably expect failover
to work at least in a basic way.

This patch fixes this by taking a special code path for SG_IO on dm-multipath
targets if CONFIG_DM_MULTIPATH_SG_IO is set.  Rather then just choosing a
single path, sending the IO to it, and failing to the caller if the IO on the
path failed, it retries the same IO on another path for certain error codes,
using blk_path_error() to determine if a retry would make sense for the given
error code. Moreover, it fails the path on which the path error occurred,
like regular block IO would.

If all paths in a multipath map are failed, the behavior depends on the
queue_if_no_path setting of the map. If it is off, multipath_prepare_ioctl()
fails with -EIO, and the search for a valid paths is stopped. If it is on,
the caller will block until either queuing is disabled (in which case IO will
error out) or at least one path is reinstated (in which case IO will resume).
This is as close to regular READ/WRITE as it gets.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c            |   5 +-
 drivers/md/Kconfig            |  11 ++++
 drivers/md/dm-core.h          |   5 ++
 drivers/md/dm-mpath.c         | 105 ++++++++++++++++++++++++++++++++++
 drivers/md/dm.c               |  26 ++++++++-
 include/linux/blkdev.h        |   2 +
 include/linux/device-mapper.h |   8 ++-
 7 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index f226cac02e88..267de7cda82f 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -281,8 +281,8 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	return ret;
 }
 
-static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
-		struct sg_io_hdr *hdr, fmode_t mode)
+int sg_io(struct request_queue *q, struct gendisk *bd_disk,
+	  struct sg_io_hdr *hdr, fmode_t mode)
 {
 	unsigned long start_time;
 	ssize_t ret = 0;
@@ -367,6 +367,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	blk_put_request(rq);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sg_io);
 
 /**
  * sg_scsi_ioctl  --  handle deprecated SCSI_IOCTL_SEND_COMMAND ioctl
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index f2014385d48b..f28f29e3bd11 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -473,6 +473,17 @@ config DM_MULTIPATH_IOA
 
 	  If unsure, say N.
 
+config DM_MULTIPATH_SG_IO
+       bool "Retry SCSI generic I/O on multipath devices"
+       depends on DM_MULTIPATH && BLK_SCSI_REQUEST
+       help
+	 With this option, SCSI generic (SG) requests issued on multipath
+	 devices will behave similar to regular block I/O: upon failure,
+	 they are repeated on a different path, and the erroring device
+	 is marked as failed.
+
+	 If unsure, say N.
+
 config DM_DELAY
 	tristate "I/O delaying target"
 	depends on BLK_DEV_DM
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 5953ff2bd260..8bd8a8e3916e 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -189,4 +189,9 @@ extern atomic_t dm_global_event_nr;
 extern wait_queue_head_t dm_global_eventq;
 void dm_issue_global_event(void);
 
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target);
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx);
+
 #endif
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index bced42f082b0..86518aec32b4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -11,6 +11,7 @@
 #include "dm-bio-record.h"
 #include "dm-path-selector.h"
 #include "dm-uevent.h"
+#include "dm-core.h"
 
 #include <linux/blkdev.h>
 #include <linux/ctype.h>
@@ -26,6 +27,7 @@
 #include <scsi/scsi_dh.h>
 #include <linux/atomic.h>
 #include <linux/blk-mq.h>
+#include <scsi/sg.h>
 
 #define DM_MSG_PREFIX "multipath"
 #define DM_PG_INIT_DELAY_MSECS 2000
@@ -2129,6 +2131,106 @@ static int multipath_busy(struct dm_target *ti)
 	return busy;
 }
 
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+static int pgpath_sg_io_ioctl(struct block_device *bdev,
+			    struct sg_io_hdr *hdr, struct multipath *m,
+			    fmode_t mode)
+{
+	int rc;
+	blk_status_t sts;
+	struct priority_group *pg;
+	struct pgpath *pgpath;
+	char path_name[BDEVNAME_SIZE];
+
+	rc = sg_io(bdev->bd_disk->queue, bdev->bd_disk, hdr, mode);
+	DMDEBUG("SG_IO via %s: rc = %d D%02xH%02xM%02xS%02x",
+		bdevname(bdev, path_name), rc,
+		hdr->driver_status, hdr->host_status,
+		hdr->msg_status, hdr->status);
+
+	/*
+	 * Errors resulting from invalid parameters shouldn't be retried
+	 * on another path.
+	 */
+	switch (rc) {
+	case -ENOIOCTLCMD:
+	case -EFAULT:
+	case -EINVAL:
+	case -EPERM:
+		return rc;
+	default:
+		break;
+	}
+
+	sts = sg_io_to_blk_status(hdr);
+	if (sts == BLK_STS_OK)
+		return 0;
+	else if (!blk_path_error(sts))
+		return blk_status_to_errno(sts);
+
+	/* path error - fail the path */
+	list_for_each_entry(pg, &m->priority_groups, list) {
+		list_for_each_entry(pgpath, &pg->pgpaths, list) {
+			if (pgpath->path.dev->bdev == bdev)
+				fail_path(pgpath);
+		}
+	}
+
+	return -EAGAIN;
+}
+
+static int multipath_sg_io_ioctl(struct block_device *bdev, fmode_t mode,
+				 unsigned int cmd, unsigned long uarg)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	void __user *arg = (void __user *)uarg;
+	struct sg_io_hdr hdr;
+	int rc;
+	bool suspended;
+	int retries = 5;
+
+	if (copy_from_user(&hdr, arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.interface_id != 'S')
+		return -EINVAL;
+
+	if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk->queue) << 9))
+		return -EIO;
+
+	do {
+		struct block_device *path_dev;
+		struct dm_target *tgt;
+		struct sg_io_hdr rhdr;
+		int srcu_idx;
+
+		suspended = false;
+		/* This will fail and break the loop if no valid paths found */
+		rc = __dm_prepare_ioctl(md, &srcu_idx, &path_dev, &tgt);
+		if (rc == -EAGAIN)
+			suspended = true;
+		else if (rc < 0)
+			DMERR("%s: failed to get path: %d", __func__, rc);
+		else {
+			/* Need to copy the sg_io_hdr, it may be modified */
+			rhdr = hdr;
+			rc = pgpath_sg_io_ioctl(path_dev, &rhdr,
+						tgt->private, mode);
+			if (rc == 0 && copy_to_user(arg, &rhdr, sizeof(rhdr)))
+				rc = -EFAULT;
+		}
+		dm_unprepare_ioctl(md, srcu_idx);
+		if (suspended) {
+			DMDEBUG("%s: suspended, retries = %d\n",
+				__func__, retries);
+			msleep(20);
+		}
+	} while (rc == -EAGAIN && (!suspended || retries-- > 0));
+
+	return rc;
+}
+#endif
+
 /*-----------------------------------------------------------------
  * Module setup
  *---------------------------------------------------------------*/
@@ -2153,6 +2255,9 @@ static struct target_type multipath_target = {
 	.prepare_ioctl = multipath_prepare_ioctl,
 	.iterate_devices = multipath_iterate_devices,
 	.busy = multipath_busy,
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	.sg_io_ioctl = multipath_sg_io_ioctl,
+#endif
 };
 
 static int __init dm_multipath_init(void)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ca2aedd8ee7d..af15d2c132ce 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -29,6 +29,7 @@
 #include <linux/part_stat.h>
 #include <linux/blk-crypto.h>
 #include <linux/keyslot-manager.h>
+#include <scsi/sg.h>
 
 #define DM_MSG_PREFIX "core"
 
@@ -522,8 +523,9 @@ static int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
 #define dm_blk_report_zones		NULL
 #endif /* CONFIG_BLK_DEV_ZONED */
 
-static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
-			    struct block_device **bdev)
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target)
 {
 	struct dm_target *tgt;
 	struct dm_table *map;
@@ -553,13 +555,24 @@ static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
 		goto retry;
 	}
 
+	if (r >= 0 && target)
+		*target = tgt;
+
 	return r;
 }
+EXPORT_SYMBOL_GPL(__dm_prepare_ioctl);
 
-static void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
+static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+			    struct block_device **bdev)
+{
+	return __dm_prepare_ioctl(md, srcu_idx, bdev, NULL);
+}
+
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
 {
 	dm_put_live_table(md, srcu_idx);
 }
+EXPORT_SYMBOL_GPL(dm_unprepare_ioctl);
 
 static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
 			unsigned int cmd, unsigned long arg)
@@ -567,6 +580,13 @@ static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
 	struct mapped_device *md = bdev->bd_disk->private_data;
 	int r, srcu_idx;
 
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	if (cmd == SG_IO && md->immutable_target &&
+	    md->immutable_target->type->sg_io_ioctl)
+		return md->immutable_target->type->sg_io_ioctl(bdev, mode,
+							       cmd, arg);
+#endif
+
 	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
 	if (r < 0)
 		goto out;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5da03edf125c..96eaf1edd7b0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -923,6 +923,8 @@ extern int scsi_cmd_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			  unsigned int, void __user *);
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
+extern int sg_io(struct request_queue *q, struct gendisk *gd,
+		 struct sg_io_hdr *hdr, fmode_t mode);
 extern int get_sg_io_hdr(struct sg_io_hdr *hdr, const void __user *argp);
 extern int put_sg_io_hdr(const struct sg_io_hdr *hdr, void __user *argp);
 
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index ff700fb6ce1d..c94ae6ee81b8 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -151,6 +151,10 @@ typedef size_t (*dm_dax_copy_iter_fn)(struct dm_target *ti, pgoff_t pgoff,
 		void *addr, size_t bytes, struct iov_iter *i);
 typedef int (*dm_dax_zero_page_range_fn)(struct dm_target *ti, pgoff_t pgoff,
 		size_t nr_pages);
+
+typedef int (*dm_sg_io_ioctl_fn)(struct block_device *bdev, fmode_t mode,
+				 unsigned int cmd, unsigned long uarg);
+
 #define PAGE_SECTORS (PAGE_SIZE / 512)
 
 void dm_error(const char *message);
@@ -204,7 +208,9 @@ struct target_type {
 	dm_dax_copy_iter_fn dax_copy_from_iter;
 	dm_dax_copy_iter_fn dax_copy_to_iter;
 	dm_dax_zero_page_range_fn dax_zero_page_range;
-
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	dm_sg_io_ioctl_fn sg_io_ioctl;
+#endif
 	/* For internal device-mapper use. */
 	struct list_head list;
 };
-- 
2.32.0

[dm-devel] [PATCH v4 3/3] dm mpath: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In virtual deployments, SCSI passthrough over dm-multipath devices is a
common setup. The qemu "pr-helper" was specifically invented for it. I
believe that this is the most important real-world scenario for sending
SG_IO ioctls to device-mapper devices.

In this configuration, guests send SCSI IO to the hypervisor in the form of
SG_IO ioctls issued by qemu. But on the device-mapper level, these SCSI
ioctls aren't treated like regular IO. Until commit 2361ae595352 ("dm mpath:
switch paths in dm_blk_ioctl() code path"), no path switching was done at
all. Worse though, if an SG_IO call fails because of a path error,
dm-multipath doesn't retry the IO on a another path; rather, the failure is
passed back to the guest, and paths are not marked as faulty.  This is in
stark contrast with regular block IO of guests on dm-multipath devices, and
certainly comes as a surprise to users who switch to SCSI passthrough in
qemu. In general, users of dm-multipath devices would probably expect failover
to work at least in a basic way.

This patch fixes this by taking a special code path for SG_IO on dm-multipath
targets if CONFIG_DM_MULTIPATH_SG_IO is set.  Rather then just choosing a
single path, sending the IO to it, and failing to the caller if the IO on the
path failed, it retries the same IO on another path for certain error codes,
using blk_path_error() to determine if a retry would make sense for the given
error code. Moreover, it fails the path on which the path error occurred,
like regular block IO would.

If all paths in a multipath map are failed, the behavior depends on the
queue_if_no_path setting of the map. If it is off, multipath_prepare_ioctl()
fails with -EIO, and the search for a valid paths is stopped. If it is on,
the caller will block until either queuing is disabled (in which case IO will
error out) or at least one path is reinstated (in which case IO will resume).
This is as close to regular READ/WRITE as it gets.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c            |   5 +-
 drivers/md/Kconfig            |  11 ++++
 drivers/md/dm-core.h          |   5 ++
 drivers/md/dm-mpath.c         | 105 ++++++++++++++++++++++++++++++++++
 drivers/md/dm.c               |  26 ++++++++-
 include/linux/blkdev.h        |   2 +
 include/linux/device-mapper.h |   8 ++-
 7 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index f226cac02e88..267de7cda82f 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -281,8 +281,8 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	return ret;
 }
 
-static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
-		struct sg_io_hdr *hdr, fmode_t mode)
+int sg_io(struct request_queue *q, struct gendisk *bd_disk,
+	  struct sg_io_hdr *hdr, fmode_t mode)
 {
 	unsigned long start_time;
 	ssize_t ret = 0;
@@ -367,6 +367,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	blk_put_request(rq);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sg_io);
 
 /**
  * sg_scsi_ioctl  --  handle deprecated SCSI_IOCTL_SEND_COMMAND ioctl
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index f2014385d48b..f28f29e3bd11 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -473,6 +473,17 @@ config DM_MULTIPATH_IOA
 
 	  If unsure, say N.
 
+config DM_MULTIPATH_SG_IO
+       bool "Retry SCSI generic I/O on multipath devices"
+       depends on DM_MULTIPATH && BLK_SCSI_REQUEST
+       help
+	 With this option, SCSI generic (SG) requests issued on multipath
+	 devices will behave similar to regular block I/O: upon failure,
+	 they are repeated on a different path, and the erroring device
+	 is marked as failed.
+
+	 If unsure, say N.
+
 config DM_DELAY
 	tristate "I/O delaying target"
 	depends on BLK_DEV_DM
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 5953ff2bd260..8bd8a8e3916e 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -189,4 +189,9 @@ extern atomic_t dm_global_event_nr;
 extern wait_queue_head_t dm_global_eventq;
 void dm_issue_global_event(void);
 
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target);
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx);
+
 #endif
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index bced42f082b0..86518aec32b4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -11,6 +11,7 @@
 #include "dm-bio-record.h"
 #include "dm-path-selector.h"
 #include "dm-uevent.h"
+#include "dm-core.h"
 
 #include <linux/blkdev.h>
 #include <linux/ctype.h>
@@ -26,6 +27,7 @@
 #include <scsi/scsi_dh.h>
 #include <linux/atomic.h>
 #include <linux/blk-mq.h>
+#include <scsi/sg.h>
 
 #define DM_MSG_PREFIX "multipath"
 #define DM_PG_INIT_DELAY_MSECS 2000
@@ -2129,6 +2131,106 @@ static int multipath_busy(struct dm_target *ti)
 	return busy;
 }
 
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+static int pgpath_sg_io_ioctl(struct block_device *bdev,
+			    struct sg_io_hdr *hdr, struct multipath *m,
+			    fmode_t mode)
+{
+	int rc;
+	blk_status_t sts;
+	struct priority_group *pg;
+	struct pgpath *pgpath;
+	char path_name[BDEVNAME_SIZE];
+
+	rc = sg_io(bdev->bd_disk->queue, bdev->bd_disk, hdr, mode);
+	DMDEBUG("SG_IO via %s: rc = %d D%02xH%02xM%02xS%02x",
+		bdevname(bdev, path_name), rc,
+		hdr->driver_status, hdr->host_status,
+		hdr->msg_status, hdr->status);
+
+	/*
+	 * Errors resulting from invalid parameters shouldn't be retried
+	 * on another path.
+	 */
+	switch (rc) {
+	case -ENOIOCTLCMD:
+	case -EFAULT:
+	case -EINVAL:
+	case -EPERM:
+		return rc;
+	default:
+		break;
+	}
+
+	sts = sg_io_to_blk_status(hdr);
+	if (sts == BLK_STS_OK)
+		return 0;
+	else if (!blk_path_error(sts))
+		return blk_status_to_errno(sts);
+
+	/* path error - fail the path */
+	list_for_each_entry(pg, &m->priority_groups, list) {
+		list_for_each_entry(pgpath, &pg->pgpaths, list) {
+			if (pgpath->path.dev->bdev == bdev)
+				fail_path(pgpath);
+		}
+	}
+
+	return -EAGAIN;
+}
+
+static int multipath_sg_io_ioctl(struct block_device *bdev, fmode_t mode,
+				 unsigned int cmd, unsigned long uarg)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	void __user *arg = (void __user *)uarg;
+	struct sg_io_hdr hdr;
+	int rc;
+	bool suspended;
+	int retries = 5;
+
+	if (copy_from_user(&hdr, arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.interface_id != 'S')
+		return -EINVAL;
+
+	if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk->queue) << 9))
+		return -EIO;
+
+	do {
+		struct block_device *path_dev;
+		struct dm_target *tgt;
+		struct sg_io_hdr rhdr;
+		int srcu_idx;
+
+		suspended = false;
+		/* This will fail and break the loop if no valid paths found */
+		rc = __dm_prepare_ioctl(md, &srcu_idx, &path_dev, &tgt);
+		if (rc == -EAGAIN)
+			suspended = true;
+		else if (rc < 0)
+			DMERR("%s: failed to get path: %d", __func__, rc);
+		else {
+			/* Need to copy the sg_io_hdr, it may be modified */
+			rhdr = hdr;
+			rc = pgpath_sg_io_ioctl(path_dev, &rhdr,
+						tgt->private, mode);
+			if (rc == 0 && copy_to_user(arg, &rhdr, sizeof(rhdr)))
+				rc = -EFAULT;
+		}
+		dm_unprepare_ioctl(md, srcu_idx);
+		if (suspended) {
+			DMDEBUG("%s: suspended, retries = %d\n",
+				__func__, retries);
+			msleep(20);
+		}
+	} while (rc == -EAGAIN && (!suspended || retries-- > 0));
+
+	return rc;
+}
+#endif
+
 /*-----------------------------------------------------------------
  * Module setup
  *---------------------------------------------------------------*/
@@ -2153,6 +2255,9 @@ static struct target_type multipath_target = {
 	.prepare_ioctl = multipath_prepare_ioctl,
 	.iterate_devices = multipath_iterate_devices,
 	.busy = multipath_busy,
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	.sg_io_ioctl = multipath_sg_io_ioctl,
+#endif
 };
 
 static int __init dm_multipath_init(void)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ca2aedd8ee7d..af15d2c132ce 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -29,6 +29,7 @@
 #include <linux/part_stat.h>
 #include <linux/blk-crypto.h>
 #include <linux/keyslot-manager.h>
+#include <scsi/sg.h>
 
 #define DM_MSG_PREFIX "core"
 
@@ -522,8 +523,9 @@ static int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
 #define dm_blk_report_zones		NULL
 #endif /* CONFIG_BLK_DEV_ZONED */
 
-static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
-			    struct block_device **bdev)
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target)
 {
 	struct dm_target *tgt;
 	struct dm_table *map;
@@ -553,13 +555,24 @@ static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
 		goto retry;
 	}
 
+	if (r >= 0 && target)
+		*target = tgt;
+
 	return r;
 }
+EXPORT_SYMBOL_GPL(__dm_prepare_ioctl);
 
-static void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
+static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+			    struct block_device **bdev)
+{
+	return __dm_prepare_ioctl(md, srcu_idx, bdev, NULL);
+}
+
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
 {
 	dm_put_live_table(md, srcu_idx);
 }
+EXPORT_SYMBOL_GPL(dm_unprepare_ioctl);
 
 static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
 			unsigned int cmd, unsigned long arg)
@@ -567,6 +580,13 @@ static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
 	struct mapped_device *md = bdev->bd_disk->private_data;
 	int r, srcu_idx;
 
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	if (cmd == SG_IO && md->immutable_target &&
+	    md->immutable_target->type->sg_io_ioctl)
+		return md->immutable_target->type->sg_io_ioctl(bdev, mode,
+							       cmd, arg);
+#endif
+
 	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
 	if (r < 0)
 		goto out;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5da03edf125c..96eaf1edd7b0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -923,6 +923,8 @@ extern int scsi_cmd_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			  unsigned int, void __user *);
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
+extern int sg_io(struct request_queue *q, struct gendisk *gd,
+		 struct sg_io_hdr *hdr, fmode_t mode);
 extern int get_sg_io_hdr(struct sg_io_hdr *hdr, const void __user *argp);
 extern int put_sg_io_hdr(const struct sg_io_hdr *hdr, void __user *argp);
 
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index ff700fb6ce1d..c94ae6ee81b8 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -151,6 +151,10 @@ typedef size_t (*dm_dax_copy_iter_fn)(struct dm_target *ti, pgoff_t pgoff,
 		void *addr, size_t bytes, struct iov_iter *i);
 typedef int (*dm_dax_zero_page_range_fn)(struct dm_target *ti, pgoff_t pgoff,
 		size_t nr_pages);
+
+typedef int (*dm_sg_io_ioctl_fn)(struct block_device *bdev, fmode_t mode,
+				 unsigned int cmd, unsigned long uarg);
+
 #define PAGE_SECTORS (PAGE_SIZE / 512)
 
 void dm_error(const char *message);
@@ -204,7 +208,9 @@ struct target_type {
 	dm_dax_copy_iter_fn dax_copy_from_iter;
 	dm_dax_copy_iter_fn dax_copy_to_iter;
 	dm_dax_zero_page_range_fn dax_zero_page_range;
-
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+	dm_sg_io_ioctl_fn sg_io_ioctl;
+#endif
 	/* For internal device-mapper use. */
 	struct list_head list;
 };
-- 
2.32.0


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210628095210.26249-3-mwilck@suse.com>
References: <20210628095210.26249-3-mwilck@suse.com>
TO: mwilck(a)suse.com
TO: Mike Snitzer <snitzer@redhat.com>
TO: Alasdair G Kergon <agk@redhat.com>
TO: Bart Van Assche <Bart.VanAssche@sandisk.com>
TO: "Martin K. Petersen" <martin.petersen@oracle.com>
TO: linux-scsi(a)vger.kernel.org
TO: dm-devel(a)redhat.com
TO: Hannes Reinecke <hare@suse.de>
CC: Daniel Wagner <dwagner@suse.de>
CC: linux-block(a)vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next next-20210628]
[cannot apply to dm/for-next block/for-next song-md/md-next v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
:::::: branch date: 6 hours ago
:::::: commit date: 6 hours ago
config: xtensa-randconfig-s032-20210628 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> block/scsi_ioctl.c:937:24: sparse: sparse: dubious: !x & y

vim +937 block/scsi_ioctl.c

45bac7756930b1 Martin Wilck 2021-06-28  931  
259453ca972ae5 Martin Wilck 2021-06-28  932  blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
259453ca972ae5 Martin Wilck 2021-06-28  933  {
259453ca972ae5 Martin Wilck 2021-06-28  934  	int result;
259453ca972ae5 Martin Wilck 2021-06-28  935  	blk_status_t sts;
259453ca972ae5 Martin Wilck 2021-06-28  936  
259453ca972ae5 Martin Wilck 2021-06-28 @937  	if (!hdr->info & SG_INFO_CHECK)
259453ca972ae5 Martin Wilck 2021-06-28  938  		return BLK_STS_OK;
259453ca972ae5 Martin Wilck 2021-06-28  939  
259453ca972ae5 Martin Wilck 2021-06-28  940  	result = hdr->status |
259453ca972ae5 Martin Wilck 2021-06-28  941  		(hdr->msg_status << 8) |
259453ca972ae5 Martin Wilck 2021-06-28  942  		(hdr->host_status << 16) |
259453ca972ae5 Martin Wilck 2021-06-28  943  		(hdr->driver_status << 24);
259453ca972ae5 Martin Wilck 2021-06-28  944  
259453ca972ae5 Martin Wilck 2021-06-28  945  	sts = __scsi_result_to_blk_status(&result, result);
259453ca972ae5 Martin Wilck 2021-06-28  946  	hdr->host_status = host_byte(result);
259453ca972ae5 Martin Wilck 2021-06-28  947  
259453ca972ae5 Martin Wilck 2021-06-28  948  	return sts;
259453ca972ae5 Martin Wilck 2021-06-28  949  }
259453ca972ae5 Martin Wilck 2021-06-28  950  EXPORT_SYMBOL(sg_io_to_blk_status);
259453ca972ae5 Martin Wilck 2021-06-28  951  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29386 bytes --]

[kbuild] Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2995 bytes --]

Hi,

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git  for-next
config: xtensa-randconfig-s032-20210628 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f 
        git remote add linux-review https://github.com/0day-ci/linux 
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> block/scsi_ioctl.c:937:24: sparse: sparse: dubious: !x & y

vim +937 block/scsi_ioctl.c

259453ca972ae5 Martin Wilck 2021-06-28  932  blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
259453ca972ae5 Martin Wilck 2021-06-28  933  {
259453ca972ae5 Martin Wilck 2021-06-28  934  	int result;
259453ca972ae5 Martin Wilck 2021-06-28  935  	blk_status_t sts;
259453ca972ae5 Martin Wilck 2021-06-28  936  
259453ca972ae5 Martin Wilck 2021-06-28 @937  	if (!hdr->info & SG_INFO_CHECK)
                                                    ^
Should be if (!(hdr->info & SG_INFO_CHECK))

259453ca972ae5 Martin Wilck 2021-06-28  938  		return BLK_STS_OK;
259453ca972ae5 Martin Wilck 2021-06-28  939  
259453ca972ae5 Martin Wilck 2021-06-28  940  	result = hdr->status |
259453ca972ae5 Martin Wilck 2021-06-28  941  		(hdr->msg_status << 8) |
259453ca972ae5 Martin Wilck 2021-06-28  942  		(hdr->host_status << 16) |
259453ca972ae5 Martin Wilck 2021-06-28  943  		(hdr->driver_status << 24);
259453ca972ae5 Martin Wilck 2021-06-28  944  
259453ca972ae5 Martin Wilck 2021-06-28  945  	sts = __scsi_result_to_blk_status(&result, result);
259453ca972ae5 Martin Wilck 2021-06-28  946  	hdr->host_status = host_byte(result);
259453ca972ae5 Martin Wilck 2021-06-28  947  
259453ca972ae5 Martin Wilck 2021-06-28  948  	return sts;
259453ca972ae5 Martin Wilck 2021-06-28  949  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[dm-devel] [kbuild] Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[Dan Carpenter]

Hi,

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git  for-next
config: xtensa-randconfig-s032-20210628 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f 
        git remote add linux-review https://github.com/0day-ci/linux 
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> block/scsi_ioctl.c:937:24: sparse: sparse: dubious: !x & y

vim +937 block/scsi_ioctl.c

259453ca972ae5 Martin Wilck 2021-06-28  932  blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
259453ca972ae5 Martin Wilck 2021-06-28  933  {
259453ca972ae5 Martin Wilck 2021-06-28  934  	int result;
259453ca972ae5 Martin Wilck 2021-06-28  935  	blk_status_t sts;
259453ca972ae5 Martin Wilck 2021-06-28  936  
259453ca972ae5 Martin Wilck 2021-06-28 @937  	if (!hdr->info & SG_INFO_CHECK)
                                                    ^
Should be if (!(hdr->info & SG_INFO_CHECK))

259453ca972ae5 Martin Wilck 2021-06-28  938  		return BLK_STS_OK;
259453ca972ae5 Martin Wilck 2021-06-28  939  
259453ca972ae5 Martin Wilck 2021-06-28  940  	result = hdr->status |
259453ca972ae5 Martin Wilck 2021-06-28  941  		(hdr->msg_status << 8) |
259453ca972ae5 Martin Wilck 2021-06-28  942  		(hdr->host_status << 16) |
259453ca972ae5 Martin Wilck 2021-06-28  943  		(hdr->driver_status << 24);
259453ca972ae5 Martin Wilck 2021-06-28  944  
259453ca972ae5 Martin Wilck 2021-06-28  945  	sts = __scsi_result_to_blk_status(&result, result);
259453ca972ae5 Martin Wilck 2021-06-28  946  	hdr->host_status = host_byte(result);
259453ca972ae5 Martin Wilck 2021-06-28  947  
259453ca972ae5 Martin Wilck 2021-06-28  948  	return sts;
259453ca972ae5 Martin Wilck 2021-06-28  949  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[kbuild] Re: [PATCH v4 2/3] scsi: scsi_ioctl: add sg_io_to_blk_status()

[Dan Carpenter]

Hi,

url:    https://github.com/0day-ci/linux/commits/mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git  for-next
config: xtensa-randconfig-s032-20210628 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/259453ca972ae531cfdca07cbf4d6bb09b8f8c9f 
        git remote add linux-review https://github.com/0day-ci/linux 
        git fetch --no-tags linux-review mwilck-suse-com/scsi-dm-dm_blk_ioctl-implement-failover-for-SG_IO-on-dm-multipath/20210628-175410
        git checkout 259453ca972ae531cfdca07cbf4d6bb09b8f8c9f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=xtensa SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> block/scsi_ioctl.c:937:24: sparse: sparse: dubious: !x & y

vim +937 block/scsi_ioctl.c

259453ca972ae5 Martin Wilck 2021-06-28  932  blk_status_t sg_io_to_blk_status(struct sg_io_hdr *hdr)
259453ca972ae5 Martin Wilck 2021-06-28  933  {
259453ca972ae5 Martin Wilck 2021-06-28  934  	int result;
259453ca972ae5 Martin Wilck 2021-06-28  935  	blk_status_t sts;
259453ca972ae5 Martin Wilck 2021-06-28  936  
259453ca972ae5 Martin Wilck 2021-06-28 @937  	if (!hdr->info & SG_INFO_CHECK)
                                                    ^
Should be if (!(hdr->info & SG_INFO_CHECK))

259453ca972ae5 Martin Wilck 2021-06-28  938  		return BLK_STS_OK;
259453ca972ae5 Martin Wilck 2021-06-28  939  
259453ca972ae5 Martin Wilck 2021-06-28  940  	result = hdr->status |
259453ca972ae5 Martin Wilck 2021-06-28  941  		(hdr->msg_status << 8) |
259453ca972ae5 Martin Wilck 2021-06-28  942  		(hdr->host_status << 16) |
259453ca972ae5 Martin Wilck 2021-06-28  943  		(hdr->driver_status << 24);
259453ca972ae5 Martin Wilck 2021-06-28  944  
259453ca972ae5 Martin Wilck 2021-06-28  945  	sts = __scsi_result_to_blk_status(&result, result);
259453ca972ae5 Martin Wilck 2021-06-28  946  	hdr->host_status = host_byte(result);
259453ca972ae5 Martin Wilck 2021-06-28  947  
259453ca972ae5 Martin Wilck 2021-06-28  948  	return sts;
259453ca972ae5 Martin Wilck 2021-06-28  949  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org