[PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[Mears, Morgan]

Add the ability to set the era counter maintained by the dm-cache era
policy shim to an arbitrary 32-bit value, to allow era rollback after
the underlying device is restored from a snapshot.

Signed-off-by: Morgan Mears <mmears@netapp.com>
---
 drivers/md/dm-cache-policy-era.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/md/dm-cache-policy-era.c b/drivers/md/dm-cache-policy-era.c index d4fc0b0..c4cf2ba 100644
--- a/drivers/md/dm-cache-policy-era.c
+++ b/drivers/md/dm-cache-policy-era.c
@@ -155,6 +155,26 @@ static int incr_era_counter(struct era_policy *era, const char *curr_era_str,
 	return r;
 }
 
+static int set_era_counter(struct era_policy *era, const char *new_era_str,
+			   era_match_fn_t dummy)
+{
+	era_t new_era_counter;
+
+	/*
+	 * Set the era counter to a user-provided value, as long as it is
+	 * in range.
+	 */
+
+	if (kstrtou32(new_era_str, 10, &new_era_counter))
+		return -EINVAL;
+	if (new_era_counter > ERA_MAX_ERA)
+		return -EOVERFLOW;
+	era->era_counter = new_era_counter;
+	smp_wmb();
+
+	return 0;
+}
+
 static void *era_cblock_to_hint(struct shim_walk_map_ctx *ctx,
 				dm_cblock_t cblock, dm_oblock_t oblock)
 {
@@ -404,6 +424,7 @@ static int era_set_config_value(struct dm_cache_policy *p, const char *key,
 	struct era_policy *era = to_era_policy(p);
 	struct config_value_handler *vh, value_handlers[] = {
 		{ "increment_era_counter",                  incr_era_counter,  NULL },
+		{ "set_era_counter",                        set_era_counter,   NULL },
 		{ "unmap_blocks_from_later_eras",           cond_unmap_by_era, era_is_gt_value },
 		{ "unmap_blocks_from_this_era_and_later",   cond_unmap_by_era, era_is_gte_value },
 		{ "unmap_blocks_from_this_era_and_earlier", cond_unmap_by_era, era_is_lte_value },
-- 

Re: [PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[Joe Thornber]

On Wed, Nov 06, 2013 at 08:59:38PM +0000, Mears, Morgan wrote:
> Add the ability to set the era counter maintained by the dm-cache era
> policy shim to an arbitrary 32-bit value, to allow era rollback after
> the underlying device is restored from a snapshot.

I wonder if we should pass in the old value, and have the call fail if
the old value is incorrect.  This would allow applications to spot if
they were competing to set the era.  Some thing like:

     set_era_counter <old value>:<new value>

> +	era->era_counter = new_era_counter;
> +	smp_wmb();

Please stop using smp_rmb() and smp_wmb().  Every time I see it used I
find bugs (and this is no exception).  Use higher level locking
abstractions (eg, spin locks), and only optimise if we have a
performance issue.

In general alarm bells ring if you use one of smp_*() without the
other.  See linux/Documentation/memory-barriers.txt for lots of
discussion.

- Joe

Re: [PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[Mears, Morgan]

On Thurs, Nov 07, 2013 at 03:51PM +0000, Joe Thornber wrote:
> On Wed, Nov 06, 2013 at 08:59:38PM +0000, Mears, Morgan wrote:
> > Add the ability to set the era counter maintained by the dm-cache era
> > policy shim to an arbitrary 32-bit value, to allow era rollback after
> > the underlying device is restored from a snapshot.
> 
> I wonder if we should pass in the old value, and have the call fail if the old
> value is incorrect.  This would allow applications to spot if they were
> competing to set the era.  Some thing like:
> 
>      set_era_counter <old value>:<new value>

Yes, I like this.  My inclination is to make the <old value>: portion optional so that the counter value can be forced if desired (for example, to set it to a saved value during a create); objections?

> > +	era->era_counter = new_era_counter;
> > +	smp_wmb();
> 
> Please stop using smp_rmb() and smp_wmb().  Every time I see it used I find
> bugs (and this is no exception).  Use higher level locking abstractions (eg, spin
> locks), and only optimise if we have a performance issue.
> 
> In general alarm bells ring if you use one of smp_*() without the other.  See
> linux/Documentation/memory-barriers.txt for lots of discussion.

Thanks Joe, will fix.

Re: [PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[Joe Thornber]

On Thu, Nov 07, 2013 at 09:24:44PM +0000, Mears, Morgan wrote:
> On Thurs, Nov 07, 2013 at 03:51PM +0000, Joe Thornber wrote:
> > On Wed, Nov 06, 2013 at 08:59:38PM +0000, Mears, Morgan wrote:
> > > Add the ability to set the era counter maintained by the dm-cache era
> > > policy shim to an arbitrary 32-bit value, to allow era rollback after
> > > the underlying device is restored from a snapshot.
> > 
> > I wonder if we should pass in the old value, and have the call fail if the old
> > value is incorrect.  This would allow applications to spot if they were
> > competing to set the era.  Some thing like:
> > 
> >      set_era_counter <old value>:<new value>
> 
> Yes, I like this.  My inclination is to make the <old value>: portion optional so that the counter value can be forced if desired (for example, to set it to a saved value during a create); objections?

In this case userspace should make a status request to get the current
era, then send the message in the <old value>:<new value> format.  If
it fails then clearly another process is interfering and you have big
issues.

> > In general alarm bells ring if you use one of smp_*() without the other.  See
> > linux/Documentation/memory-barriers.txt for lots of discussion.
> 
> Thanks Joe, will fix.

Already done in my cache-coherency-changes branch.  I've changed the
current_era counter to be an atomic64_t.

- Joe

Re: [PATCH] Add a set_era_counter config key to the dm-cache era policy shim

[Mears, Morgan]

On Fri, Nov 08, 2013 at 06:39:22AM -0500, Joe Thornber wrote:
> On Thu, Nov 07, 2013 at 09:24:44PM +0000, Mears, Morgan wrote:
> > On Thurs, Nov 07, 2013 at 03:51PM +0000, Joe Thornber wrote:
> > > On Wed, Nov 06, 2013 at 08:59:38PM +0000, Mears, Morgan wrote:
> > > > Add the ability to set the era counter maintained by the dm-cache
> > > > era policy shim to an arbitrary 32-bit value, to allow era
> > > > rollback after the underlying device is restored from a snapshot.
> > >
> > > I wonder if we should pass in the old value, and have the call fail
> > > if the old value is incorrect.  This would allow applications to
> > > spot if they were competing to set the era.  Some thing like:
> > >
> > >      set_era_counter <old value>:<new value>
> >
> > Yes, I like this.  My inclination is to make the <old value>: portion optional
> so that the counter value can be forced if desired (for example, to set it to a
> saved value during a create); objections?
> 
> In this case userspace should make a status request to get the current era,
> then send the message in the <old value>:<new value> format.  If it fails then
> clearly another process is interfering and you have big issues.

The use case I was concerned about is setting the initial era counter value
during cache creation.  In this case no status request is possible.   However
after more thought, this is not an issue (since the set_era_counter is
processed before the era counter value is recovered from the metadata), so
agreed, the old value should be required.