Re: [PATCH v2 05/17] bcache: fix super block seq numbers comparision in register_cache_set()

[Hannes Reinecke <hare@suse.de>]

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
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer