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