[PATCH, RFC] dm: relax discard restrictions

[PATCH, RFC] dm: relax discard restrictions

[Christoph Hellwig]

With targets like dm-thinp we might want to allow discard even if the
underlying devices don't support them natively.  This patch is the
extreme variant of not restricting discard support at all, but maybe
there are other options.  Or maybe we should keep the old
dm_table_supports_discards function as a helper for targets that
want simple discard passthrough?

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c	2011-04-27 16:44:09.231032143 +0200
+++ linux-2.6/drivers/md/dm-table.c	2011-04-27 16:51:39.415259952 +0200
@@ -1136,10 +1136,10 @@ void dm_table_set_restrictions(struct dm
 	 */
 	q->limits = *limits;
 
-	if (!dm_table_supports_discards(t))
-		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, q);
-	else
+	if (t->discards_supported)
 		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
+	else
+		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, q);
 
 	dm_table_set_integrity(t);
 
@@ -1303,39 +1303,6 @@ struct mapped_device *dm_table_get_md(st
 	return t->md;
 }
 
-static int device_discard_capable(struct dm_target *ti, struct dm_dev *dev,
-				  sector_t start, sector_t len, void *data)
-{
-	struct request_queue *q = bdev_get_queue(dev->bdev);
-
-	return q && blk_queue_discard(q);
-}
-
-bool dm_table_supports_discards(struct dm_table *t)
-{
-	struct dm_target *ti;
-	unsigned i = 0;
-
-	if (!t->discards_supported)
-		return 0;
-
-	/*
-	 * Ensure that at least one underlying device supports discards.
-	 * t->devices includes internal dm devices such as mirror logs
-	 * so we need to use iterate_devices here, which targets
-	 * supporting discard must provide.
-	 */
-	while (i < dm_table_get_num_targets(t)) {
-		ti = dm_table_get_target(t, i++);
-
-		if (ti->type->iterate_devices &&
-		    ti->type->iterate_devices(ti, device_discard_capable, NULL))
-			return 1;
-	}
-
-	return 0;
-}
-
 EXPORT_SYMBOL(dm_vcalloc);
 EXPORT_SYMBOL(dm_get_device);
 EXPORT_SYMBOL(dm_put_device);
Index: linux-2.6/drivers/md/dm.h
===================================================================
--- linux-2.6.orig/drivers/md/dm.h	2011-04-27 16:50:32.992286464 +0200
+++ linux-2.6/drivers/md/dm.h	2011-04-27 16:50:34.898942802 +0200
@@ -61,7 +61,6 @@ int dm_table_any_congested(struct dm_tab
 int dm_table_any_busy_target(struct dm_table *t);
 unsigned dm_table_get_type(struct dm_table *t);
 bool dm_table_request_based(struct dm_table *t);
-bool dm_table_supports_discards(struct dm_table *t);
 int dm_table_alloc_md_mempools(struct dm_table *t);
 void dm_table_free_md_mempools(struct dm_table *t);
 struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);

Re: [PATCH, RFC] dm: relax discard restrictions

[Mike Snitzer]

On Wed, Apr 27 2011 at 11:06am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> With targets like dm-thinp we might want to allow discard even if the
> underlying devices don't support them natively.  This patch is the
> extreme variant of not restricting discard support at all, but maybe
> there are other options.  Or maybe we should keep the old
> dm_table_supports_discards function as a helper for targets that
> want simple discard passthrough?

I think we could accomplish the same by adding a target override flag
(e.g. ti->discards_supported) that enables dm_table_supports_discards()
to short-circuit device_discard_capable's native discard checking.

Mike

[PATCH] dm: allow a target to relax discard restrictions

[Mike Snitzer]

A target, like the upcoming thin provisioning target, may want to allow
discards even if the underlying devices do not support them natively.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c         |    4 ++++
 include/linux/device-mapper.h |    6 ++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index cb8380c..75b0430 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1354,6 +1354,10 @@ bool dm_table_supports_discards(struct dm_table *t)
 	while (i < dm_table_get_num_targets(t)) {
 		ti = dm_table_get_target(t, i++);
 
+		/* target supports discards even if underlying devices cannot */
+		if (ti->discards_supported)
+			return 1;
+
 		if (ti->type->iterate_devices &&
 		    ti->type->iterate_devices(ti, device_discard_capable, NULL))
 			return 1;
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 32a4423..c9e7ff5 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -191,6 +191,12 @@ struct dm_target {
 
 	/* Used to provide an error string from the ctr */
 	char *error;
+
+	/*
+	 * Enable discards even if the table's underlying devices
+	 * do not have native discard support.
+	 */
+	unsigned discards_supported:1;
 };
 
 /* Each target can link one of these into the table */

Re: [PATCH] dm: allow a target to relax discard restrictions

[Christoph Hellwig]

On Wed, Apr 27, 2011 at 05:55:38PM -0400, Mike Snitzer wrote:
> A target, like the upcoming thin provisioning target, may want to allow
> discards even if the underlying devices do not support them natively.
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>

This would work, but I thing it's really confusing to have your
new discards_supported flag in the dm_target structure in addition
to the existing discards_supported in the dm_table and the
num_discard_requests field there.

May we should allow the target to set a tristate

enum discard_enabled {
	NEVER,
	ALWAYS,
	PASSTHROUGH
}

to indicate it's discard support?

Re: dm: allow a target to relax discard restrictions

[Mike Snitzer]

On Thu, Apr 28 2011 at  4:41am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Apr 27, 2011 at 05:55:38PM -0400, Mike Snitzer wrote:
> > A target, like the upcoming thin provisioning target, may want to allow
> > discards even if the underlying devices do not support them natively.
> > 
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> 
> This would work, but I thing it's really confusing to have your
> new discards_supported flag in the dm_target structure in addition
> to the existing discards_supported in the dm_table and the
> num_discard_requests field there.

It's only confusing if you allow it to be ;)  More seriously: I agree,
the target override might lend itself to confusion.

> May we should allow the target to set a tristate
> 
> enum discard_enabled {
> 	NEVER,
> 	ALWAYS,
> 	PASSTHROUGH
> }
> 
> to indicate it's discard support?

[feel free to skip to my question at the very end if I'm boring]

The above would require all targets with num_discard_requests!=0 to have
an additional explicit PASSTHROUGH opt-in.  Seems like a bit much.
Could fix that if we reordered PASSTHROUGH to be the default (0).

But I don't think having an explicit NEVER buys us much; avoids checking
the target for num_discard_requests>0 but that's not a huge win or
anything.  And it would require each target to set NEVER to get that
mini-optimization.

So that leaves:
enum discard_enabled {
       PASSTHROUGH,
       ALWAYS
}

Which is best expressed with a single flag.

With existing code (and proposed target 'discards_supported' override)
NEVER       = ti->num_discard_requests=0
ALWAYS      = ti->discards_supported=1
PASSTHROUGH = ti->num_discard_requests > 0

Would just renaming the target's 'discards_supported' override to
'discards_always_supported' help?  If not that name some other name?

Mike