[PATCH v2 0/4] Selectable checksum implementation

[PATCH v2 0/4] Selectable checksum implementation

[David Sterba]

Add a possibility to load accelerated checksum implementation after a
filesystem has been mounted. Detailed description in patch 3.

v2:
- fix initialization of default slot

David Sterba (4):
  btrfs: prepare more slots for checksum shash
  btrfs: assign checksum shash slots on init
  btrfs: add checksum implementation selection after mount
  btrfs: sysfs: print all loaded csums implementations

 fs/btrfs/check-integrity.c |   4 +-
 fs/btrfs/ctree.h           |  13 ++++-
 fs/btrfs/disk-io.c         |  30 +++++++----
 fs/btrfs/file-item.c       |   4 +-
 fs/btrfs/inode.c           |   2 +-
 fs/btrfs/scrub.c           |  12 ++---
 fs/btrfs/super.c           |   2 -
 fs/btrfs/sysfs.c           | 101 +++++++++++++++++++++++++++++++++++--
 8 files changed, 141 insertions(+), 27 deletions(-)

-- 
2.36.1

[PATCH v2 1/4] btrfs: prepare more slots for checksum shash

[David Sterba]

There's now one checksum template fs_info::csum_shash that contains
whatever the crypto API decides is the best available implementation. To
allow loading other implementations after mount add more slots and
symbolic names. The slot 0 is always used when calculating checksum,
other slots may be set later or left empty.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/check-integrity.c |  4 ++--
 fs/btrfs/ctree.h           | 13 ++++++++++++-
 fs/btrfs/disk-io.c         | 19 ++++++++++---------
 fs/btrfs/file-item.c       |  4 ++--
 fs/btrfs/inode.c           |  2 +-
 fs/btrfs/scrub.c           | 12 ++++++------
 fs/btrfs/sysfs.c           |  2 +-
 7 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 5d20137b7b67..dbabdd01ed66 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -1647,7 +1647,7 @@ static noinline_for_stack int btrfsic_test_for_metadata(
 		char **datav, unsigned int num_pages)
 {
 	struct btrfs_fs_info *fs_info = state->fs_info;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	struct btrfs_header *h;
 	u8 csum[BTRFS_CSUM_SIZE];
 	unsigned int i;
@@ -1660,7 +1660,7 @@ static noinline_for_stack int btrfsic_test_for_metadata(
 	if (memcmp(h->fsid, fs_info->fs_devices->fsid, BTRFS_FSID_SIZE))
 		return 1;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 	crypto_shash_init(shash);
 
 	for (i = 0; i < num_pages; i++) {
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index e6a084a9214c..1e55dfa49827 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -674,6 +674,13 @@ struct btrfs_commit_stats {
 	u64 total_commit_dur;
 };
 
+enum btrfs_csum_impl {
+	CSUM_DEFAULT,
+	CSUM_GENERIC,
+	CSUM_ACCEL,
+	CSUM_COUNT
+};
+
 struct btrfs_fs_info {
 	u8 chunk_tree_uuid[BTRFS_UUID_SIZE];
 	unsigned long flags;
@@ -1061,7 +1068,11 @@ struct btrfs_fs_info {
 	spinlock_t swapfile_pins_lock;
 	struct rb_root swapfile_pins;
 
-	struct crypto_shash *csum_shash;
+	/*
+	 * Templates for checksum calculation, slot 0 is for the currently used
+	 * one, other slots are optional for available implementations.
+	 */
+	struct crypto_shash *csum_shash[CSUM_COUNT];
 
 	/* Type of exclusive operation running, protected by super_lock */
 	enum btrfs_exclusive_operation exclusive_operation;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6ac8d73d4b51..3c2251199f0c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -65,8 +65,9 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info);
 
 static void btrfs_free_csum_hash(struct btrfs_fs_info *fs_info)
 {
-	if (fs_info->csum_shash)
-		crypto_free_shash(fs_info->csum_shash);
+	for (int i = 0; i < CSUM_COUNT; i++)
+		if (fs_info->csum_shash[i])
+			crypto_free_shash(fs_info->csum_shash[i]);
 }
 
 /*
@@ -176,11 +177,11 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
 	struct btrfs_fs_info *fs_info = buf->fs_info;
 	const int num_pages = num_extent_pages(buf);
 	const int first_page_part = min_t(u32, PAGE_SIZE, fs_info->nodesize);
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	char *kaddr;
 	int i;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 	crypto_shash_init(shash);
 	kaddr = page_address(buf->pages[0]) + offset_in_page(buf->start);
 	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
@@ -255,9 +256,9 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 	struct btrfs_super_block *disk_sb =
 		(struct btrfs_super_block *)raw_disk_sb;
 	char result[BTRFS_CSUM_SIZE];
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 
 	/*
 	 * The super_block structure does not span the whole
@@ -2429,7 +2430,7 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
 		return PTR_ERR(csum_shash);
 	}
 
-	fs_info->csum_shash = csum_shash;
+	fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
 
 	btrfs_info(fs_info, "using %s (%s) checksum algorithm",
 			btrfs_super_csum_name(csum_type),
@@ -4012,7 +4013,7 @@ static int write_dev_supers(struct btrfs_device *device,
 {
 	struct btrfs_fs_info *fs_info = device->fs_info;
 	struct address_space *mapping = device->bdev->bd_inode->i_mapping;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	int i;
 	int errors = 0;
 	int ret;
@@ -4021,7 +4022,7 @@ static int write_dev_supers(struct btrfs_device *device,
 	if (max_mirrors == 0)
 		max_mirrors = BTRFS_SUPER_MIRROR_MAX;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 
 	for (i = 0; i < max_mirrors; i++) {
 		struct page *page;
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index c828f971a346..6e416230733b 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -635,7 +635,7 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
 				u64 offset, bool one_ordered)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	struct btrfs_ordered_sum *sums;
 	struct btrfs_ordered_extent *ordered = NULL;
 	const bool use_page_offsets = (offset == (u64)-1);
@@ -663,7 +663,7 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
 	sums->bytenr = bio->bi_iter.bi_sector << 9;
 	index = 0;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 
 	bio_for_each_segment(bvec, bio, iter) {
 		if (use_page_offsets)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ecc5fa3343fc..a16a7379c58d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3429,7 +3429,7 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
 
 	ASSERT(pgoff + fs_info->sectorsize <= PAGE_SIZE);
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 
 	kaddr = kmap_local_page(page) + pgoff;
 	crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 3afe5fa50a63..f67a35cf0446 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1775,7 +1775,7 @@ static int scrub_checksum_data(struct scrub_block *sblock)
 {
 	struct scrub_ctx *sctx = sblock->sctx;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	u8 csum[BTRFS_CSUM_SIZE];
 	struct scrub_sector *sector;
 	char *kaddr;
@@ -1787,7 +1787,7 @@ static int scrub_checksum_data(struct scrub_block *sblock)
 
 	kaddr = page_address(sector->page);
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 	crypto_shash_init(shash);
 
 	/*
@@ -1806,7 +1806,7 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
 	struct scrub_ctx *sctx = sblock->sctx;
 	struct btrfs_header *h;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	u8 calculated_csum[BTRFS_CSUM_SIZE];
 	u8 on_disk_csum[BTRFS_CSUM_SIZE];
 	/*
@@ -1850,7 +1850,7 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
 		   BTRFS_UUID_SIZE))
 		sblock->header_error = 1;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 	crypto_shash_init(shash);
 	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
 			    sectorsize - BTRFS_CSUM_SIZE);
@@ -1872,7 +1872,7 @@ static int scrub_checksum_super(struct scrub_block *sblock)
 	struct btrfs_super_block *s;
 	struct scrub_ctx *sctx = sblock->sctx;
 	struct btrfs_fs_info *fs_info = sctx->fs_info;
-	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
+	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash[CSUM_DEFAULT]);
 	u8 calculated_csum[BTRFS_CSUM_SIZE];
 	struct scrub_sector *sector;
 	char *kaddr;
@@ -1893,7 +1893,7 @@ static int scrub_checksum_super(struct scrub_block *sblock)
 	if (!scrub_check_fsid(s->fsid, sector))
 		++fail_cor;
 
-	shash->tfm = fs_info->csum_shash;
+	shash->tfm = fs_info->csum_shash[CSUM_DEFAULT];
 	crypto_shash_init(shash);
 	crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
 			BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 32714ef8e22b..cc943b236c92 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1097,7 +1097,7 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
 
 	return sysfs_emit(buf, "%s (%s)\n",
 			  btrfs_super_csum_name(csum_type),
-			  crypto_shash_driver_name(fs_info->csum_shash));
+			  crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]));
 }
 
 BTRFS_ATTR(, checksum, btrfs_checksum_show);
-- 
2.36.1

Re: [PATCH v2 1/4] btrfs: prepare more slots for checksum shash

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 1/4] btrfs: prepare more slots for checksum shash

[David Sterba]

On Tue, Aug 02, 2022 at 02:28:21PM +0200, David Sterba wrote:
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -65,8 +65,9 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info);
>  
>  static void btrfs_free_csum_hash(struct btrfs_fs_info *fs_info)
>  {
> -	if (fs_info->csum_shash)
> -		crypto_free_shash(fs_info->csum_shash);
> +	for (int i = 0; i < CSUM_COUNT; i++)

This must start from 1, otherwise it would double free one of the
hashes.

> +		if (fs_info->csum_shash[i])
> +			crypto_free_shash(fs_info->csum_shash[i]);
>  }

Re: [PATCH v2 1/4] btrfs: prepare more slots for checksum shash

[Anand Jain]

> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 6ac8d73d4b51..3c2251199f0c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -65,8 +65,9 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info);
>   
>   static void btrfs_free_csum_hash(struct btrfs_fs_info *fs_info)
>   {
> -	if (fs_info->csum_shash)
> -		crypto_free_shash(fs_info->csum_shash);
> +	for (int i = 0; i < CSUM_COUNT; i++)
> +		if (fs_info->csum_shash[i])
> +			crypto_free_shash(fs_info->csum_shash[i]);
>   }
>   

Right, for-loop has to start from 1, and

> @@ -2429,7 +2430,7 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
>   		return PTR_ERR(csum_shash);
>   	}
>   
> -	fs_info->csum_shash = csum_shash;
> +	fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
>   
>   	btrfs_info(fs_info, "using %s (%s) checksum algorithm",
>   			btrfs_super_csum_name(csum_type),

Merge patch 2/4 with 1/4. So csum_shash is in its respective slot.

[PATCH v2 2/4] btrfs: assign checksum shash slots on init

[David Sterba]

When initializing checksum implementation on first mount, assign it to
the proper slot based on the driver name. If it contains "generic" it's
considered the non-accelerated one. Based on that properly set the
BTRFS_FS_CSUM_IMPL_FAST bit, previously it could be mistakenly set as
fast despite a different checksum (eg. sha256) with generic
implementation.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 11 +++++++++++
 fs/btrfs/super.c   |  2 --
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3c2251199f0c..5ca32bbdad53 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2430,7 +2430,18 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
 		return PTR_ERR(csum_shash);
 	}
 
+	/*
+	 * Find the fastest implementation available, but keep the slots
+	 * matching the type.
+	 */
 	fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
+	if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
+		fs_info->csum_shash[CSUM_GENERIC] = csum_shash;
+		clear_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
+	} else {
+		fs_info->csum_shash[CSUM_ACCEL] = csum_shash;
+		set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
+	}
 
 	btrfs_info(fs_info, "using %s (%s) checksum algorithm",
 			btrfs_super_csum_name(csum_type),
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1032aaa2c2f4..595b8d92ef86 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1819,8 +1819,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	} else {
 		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
 		btrfs_sb(s)->bdev_holder = fs_type;
-		if (!strstr(crc32c_impl(), "generic"))
-			set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
 		error = btrfs_fill_super(s, fs_devices, data);
 	}
 	if (!error)
-- 
2.36.1

Re: [PATCH v2 2/4] btrfs: assign checksum shash slots on init

[kernel test robot]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20220728]
[cannot apply to linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Sterba/Selectable-checksum-implementation/20220802-203426
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm64-randconfig-r005-20220801 (https://download.01.org/0day-ci/archive/20220803/202208030418.x0l9k9HX-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/335830d3f7aa692840402c4813c125f96f510812
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Sterba/Selectable-checksum-implementation/20220802-203426
        git checkout 335830d3f7aa692840402c4813c125f96f510812
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/btrfs/disk-io.c:2438:49: error: too few arguments to function call, expected 2, have 1
           if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
               ~~~~~~                                     ^
>> fs/btrfs/disk-io.c:2438:63: error: expected expression
           if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
                                                                        ^
   2 errors generated.


vim +2438 fs/btrfs/disk-io.c

  2419	
  2420	static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
  2421	{
  2422		struct crypto_shash *csum_shash;
  2423		const char *csum_driver = btrfs_super_csum_driver(csum_type);
  2424	
  2425		csum_shash = crypto_alloc_shash(csum_driver, 0, 0);
  2426	
  2427		if (IS_ERR(csum_shash)) {
  2428			btrfs_err(fs_info, "error allocating %s hash for checksum",
  2429				  csum_driver);
  2430			return PTR_ERR(csum_shash);
  2431		}
  2432	
  2433		/*
  2434		 * Find the fastest implementation available, but keep the slots
  2435		 * matching the type.
  2436		 */
  2437		fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
> 2438		if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
  2439			fs_info->csum_shash[CSUM_GENERIC] = csum_shash;
  2440			clear_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
  2441		} else {
  2442			fs_info->csum_shash[CSUM_ACCEL] = csum_shash;
  2443			set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
  2444		}
  2445	
  2446		btrfs_info(fs_info, "using %s (%s) checksum algorithm",
  2447				btrfs_super_csum_name(csum_type),
  2448				crypto_shash_driver_name(csum_shash));
  2449		return 0;
  2450	}
  2451	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 2/4] btrfs: assign checksum shash slots on init

[kernel test robot]

Hi David,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20220728]
[cannot apply to linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Sterba/Selectable-checksum-implementation/20220802-203426
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220803/202208030448.34fSi3hk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/335830d3f7aa692840402c4813c125f96f510812
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review David-Sterba/Selectable-checksum-implementation/20220802-203426
        git checkout 335830d3f7aa692840402c4813c125f96f510812
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/disk-io.c: In function 'btrfs_init_csum_hash':
   fs/btrfs/disk-io.c:2438:13: error: too few arguments to function 'strstr'
    2438 |         if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
         |             ^~~~~~
   In file included from arch/x86/include/asm/string.h:3,
                    from include/linux/string.h:20,
                    from arch/x86/include/asm/page_32.h:22,
                    from arch/x86/include/asm/page.h:14,
                    from arch/x86/include/asm/thread_info.h:12,
                    from include/linux/thread_info.h:60,
                    from arch/x86/include/asm/preempt.h:7,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:55,
                    from include/linux/wait.h:9,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from fs/btrfs/disk-io.c:6:
   arch/x86/include/asm/string_32.h:185:14: note: declared here
     185 | extern char *strstr(const char *cs, const char *ct);
         |              ^~~~~~
>> fs/btrfs/disk-io.c:2438:57: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    2438 |         if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
         |                                                         ^
   fs/btrfs/disk-io.c:2438:70: error: expected expression before '!=' token
    2438 |         if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
         |                                                                      ^~
>> fs/btrfs/disk-io.c:2438:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    2438 |         if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
         |         ^~
   fs/btrfs/disk-io.c:2438:77: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    2438 |         if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
         |                                                                             ^
   fs/btrfs/disk-io.c:2438:77: error: expected statement before ')' token
   fs/btrfs/disk-io.c:2441:11: error: 'else' without a previous 'if'
    2441 |         } else {
         |           ^~~~


vim +2438 fs/btrfs/disk-io.c

  2419	
  2420	static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
  2421	{
  2422		struct crypto_shash *csum_shash;
  2423		const char *csum_driver = btrfs_super_csum_driver(csum_type);
  2424	
  2425		csum_shash = crypto_alloc_shash(csum_driver, 0, 0);
  2426	
  2427		if (IS_ERR(csum_shash)) {
  2428			btrfs_err(fs_info, "error allocating %s hash for checksum",
  2429				  csum_driver);
  2430			return PTR_ERR(csum_shash);
  2431		}
  2432	
  2433		/*
  2434		 * Find the fastest implementation available, but keep the slots
  2435		 * matching the type.
  2436		 */
  2437		fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
> 2438		if (strstr(crypto_shash_driver_name(csum_shash)), "generic") != NULL) {
  2439			fs_info->csum_shash[CSUM_GENERIC] = csum_shash;
  2440			clear_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
  2441		} else {
  2442			fs_info->csum_shash[CSUM_ACCEL] = csum_shash;
  2443			set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
  2444		}
  2445	
  2446		btrfs_info(fs_info, "using %s (%s) checksum algorithm",
  2447				btrfs_super_csum_name(csum_type),
  2448				crypto_shash_driver_name(csum_shash));
  2449		return 0;
  2450	}
  2451	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[David Sterba]

The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
and the crypto module implementing it. In the scenario when there's only
the default generic implementation available during mount it's selected,
even if there's an unloaded module with accelerated version.

This happens with sha256 that's often built-in so the crypto API may not
trigger loading the modules and select the fastest implementation. Such
filesystem could be left using in the generic for the whole time.
Remount can't fix that and full umount/mount cycle may be impossible eg.
for a root filesystem.

Allow writing strings to the sysfs checksum file that will trigger
loading the crypto shash again and check if the found module is the
desired one.

Possible values:

- default - select whatever is considered default by crypto subsystem,
            either generic or accelerated
- accel   - try loading an accelerated implementation (must not contain
            "generic" in the name)
- generic - load and select the generic implementation

A typical scenario, assuming sha256 is built-in:

  $ mkfs.btrfs --csum sha256
  $ lsmod | grep sha256
  $ mount /dev /mnt
  $ cat ...FSID/checksum
  sha256 (sha256-generic)
  $ modprobe sha256
  $ lsmod | grep sha256
  sha256_ssse3
  $ echo accel > ...FSID/checksum
  sha256 (sha256-ni)

The crypto shash for all slots has the same lifetime as the mount, so
it's not possible to unload the crypto module.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/sysfs.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index cc943b236c92..0044644056ed 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1100,7 +1100,91 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
 			  crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]));
 }
 
-BTRFS_ATTR(, checksum, btrfs_checksum_show);
+static const char csum_impl[][8] = {
+	[CSUM_DEFAULT]	= "default",
+	[CSUM_GENERIC]	= "generic",
+	[CSUM_ACCEL]	= "accel",
+};
+
+static int select_checksum(struct btrfs_fs_info *fs_info, enum btrfs_csum_impl type)
+{
+	/* Already set */
+	if (fs_info->csum_shash[CSUM_DEFAULT] == fs_info->csum_shash[type])
+		return 0;
+
+	/* Allocate new if needed */
+	if (!fs_info->csum_shash[type]) {
+		const u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
+		const char *csum_driver = btrfs_super_csum_driver(csum_type);
+		struct crypto_shash *shash1, *tmp;
+		char full_name[32];
+		u32 mask = 0;
+
+		/*
+		 * Generic must be requested explicitly, otherwise it could
+		 * be accelerated implementation with highest priority.
+		 */
+		scnprintf(full_name, sizeof(full_name), "%s%s", csum_driver,
+			  (type == CSUM_GENERIC ? "-generic" : ""));
+
+		shash1 = crypto_alloc_shash(full_name, 0, mask);
+		if (IS_ERR(shash1))
+			return PTR_ERR(shash1);
+
+		/* Accelerated requested but generic found */
+		if (type != CSUM_GENERIC &&
+		    strstr(crypto_shash_driver_name(shash1), "generic")) {
+			crypto_free_shash(shash1);
+			return -ENOENT;
+		}
+
+		tmp = cmpxchg(&fs_info->csum_shash[type], NULL, shash1);
+		if (tmp) {
+			/* Something raced in */
+			crypto_free_shash(shash1);
+		}
+	}
+
+	/* Select it */
+	fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[type];
+	return 0;
+}
+
+static bool strmatch(const char *buffer, const char *string);
+
+static ssize_t btrfs_checksum_store(struct kobject *kobj,
+				    struct kobj_attribute *a,
+				    const char *buf, size_t len)
+{
+	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
+
+	if (!fs_info)
+		return -EPERM;
+
+	/* Pick the best available, generic or accelerated */
+	if (strmatch(buf, csum_impl[CSUM_DEFAULT])) {
+		if (fs_info->csum_shash[CSUM_ACCEL]) {
+			fs_info->csum_shash[CSUM_DEFAULT] =
+				fs_info->csum_shash[CSUM_ACCEL];
+			return len;
+		}
+		ASSERT(fs_info->csum_shash[CSUM_GENERIC]);
+		fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[CSUM_GENERIC];
+		return len;
+	}
+
+	for (int i = 1; i < CSUM_COUNT; i++) {
+		if (strmatch(buf, csum_impl[i])) {
+			int ret;
+
+			ret = select_checksum(fs_info, i);
+			return ret < 0 ? ret : len;
+		}
+	}
+
+	return -EINVAL;
+}
+BTRFS_ATTR_RW(, checksum, btrfs_checksum_show, btrfs_checksum_store);
 
 static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
 		struct kobj_attribute *a, char *buf)
-- 
2.36.1

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[Johannes Thumshirn]

On 02.08.22 14:33, David Sterba wrote:
> +static bool strmatch(const char *buffer, const char *string);
> +
> +static ssize_t btrfs_checksum_store(struct kobject *kobj,
> +				    struct kobj_attribute *a,
> +				    const char *buf, size_t len)
> +{
> +	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> +
> +	if (!fs_info)
> +		return -EPERM;
> +
> +	/* Pick the best available, generic or accelerated */
> +	if (strmatch(buf, csum_impl[CSUM_DEFAULT])) {

Same question as with v1, why not sysfs_streq()?

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[David Sterba]

On Tue, Aug 02, 2022 at 01:00:28PM +0000, Johannes Thumshirn wrote:
> On 02.08.22 14:33, David Sterba wrote:
> > +static bool strmatch(const char *buffer, const char *string);
> > +
> > +static ssize_t btrfs_checksum_store(struct kobject *kobj,
> > +				    struct kobj_attribute *a,
> > +				    const char *buf, size_t len)
> > +{
> > +	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> > +
> > +	if (!fs_info)
> > +		return -EPERM;
> > +
> > +	/* Pick the best available, generic or accelerated */
> > +	if (strmatch(buf, csum_impl[CSUM_DEFAULT])) {
> 
> Same question as with v1, why not sysfs_streq()?

The semantics a bit different with our strmatch that skips initial
whitespace, but otherwise should be same. Nevertheless we should
probablly drop strmatch and use sysfs_streq. I'll change it in this
patch and send another to delete the other one.

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[Anand Jain]

On 02/08/2022 20:28, David Sterba wrote:
> The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
> and the crypto module implementing it. In the scenario when there's only
> the default generic implementation available during mount it's selected,
> even if there's an unloaded module with accelerated version.
> 
> This happens with sha256 that's often built-in so the crypto API may not
> trigger loading the modules and select the fastest implementation. Such
> filesystem could be left using in the generic for the whole time.
> Remount can't fix that and full umount/mount cycle may be impossible eg.
> for a root filesystem.
> 
> Allow writing strings to the sysfs checksum file that will trigger
> loading the crypto shash again and check if the found module is the
> desired one.
> 
> Possible values:
> 
> - default - select whatever is considered default by crypto subsystem,
>              either generic or accelerated
> - accel   - try loading an accelerated implementation (must not contain
>              "generic" in the name)
> - generic - load and select the generic implementation
> 
> A typical scenario, assuming sha256 is built-in:
> 
>    $ mkfs.btrfs --csum sha256
>    $ lsmod | grep sha256
>    $ mount /dev /mnt
>    $ cat ...FSID/checksum
>    sha256 (sha256-generic)
>    $ modprobe sha256
>    $ lsmod | grep sha256
>    sha256_ssse3
>    $ echo accel > ...FSID/checksum
>    sha256 (sha256-ni)

I am not sure if I understand the use of slots 1 and 2 correctly.
As each checksum type can be either generic or accelerated, slot 0 is
the default which tells the preferred method. So slot0 is either
CSUM_GENERIC or CSUM_ACCEL. And by default, we prefer accelerated when
available or user requests. So I am still not getting the purpose of
slot1 and 2. Instead, overwriting slot0 will still do the job without
slot1 and 2.

> The crypto shash for all slots has the same lifetime as the mount, so
> it's not possible to unload the crypto module.

How does the update of the accelerated module work if a btrfs is a
non-root filesystem? Does it need a reboot?

-Anand


> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/sysfs.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index cc943b236c92..0044644056ed 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1100,7 +1100,91 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
>   			  crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]));
>   }
>   
> -BTRFS_ATTR(, checksum, btrfs_checksum_show);
> +static const char csum_impl[][8] = {
> +	[CSUM_DEFAULT]	= "default",
> +	[CSUM_GENERIC]	= "generic",
> +	[CSUM_ACCEL]	= "accel",
> +};
> +
> +static int select_checksum(struct btrfs_fs_info *fs_info, enum btrfs_csum_impl type)
> +{
> +	/* Already set */
> +	if (fs_info->csum_shash[CSUM_DEFAULT] == fs_info->csum_shash[type])
> +		return 0;
> +
> +	/* Allocate new if needed */
> +	if (!fs_info->csum_shash[type]) {
> +		const u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
> +		const char *csum_driver = btrfs_super_csum_driver(csum_type);
> +		struct crypto_shash *shash1, *tmp;
> +		char full_name[32];
> +		u32 mask = 0;
> +
> +		/*
> +		 * Generic must be requested explicitly, otherwise it could
> +		 * be accelerated implementation with highest priority.
> +		 */
> +		scnprintf(full_name, sizeof(full_name), "%s%s", csum_driver,
> +			  (type == CSUM_GENERIC ? "-generic" : ""));
> +
> +		shash1 = crypto_alloc_shash(full_name, 0, mask);
> +		if (IS_ERR(shash1))
> +			return PTR_ERR(shash1);
> +
> +		/* Accelerated requested but generic found */
> +		if (type != CSUM_GENERIC &&
> +		    strstr(crypto_shash_driver_name(shash1), "generic")) {
> +			crypto_free_shash(shash1);
> +			return -ENOENT;
> +		}
> +
> +		tmp = cmpxchg(&fs_info->csum_shash[type], NULL, shash1);
> +		if (tmp) {
> +			/* Something raced in */
> +			crypto_free_shash(shash1);
> +		}
> +	}
> +
> +	/* Select it */
> +	fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[type];
> +	return 0;
> +}
> +
> +static bool strmatch(const char *buffer, const char *string);
> +
> +static ssize_t btrfs_checksum_store(struct kobject *kobj,
> +				    struct kobj_attribute *a,
> +				    const char *buf, size_t len)
> +{
> +	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> +
> +	if (!fs_info)
> +		return -EPERM;
> +
> +	/* Pick the best available, generic or accelerated */
> +	if (strmatch(buf, csum_impl[CSUM_DEFAULT])) {
> +		if (fs_info->csum_shash[CSUM_ACCEL]) {
> +			fs_info->csum_shash[CSUM_DEFAULT] =
> +				fs_info->csum_shash[CSUM_ACCEL];
> +			return len;
> +		}
> +		ASSERT(fs_info->csum_shash[CSUM_GENERIC]);
> +		fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[CSUM_GENERIC];
> +		return len;
> +	}
> +
> +	for (int i = 1; i < CSUM_COUNT; i++) {
> +		if (strmatch(buf, csum_impl[i])) {
> +			int ret;
> +
> +			ret = select_checksum(fs_info, i);
> +			return ret < 0 ? ret : len;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +BTRFS_ATTR_RW(, checksum, btrfs_checksum_show, btrfs_checksum_store);
>   
>   static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
>   		struct kobj_attribute *a, char *buf)

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[David Sterba]

On Wed, Aug 03, 2022 at 08:06:46AM +0800, Anand Jain wrote:
> On 02/08/2022 20:28, David Sterba wrote:
> > The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
> > and the crypto module implementing it. In the scenario when there's only
> > the default generic implementation available during mount it's selected,
> > even if there's an unloaded module with accelerated version.
> > 
> > This happens with sha256 that's often built-in so the crypto API may not
> > trigger loading the modules and select the fastest implementation. Such
> > filesystem could be left using in the generic for the whole time.
> > Remount can't fix that and full umount/mount cycle may be impossible eg.
> > for a root filesystem.
> > 
> > Allow writing strings to the sysfs checksum file that will trigger
> > loading the crypto shash again and check if the found module is the
> > desired one.
> > 
> > Possible values:
> > 
> > - default - select whatever is considered default by crypto subsystem,
> >              either generic or accelerated
> > - accel   - try loading an accelerated implementation (must not contain
> >              "generic" in the name)
> > - generic - load and select the generic implementation
> > 
> > A typical scenario, assuming sha256 is built-in:
> > 
> >    $ mkfs.btrfs --csum sha256
> >    $ lsmod | grep sha256
> >    $ mount /dev /mnt
> >    $ cat ...FSID/checksum
> >    sha256 (sha256-generic)
> >    $ modprobe sha256
> >    $ lsmod | grep sha256
> >    sha256_ssse3
> >    $ echo accel > ...FSID/checksum
> >    sha256 (sha256-ni)
> 
> I am not sure if I understand the use of slots 1 and 2 correctly.
> As each checksum type can be either generic or accelerated, slot 0 is
> the default which tells the preferred method. So slot0 is either
> CSUM_GENERIC or CSUM_ACCEL. And by default, we prefer accelerated when
> available or user requests. So I am still not getting the purpose of
> slot1 and 2. Instead, overwriting slot0 will still do the job without
> slot1 and 2.

We need to keep track of all allocated hashes so they don't leak, which
would happen if slot 0 is overwritten after the switch. Also we must not
free the hash from sysfs because it could be in use. That's why there
are slots that keep track of the allocated structures and slot 0 that's
provided to use it for checksumming, merely as an alias.

The pointer switch is atomic (regarding the pointer value, not the
atomic_t semantics) so once some function uses it for shash it'll stay
there even if it won't be the default anymore.

I could add READ_ONCE/WRITE_ONCE for the slots so it's clear the access
is done in some unusal way, as we do with other sysfs variables.

> > The crypto shash for all slots has the same lifetime as the mount, so
> > it's not possible to unload the crypto module.
> 
> How does the update of the accelerated module work if a btrfs is a
> non-root filesystem? Does it need a reboot?

By update you mean rmmod/insmod with eventual update of the .ko file?
That won't work due to reference counts, so reloading the module would
require unmounting the filesystem, or reboot.

Freeing the allocated hash structure could be implemented by usage
counters when there's some checksumming in progress, the sysfs write
would wait until there are no users left and then free it.

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[Anand Jain]

On 03/08/2022 22:00, David Sterba wrote:
> On Wed, Aug 03, 2022 at 08:06:46AM +0800, Anand Jain wrote:
>> On 02/08/2022 20:28, David Sterba wrote:
>>> The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
>>> and the crypto module implementing it. In the scenario when there's only
>>> the default generic implementation available during mount it's selected,
>>> even if there's an unloaded module with accelerated version.
>>>
>>> This happens with sha256 that's often built-in so the crypto API may not
>>> trigger loading the modules and select the fastest implementation. Such
>>> filesystem could be left using in the generic for the whole time.
>>> Remount can't fix that and full umount/mount cycle may be impossible eg.
>>> for a root filesystem.
>>>
>>> Allow writing strings to the sysfs checksum file that will trigger
>>> loading the crypto shash again and check if the found module is the
>>> desired one.
>>>
>>> Possible values:
>>>
>>> - default - select whatever is considered default by crypto subsystem,
>>>               either generic or accelerated
>>> - accel   - try loading an accelerated implementation (must not contain
>>>               "generic" in the name)
>>> - generic - load and select the generic implementation
>>>
>>> A typical scenario, assuming sha256 is built-in:
>>>
>>>     $ mkfs.btrfs --csum sha256
>>>     $ lsmod | grep sha256
>>>     $ mount /dev /mnt
>>>     $ cat ...FSID/checksum
>>>     sha256 (sha256-generic)
>>>     $ modprobe sha256
>>>     $ lsmod | grep sha256
>>>     sha256_ssse3
>>>     $ echo accel > ...FSID/checksum
>>>     sha256 (sha256-ni)
>>
>> I am not sure if I understand the use of slots 1 and 2 correctly.
>> As each checksum type can be either generic or accelerated, slot 0 is
>> the default which tells the preferred method. So slot0 is either
>> CSUM_GENERIC or CSUM_ACCEL. And by default, we prefer accelerated when
>> available or user requests. So I am still not getting the purpose of
>> slot1 and 2. Instead, overwriting slot0 will still do the job without
>> slot1 and 2.
> 
> We need to keep track of all allocated hashes so they don't leak, which
> would happen if slot 0 is overwritten after the switch. Also we must not
> free the hash from sysfs because it could be in use. That's why there
> are slots that keep track of the allocated structures and slot 0 that's
> provided to use it for checksumming, merely as an alias.
> 

> The pointer switch is atomic (regarding the pointer value, not the
> atomic_t semantics) so once some function uses it for shash it'll stay
> there even if it won't be the default anymore.

How would that help? The read part depends on the CSUM_DEFAULT, which 
can change per user request. As below.

+	/* Select it */
+	fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[type];


> 
> I could add READ_ONCE/WRITE_ONCE for the slots so it's clear the access
> is done in some unusal way, as we do with other sysfs variables.
> 

IMO it is a must as we do read twice in all functions. For example:

disk-io.c:	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
disk-io.c:	shash->tfm = fs_info->csum_shash;


>>> The crypto shash for all slots has the same lifetime as the mount, so
>>> it's not possible to unload the crypto module.
>>
>> How does the update of the accelerated module work if a btrfs is a
>> non-root filesystem? Does it need a reboot?
> 
> By update you mean rmmod/insmod with eventual update of the .ko file?
Yes.

> That won't work due to reference counts, so reloading the module would
> require unmounting the filesystem, or reboot.

ok.

> Freeing the allocated hash structure could be implemented by usage
> counters when there's some checksumming in progress, the sysfs write
> would wait until there are no users left and then free it.

Looks like we need something that.


A basic question

Isn't that a module load at the boot will solve the problem?

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[Anand Jain]

On 02/08/2022 20:28, David Sterba wrote:
> The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
> and the crypto module implementing it.


> In the scenario when there's only
> the default generic implementation available during mount it's selected,
> even if there's an unloaded module with accelerated version.

> This happens with sha256 that's often built-in so the crypto API may not
> trigger loading the modules and select the fastest implementation.

Hmm ok.
What happens in the scenario if the accelerated module is loaded? Will 
the crypto API use the accelerated module?

- Anand

> Such
> filesystem could be left using in the generic for the whole time.
> Remount can't fix that and full umount/mount cycle may be impossible eg.
> for a root filesystem.
> 
> Allow writing strings to the sysfs checksum file that will trigger
> loading the crypto shash again and check if the found module is the
> desired one.
> 
> Possible values:
> 
> - default - select whatever is considered default by crypto subsystem,
>              either generic or accelerated
> - accel   - try loading an accelerated implementation (must not contain
>              "generic" in the name)
> - generic - load and select the generic implementation
> 
> A typical scenario, assuming sha256 is built-in:
> 
>    $ mkfs.btrfs --csum sha256
>    $ lsmod | grep sha256
>    $ mount /dev /mnt
>    $ cat ...FSID/checksum
>    sha256 (sha256-generic)
>    $ modprobe sha256
>    $ lsmod | grep sha256
>    sha256_ssse3
>    $ echo accel > ...FSID/checksum
>    sha256 (sha256-ni)
> 
> The crypto shash for all slots has the same lifetime as the mount, so
> it's not possible to unload the crypto module.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/sysfs.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index cc943b236c92..0044644056ed 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1100,7 +1100,91 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
>   			  crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]));
>   }
>   
> -BTRFS_ATTR(, checksum, btrfs_checksum_show);
> +static const char csum_impl[][8] = {
> +	[CSUM_DEFAULT]	= "default",
> +	[CSUM_GENERIC]	= "generic",
> +	[CSUM_ACCEL]	= "accel",
> +};
> +
> +static int select_checksum(struct btrfs_fs_info *fs_info, enum btrfs_csum_impl type)
> +{
> +	/* Already set */
> +	if (fs_info->csum_shash[CSUM_DEFAULT] == fs_info->csum_shash[type])
> +		return 0;
> +
> +	/* Allocate new if needed */
> +	if (!fs_info->csum_shash[type]) {
> +		const u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
> +		const char *csum_driver = btrfs_super_csum_driver(csum_type);
> +		struct crypto_shash *shash1, *tmp;
> +		char full_name[32];
> +		u32 mask = 0;
> +
> +		/*
> +		 * Generic must be requested explicitly, otherwise it could
> +		 * be accelerated implementation with highest priority.
> +		 */
> +		scnprintf(full_name, sizeof(full_name), "%s%s", csum_driver,
> +			  (type == CSUM_GENERIC ? "-generic" : ""));
> +
> +		shash1 = crypto_alloc_shash(full_name, 0, mask);
> +		if (IS_ERR(shash1))
> +			return PTR_ERR(shash1);
> +
> +		/* Accelerated requested but generic found */
> +		if (type != CSUM_GENERIC &&
> +		    strstr(crypto_shash_driver_name(shash1), "generic")) {
> +			crypto_free_shash(shash1);
> +			return -ENOENT;
> +		}
> +
> +		tmp = cmpxchg(&fs_info->csum_shash[type], NULL, shash1);
> +		if (tmp) {
> +			/* Something raced in */
> +			crypto_free_shash(shash1);
> +		}
> +	}
> +
> +	/* Select it */
> +	fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[type];
> +	return 0;
> +}
> +
> +static bool strmatch(const char *buffer, const char *string);
> +
> +static ssize_t btrfs_checksum_store(struct kobject *kobj,
> +				    struct kobj_attribute *a,
> +				    const char *buf, size_t len)
> +{
> +	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
> +
> +	if (!fs_info)
> +		return -EPERM;
> +
> +	/* Pick the best available, generic or accelerated */
> +	if (strmatch(buf, csum_impl[CSUM_DEFAULT])) {
> +		if (fs_info->csum_shash[CSUM_ACCEL]) {
> +			fs_info->csum_shash[CSUM_DEFAULT] =
> +				fs_info->csum_shash[CSUM_ACCEL];
> +			return len;
> +		}
> +		ASSERT(fs_info->csum_shash[CSUM_GENERIC]);
> +		fs_info->csum_shash[CSUM_DEFAULT] = fs_info->csum_shash[CSUM_GENERIC];
> +		return len;
> +	}
> +
> +	for (int i = 1; i < CSUM_COUNT; i++) {
> +		if (strmatch(buf, csum_impl[i])) {
> +			int ret;
> +
> +			ret = select_checksum(fs_info, i);
> +			return ret < 0 ? ret : len;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +BTRFS_ATTR_RW(, checksum, btrfs_checksum_show, btrfs_checksum_store);
>   
>   static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
>   		struct kobj_attribute *a, char *buf)

Re: [PATCH v2 3/4] btrfs: add checksum implementation selection after mount

[David Sterba]

On Wed, Aug 03, 2022 at 08:22:05AM +0800, Anand Jain wrote:
> On 02/08/2022 20:28, David Sterba wrote:
> > The sysfs file /sys/fs/btrfs/FSID/checksum shows the filesystem checksum
> > and the crypto module implementing it.
> 
> 
> > In the scenario when there's only
> > the default generic implementation available during mount it's selected,
> > even if there's an unloaded module with accelerated version.
> 
> > This happens with sha256 that's often built-in so the crypto API may not
> > trigger loading the modules and select the fastest implementation.
> 
> Hmm ok.
> What happens in the scenario if the accelerated module is loaded? Will 
> the crypto API use the accelerated module?

Yes, each implementation of the hash has a priority and the loading
mechanism goes through all registered implementations and picks the one
with highest priority. Generic is 100, sha256-ni is 250, there are 3
more implementations based on CPU flags, you can see it in
arch/x86_64/crypto/sha256_ssse3_glue.c .

There are some tricks for crc32c that trigger loading the accelerated
module even if there's the generic available, and since sha256 has
become mandatory for some security features (signing) the generic one is
always available. Similar mechanism as for crc32c is not done.

[PATCH v2 4/4] btrfs: sysfs: print all loaded csums implementations

[David Sterba]

Extend the sysfs FSID/checksum file and append lines with the selector
strings and implementation if loaded, or 'none'.

Output may look like:

  crc32c (crc32c-generic)
  generic: crc32c-generic
  accel:   crc32c-intel

Scripts that rely on single line in the file need to be updated.

All available and loaded implementations can be found in /proc/crypto .

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/sysfs.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 0044644056ed..2f8bd75ce69d 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1095,9 +1095,18 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
 
-	return sysfs_emit(buf, "%s (%s)\n",
-			  btrfs_super_csum_name(csum_type),
-			  crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]));
+	return sysfs_emit(buf,
+			"%s (%s)\n"
+			"generic: %s\n"
+			"accel:   %s\n",
+			btrfs_super_csum_name(csum_type),
+			crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]),
+			fs_info->csum_shash[CSUM_GENERIC]
+			? crypto_shash_driver_name(fs_info->csum_shash[CSUM_GENERIC])
+			: "none",
+			fs_info->csum_shash[CSUM_ACCEL]
+			? crypto_shash_driver_name(fs_info->csum_shash[CSUM_ACCEL])
+			: "none");
 }
 
 static const char csum_impl[][8] = {
-- 
2.36.1

Re: [PATCH v2 4/4] btrfs: sysfs: print all loaded csums implementations

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH v2 0/4] Selectable checksum implementation

[David Sterba]

Add a possibility to load accelerated checksum implementation after a
filesystem has been mounted. Detailed description in patch 3.

v2: rebased on misc-next

David Sterba (4):
  btrfs: prepare more slots for checksum shash
  btrfs: assign checksum shash slots on init
  btrfs: add checksum implementation selection after mount
  btrfs: sysfs: print all loaded csums implementations

 fs/btrfs/check-integrity.c |   4 +-
 fs/btrfs/ctree.h           |  13 ++++-
 fs/btrfs/disk-io.c         |  30 +++++++----
 fs/btrfs/file-item.c       |   4 +-
 fs/btrfs/inode.c           |   2 +-
 fs/btrfs/scrub.c           |  12 ++---
 fs/btrfs/super.c           |   2 -
 fs/btrfs/sysfs.c           | 101 +++++++++++++++++++++++++++++++++++--
 8 files changed, 141 insertions(+), 27 deletions(-)

-- 
2.36.1

[PATCH v2 2/4] btrfs: assign checksum shash slots on init

[David Sterba]

When initializing checksum implementation on first mount, assign it to
the proper slot based on the driver name. If it contains "generic" it's
considered the non-accelerated one. Based on that properly set the
BTRFS_FS_CSUM_IMPL_FAST bit, previously it could be mistakenly set as
fast despite a different checksum (eg. sha256) with generic
implementation.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 13 ++++++++++++-
 fs/btrfs/super.c   |  2 --
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3c2251199f0c..247187427760 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2430,7 +2430,18 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
 		return PTR_ERR(csum_shash);
 	}
 
-	fs_info->csum_shash[CSUM_DEFAULT] = csum_shash;
+	/*
+	 * Find the fastest implementation available, but keep the slots
+	 * matching the type.
+	 */
+	if (strstr(crypto_shash_driver_name(fs_info->csum_shash[CSUM_DEFAULT]),
+		   "generic") != NULL) {
+		fs_info->csum_shash[CSUM_GENERIC] = csum_shash;
+		clear_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
+	} else {
+		fs_info->csum_shash[CSUM_ACCEL] = csum_shash;
+		set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
+	}
 
 	btrfs_info(fs_info, "using %s (%s) checksum algorithm",
 			btrfs_super_csum_name(csum_type),
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 1032aaa2c2f4..595b8d92ef86 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1819,8 +1819,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	} else {
 		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
 		btrfs_sb(s)->bdev_holder = fs_type;
-		if (!strstr(crc32c_impl(), "generic"))
-			set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
 		error = btrfs_fill_super(s, fs_devices, data);
 	}
 	if (!error)
-- 
2.36.1