diff mbox series

[RFC,3/4] btrfs: use xxhash64 for checksumming

Message ID 4ac3332af1fa5ba7ecb92181329151382b800f3d.1564046812.git.jthumshirn@suse.de (mailing list archive)
State New, archived
Headers show
Series Support xxhash64 checksums | expand

Commit Message

Johannes Thumshirn July 25, 2019, 9:33 a.m. UTC
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(+)

Comments

Qu Wenruo July 25, 2019, 12:02 p.m. UTC | #1
On 2019/7/25 下午5:33, 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

Just an off topic idea, can we make such CRYPTO_* support configurable?
E.g. make something like CONFIG_BTRFS_CRYPTO_XXHASH.

Not sure if everyone would like to pull all hash algorithm.

Thanks,
Qu
>  	select ZLIB_INFLATE
>  	select ZLIB_DEFLATE
>  	select LZO_COMPRESS
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 099401f5dd47..b34f22e55304 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -90,6 +90,7 @@ static const struct btrfs_csums {
>  	const char	*name;
>  } btrfs_csums[] = {
>  	BTRFS_CHECKSUM_TYPE(BTRFS_CSUM_TYPE_CRC32, 4, "crc32c"),
> +	BTRFS_CHECKSUM_TYPE(BTRFS_CSUM_TYPE_XXHASH, 8, "xxhash64"),
>  };
>  
>  #define BTRFS_EMPTY_DIR_SIZE 0
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5f7ee70b3d1a..54a8ef489850 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -351,6 +351,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 78de9d5d80c6..7312f675d702 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2464,3 +2464,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 babef98181a2..af4f5dec10b7 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,
>  };
>  
>  /*
>
Johannes Thumshirn July 25, 2019, 2:18 p.m. UTC | #2
On Thu, Jul 25, 2019 at 08:02:12PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/7/25 下午5:33, 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
> 
> Just an off topic idea, can we make such CRYPTO_* support configurable?
> E.g. make something like CONFIG_BTRFS_CRYPTO_XXHASH.
> 
> Not sure if everyone would like to pull all hash algorithm.
 
This is something I thought about as well, but I was afraid of people shooting
themselves in the foot if they forget to switch them on and mkfs with the
wrong option.

Not sure what's the better way here? Dave?
David Sterba July 26, 2019, 1:45 p.m. UTC | #3
On Thu, Jul 25, 2019 at 04:18:37PM +0200, Johannes Thumshirn wrote:
> On Thu, Jul 25, 2019 at 08:02:12PM +0800, Qu Wenruo wrote:
> > 
> > 
> > On 2019/7/25 下午5:33, 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
> > 
> > Just an off topic idea, can we make such CRYPTO_* support configurable?
> > E.g. make something like CONFIG_BTRFS_CRYPTO_XXHASH.
> > 
> > Not sure if everyone would like to pull all hash algorithm.
>  
> This is something I thought about as well, but I was afraid of people shooting
> themselves in the foot if they forget to switch them on and mkfs with the
> wrong option.

> Not sure what's the better way here? Dave?

I'd go to pull everything unconditionally, which means that all
dependent modules are built and does not require the user to tweak the
config.

I understand that there are setups that don't want to provide all hash
algorithms eg. due to space constraints. That will be still possible and
puts the "burden" on the distributor/integrator. Simply don't provide
the .ko files. The crypto API can detect that during mount the shash
descriptor cannot be instantiated and will fail. In specialized setups
this is ok, because lack of the hash algorithm is known.

For everybody else, the filesystem should come with all parts included
so the features work.

The situation with zlib/lzo/zstd is different because we use the library
functions, not the separate .ko modules. This is a bit more convenient.

We don't want to do that with the hash algorithms though because there
are usually optimized verions that we do want to use, and the crypto API
does all the work.
diff mbox series

Patch

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 099401f5dd47..b34f22e55304 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -90,6 +90,7 @@  static const struct btrfs_csums {
 	const char	*name;
 } btrfs_csums[] = {
 	BTRFS_CHECKSUM_TYPE(BTRFS_CSUM_TYPE_CRC32, 4, "crc32c"),
+	BTRFS_CHECKSUM_TYPE(BTRFS_CSUM_TYPE_XXHASH, 8, "xxhash64"),
 };
 
 #define BTRFS_EMPTY_DIR_SIZE 0
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5f7ee70b3d1a..54a8ef489850 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -351,6 +351,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 78de9d5d80c6..7312f675d702 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2464,3 +2464,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 babef98181a2..af4f5dec10b7 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,
 };
 
 /*