[v2,05/17] bcache: fix super block seq numbers comparision in register_cache_set()
diff mbox series

Message ID 20200715054612.6349-6-colyli@suse.de
State New
Headers show
Series
  • bcache: extend bucket size to 32bit width
Related show

Commit Message

Coly Li July 15, 2020, 5:46 a.m. UTC
In register_cache_set(), c is pointer to struct cache_set, and ca is
pointer to struct cache, if ca->sb.seq > c->sb.seq, it means this
registering cache has up to date version and other members, the in-
memory version and other members should be updated to the newer value.

But current implementation makes a cache set only has a single cache
device, so the above assumption works well except for a special case.
The execption is when a cache device new created and both ca->sb.seq and
c->sb.seq are 0, because the super block is never flushed out yet. In
the location for the following if() check,
2156         if (ca->sb.seq > c->sb.seq) {
2157                 c->sb.version           = ca->sb.version;
2158                 memcpy(c->sb.set_uuid, ca->sb.set_uuid, 16);
2159                 c->sb.flags             = ca->sb.flags;
2160                 c->sb.seq               = ca->sb.seq;
2161                 pr_debug("set version = %llu\n", c->sb.version);
2162         }
c->sb.version is not initialized yet and valued 0. When ca->sb.seq is 0,
the if() check will fail (because both values are 0), and the cache set
version, set_uuid, flags and seq won't be updated.

The above problem is hiden for current code, because the bucket size is
compatible among different super block version. And the next time when
running cache set again, ca->sb.seq will be larger than 0 and cache set
super block version will be updated properly.

But if the large bucket feature is enabled,  sb->bucket_size is the low
16bits of the bucket size. For a power of 2 value, when the actual
bucket size exceeds 16bit width, sb->bucket_size will always be 0. Then
read_super_basic() will fail because the if() check to
is_power_of_2(sb->bucket_size) is false. This is how the long time
hidden bug is triggered.

This patch modifies the if() check to the following way,
2156         if (ca->sb.seq > c->sb.seq || c->sb.seq == 0) {
Then cache set's version, set_uuid, flags and seq will always be updated
corectly including for a new created cache device.

Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/super.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Hannes Reinecke July 15, 2020, 6:10 a.m. UTC | #1
On 7/15/20 7:46 AM, Coly Li wrote:
> In register_cache_set(), c is pointer to struct cache_set, and ca is
> pointer to struct cache, if ca->sb.seq > c->sb.seq, it means this
> registering cache has up to date version and other members, the in-
> memory version and other members should be updated to the newer value.
> 
> But current implementation makes a cache set only has a single cache
> device, so the above assumption works well except for a special case.
> The execption is when a cache device new created and both ca->sb.seq and
> c->sb.seq are 0, because the super block is never flushed out yet. In
> the location for the following if() check,
> 2156         if (ca->sb.seq > c->sb.seq) {
> 2157                 c->sb.version           = ca->sb.version;
> 2158                 memcpy(c->sb.set_uuid, ca->sb.set_uuid, 16);
> 2159                 c->sb.flags             = ca->sb.flags;
> 2160                 c->sb.seq               = ca->sb.seq;
> 2161                 pr_debug("set version = %llu\n", c->sb.version);
> 2162         }
> c->sb.version is not initialized yet and valued 0. When ca->sb.seq is 0,
> the if() check will fail (because both values are 0), and the cache set
> version, set_uuid, flags and seq won't be updated.
> 
> The above problem is hiden for current code, because the bucket size is
> compatible among different super block version. And the next time when
> running cache set again, ca->sb.seq will be larger than 0 and cache set
> super block version will be updated properly.
> 
> But if the large bucket feature is enabled,  sb->bucket_size is the low
> 16bits of the bucket size. For a power of 2 value, when the actual
> bucket size exceeds 16bit width, sb->bucket_size will always be 0. Then
> read_super_basic() will fail because the if() check to
> is_power_of_2(sb->bucket_size) is false. This is how the long time
> hidden bug is triggered.
> 
> This patch modifies the if() check to the following way,
> 2156         if (ca->sb.seq > c->sb.seq || c->sb.seq == 0) {
> Then cache set's version, set_uuid, flags and seq will always be updated
> corectly including for a new created cache device.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> ---
>   drivers/md/bcache/super.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 1d5b2b3ad93a..0a51ea857e3f 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -2146,7 +2146,14 @@ static const char *register_cache_set(struct cache *ca)
>   	    sysfs_create_link(&c->kobj, &ca->kobj, buf))
>   		goto err;
>   
> -	if (ca->sb.seq > c->sb.seq) {
> +	/*
> +	 * A special case is both ca->sb.seq and c->sb.seq are 0,
> +	 * such condition happens on a new created cache device whose
> +	 * super block is never flushed yet. In this case c->sb.version
> +	 * and other members should be updated too, otherwise we will
> +	 * have a mistaken super block version in cache set.
> +	 */
> +	if (ca->sb.seq > c->sb.seq || c->sb.seq == 0) {
>   		c->sb.version		= ca->sb.version;
>   		memcpy(c->sb.set_uuid, ca->sb.set_uuid, 16);
>   		c->sb.flags             = ca->sb.flags;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Patch
diff mbox series

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 1d5b2b3ad93a..0a51ea857e3f 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -2146,7 +2146,14 @@  static const char *register_cache_set(struct cache *ca)
 	    sysfs_create_link(&c->kobj, &ca->kobj, buf))
 		goto err;
 
-	if (ca->sb.seq > c->sb.seq) {
+	/*
+	 * A special case is both ca->sb.seq and c->sb.seq are 0,
+	 * such condition happens on a new created cache device whose
+	 * super block is never flushed yet. In this case c->sb.version
+	 * and other members should be updated too, otherwise we will
+	 * have a mistaken super block version in cache set.
+	 */
+	if (ca->sb.seq > c->sb.seq || c->sb.seq == 0) {
 		c->sb.version		= ca->sb.version;
 		memcpy(c->sb.set_uuid, ca->sb.set_uuid, 16);
 		c->sb.flags             = ca->sb.flags;