| Message ID | 33A0129EBFD46748804DE81B354CA1B21C028E44@SACEXCMBX06-PRD.hq.netapp.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 },
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(+)