dm: relax discard restrictions

RFC PATCH In review Christoph Hellwig 2 participants 5 messages Apr 27, 2011 – Apr 28, 2011 Device mapper

Share & tools

Christoph Hellwig

author

2011-04-27 15:06 UTC #0 lore raw

RFC PATCH 1/1 dm: relax discard restrictions

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 <[email protected]>

drivers/md/dm-table.c +3 ~~-36~~

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
@@ -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);

drivers/md/dm.h +0 -1

Index: linux-2.6/drivers/md/dm.h
===================================================================
@@ -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);

Mike Snitzer

2011-04-27 15:19 UTC #1 lore raw

Quoted 6 lines · 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?

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

Mike Snitzer

2011-04-27 21:55 UTC #2 lore raw

PATCH 1/1 dm: allow a target to relax discard restrictions

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 <[email protected]>

---
 drivers/md/dm-table.c         |    4 ++++
 include/linux/device-mapper.h |    6 ++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

drivers/md/dm-table.c +4 -0

index cb8380c..75b0430 100644
@@ -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;

include/linux/device-mapper.h +6 -0

index 32a4423..c9e7ff5 100644
@@ -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 */

Christoph Hellwig

author

2011-04-28 08:41 UTC #3 lore raw

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

Quoted 4 lines · 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 <[email protected]>

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?

Mike Snitzer

2011-04-28 15:04 UTC #4 lore raw

Re: dm: allow a target to relax discard restrictions

Quoted 10 lines · Christoph Hellwig

Quoted 4 lines · 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 <[email protected]>
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.

Quoted 9 lines

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