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 03-26-2012, 02:15 PM
Joe Thornber
 
Default dm thin: fix pool target flags that control discard

On Fri, Mar 23, 2012 at 05:55:02PM -0400, Mike Snitzer wrote:
> On Fri, Mar 23 2012 at 8:37am -0400,
> Alasdair G Kergon <agk@redhat.com> wrote:
>
> > On Fri, Mar 16, 2012 at 03:22:34PM +0000, Joe Thornber wrote:
> > > + 'ignore_discard': disable discard support
> > > + 'no_discard_passdown': don't pass discards down to the underlying data device
> >
> > If someone reloads the pool target changing either of these options, how do connected
> > thin targets pick up the change? If they don't pick it up automatically, how do you
> > determine whether they did or didn't pick it up - is that internal state not
> > exposed to userspace yet?
>
> Here are various fixes for dm_thin-add-pool-target-flags-to-control-discard.patch
>
> o wire up 'discard_passdown' so that it does control whether or not the
> discard is passed to the pool's underlying data device

This already works, see test_{enable,disable}_passdown() tests here:

https://github.com/jthornber/thinp-test-suite/blob/master/discard_tests.rb

You've got to remember that there are 2 different interacting targets:

i) discard enabled in the 'thin' target will cause mappings to be removed from the btree.

ii) discard enabled in the 'pool' device will cause discards to be passed down.

The following logic is in the pool_ctr:

if (pf.discard_enabled && pf.discard_passdown) {
ti->discards_supported = 1;
ti->num_discard_requests = 1;
}


> o disallow disabling discards if a pool was already configured with
> discards enabled (the default)
> - justification is inlined in the code
> - required pool_ctr knowing whether pool was created or not; so added
> 'created' flag to __pool_find()

ack, this needs fixing.

> o if 'discard_passdown' is enabled (the default) verify that the pool's
> data device supports discards; if not warn and disable discard_passdown

Why? Do other targets do this? (no)

> o the pool device's discard support should _not_ be limited by whether
> 'discard_passdown' is enabled or not.

Yes it should.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-26-2012, 03:33 PM
Mike Snitzer
 
Default dm thin: fix pool target flags that control discard

On Mon, Mar 26 2012 at 10:15am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Mar 23, 2012 at 05:55:02PM -0400, Mike Snitzer wrote:
> > On Fri, Mar 23 2012 at 8:37am -0400,
> > Alasdair G Kergon <agk@redhat.com> wrote:
> >
> > > On Fri, Mar 16, 2012 at 03:22:34PM +0000, Joe Thornber wrote:
> > > > + 'ignore_discard': disable discard support
> > > > + 'no_discard_passdown': don't pass discards down to the underlying data device
> > >
> > > If someone reloads the pool target changing either of these options, how do connected
> > > thin targets pick up the change? If they don't pick it up automatically, how do you
> > > determine whether they did or didn't pick it up - is that internal state not
> > > exposed to userspace yet?
> >
> > Here are various fixes for dm_thin-add-pool-target-flags-to-control-discard.patch
> >
> > o wire up 'discard_passdown' so that it does control whether or not the
> > discard is passed to the pool's underlying data device
>
> This already works, see test_{enable,disable}_passdown() tests here:
>
> https://github.com/jthornber/thinp-test-suite/blob/master/discard_tests.rb
>
> You've got to remember that there are 2 different interacting targets:
>
> i) discard enabled in the 'thin' target will cause mappings to be removed from the btree.
>
> ii) discard enabled in the 'pool' device will cause discards to be passed down.
>
> The following logic is in the pool_ctr:
>
> if (pf.discard_enabled && pf.discard_passdown) {
> ti->discards_supported = 1;
> ti->num_discard_requests = 1;
> }

We reasoned through this already, your code did work. But for the
benefit of others this is why it worked:

thin will process_discard() via bio being deferring in thin_bio_map() --
provided pf.discard_enabled. Then thin will pass the discard on to the
pool device regardless of pf.discard_passdown. dm-core will then drop
the discard because ti->num_discard_requests is 0; but that results in
-EOPNOTSUPP.

Thing is, we don't want to return -EOPNOTSUPP if passdown was disabled.
So I think my change is an improvement (because process_prepared_discard
will now properly bio_endio(m->bio, 0) the discard).

> > o disallow disabling discards if a pool was already configured with
> > discards enabled (the default)
> > - justification is inlined in the code
> > - required pool_ctr knowing whether pool was created or not; so added
> > 'created' flag to __pool_find()
>
> ack, this needs fixing.

We've discussed that it is easier to just disallow changing discard
configuration. And that there was a bug in my early return. So you'll
be sending a follow-up fixup patch.

> > o if 'discard_passdown' is enabled (the default) verify that the pool's
> > data device supports discards; if not warn and disable discard_passdown
>
> Why? Do other targets do this? (no)

The thin target is the first target to use ti->discards_supported.
Setting that will cause dm_table_supports_discards to skip checking if
the target's underlying device(s) natively support discards.

So passdown needs to only be allowed if the underlying data device
supports discard -- otherwise we'll get a bunch of failed discards from
the SCSI layer, etc.

> > o the pool device's discard support should _not_ be limited by whether
> > 'discard_passdown' is enabled or not.
>
> Yes it should.

Yeah, like I said above your approach works (except for the
-EOPNOTSUPP). I just happen to prefer dm-thin taking a more active role
by short-circuting the discard_passdown directly.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-26-2012, 03:34 PM
Joe Thornber
 
Default dm thin: fix pool target flags that control discard

On Mon, Mar 26, 2012 at 03:15:21PM +0100, Joe Thornber wrote:
> > o disallow disabling discards if a pool was already configured with
> > discards enabled (the default)
> > - justification is inlined in the code
> > - required pool_ctr knowing whether pool was created or not; so added
> > 'created' flag to __pool_find()
>
> ack, this needs fixing.

The following patch prevents people changing the top-level discard
flag when reloading a pool target.

I'll push this test in a bit (dm-cache changes holding things up).

# we don't allow people to change their minds about top level
# discard support.
def test_change_discard_with_reload_fails
with_standard_pool(@size, :discard => true) do |pool|
assert_raise(ExitError) do
table = Table.new(ThinPool.new(@size, @metadata_dev, @data_dev,
@data_block_size, @low_water_mark, false, false, false))
pool.load(table)
end
end

with_standard_pool(@size, :discard => false) do |pool|
assert_raise(ExitError) do
table = Table.new(ThinPool.new(@size, @metadata_dev, @data_dev,
@data_block_size, @low_water_mark, false, true, false))
pool.load(table)
end
end
end


- Joe


commit 9f075e7f5330039addba213a0f512ab7c0ea3ecf
Author: Joe Thornber <ejt@redhat.com>
Date: Mon Mar 26 16:26:33 2012 +0100

dm-thin: don't allow the top level discard flag to be changed on reload.

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index e8fe5db..e5a6ed4 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -1445,10 +1445,12 @@ static void __pool_dec(struct pool *pool)

static struct pool *__pool_find(struct mapped_device *pool_md,
struct block_device *metadata_dev,
- unsigned long block_size, char **error)
+ unsigned long block_size, char **error,
+ int *created)
{
struct pool *pool = __pool_table_lookup_metadata_dev(metadata_dev);

+ *created = 0;
if (pool) {
if (pool->pool_md != pool_md)
return ERR_PTR(-EBUSY);
@@ -1461,8 +1463,10 @@ static struct pool *__pool_find(struct mapped_device *pool_md,
return ERR_PTR(-EINVAL);
__pool_inc(pool);

- } else
+ } else {
pool = pool_create(pool_md, metadata_dev, block_size, error);
+ *created = 1;
+ }
}

return pool;
@@ -1542,7 +1546,7 @@ static int parse_pool_features(struct dm_arg_set *as, struct pool_features *pf,
*/
static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
- int r;
+ int r, pool_created;
struct pool_c *pt;
struct pool *pool;
struct pool_features pf;
@@ -1617,12 +1621,24 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
}

pool = __pool_find(dm_table_get_md(ti->table), metadata_dev->bdev,
- block_size, &ti->error);
+ block_size, &ti->error, &pool_created);
if (IS_ERR(pool)) {
r = PTR_ERR(pool);
goto out_free_pt;
}

+ /*
+ * 'pool_created' reflects whether this is the first table load.
+ * Top level discard support is not allowed to be changed after
+ * initial load. This would require a pool reload to trigger thin
+ * device changes.
+ */
+ if (!pool_created && pf.discard_enabled != pool->pf.discard_enabled) {
+ ti->error = "Discard support cannot be changed once enabled";
+ r = -EINVAL;
+ goto out_flags_changed;
+ }
+
pt->pool = pool;
pt->ti = ti;
pt->metadata_dev = metadata_dev;
@@ -1643,6 +1659,8 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)

return 0;

+out_flags_changed:
+ __pool_dec(pool);
out_free_pt:
kfree(pt);
out:

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-26-2012, 03:46 PM
Joe Thornber
 
Default dm thin: fix pool target flags that control discard

commit 1f04d92a1dd6173e38d7dc652dfe22876c40957e
Author: Joe Thornber <ejt@redhat.com>
Date: Mon Mar 26 16:43:36 2012 +0100

dm-thin: don't set ti->discards_supported for the pool target

The pool is little more than a linear target; the default dm decision
about whether to support discards is adequate.

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index e5a6ed4..015cdce 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -1646,10 +1646,8 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
pt->low_water_blocks = low_water_blocks;
pt->pf = pf;
ti->num_flush_requests = 1;
- if (pf.discard_enabled && pf.discard_passdown) {
- ti->discards_supported = 1;
+ if (pf.discard_enabled && pf.discard_passdown)
ti->num_discard_requests = 1;
- }
ti->private = pt;

pt->callbacks.congested_fn = pool_is_congested;

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-26-2012, 07:56 PM
Mike Snitzer
 
Default dm thin: fix pool target flags that control discard

On Mon, Mar 26 2012 at 11:33am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Mar 26 2012 at 10:15am -0400,
> Joe Thornber <thornber@redhat.com> wrote:
>
> > On Fri, Mar 23, 2012 at 05:55:02PM -0400, Mike Snitzer wrote:
> > > On Fri, Mar 23 2012 at 8:37am -0400,
> > > Alasdair G Kergon <agk@redhat.com> wrote:
> > >
> > > > On Fri, Mar 16, 2012 at 03:22:34PM +0000, Joe Thornber wrote:
> > > > > + 'ignore_discard': disable discard support
> > > > > + 'no_discard_passdown': don't pass discards down to the underlying data device
> > > >
> > > > If someone reloads the pool target changing either of these options, how do connected
> > > > thin targets pick up the change? If they don't pick it up automatically, how do you
> > > > determine whether they did or didn't pick it up - is that internal state not
> > > > exposed to userspace yet?
> > >
> > > Here are various fixes for dm_thin-add-pool-target-flags-to-control-discard.patch
> > >
> > > o wire up 'discard_passdown' so that it does control whether or not the
> > > discard is passed to the pool's underlying data device
> >
> > This already works, see test_{enable,disable}_passdown() tests here:
> >
> > https://github.com/jthornber/thinp-test-suite/blob/master/discard_tests.rb
> >
> > You've got to remember that there are 2 different interacting targets:
> >
> > i) discard enabled in the 'thin' target will cause mappings to be removed from the btree.
> >
> > ii) discard enabled in the 'pool' device will cause discards to be passed down.
> >
> > The following logic is in the pool_ctr:
> >
> > if (pf.discard_enabled && pf.discard_passdown) {
> > ti->discards_supported = 1;
> > ti->num_discard_requests = 1;
> > }
>
> We reasoned through this already, your code did work. But for the
> benefit of others this is why it worked:
>
> thin will process_discard() via bio being deferring in thin_bio_map() --
> provided pf.discard_enabled. Then thin will pass the discard on to the
> pool device regardless of pf.discard_passdown. dm-core will then drop
> the discard because ti->num_discard_requests is 0; but that results in
> -EOPNOTSUPP.
>
> Thing is, we don't want to return -EOPNOTSUPP if passdown was disabled.
> So I think my change is an improvement (because process_prepared_discard
> will now properly bio_endio(m->bio, 0) the discard).
>
> > > o disallow disabling discards if a pool was already configured with
> > > discards enabled (the default)
> > > - justification is inlined in the code
> > > - required pool_ctr knowing whether pool was created or not; so added
> > > 'created' flag to __pool_find()
> >
> > ack, this needs fixing.
>
> We've discussed that it is easier to just disallow changing discard
> configuration. And that there was a bug in my early return. So you'll
> be sending a follow-up fixup patch.

Here is a fixup patch that builds on what Alasdair already has staged
in dm_thin-add-pool-target-flags-to-control-discard.patch

I've added comments to help explain the subtlety of the initial thinp
discard code.

---
drivers/md/dm-thin.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)

Index: linux/drivers/md/dm-thin.c
================================================== =================
--- linux.orig/drivers/md/dm-thin.c
+++ linux/drivers/md/dm-thin.c
@@ -1972,19 +1972,20 @@ static int pool_ctr(struct dm_target *ti

/*
* 'pool_created' reflects whether this is the first table load.
- * Discard support is not allowed to be disabled after initial load.
- * Disabling it would require a pool reload to trigger thin device
- * changes (e.g. ti->discards_supported and QUEUE_FLAG_DISCARD).
+ * Top level discard support is not allowed to be changed after
+ * initial load. This would require a pool reload to trigger thin
+ * device changes.
*/
- if (!pool_created && !pf.discard_enabled && pool->pf.discard_enabled) {
+ if (!pool_created && pf.discard_enabled != pool->pf.discard_enabled) {
ti->error = "Discard support cannot be disabled once enabled";
r = -EINVAL;
- goto out_free_pt;
+ goto out_flags_changed;
}

/*
* If discard_passdown was enabled verify that the data device
- * supports discards. Disable discard_passdown if not.
+ * supports discards. Disable discard_passdown if not; otherwise
+ * -EOPNOTSUPP will be returned.
*/
if (pf.discard_passdown) {
struct request_queue *q = bdev_get_queue(data_dev->bdev);
@@ -2001,8 +2002,18 @@ static int pool_ctr(struct dm_target *ti
pt->low_water_blocks = low_water_blocks;
pt->pf = pf;
ti->num_flush_requests = 1;
- if (pf.discard_enabled) {
+ /*
+ * Only need to enable discards if the pool should pass
+ * them down to the data device. The thin device's discard
+ * processing will cause mappings to be removed from the btree.
+ */
+ if (pf.discard_enabled && pf.discard_passdown) {
ti->num_discard_requests = 1;
+ /*
+ * Setting 'discards_supported' circumvents the normal
+ * stacking of discard limits (this keeps the pool and
+ * thin devices' discard limits consistent).
+ */
ti->discards_supported = 1;
}
ti->private = pt;
@@ -2014,6 +2025,8 @@ static int pool_ctr(struct dm_target *ti

return 0;

+out_flags_changed:
+ __pool_dec(pool);
out_free_pt:
kfree(pt);
out:
@@ -2408,6 +2421,9 @@ static int pool_merge(struct dm_target *

static void set_discard_limits(struct pool *pool, struct queue_limits *limits)
{
+ /*
+ * FIXME: these limits may be incompatible with the pool's data device
+ */
limits->max_discard_sectors = pool->sectors_per_block;

/*

--
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 08:04 PM.

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