[PATCH v3 0/4] btrfs: support xxhash64 checksums

[PATCH v3 0/4] btrfs: support xxhash64 checksums

[Johannes Thumshirn]

Now that Nikolay's XXHASH64 support for the Crypto API has landed and BTRFS is
prepared for an easy addition of new checksums, this patchset implements
XXHASH64 as a second, fast but not cryptographically secure checksum hash.

For changes since v2, please see the individual patches.

David Sterba (1):
  btrfs: sysfs: export supported checksums

Johannes Thumshirn (3):
  btrfs: turn checksum type define into a enum
  btrfs: create structure to encode checksum type and length
  btrfs: use xxhash64 for checksumming

 fs/btrfs/Kconfig                |  1 +
 fs/btrfs/ctree.h                | 14 +++++++++-----
 fs/btrfs/disk-io.c              |  1 +
 fs/btrfs/super.c                |  1 +
 fs/btrfs/sysfs.c                | 29 +++++++++++++++++++++++++++++
 include/uapi/linux/btrfs_tree.h |  5 ++++-
 6 files changed, 45 insertions(+), 6 deletions(-)

-- 
2.16.4

[PATCH v3 1/4] btrfs: turn checksum type define into a enum

[Johannes Thumshirn]

Turn the checksum type definition into a enum. This eases later addition
of new checksums.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 include/uapi/linux/btrfs_tree.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index 71246c1941aa..b65c7ee75bc7 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -300,7 +300,9 @@
 #define BTRFS_CSUM_SIZE 32
 
 /* csum types */
-#define BTRFS_CSUM_TYPE_CRC32	0
+enum btrfs_csum_type {
+	BTRFS_CSUM_TYPE_CRC32	= 0,
+};
 
 /*
  * flags definitions for directory entry item type
-- 
2.16.4

[PATCH v3 2/4] btrfs: create structure to encode checksum type and length

[Johannes Thumshirn]

Create a structure to encode the type and length for the known on-disk
checksums.

This makes it easier to add new checksums later.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

---
Changes to v2:
- Really remove initializer macro *doh*

Changes to v1:
- Remove initializer macro (David)
---
 fs/btrfs/ctree.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index b161224b5a0b..139354d02dfa 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -82,9 +82,12 @@ struct btrfs_ref;
  */
 #define BTRFS_LINK_MAX 65535U
 
-/* four bytes for CRC32 */
-static const int btrfs_csum_sizes[] = { 4 };
-static const char *btrfs_csum_names[] = { "crc32c" };
+static const struct btrfs_csums {
+	u16		size;
+	const char	*name;
+} btrfs_csums[] = {
+	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
+};
 
 #define BTRFS_EMPTY_DIR_SIZE 0
 
@@ -2207,13 +2210,13 @@ static inline int btrfs_super_csum_size(const struct btrfs_super_block *s)
 	/*
 	 * csum type is validated at mount time
 	 */
-	return btrfs_csum_sizes[t];
+	return btrfs_csums[t].size;
 }
 
 static inline const char *btrfs_super_csum_name(u16 csum_type)
 {
 	/* csum type is validated at mount time */
-	return btrfs_csum_names[csum_type];
+	return btrfs_csums[csum_type].name;
 }
 
 /*
-- 
2.16.4

Re: [PATCH v3 2/4] btrfs: create structure to encode checksum type and length

[David Sterba]

On Mon, Aug 26, 2019 at 01:48:32PM +0200, Johannes Thumshirn wrote:
> Create a structure to encode the type and length for the known on-disk
> checksums.
> 
> This makes it easier to add new checksums later.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> 
> ---
> Changes to v2:
> - Really remove initializer macro *doh*
> 
> Changes to v1:
> - Remove initializer macro (David)
> ---
>  fs/btrfs/ctree.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index b161224b5a0b..139354d02dfa 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -82,9 +82,12 @@ struct btrfs_ref;
>   */
>  #define BTRFS_LINK_MAX 65535U
>  
> -/* four bytes for CRC32 */
> -static const int btrfs_csum_sizes[] = { 4 };
> -static const char *btrfs_csum_names[] = { "crc32c" };
> +static const struct btrfs_csums {
> +	u16		size;
> +	const char	*name;
> +} btrfs_csums[] = {
> +	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> +};

In one of the previous iterations, I pointed out that the definition is
in a header, thus each file that includes "ctree.h" (many of them)
has a private copy of the table. With just crc32c it's just a few bytes
that gets lost in the noise but now the table is going to be larger the
impact will be noticeable.

Re: [PATCH v3 2/4] btrfs: create structure to encode checksum type and length

[David Sterba]

On Wed, Aug 28, 2019 at 09:19:52PM +0200, David Sterba wrote:
> On Mon, Aug 26, 2019 at 01:48:32PM +0200, Johannes Thumshirn wrote:
> > Create a structure to encode the type and length for the known on-disk
> > checksums.
> > 
> > This makes it easier to add new checksums later.
> > 
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > 
> > ---
> > Changes to v2:
> > - Really remove initializer macro *doh*
> > 
> > Changes to v1:
> > - Remove initializer macro (David)
> > ---
> >  fs/btrfs/ctree.h | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index b161224b5a0b..139354d02dfa 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -82,9 +82,12 @@ struct btrfs_ref;
> >   */
> >  #define BTRFS_LINK_MAX 65535U
> >  
> > -/* four bytes for CRC32 */
> > -static const int btrfs_csum_sizes[] = { 4 };
> > -static const char *btrfs_csum_names[] = { "crc32c" };
> > +static const struct btrfs_csums {
> > +	u16		size;
> > +	const char	*name;
> > +} btrfs_csums[] = {
> > +	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> > +};
> 
> In one of the previous iterations, I pointed out that the definition is
> in a header, thus each file that includes "ctree.h" (many of them)
> has a private copy of the table. With just crc32c it's just a few bytes
> that gets lost in the noise but now the table is going to be larger the
> impact will be noticeable.

With definitions moved to ctree.c and only exported the two helpers that
need btrfs_csumss

   text    data     bss     dec     hex filename
1080108   17316   14912 1112336  10f910 btrfs.ko.before
1079655   17316   14912 1111883  10f74b btrfs.ko.after

The difference is 453.

[PATCH v3 3/4] btrfs: use xxhash64 for checksumming

[Johannes Thumshirn]

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/Kconfig                | 1 +
 fs/btrfs/ctree.h                | 1 +
 fs/btrfs/disk-io.c              | 1 +
 fs/btrfs/super.c                | 1 +
 include/uapi/linux/btrfs_tree.h | 1 +
 5 files changed, 5 insertions(+)

diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
index 38651fae7f21..6d5a01c57da3 100644
--- a/fs/btrfs/Kconfig
+++ b/fs/btrfs/Kconfig
@@ -5,6 +5,7 @@ config BTRFS_FS
 	select CRYPTO
 	select CRYPTO_CRC32C
 	select LIBCRC32C
+	select CRYPTO_XXHASH
 	select ZLIB_INFLATE
 	select ZLIB_DEFLATE
 	select LZO_COMPRESS
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 139354d02dfa..c62729d5aa6e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -87,6 +87,7 @@ static const struct btrfs_csums {
 	const char	*name;
 } btrfs_csums[] = {
 	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
+	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
 };
 
 #define BTRFS_EMPTY_DIR_SIZE 0
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 99dfd889b9f7..ac039a4d23ff 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -352,6 +352,7 @@ static bool btrfs_supported_super_csum(u16 csum_type)
 {
 	switch (csum_type) {
 	case BTRFS_CSUM_TYPE_CRC32:
+	case BTRFS_CSUM_TYPE_XXHASH:
 		return true;
 	default:
 		return false;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1b151af25772..60116d0410e5 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2456,3 +2456,4 @@ module_exit(exit_btrfs_fs)
 
 MODULE_LICENSE("GPL");
 MODULE_SOFTDEP("pre: crc32c");
+MODULE_SOFTDEP("pre: xxhash64");
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index b65c7ee75bc7..ba2f125a3a1c 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -302,6 +302,7 @@
 /* csum types */
 enum btrfs_csum_type {
 	BTRFS_CSUM_TYPE_CRC32	= 0,
+	BTRFS_CSUM_TYPE_XXHASH	= 1,
 };
 
 /*
-- 
2.16.4

Re: [PATCH v3 3/4] btrfs: use xxhash64 for checksumming

[Nikolay Borisov]

nit: I'd name this commit  "Add xxhash64 to supported checksum hashes"

Personally, I interpret 'use <some hash> for checksumming' as if you are
modifying code to use that hash. But in fact you are not, at least not
in that patch.

On 26.08.19 г. 14:48 ч., Johannes Thumshirn wrote:
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/Kconfig                | 1 +
>  fs/btrfs/ctree.h                | 1 +
>  fs/btrfs/disk-io.c              | 1 +
>  fs/btrfs/super.c                | 1 +
>  include/uapi/linux/btrfs_tree.h | 1 +
>  5 files changed, 5 insertions(+)
> 
> diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
> index 38651fae7f21..6d5a01c57da3 100644
> --- a/fs/btrfs/Kconfig
> +++ b/fs/btrfs/Kconfig
> @@ -5,6 +5,7 @@ config BTRFS_FS
>  	select CRYPTO
>  	select CRYPTO_CRC32C
>  	select LIBCRC32C
> +	select CRYPTO_XXHASH
>  	select ZLIB_INFLATE
>  	select ZLIB_DEFLATE
>  	select LZO_COMPRESS
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 139354d02dfa..c62729d5aa6e 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -87,6 +87,7 @@ static const struct btrfs_csums {
>  	const char	*name;
>  } btrfs_csums[] = {
>  	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> +	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
>  };
>  
>  #define BTRFS_EMPTY_DIR_SIZE 0
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 99dfd889b9f7..ac039a4d23ff 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -352,6 +352,7 @@ static bool btrfs_supported_super_csum(u16 csum_type)
>  {
>  	switch (csum_type) {
>  	case BTRFS_CSUM_TYPE_CRC32:
> +	case BTRFS_CSUM_TYPE_XXHASH:
>  		return true;
>  	default:
>  		return false;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 1b151af25772..60116d0410e5 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2456,3 +2456,4 @@ module_exit(exit_btrfs_fs)
>  
>  MODULE_LICENSE("GPL");
>  MODULE_SOFTDEP("pre: crc32c");
> +MODULE_SOFTDEP("pre: xxhash64");
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index b65c7ee75bc7..ba2f125a3a1c 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -302,6 +302,7 @@
>  /* csum types */
>  enum btrfs_csum_type {
>  	BTRFS_CSUM_TYPE_CRC32	= 0,
> +	BTRFS_CSUM_TYPE_XXHASH	= 1,
>  };
>  
>  /*
> 

Re: [PATCH v3 3/4] btrfs: use xxhash64 for checksumming

[David Sterba]

On Mon, Aug 26, 2019 at 06:38:13PM +0300, Nikolay Borisov wrote:
> nit: I'd name this commit  "Add xxhash64 to supported checksum hashes"
> 
> Personally, I interpret 'use <some hash> for checksumming' as if you are
> modifying code to use that hash. But in fact you are not, at least not
> in that patch.

It could be percieved as nitpicking, but this kind of feedback is a
check that the author's intentions are understood by someone else.  Some
subtleties or nuances can be missed by authors and this is maybe
inevitable when one spends significant time on the code or changelog.
The fresh look and first impression is not possible anymore. But this is
how patches are read when found git log.

In this case I agree with you and the use use of 'use' is a bit
misleading, suggesting that xxhash is now default.

Re: [PATCH v3 3/4] btrfs: use xxhash64 for checksumming

[Johannes Thumshirn]

On 27/08/2019 01:21, David Sterba wrote:
> On Mon, Aug 26, 2019 at 06:38:13PM +0300, Nikolay Borisov wrote:
>> nit: I'd name this commit  "Add xxhash64 to supported checksum hashes"
>>
>> Personally, I interpret 'use <some hash> for checksumming' as if you are
>> modifying code to use that hash. But in fact you are not, at least not
>> in that patch.
> 
> It could be percieved as nitpicking, but this kind of feedback is a
> check that the author's intentions are understood by someone else.  Some
> subtleties or nuances can be missed by authors and this is maybe
> inevitable when one spends significant time on the code or changelog.
> The fresh look and first impression is not possible anymore. But this is
> how patches are read when found git log.
> 
> In this case I agree with you and the use use of 'use' is a bit
> misleading, suggesting that xxhash is now default.

No problem. Dave are going to fix it up or do you want me to re-submit?
I don't really care either way.

Byte,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 247165, AG München)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v3 4/4] btrfs: sysfs: export supported checksums

[Johannes Thumshirn]

From: David Sterba <dsterba@suse.com>

Export supported checksum algorithms via sysfs.

Signed-off-by: David Sterba <dsterba@suse.com>
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

---
Changes to v2:
- Prevent possible overflow of sysfs attribute
Changes to v1:
- Removed btrfs_checksums_store() function (Nik)
- Renamed sysfs file to supported_checksums
---
 fs/btrfs/sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index f6d3c80f2e28..cae9c99253c7 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -246,6 +246,24 @@ static umode_t btrfs_feature_visible(struct kobject *kobj,
 	return mode;
 }
 
+static ssize_t btrfs_supported_checksums_show(struct kobject *kobj,
+					      struct kobj_attribute *a,
+					      char *buf)
+{
+	ssize_t ret = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(btrfs_csums); i++) {
+		ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
+				(i == 0 ? "" : ", "),
+				btrfs_csums[i].name);
+
+	}
+
+	ret += snprintf(buf + ret, PAGE_SIZE - ret, "\n");
+	return ret;
+}
+
 BTRFS_FEAT_ATTR_INCOMPAT(mixed_backref, MIXED_BACKREF);
 BTRFS_FEAT_ATTR_INCOMPAT(default_subvol, DEFAULT_SUBVOL);
 BTRFS_FEAT_ATTR_INCOMPAT(mixed_groups, MIXED_GROUPS);
@@ -259,6 +277,14 @@ BTRFS_FEAT_ATTR_INCOMPAT(no_holes, NO_HOLES);
 BTRFS_FEAT_ATTR_INCOMPAT(metadata_uuid, METADATA_UUID);
 BTRFS_FEAT_ATTR_COMPAT_RO(free_space_tree, FREE_SPACE_TREE);
 
+static struct btrfs_feature_attr btrfs_attr_features_checksums_name = {
+	.kobj_attr = __INIT_KOBJ_ATTR(supported_checksums, S_IRUGO,
+				      btrfs_supported_checksums_show,
+				      NULL),
+	.feature_set	= FEAT_INCOMPAT,
+	.feature_bit	= 0,
+};
+
 static struct attribute *btrfs_supported_feature_attrs[] = {
 	BTRFS_FEAT_ATTR_PTR(mixed_backref),
 	BTRFS_FEAT_ATTR_PTR(default_subvol),
@@ -272,6 +298,9 @@ static struct attribute *btrfs_supported_feature_attrs[] = {
 	BTRFS_FEAT_ATTR_PTR(no_holes),
 	BTRFS_FEAT_ATTR_PTR(metadata_uuid),
 	BTRFS_FEAT_ATTR_PTR(free_space_tree),
+
+	&btrfs_attr_features_checksums_name.kobj_attr.attr,
+
 	NULL
 };
 
-- 
2.16.4

Re: [PATCH v3 0/4] btrfs: support xxhash64 checksums

[Nikolay Borisov]

On 26.08.19 г. 14:48 ч., Johannes Thumshirn wrote:
> Now that Nikolay's XXHASH64 support for the Crypto API has landed and BTRFS is
> prepared for an easy addition of new checksums, this patchset implements
> XXHASH64 as a second, fast but not cryptographically secure checksum hash.
> 
> For changes since v2, please see the individual patches.
> 
> David Sterba (1):
>   btrfs: sysfs: export supported checksums
> 
> Johannes Thumshirn (3):
>   btrfs: turn checksum type define into a enum
>   btrfs: create structure to encode checksum type and length
>   btrfs: use xxhash64 for checksumming
> 
>  fs/btrfs/Kconfig                |  1 +
>  fs/btrfs/ctree.h                | 14 +++++++++-----
>  fs/btrfs/disk-io.c              |  1 +
>  fs/btrfs/super.c                |  1 +
>  fs/btrfs/sysfs.c                | 29 +++++++++++++++++++++++++++++
>  include/uapi/linux/btrfs_tree.h |  5 ++++-
>  6 files changed, 45 insertions(+), 6 deletions(-)

Short and sweet, apart from the minor nit on 3/4 you can add:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

>