diff mbox series

dm: avoid filesystem lookup in dm_get_dev_t()

Message ID 20201210092459.81939-1-hare@suse.de (mailing list archive)
State Accepted, archived
Delegated to: Mike Snitzer
Headers show
Series dm: avoid filesystem lookup in dm_get_dev_t() | expand

Commit Message

Hannes Reinecke Dec. 10, 2020, 9:24 a.m. UTC
dm_get_dev_t() is just used to convert an arbitrary 'path' string
into a dev_t. It doesn't presume that the device is present; that
check will be done later, as the only caller is dm_get_device(),
which does a dm_get_table_device() later on, which will properly
open the device.
So if the path string already _is_ in major:minor representation
we can convert it directly, avoiding a recursion into the filesystem
to lookup the block device.
This avoids a hang in multipath_message() when the filesystem is
inaccessible.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-table.c     |  6 ++++++
 drivers/scsi/scsi_error.c | 18 ++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

Comments

Martin Wilck Dec. 10, 2020, 5:11 p.m. UTC | #1
On Thu, 2020-12-10 at 10:24 +0100, Hannes Reinecke wrote:
> dm_get_dev_t() is just used to convert an arbitrary 'path' string
> into a dev_t. It doesn't presume that the device is present; that
> check will be done later, as the only caller is dm_get_device(),
> which does a dm_get_table_device() later on, which will properly
> open the device.
> So if the path string already _is_ in major:minor representation
> we can convert it directly, avoiding a recursion into the filesystem
> to lookup the block device.
> This avoids a hang in multipath_message() when the filesystem is
> inaccessible.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Ack, although I believe the scsi/__GENKSYMS__ part doesn't belong here.

Note that this is essentially a revert/fix of 644bda6f3460 ("dm table:
fall back to getting device using name_to_dev_t()"). Added the author
of that patch to CC.

Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Martin Wilck Jan. 21, 2021, 3:02 p.m. UTC | #2
On Thu, 2020-12-10 at 18:11 +0100, Martin Wilck wrote:
> On Thu, 2020-12-10 at 10:24 +0100, Hannes Reinecke wrote:
> > dm_get_dev_t() is just used to convert an arbitrary 'path' string
> > into a dev_t. It doesn't presume that the device is present; that
> > check will be done later, as the only caller is dm_get_device(),
> > which does a dm_get_table_device() later on, which will properly
> > open the device.
> > So if the path string already _is_ in major:minor representation
> > we can convert it directly, avoiding a recursion into the
> > filesystem
> > to lookup the block device.
> > This avoids a hang in multipath_message() when the filesystem is
> > inaccessible.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> Ack, although I believe the scsi/__GENKSYMS__ part doesn't belong
> here.
> 
> Note that this is essentially a revert/fix of 644bda6f3460 ("dm
> table:
> fall back to getting device using name_to_dev_t()"). Added the author
> of that patch to CC.

Mike, do you need anything more to apply this one? Do you want a
cleaned-up resend?

Cheers,
Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer Jan. 21, 2021, 4:09 p.m. UTC | #3
On Thu, Jan 21 2021 at 10:02am -0500,
Martin Wilck <mwilck@suse.com> wrote:

> On Thu, 2020-12-10 at 18:11 +0100, Martin Wilck wrote:
> > On Thu, 2020-12-10 at 10:24 +0100, Hannes Reinecke wrote:
> > > dm_get_dev_t() is just used to convert an arbitrary 'path' string
> > > into a dev_t. It doesn't presume that the device is present; that
> > > check will be done later, as the only caller is dm_get_device(),
> > > which does a dm_get_table_device() later on, which will properly
> > > open the device.
> > > So if the path string already _is_ in major:minor representation
> > > we can convert it directly, avoiding a recursion into the
> > > filesystem
> > > to lookup the block device.
> > > This avoids a hang in multipath_message() when the filesystem is
> > > inaccessible.
> > > 
> > > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > 
> > Ack, although I believe the scsi/__GENKSYMS__ part doesn't belong
> > here.
> > 
> > Note that this is essentially a revert/fix of 644bda6f3460 ("dm
> > table:
> > fall back to getting device using name_to_dev_t()"). Added the author
> > of that patch to CC.
> 
> Mike, do you need anything more to apply this one? Do you want a
> cleaned-up resend?

It got hung up with Christoph correctly requesting more discussion, last
linux-block/lkml mail on the associated thread I kicked off is here:
https://lkml.org/lkml/2020/12/23/76

Basically if Hannes or yourself would like to review that thread and
send a relevant v2 it'd really help move this forward.  I'm bogged down
with too many competing tasks.  You guys may be able to act on this line
of development/fixing quicker than I'll get to it.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig Jan. 21, 2021, 5:03 p.m. UTC | #4
On Thu, Jan 21, 2021 at 11:09:33AM -0500, Mike Snitzer wrote:
> > Mike, do you need anything more to apply this one? Do you want a
> > cleaned-up resend?
> 
> It got hung up with Christoph correctly requesting more discussion, last
> linux-block/lkml mail on the associated thread I kicked off is here:
> https://lkml.org/lkml/2020/12/23/76
> 
> Basically if Hannes or yourself would like to review that thread and
> send a relevant v2 it'd really help move this forward.  I'm bogged down
> with too many competing tasks.  You guys may be able to act on this line
> of development/fixing quicker than I'll get to it.

I'll get back to this ASAP, sorry.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Martin Wilck Jan. 21, 2021, 5:41 p.m. UTC | #5
On Thu, 2021-01-21 at 18:03 +0100, Christoph Hellwig wrote:
> On Thu, Jan 21, 2021 at 11:09:33AM -0500, Mike Snitzer wrote:
> > > Mike, do you need anything more to apply this one? Do you want a
> > > cleaned-up resend?
> > 
> > It got hung up with Christoph correctly requesting more discussion,
> > last
> > linux-block/lkml mail on the associated thread I kicked off is
> > here:
> > https://lkml.org/lkml/2020/12/23/76
> > 
> > Basically if Hannes or yourself would like to review that thread
> > and
> > send a relevant v2 it'd really help move this forward.  I'm bogged
> > down
> > with too many competing tasks.  You guys may be able to act on this
> > line
> > of development/fixing quicker than I'll get to it.
> 
> I'll get back to this ASAP, sorry.

AFAICS reverting 644bda6f3460, which is basically what Hannes did,
is consensus. Christoph suggested also to revert the export of
name_to_dev_t(), but that is used in a couple of other places too,
unexporting it will require a bit more work.

I'd like to move forward with the issue of blocking dm ioctls, so
I'll submit a v2. Feel free to use or discard it as you please.

Thanks,
Martin



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0142dc0e6798..d71808be006b 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -411,7 +411,13 @@  dev_t dm_get_dev_t(const char *path)
 {
 	dev_t dev;
 	struct block_device *bdev;
+	unsigned int maj, min;
 
+	if (sscanf(path, "%u:%u", &maj, &min) == 2) {
+		dev = MKDEV(maj, min);
+		if (maj == MAJOR(dev) && min == MINOR(dev))
+			return dev;
+	}
 	bdev = lookup_bdev(path);
 	if (IS_ERR(bdev))
 		dev = name_to_dev_t(path);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 6e01e90d83af..deee3e99aed0 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -133,16 +133,6 @@  scmd_eh_abort_handler(struct work_struct *work)
 	struct scsi_device *sdev = scmd->device;
 	int rtn;
 
-#ifndef __GENKSYMS__
-	if (scmd->eh_eflags & SCSI_EH_INTERNAL_TIMEOUT) {
-		SCSI_LOG_ERROR_RECOVERY(3, scmd_printk(KERN_INFO, scmd,
-			"internal timeout\n"));
-		set_host_byte(scmd, DID_TIME_OUT);
-		scmd->eh_eflags &= ~SCSI_EH_INTERNAL_TIMEOUT;
-		scsi_finish_command(scmd);
-		return;
-	}
-#endif
 	if (scsi_host_eh_past_deadline(sdev->host)) {
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
@@ -934,6 +924,14 @@  static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 				 struct scsi_cmnd *scmd)
 {
+#ifndef __GENKSYMS__
+	if (scmd->eh_eflags & SCSI_EH_INTERNAL_TIMEOUT) {
+		SCSI_LOG_ERROR_RECOVERY(3, scmd_printk(KERN_INFO, scmd,
+			"internal timeout\n"));
+		scmd->eh_eflags &= ~SCSI_EH_INTERNAL_TIMEOUT;
+		return SUCCESS;
+	}
+#endif
 	if (!hostt->eh_abort_handler)
 		return FAILED;