[PATCH] dm table: simplify discard support processing

[PATCH] dm table: simplify discard support processing

[Mike Snitzer]

Remove 'discards_supported' from the dm_table structure.  The same
information can be easily discovered from the table's target(s) in
dm_table_supports_discards().

Also DMWARN if a target sets 'discards_supported' override but forgets
to set 'num_discard_requests'.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 367a2e0..b5bdeb3 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -54,7 +54,6 @@ struct dm_table {
 	sector_t *highs;
 	struct dm_target *targets;
 
-	unsigned discards_supported:1;
 	unsigned integrity_supported:1;
 
 	/*
@@ -207,7 +206,6 @@ int dm_table_create(struct dm_table **result, fmode_t mode,
 	INIT_LIST_HEAD(&t->devices);
 	INIT_LIST_HEAD(&t->target_callbacks);
 	atomic_set(&t->holders, 0);
-	t->discards_supported = 1;
 
 	if (!num_targets)
 		num_targets = KEYS_PER_NODE;
@@ -788,8 +786,9 @@ int dm_table_add_target(struct dm_table *t, const char *type,
 
 	t->highs[t->num_targets++] = tgt->begin + tgt->len - 1;
 
-	if (!tgt->num_discard_requests)
-		t->discards_supported = 0;
+	if (!tgt->num_discard_requests && tgt->discards_supported)
+		DMWARN("%s: %s: must set num_discard_requests if discards_supported, "
+		       "disabling discards", dm_device_name(t->md), type);
 
 	return 0;
 
@@ -1413,9 +1412,6 @@ bool dm_table_supports_discards(struct dm_table *t)
 	struct dm_target *ti;
 	unsigned i = 0;
 
-	if (!t->discards_supported)
-		return 0;
-
 	/*
 	 * Unless any target used by the table set discards_supported,
 	 * require at least one underlying device to support discards.
@@ -1426,6 +1422,9 @@ bool dm_table_supports_discards(struct dm_table *t)
 	while (i < dm_table_get_num_targets(t)) {
 		ti = dm_table_get_target(t, i++);
 
+		if (!ti->num_discard_requests)
+			return 0;
+		
 		if (ti->discards_supported)
 			return 1;
 

Re: [PATCH] dm table: simplify discard support processing

[Milan Broz]

On 07/14/2011 04:30 PM, Mike Snitzer wrote:
> Remove 'discards_supported' from the dm_table structure.  The same
> information can be easily discovered from the table's target(s) in
> dm_table_supports_discards().
> 
> Also DMWARN if a target sets 'discards_supported' override but forgets
> to set 'num_discard_requests'.

Why not BUG_ON? It is bug in code on static attribute, isn't? :-)

> @@ -1426,6 +1422,9 @@ bool dm_table_supports_discards(struct dm_table *t)
>  	while (i < dm_table_get_num_targets(t)) {
>  		ti = dm_table_get_target(t, i++);
>  
> +		if (!ti->num_discard_requests)
> +			return 0;
> +		

>  		if (ti->discards_supported)
>  			return 1;

What if next target has ti->num_discard_requests = 0 here?
Shouldn't it loop through the all targets always?

Milan

Re: dm table: simplify discard support processing

[Mike Snitzer]

On Thu, Jul 14 2011 at 11:43am -0400,
Milan Broz <mbroz@redhat.com> wrote:

> On 07/14/2011 04:30 PM, Mike Snitzer wrote:
> > Remove 'discards_supported' from the dm_table structure.  The same
> > information can be easily discovered from the table's target(s) in
> > dm_table_supports_discards().
> > 
> > Also DMWARN if a target sets 'discards_supported' override but forgets
> > to set 'num_discard_requests'.
> 
> Why not BUG_ON? It is bug in code on static attribute, isn't? :-)

Because we don't _need_ to BUG the system over programmer error for
how that target implements discards (given discard support is basically
optional, sometimes nice to have, increasingly nice to have in future).

> > @@ -1426,6 +1422,9 @@ bool dm_table_supports_discards(struct dm_table *t)
> >  	while (i < dm_table_get_num_targets(t)) {
> >  		ti = dm_table_get_target(t, i++);
> >  
> > +		if (!ti->num_discard_requests)
> > +			return 0;
> > +		
> 
> >  		if (ti->discards_supported)
> >  			return 1;
> 
> What if next target has ti->num_discard_requests = 0 here?
> Shouldn't it loop through the all targets always?

Yes it should, but I'm not sure if we are on the same page as to why.

Background:
If a table has a target with discards_supported=1 then the final DM
device's queue must export the ability to handle discards.  But only
targets with num_discard_requests>0 will actually have discards past to
them.

ti->discards_supported is used for targets like thinp.  Even if the the
thinp pool's underlying data device doesn't support discards the 'thin'
and 'thin-pool' targets do.

So back to the original question: yes, we need to look at all targets to
make sure that one of them doesn't have discards_supported set.

I'll fix this up and send v2.

Thanks,
Mike

[PATCH v2] dm table: simplify discard support processing

[Mike Snitzer]

Remove 'discards_supported' from the dm_table structure.  The same
information can be easily discovered from the table's target(s) in
dm_table_supports_discards().

Also DMWARN if a target sets 'discards_supported' override but forgets
to set 'num_discard_requests'.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

v2: have dm_table_supports_discards() keep checking table's targets if
    !ti->num_discard_requests

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 367a2e0..b5bdeb3 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -54,7 +54,6 @@ struct dm_table {
 	sector_t *highs;
 	struct dm_target *targets;
 
-	unsigned discards_supported:1;
 	unsigned integrity_supported:1;
 
 	/*
@@ -207,7 +206,6 @@ int dm_table_create(struct dm_table **result, fmode_t mode,
 	INIT_LIST_HEAD(&t->devices);
 	INIT_LIST_HEAD(&t->target_callbacks);
 	atomic_set(&t->holders, 0);
-	t->discards_supported = 1;
 
 	if (!num_targets)
 		num_targets = KEYS_PER_NODE;
@@ -788,8 +786,9 @@ int dm_table_add_target(struct dm_table *t, const char *type,
 
 	t->highs[t->num_targets++] = tgt->begin + tgt->len - 1;
 
-	if (!tgt->num_discard_requests)
-		t->discards_supported = 0;
+	if (!tgt->num_discard_requests && tgt->discards_supported)
+		DMWARN("%s: %s: must set num_discard_requests if discards_supported, "
+		       "disabling discards", dm_device_name(t->md), type);
 
 	return 0;
 
@@ -1413,9 +1412,6 @@ bool dm_table_supports_discards(struct dm_table *t)
 	struct dm_target *ti;
 	unsigned i = 0;
 
-	if (!t->discards_supported)
-		return 0;
-
 	/*
 	 * Unless any target used by the table set discards_supported,
 	 * require at least one underlying device to support discards.
@@ -1426,6 +1422,9 @@ bool dm_table_supports_discards(struct dm_table *t)
 	while (i < dm_table_get_num_targets(t)) {
 		ti = dm_table_get_target(t, i++);
 
+		if (!ti->num_discard_requests)
+			continue;
+		
 		if (ti->discards_supported)
 			return 1;
 

Re: [PATCH v2] dm table: simplify discard support processing

[Mike Snitzer]

Updated patch header to also point out the fix:

Remove 'discards_supported' from the dm_table structure.  The same
information can be easily discovered from the table's target(s) in
dm_table_supports_discards().

This change fixes an issue with per-target 'discards_supported' being
ineffective, at insuring the final DM device advertises discard support,
due to a different target not having 'num_discard_requests' > 0 -- this
resulted in the overall table's 'discards_supported' being set to 0.

Also DMWARN if a target sets 'discards_supported' override but forgets
to set 'num_discard_requests'.

[PATCH v3] dm table: fix and simplify discard support processing

[Mike Snitzer]

Remove 'discards_supported' from the dm_table structure.  The same
information can be easily discovered from the table's target(s) in
dm_table_supports_discards().

Before this fix dm_table_supports_discards() would skip checking the
individual targets' 'discards_supported' flag if any one target in the
table didn't set num_discard_requests > 0.  Now the per-target
'discards_supported' flag is effective at insuring the final DM device
advertises discard support.  But, to be clear, targets that don't
support discards (!num_discard_requests) will not receive discard
requests.

Also DMWARN if a target sets 'discards_supported' override but forgets
to set 'num_discard_requests'.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

v3: rebased ontop the latest editing tree

Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -54,7 +54,6 @@ struct dm_table {
 	sector_t *highs;
 	struct dm_target *targets;
 
-	unsigned discards_supported:1;
 	unsigned integrity_supported:1;
 	unsigned singleton:1;
 
@@ -209,7 +208,6 @@ int dm_table_create(struct dm_table **re
 	INIT_LIST_HEAD(&t->devices);
 	INIT_LIST_HEAD(&t->target_callbacks);
 	atomic_set(&t->holders, 0);
-	t->discards_supported = 1;
 
 	if (!num_targets)
 		num_targets = KEYS_PER_NODE;
@@ -809,8 +807,9 @@ int dm_table_add_target(struct dm_table 
 
 	t->highs[t->num_targets++] = tgt->begin + tgt->len - 1;
 
-	if (!tgt->num_discard_requests)
-		t->discards_supported = 0;
+	if (!tgt->num_discard_requests && tgt->discards_supported)
+		DMWARN("%s: %s: must set num_discard_requests if discards_supported, "
+		       "disabling discards", dm_device_name(t->md), type);
 
 	return 0;
 
@@ -1438,9 +1437,6 @@ bool dm_table_supports_discards(struct d
 	struct dm_target *ti;
 	unsigned i = 0;
 
-	if (!t->discards_supported)
-		return 0;
-
 	/*
 	 * Unless any target used by the table set discards_supported,
 	 * require at least one underlying device to support discards.
@@ -1451,6 +1447,9 @@ bool dm_table_supports_discards(struct d
 	while (i < dm_table_get_num_targets(t)) {
 		ti = dm_table_get_target(t, i++);
 
+		if (!ti->num_discard_requests)
+			continue;
+
 		if (ti->discards_supported)
 			return 1;