FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 09-12-2012, 07:37 PM
Mike Snitzer
 
Default dm table: do not allow queue limits that will exceed hardware limits

DM recalculates queue limits based only on devices which currently exist
in the table. This creates a problem in the event all devices are
temporarily removed such as all fibre channel paths being lost in
multipath. DM will reset the limits to the maximum permissible, which
can then assemble requests which exceed the limits of the paths when the
paths are restored. The request will fail the blk_rq_check_limits()
test when sent to a path with lower limits, and will be retried without
end by multipath.

This becomes a much bigger issue after commit fe86cdcef ("block: do not
artificially constrain max_sectors for stacking drivers"). Previously,
most storage had max_sector limits which exceeded the default value
used. This meant most setups wouldn't trigger this issue as the default
values used when there were no paths were still less than the limits of
the underlying devices. Now that the default stacking values are no
longer constrained, any hardware setup can potentially hit this issue.

So add a safety net that will establish safe default limits, via
blk_set_default_limits, in the event that a table temporarily doesn't
have any component devices.

Reported-by: David Jeffery <djeffery@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
drivers/md/dm-table.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f6979ad..9b931b4 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1264,6 +1264,15 @@ combine_limits:
(unsigned long long) ti->len);
}

+ /*
+ * If a table doesn't have any component devices (e.g. multipath
+ * loses all paths) don't allow the queue_limits to be left at
+ * their maximum (as established by blk_set_stacking_limits() so
+ * limits could be inherited from component devices).
+ */
+ if (limits->max_sectors == UINT_MAX)
+ blk_set_default_limits(limits);
+
return validate_hardware_logical_block_alignment(table, limits);
}

--
1.7.1

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-14-2012, 08:41 PM
Mike Snitzer
 
Default dm table: do not allow queue limits that will exceed hardware limits

DM recalculates queue limits based only on devices which currently exist
in the table. This creates a problem in the event all devices are
temporarily removed such as all fibre channel paths being lost in
multipath. DM will reset the limits to the maximum permissible, which
can then assemble requests which exceed the limits of the paths when the
paths are restored. The request will fail the blk_rq_check_limits()
test when sent to a path with lower limits, and will be retried without
end by multipath. This became a much bigger issue after commit
fe86cdcef ("block: do not artificially constrain max_sectors for
stacking drivers").

Add a safety net that will establish safe default limits, via
blk_set_default_limits, in the event that a table temporarily doesn't
have any component devices. The default limits may be too conservative
once paths are reinstated but adding a path to a multipath device
requires a table reload. Once the table is reloaded proper limits
will be established based on the available component device(s).

Reported-by: David Jeffery <djeffery@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
drivers/md/dm-table.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)

v2: adjust the patch header to be more succinct

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f6979ad..9b931b4 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1264,6 +1264,15 @@ combine_limits:
(unsigned long long) ti->len);
}

+ /*
+ * If a table doesn't have any component devices (e.g. multipath
+ * loses all paths) don't allow the queue_limits to be left at
+ * their maximum (as established by blk_set_stacking_limits() so
+ * limits could be inherited from component devices).
+ */
+ if (limits->max_sectors == UINT_MAX)
+ blk_set_default_limits(limits);
+
return validate_hardware_logical_block_alignment(table, limits);
}

--
1.7.1

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-17-2012, 07:44 PM
David Jeffery
 
Default dm table: do not allow queue limits that will exceed hardware limits

----- Original Message -----
> v2: adjust the patch header to be more succinct
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index f6979ad..9b931b4 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1264,6 +1264,15 @@ combine_limits:
> (unsigned long long) ti->len);
> }
>
> + /*
> + * If a table doesn't have any component devices (e.g. multipath
> + * loses all paths) don't allow the queue_limits to be left at
> + * their maximum (as established by blk_set_stacking_limits() so
> + * limits could be inherited from component devices).
> + */
> + if (limits->max_sectors == UINT_MAX)
> + blk_set_default_limits(limits);
> +
> return validate_hardware_logical_block_alignment(table, limits);
> }
>
> --
> 1.7.1
>
>

Instead of setting to defaults, how about maintaining previous limits?
The initial queue setup sets defaults when a queue is first configured,
and this maintains known, working limits if all paths are temporarly
lost. For example, I have a test setup with a lower than normal max
segment list. It can fail a test with the previous patch as the
default limits exceed the hardware limits. But this setup will work if
we leave queue limits unchanged in the special case of there being no
target devices.

David Jeffery

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4e09b6f..89ec9ee 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2425,6 +2425,15 @@ struct dm_table *dm_swap_table(struct mapped_device *md, struct dm_table *table)
goto out;
}

+ /*
+ * If a table doesn't have any component devices (e.g. multipath
+ * loses all paths) don't allow the queue_limits to be left at
+ * their maximum (as established by blk_set_stacking_limits()).
+ * Instead, maintain the previous limits.
+ */
+ if (limits.max_sectors == UINT_MAX)
+ limits = md->queue->limits;
+
map = __bind(md, table, &limits);

out:


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-18-2012, 01:02 PM
Mike Snitzer
 
Default dm table: do not allow queue limits that will exceed hardware limits

On Tue, Sep 18 2012 at 7:40am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Mon, Sep 17, 2012 at 03:44:29PM -0400, David Jeffery wrote:
> > @@ -2425,6 +2425,15 @@ struct dm_table *dm_swap_table(struct mapped_device *md, struct dm_table *table)
>
> > + if (limits.max_sectors == UINT_MAX)
>
> Specifically, I don't want dm.c to be peering directly into limits.

That's fine. I agree, it is wrong to have DM code be so fragile
relative to potential block layer changes.

> It just called calculate_queue_limits() above that.
> Why is calculate_queue_limits getting the limits wrong?

Because, in the mpath case David reported, there are no longer any
devices in the table. So .iterate_devices doesn't have anything to work
with; which means the blk_set_stacking_limits()' limits are left
unchanged (e.g. limits.max_sectors is UINT_MAX).

I'll try to think of a way to get dm_calculate_queue_limits() to detect:
1) that there are no devices in the table
2) that there is a live table that already has established limits
3) if 1 && 2, then use existing limits (should address David's request)

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

Thread Tools




All times are GMT. The time now is 04:53 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org