[PATCH 0/6] bcache-tools: improve large bucket on-disk layout

[PATCH 0/6] bcache-tools: improve large bucket on-disk layout

[Coly Li]

Current large bucket layout is a preparation for the future zoned cache
device, by which the cache device bucket can be aligned to zone size and
boundary of the zoned SSD.

Original patch introduces bucket_size_hi in struct cache_sb_disk, this
is problematic and can be improved,
- The bucket size is always power-of-2 value, it is unnecessary to add
  extra space in struct cache_sb_disk to store the real size value. Just
  using existing bucket_size to store the order of power-of-2 value is
  enough.
- The added bucket_size_hi is behind d[SB_JOURNAL_BUCKETS] in struct
  cache_sb_disk, which breaks a very implicit restriction from macro
  csum_set(). This restriction requires member d[] must be the last
  member in data structure to calculate the checksum.

This series is to improve the above issue from user space tools. Basic
ideas of this series are,
- Add BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE incompat feature bit.
  When it is set, for bucket size >16MB, bucket_size stores the order
  of power-of-2 bucket size value. Then no extra space introduced and
  csum_set() has all data to calculate super block checksum.
- Rename struct cache_sb_disk's bucket_size_hi to obso_bucket_size_hi,
  and rename the incompat feature bit BCH_FEATURE_INCOMPAT_LARGE_BUCKET
  to BCH_FEATURE_INCOMPAT_OBSO_LARGE_BUCKET, to indicate they are not
  used anymore and obsoleted.
- If a cache device was created for bucket size >16MB with obsoleted
  layout, "bcache show" command still display the expected bucket size
  value for legacy compatibility purpose.

Although there almost no one else uses bucket size >16MB except me (the
btree node usage code is not optimized yet), it is still necessary to
fix and improve the layout usage as soon as possible.

Coly Li
---
Coly Li (6):
  bcache.h: fix typo from SUUP to SUPP
  bcache-tools: only call set_bucket_size() for cache device
  bcache.h: add BCH_FEATURE_INCOMPAT_LARGE_BUCKET to
    BCH_FEATURE_INCOMPAT_SUPP
  bcache-tools: check incompatible feature set
  bcache-tools: introduce BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE for
    large bucket
  bcache-tools: display obsoleted bucket size configuration

 bcache.h   | 17 +++++++++++------
 features.c |  4 +++-
 lib.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
 lib.h      |  2 +-
 make.c     |  3 ++-
 5 files changed, 65 insertions(+), 13 deletions(-)

-- 
2.26.2

[PATCH 1/6] bcache.h: fix typo from SUUP to SUPP

[Coly Li]

This patch fixes the following typos,
from BCH_FEATURE_COMPAT_SUUP to BCH_FEATURE_COMPAT_SUPP
from BCH_FEATURE_INCOMPAT_SUUP to BCH_FEATURE_INCOMPAT_SUPP
from BCH_FEATURE_INCOMPAT_SUUP to BCH_FEATURE_RO_COMPAT_SUPP

Signed-off-by: Coly Li <colyli@suse.de>
---
 bcache.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bcache.h b/bcache.h
index 6aef9c4..50dd2b5 100644
--- a/bcache.h
+++ b/bcache.h
@@ -200,9 +200,9 @@ uint64_t crc64(const void *data, size_t len);
 #define BCH_FEATURE_INCOMPAT	2
 #define BCH_FEATURE_TYPE_MASK	0x03
 
-#define BCH_FEATURE_COMPAT_SUUP		0
-#define BCH_FEATURE_INCOMPAT_SUUP	0
-#define BCH_FEATURE_RO_COMPAT_SUUP	0
+#define BCH_FEATURE_COMPAT_SUPP		0
+#define BCH_FEATURE_INCOMPAT_SUPP	0
+#define BCH_FEATURE_RO_COMPAT_SUPP	0
 
 #define BCH_HAS_COMPAT_FEATURE(sb, mask) \
 		((sb)->feature_compat & (mask))
-- 
2.26.2

[PATCH 2/6] bcache-tools: only call set_bucket_size() for cache device

[Coly Li]

It doesn't make sense to set bucket size for backing device, this patch
moves set_bucket_size() into the code block for cache device only.

Signed-off-by: Coly Li <colyli@suse.de>
---
 make.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/make.c b/make.c
index ad89377..a3f97f6 100644
--- a/make.c
+++ b/make.c
@@ -340,7 +340,6 @@ static void write_sb(char *dev, unsigned int block_size,
 	uuid_generate(sb.uuid);
 	memcpy(sb.set_uuid, set_uuid, sizeof(sb.set_uuid));
 
-	set_bucket_size(&sb, bucket_size);
 	sb.block_size	= block_size;
 
 	uuid_unparse(sb.uuid, uuid_str);
@@ -383,6 +382,8 @@ static void write_sb(char *dev, unsigned int block_size,
 		       data_offset);
 		putchar('\n');
 	} else {
+		set_bucket_size(&sb, bucket_size);
+
 		sb.nbuckets		= getblocks(fd) / sb.bucket_size;
 		sb.nr_in_set		= 1;
 		/* 23 is (SB_SECTOR + SB_SIZE) - 1 sectors */
-- 
2.26.2

[PATCH 3/6] bcache.h: add BCH_FEATURE_INCOMPAT_LARGE_BUCKET to BCH_FEATURE_INCOMPAT_SUPP

[Coly Li]

BCH_FEATURE_INCOMPAT_LARGE_BUCKET is a feature to support 32bits bucket
size, which is incompatible feature for existing on-disk layout. This
patch adds this feature bit to BCH_FEATURE_INCOMPAT_SUPP feature set.

Signed-off-by: Coly Li <colyli@suse.de>
---
 bcache.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bcache.h b/bcache.h
index 50dd2b5..58e973c 100644
--- a/bcache.h
+++ b/bcache.h
@@ -201,8 +201,8 @@ uint64_t crc64(const void *data, size_t len);
 #define BCH_FEATURE_TYPE_MASK	0x03
 
 #define BCH_FEATURE_COMPAT_SUPP		0
-#define BCH_FEATURE_INCOMPAT_SUPP	0
 #define BCH_FEATURE_RO_COMPAT_SUPP	0
+#define BCH_FEATURE_INCOMPAT_SUPP	BCH_FEATURE_INCOMPAT_LARGE_BUCKET
 
 #define BCH_HAS_COMPAT_FEATURE(sb, mask) \
 		((sb)->feature_compat & (mask))
-- 
2.26.2

[PATCH 4/6] bcache-tools: check incompatible feature set

[Coly Li]

When bcache-tools is not updated with kernel driver (or on the versus),
we need to make sure the feature set conflict can be detected to avoid
unexpected data operation.

This patch checks whether there is unsupported {compatc, ro_compat,
incompat} features detected in detail_dev(). If there is, prints out
error message and return error to its caller.

Signed-off-by: Coly Li <colyli@suse.de>
---
 lib.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib.c b/lib.c
index 29172f5..a529ad3 100644
--- a/lib.c
+++ b/lib.c
@@ -442,6 +442,33 @@ int detail_dev(char *devname, struct bdev *bd, struct cdev *cd, int *type)
 		goto Fail;
 	}
 
+	/* Check for incompat feature set */
+	if (sb.version >= BCACHE_SB_VERSION_BDEV_WITH_FEATURES ||
+	    sb.version >= BCACHE_SB_VERSION_CDEV_WITH_FEATURES) {
+		uint64_t features;
+
+		features = sb.feature_compat & ~BCH_FEATURE_COMPAT_SUPP;
+		if (features) {
+			fprintf(stderr,
+				"Unsupported compatible feature found\n");
+			goto Fail;
+		}
+
+		features = sb.feature_ro_compat & ~BCH_FEATURE_RO_COMPAT_SUPP;
+		if (features) {
+			fprintf(stderr,
+				"Unsupported read-only compatible feature found\n");
+			goto Fail;
+		}
+
+		features = sb.feature_incompat & ~BCH_FEATURE_INCOMPAT_SUPP;
+		if (features) {
+			fprintf(stderr,
+				"Unsupported incompatible feature found\n");
+			goto Fail;
+		}
+	}
+
 	*type = sb.version;
 	if (sb.version == BCACHE_SB_VERSION_BDEV ||
 	    sb.version == BCACHE_SB_VERSION_BDEV_WITH_OFFSET ||
-- 
2.26.2

[PATCH 5/6] bcache-tools: introduce BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE for large bucket

[Coly Li]

When large bucket feature was added, BCH_FEATURE_INCOMPAT_LARGE_BUCKET
was introduced into incompat feature set. It indicates bucket_size_hi is
added at the tail of struct cache_sb_disk, to extend current 16bit
bucket size to 32bit with existing bucket_size in struct cache_sb_disk.

This is not a good idea, there are two obvious problems,
- Bucket size is always value power of 2, if store log2(bucket size) in
  existing bucket_size of struct cache_sb_disk, it is unnecessary to add
  bucket_size_hi.
- Macro csum_set() assumes d[SB_JOURNAL_BUCKETS] is the last member in
  struct cache_sb_disk, bucket_size_hi was added after d[] which makes
  csum_set calculate an unexpected super block checksum.

To fix the above problems, this patch introduces a new incompat feature
bit BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE, when this bit is set, it
means bucket_size in struct cache_sb_disk stores the order of power of
2 bucket size value. When user specifies a bucket size larger than 32768
sectors, BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE will be set to
incompat feature set, and bucket_size stores log2(bucket size) more
than store the real bucket size value.

The obsoleted BCH_FEATURE_INCOMPAT_LARGE_BUCKET won't be used anymore,
bcache-tools only display "obso_large_bucket" for cache device created
with BCH_FEATURE_INCOMPAT_LARGE_BUCKET, kernel driver will make bcache
device as read-only if BCH_FEATURE_INCOMPAT_LARGE_BUCKET is set.

With this change, the unnecessary extra space extend of bcache on-disk
super block can be avoided, and csum_set() may generate expected check
sum as well.

Fixes: 8c063851990a ("bcache-tools: add large_bucket incompat feature")
Signed-off-by: Coly Li <colyli@suse.de>
---
 bcache.h   | 13 +++++++++----
 features.c |  4 +++-
 lib.c      | 17 +++++++++++++++--
 lib.h      |  2 +-
 4 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/bcache.h b/bcache.h
index 58e973c..6dcdbb7 100644
--- a/bcache.h
+++ b/bcache.h
@@ -100,7 +100,7 @@ struct cache_sb_disk {
 		__le16		keys;
 	};
 	__le64			d[SB_JOURNAL_BUCKETS];	/* journal buckets */
-	__le16			bucket_size_hi;
+	__le16			obso_bucket_size_hi;	/* obsoleted */
 };
 
 /*
@@ -202,7 +202,8 @@ uint64_t crc64(const void *data, size_t len);
 
 #define BCH_FEATURE_COMPAT_SUPP		0
 #define BCH_FEATURE_RO_COMPAT_SUPP	0
-#define BCH_FEATURE_INCOMPAT_SUPP	BCH_FEATURE_INCOMPAT_LARGE_BUCKET
+#define BCH_FEATURE_INCOMPAT_SUPP	(BCH_FEATURE_INCOMPAT_OBSO_LARGE_BUCKET| \
+					 BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE)
 
 #define BCH_HAS_COMPAT_FEATURE(sb, mask) \
 		((sb)->feature_compat & (mask))
@@ -214,7 +215,10 @@ uint64_t crc64(const void *data, size_t len);
 /* Feature set definition */
 
 /* Incompat feature set */
-#define BCH_FEATURE_INCOMPAT_LARGE_BUCKET	0x0001 /* 32bit bucket size */
+/* 32bit bucket size, obsoleted */
+#define BCH_FEATURE_INCOMPAT_OBSO_LARGE_BUCKET		0x0001
+/* real bucket size is (1 << bucket_size) */
+#define BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE	0x0002
 
 #define BCH_FEATURE_COMPAT_FUNCS(name, flagname) \
 static inline int bch_has_feature_##name(struct cache_sb *sb) \
@@ -267,6 +271,7 @@ static inline void bch_clear_feature_##name(struct cache_sb *sb) \
 		~BCH##_FEATURE_INCOMPAT_##flagname; \
 }
 
-BCH_FEATURE_INCOMPAT_FUNCS(large_bucket, LARGE_BUCKET);
+BCH_FEATURE_INCOMPAT_FUNCS(obso_large_bucket, OBSO_LARGE_BUCKET);
+BCH_FEATURE_INCOMPAT_FUNCS(large_bucket, LOG_LARGE_BUCKET_SIZE);
 
 #endif
diff --git a/features.c b/features.c
index 181e348..f7f6224 100644
--- a/features.c
+++ b/features.c
@@ -20,7 +20,9 @@ struct feature {
 };
 
 static struct feature feature_list[] = {
-	{BCH_FEATURE_INCOMPAT, BCH_FEATURE_INCOMPAT_LARGE_BUCKET,
+	{BCH_FEATURE_INCOMPAT, BCH_FEATURE_INCOMPAT_OBSO_LARGE_BUCKET,
+		"obso_large_bucket"},
+	{BCH_FEATURE_INCOMPAT, BCH_FEATURE_INCOMPAT_LOG_LARGE_BUCKET_SIZE,
 		"large_bucket"},
 	{0, 0, 0 },
 };
diff --git a/lib.c b/lib.c
index a529ad3..e5624c7 100644
--- a/lib.c
+++ b/lib.c
@@ -21,6 +21,19 @@
  * utils function
  */
 
+static unsigned int log2_u32(uint32_t n)
+{
+	int r = 0;
+
+	n = n >> 1;
+	while (n) {
+		n = n >> 1;
+		r++;
+	}
+
+	return r;
+}
+
 static void trim_prefix(char *dest, char *src, int num)
 {
 	strcpy(dest, src + num);
@@ -772,7 +785,7 @@ struct cache_sb *to_cache_sb(struct cache_sb *sb,
 
 	if (sb->version >= BCACHE_SB_VERSION_CDEV_WITH_FEATURES &&
 	    bch_has_feature_large_bucket(sb))
-		sb->bucket_size += le16_to_cpu(sb_disk->bucket_size_hi) << 16;
+		sb->bucket_size = 1 << le16_to_cpu(sb_disk->bucket_size);
 
 	return sb;
 }
@@ -824,7 +837,7 @@ struct cache_sb_disk *to_cache_sb_disk(struct cache_sb_disk *sb_disk,
 
 	if (sb->version >= BCACHE_SB_VERSION_CDEV_WITH_FEATURES &&
 	    bch_has_feature_large_bucket(sb))
-		sb_disk->bucket_size_hi = cpu_to_le16(sb->bucket_size >> 16);
+		sb_disk->bucket_size = cpu_to_le16(log2_u32(sb->bucket_size));
 
 	return sb_disk;
 }
diff --git a/lib.h b/lib.h
index 9b5ed02..0357a11 100644
--- a/lib.h
+++ b/lib.h
@@ -13,7 +13,7 @@ struct dev {
 	char		label[SB_LABEL_SIZE + 1];
 	char		uuid[40];
 	uint16_t	sectors_per_block;
-	uint16_t	sectors_per_bucket;
+	uint32_t	sectors_per_bucket;
 	char		cset[40];
 	char		state[40];
 	char		bname[40];
-- 
2.26.2

[PATCH 6/6] bcache-tools: display obsoleted bucket size configuration

[Coly Li]

Although BCH_FEATURE_INCOMPAT_LARGE_BUCKET is obsoleted and we don't
support it anymore,  we still need to display the obsoleted bucket
size combines by,
	bucket_size + (obso_bucket_size_hi << 16)
for the legancy consistency purpose.

This patch checks bch_has_feature_obso_large_bucket() in to_cache_sb(),
if it is true, still try to combine and display the bucket size from
obso_bucket_size_hi.

Signed-off-by: Coly Li <colyli@suse.de>
---
 lib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib.c b/lib.c
index e5624c7..0d83c23 100644
--- a/lib.c
+++ b/lib.c
@@ -783,9 +783,13 @@ struct cache_sb *to_cache_sb(struct cache_sb *sb,
 		sb->feature_ro_compat = le64_to_cpu(sb_disk->feature_ro_compat);
 	}
 
-	if (sb->version >= BCACHE_SB_VERSION_CDEV_WITH_FEATURES &&
-	    bch_has_feature_large_bucket(sb))
-		sb->bucket_size = 1 << le16_to_cpu(sb_disk->bucket_size);
+	if (sb->version >= BCACHE_SB_VERSION_CDEV_WITH_FEATURES) {
+		if (bch_has_feature_large_bucket(sb))
+			sb->bucket_size = 1 << le16_to_cpu(sb_disk->bucket_size);
+		else if (bch_has_feature_obso_large_bucket(sb))
+			sb->bucket_size +=
+				le16_to_cpu(sb_disk->obso_bucket_size_hi) << 16;
+	}
 
 	return sb;
 }
-- 
2.26.2