On 11/02/2015 03:12 PM, Mike Snitzer wrote:
> On Mon, Nov 02 2015 at 8:56am -0500,
> Hannes Reinecke <hare@xxxxxxx> wrote:
>
>> On 11/02/2015 02:31 PM, Mike Snitzer wrote:
>>> On Mon, Nov 02 2015 at 2:28am -0500,
>>> Hannes Reinecke <hare@xxxxxxx> wrote:
>>>
>>>> On 10/31/2015 11:47 PM, Mike Snitzer wrote:
>>>>>
>>>>> For Hannes, and in my head, it didn't matter if a future bdev satisfies
>>>>> the length condition. I don't think Hannes was trying to guard against
>>>>> dangerous partition ioctls being issued by udev... but now I _do_
>>>>> question what exactly _is_ the point of his commit 21a2807bc3f. Which
>>>>> invalid ioctls, from udev, did Hannes' change actually disallow?
>>>>>
>>>
>>> FYI, I meant s/21a2807bc3f/a1989b33/
>>>
>>>> The reasoning is thus:
>>>>
>>>> With the original code we would need to wait for path activation
>>>> before we would be able to figure out if the ioctl is valid.
>>>> However, the callback to verify the ioctl is sd_ioctl(), which as
>>>> a first step calls scsi_verify_ioctl().
>>>> So my reasoning was that we can as well call scsi_verify_ioctl()
>>>> early, and allow it to filter out known invalid ioctls.
>>>> Then we wouldn't need to wait for path activation and return
>>>> immediately.
>>>
>>> Right, I understood that to be your intent. And it seemed reasonible.
>>> Unfortuntely, as we now know, scsi_verify_blk_ioctl() only really cares
>>> to filter out ioctls that are invalid for partitions.
>>>
>>> I'm still curious: which ioctls were being issued by udev that your
>>> commit a1989b33 "fixed" (so udev workers didn't hang)? Is the right fix
>>> to modify udev to not issue such ioctls? What was invalid about the
>>> udev issued ioctls?
>>>
>> Thing is, it's not udev itself which issues ioctl. It's the programs
>> started by udev via the various rules (PROGRAM or IMPORT keywords).
>> They might (and occasionally, will) issue ioctls, and we have no
>> means of controlling them.
>>
>>>> Incidentally, in the 'r == -ENOTCONN' case, we're waiting
>>>> for path activation, but then just bail out with -ENOTCONN.
>>>> As we're not resetting -ENOTCONN, where's the point in activate the
>>>> paths here?
>>>> Shouldn't we retry to figure out if -ENOTCONN is still true?
>>>
>>> We do, in DM core, see _your_ commit that added this ;)
>>> 6c182cd88d ("dm mpath: fix ioctl deadlock when no paths")
>>>
>> Indeed.
>>
>> But then the real question remains:
>>
>> What is the 'correct' behaviour for ioctls when no path retry
>> is active (or when no paths are present)?
>>
>> Should we start path activation?
>> If so, should we wait for path activation to finish, risking udev
>> killing the worker for that event (udev has a built-in timeout of
>> 120 seconds, which we might easily exceed when we need to activate
>> paths for large installations or slow path activation ... just
>> thinking of NetApp takeover/giveback cycle)?
>> If we're not waiting for path activation, where would be the point
>> in starting it, seeing that we're not actually interested in the result?
>
> We only start it: if (r == -ENOTCONN && !fatal_signal_pending(current))
>
> -ENOTCONN is set with:
> if ((pgpath && m->queue_io) || (!pgpath && m->queue_if_no_path))
>
I know.
>> And if we shouldn't start a path activation, what is the point of
>> having code for it in the first place?
>
> The point is we have reason to believe paths will be coming back or that
> the user wants to queue_if_no_path.
>
Yes. But taken at face value, it means that any I/O will be stuck in
the queue for an indefinite amount of time.
Which doesn't work for things like udev, which _requires_ an answer
after a certain time.
And if that answer isn't forthcoming, the entire udev worker is
killed, rendering the device useless as the event is never delivered.
So if the user has set queue_if_no_path, we might never get a path
down event delivered as udev is stuck.
What do we do then?
Tell the customer "Hey, you screwed up" is possibly not the best of
ideas, especially not as multipath sets an unconditional
queue_if_no_path per default for quite some arrays.
I can see a point for waiting the path initialisation to complete
(ie if queue_io is active). But for queue_if_no_path I'd rather
return an error directly (like -EAGAIN).
With that change we could retry path selection until queue_io is
done, and would be able to retrieve a valid status (as we'd always
have a bdev).
See the attached patch for a detailed explanation.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
From b0d5848e91cfc3b97adb49121085c994b707eac3 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@xxxxxxx>
Date: Mon, 2 Nov 2015 15:33:58 +0100
Subject: [PATCH] dm-mpath: Retry ioctl if paths need to be initialized
If paths need to be initialized (m->queue_io is set) we should
be starting the initialization and retry the ioctl to retrieve
the final status.
At the same time, we should _not_ wait for queue_if_no_path
to go away as this might take forever, resulting in the
ioctl to be stuck.
Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
---
drivers/md/dm-mpath.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 5a67671..7352397 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1543,6 +1543,7 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
unsigned long flags;
int r;
+again:
bdev = NULL;
mode = 0;
r = 0;
@@ -1559,8 +1560,10 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
mode = pgpath->path.dev->mode;
}
- if ((pgpath && m->queue_io) || (!pgpath && m->queue_if_no_path))
+ if (pgpath && m->queue_io)
r = -ENOTCONN;
+ else if (!pgpath && m->queue_if_no_path)
+ r = -EAGAIN;
else if (!bdev)
r = -EIO;
@@ -1569,7 +1572,7 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
/*
* Only pass ioctls through if the device sizes match exactly.
*/
- if (!bdev || ti->len != i_size_read(bdev->bd_inode) >> SECTOR_SHIFT) {
+ if (r == -ENOTCONN && ti->len != i_size_read(bdev->bd_inode) >> SECTOR_SHIFT) {
int err = scsi_verify_blk_ioctl(NULL, cmd);
if (err)
r = err;
@@ -1585,6 +1588,8 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
__pg_init_all_paths(m);
spin_unlock_irqrestore(&m->lock, flags);
dm_table_run_md_queue_async(m->ti->table);
+ msleep(10);
+ goto again;
}
return r ? : __blkdev_driver_ioctl(bdev, mode, cmd, arg);
--
1.8.5.6
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
