mbox series

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

Message ID 20210628095210.26249-1-mwilck@suse.com
Headers show
Series scsi/dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath | expand

Message

Martin Wilck June 28, 2021, 9:52 a.m. UTC
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(-)

Comments

Martin Wilck June 28, 2021, 2:57 p.m. UTC | #1
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
Christoph Hellwig June 29, 2021, 12:59 p.m. UTC | #2
On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:
> Hello Christoph,

> 

> On Mo, 2021-06-28 at 11:53 +0200, Christoph Hellwig wrote:

> > On Mon, Jun 28, 2021 at 11:52:08AM +0200, mwilck@suse.com wrote:

> > > From: Martin Wilck <mwilck@suse.com>

> > > 

> > > This makes it possible to use scsi_result_to_blk_status() from

> > > code that shouldn't depend on scsi_mod (e.g. device mapper).

> > 

> > This really has no business being used outside of low-level SCSI

> > code.

> 

> And this is where my patch set uses it. Can you recommend a better

> way how to access this algorithm, without making dm_mod.ko or dm-

> mpath.ko depend on scsi_mod.ko, and without open-coding it

> independently in a different code path?


> The sg_io-on-multipath code needs to answer the question "is this a

> path or a target error?". Therefore it calls blk_path_error(), which

> requires obtaining a blk_status_t first. But that's not available in

> the sg_io code path. So how should I deal with this situation?


Not by exporting random crap from the SCSI code.
Martin Wilck June 29, 2021, 7:23 p.m. UTC | #3
On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:
> On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:

> 

> > The sg_io-on-multipath code needs to answer the question "is this a

> > path or a target error?". Therefore it calls blk_path_error(),

> > which

> > requires obtaining a blk_status_t first. But that's not available

> > in

> > the sg_io code path. So how should I deal with this situation?

> 

> Not by exporting random crap from the SCSI code.


So, you'd prefer inlining scsi_result_to_blk_status()?

Thanks,
Martin
Keith Busch June 29, 2021, 9:23 p.m. UTC | #4
On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:
> On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:

> > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:

> > 

> > > The sg_io-on-multipath code needs to answer the question "is this a

> > > path or a target error?". Therefore it calls blk_path_error(),

> > > which

> > > requires obtaining a blk_status_t first. But that's not available

> > > in

> > > the sg_io code path. So how should I deal with this situation?

> > 

> > Not by exporting random crap from the SCSI code.

> 

> So, you'd prefer inlining scsi_result_to_blk_status()?


I don't think you need to. The only scsi_result_to_blk_status() caller
is sg_io_to_blk_status(). That's already in the same file as
scsi_result_to_blk_status() so no need to export it. You could even make
it static.
Martin Wilck June 30, 2021, 8:12 a.m. UTC | #5
On Di, 2021-06-29 at 14:23 -0700, Keith Busch wrote:
> On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:

> > On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:

> > > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:

> > > 

> > > > The sg_io-on-multipath code needs to answer the question "is

> > > > this a

> > > > path or a target error?". Therefore it calls blk_path_error(),

> > > > which

> > > > requires obtaining a blk_status_t first. But that's not

> > > > available

> > > > in

> > > > the sg_io code path. So how should I deal with this situation?

> > > 

> > > Not by exporting random crap from the SCSI code.

> > 

> > So, you'd prefer inlining scsi_result_to_blk_status()?

> 

> I don't think you need to. The only scsi_result_to_blk_status()

> caller

> is sg_io_to_blk_status(). That's already in the same file as

> scsi_result_to_blk_status() so no need to export it. You could even

> make

> it static.


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

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

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

> On Di, 2021-06-29 at 14:23 -0700, Keith Busch wrote:

> > On Tue, Jun 29, 2021 at 09:23:18PM +0200, Martin Wilck wrote:

> > > On Di, 2021-06-29 at 14:59 +0200, Christoph Hellwig wrote:

> > > > On Mon, Jun 28, 2021 at 04:57:33PM +0200, Martin Wilck wrote:

> > > > 

> > > > > The sg_io-on-multipath code needs to answer the question "is

> > > > > this a

> > > > > path or a target error?". Therefore it calls blk_path_error(),

> > > > > which

> > > > > requires obtaining a blk_status_t first. But that's not

> > > > > available

> > > > > in

> > > > > the sg_io code path. So how should I deal with this situation?

> > > > 

> > > > Not by exporting random crap from the SCSI code.


Helpful as always Christoph... ;)

> > > So, you'd prefer inlining scsi_result_to_blk_status()?

> > 

> > I don't think you need to. The only scsi_result_to_blk_status()

> > caller

> > is sg_io_to_blk_status(). That's already in the same file as

> > scsi_result_to_blk_status() so no need to export it. You could even

> > make

> > it static.

> 

> Thanks for your suggestion. I'd be lucky if this was true. But the most

> important users of scsi_result_to_blk_status() are in scsi_lib.c

> (scsi_io_completion_action(), scsi_io_completion_nz_result()).

> 

> If I move scsi_result_to_blk_status() to vmlinux without exporting it,

> it won't be available in the SCSI core any more, at least not with

> CONFIG_SCSI=m.


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

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

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

Mike
Martin Wilck June 30, 2021, 4:54 p.m. UTC | #7
On Mi, 2021-06-30 at 12:01 -0400, Mike Snitzer wrote:
> On Wed, Jun 30 2021 at  4:12P -0400,

> Martin Wilck <mwilck@suse.com> wrote:

> > 

> > Thanks for your suggestion. I'd be lucky if this was true. But the

> > most

> > important users of scsi_result_to_blk_status() are in scsi_lib.c

> > (scsi_io_completion_action(), scsi_io_completion_nz_result()).

> > 

> > If I move scsi_result_to_blk_status() to vmlinux without exporting

> > it,

> > it won't be available in the SCSI core any more, at least not with

> > CONFIG_SCSI=m.

> 

> For what you're trying to accomplish with this patchset, you only

> need

> sg_io_to_blk_status() exported.

> 

> So check with Martin and/or Bart on the best way to give

> sg_io_to_blk_status() access to the equivalent of your

> __scsi_result_to_blk_status() without exporting it.

> 

> I'd start by just folding patches 1 and 2, fixing up the logic Dan

> Carpenter pointed ouit, and only exporting sg_io_to_blk_status().


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

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

Thanks,
Martin
Bart Van Assche June 30, 2021, 10:08 p.m. UTC | #8
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.
Christoph Hellwig July 1, 2021, 6:19 a.m. UTC | #9
On Wed, Jun 30, 2021 at 03:08:11PM -0700, Bart Van Assche wrote:
> On 6/30/21 9:54 AM, Martin Wilck wrote:

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

> 

> The code in block/scsi_ioctl.c exists in the block layer since until

> recently most of it was used by both the IDE and SCSI code. Now that

> drivers/ide is gone (thanks Christoph!), how about moving block/bsg.c

> and block/scsi_ioctl.c to drivers/scsi/? As far as I can see the BSG

> code is only used by SCSI drivers:


I have a series to clean this mess up:

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

But more importantly, dm has no business interpreting the errors.  Just
like how SG_IO processing generally does not look at the error and
just returns it to userspace and leaves error handling to the caller
so should be the decision to resubmit it in a multi-path setup.
Paolo Bonzini July 6, 2021, 4:40 p.m. UTC | #10
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