Re: [PATCH v3 16/18] jbd2: switch to using the crc32c library

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Nov 03, 2024 at 02:31:52PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Now that the crc32c() library function directly takes advantage of
> architecture-specific optimizations, it is unnecessary to go through the
> crypto API.  Just use crc32c().  This is much simpler, and it improves
> performance due to eliminating the crypto API overhead.
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/jbd2/Kconfig      |  2 --
>  fs/jbd2/journal.c    | 25 ++-----------------------
>  include/linux/jbd2.h | 31 +++----------------------------
>  3 files changed, 5 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/jbd2/Kconfig b/fs/jbd2/Kconfig
> index 4ad2c67f93f1..9c19e1512101 100644
> --- a/fs/jbd2/Kconfig
> +++ b/fs/jbd2/Kconfig
> @@ -1,11 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config JBD2
>  	tristate
>  	select CRC32
> -	select CRYPTO
> -	select CRYPTO_CRC32C
>  	help
>  	  This is a generic journaling layer for block devices that support
>  	  both 32-bit and 64-bit block numbers.  It is currently used by
>  	  the ext4 and OCFS2 filesystems, but it could also be used to add
>  	  journal support to other file systems or block devices such
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 97f487c3d8fc..56cea5a738a7 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1373,24 +1373,16 @@ static int journal_check_superblock(journal_t *journal)
>  		printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
>  		       "at the same time!\n");
>  		return err;
>  	}
>  
> -	/* Load the checksum driver */
>  	if (jbd2_journal_has_csum_v2or3_feature(journal)) {
>  		if (sb->s_checksum_type != JBD2_CRC32C_CHKSUM) {
>  			printk(KERN_ERR "JBD2: Unknown checksum type\n");
>  			return err;
>  		}
>  
> -		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> -		if (IS_ERR(journal->j_chksum_driver)) {
> -			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> -			err = PTR_ERR(journal->j_chksum_driver);
> -			journal->j_chksum_driver = NULL;
> -			return err;
> -		}
>  		/* Check superblock checksum */
>  		if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
>  			printk(KERN_ERR "JBD2: journal checksum error\n");
>  			err = -EFSBADCRC;
>  			return err;
> @@ -1611,12 +1603,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  
>  	return journal;
>  
>  err_cleanup:
>  	percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> -	if (journal->j_chksum_driver)
> -		crypto_free_shash(journal->j_chksum_driver);
>  	kfree(journal->j_wbuf);
>  	jbd2_journal_destroy_revoke(journal);
>  	journal_fail_superblock(journal);
>  	kfree(journal);
>  	return ERR_PTR(err);
> @@ -2194,12 +2184,10 @@ int jbd2_journal_destroy(journal_t *journal)
>  	if (journal->j_proc_entry)
>  		jbd2_stats_proc_exit(journal);
>  	iput(journal->j_inode);
>  	if (journal->j_revoke)
>  		jbd2_journal_destroy_revoke(journal);
> -	if (journal->j_chksum_driver)
> -		crypto_free_shash(journal->j_chksum_driver);
>  	kfree(journal->j_fc_wbuf);
>  	kfree(journal->j_wbuf);
>  	kfree(journal);
>  
>  	return err;
> @@ -2340,23 +2328,14 @@ int jbd2_journal_set_features(journal_t *journal, unsigned long compat,
>  			pr_err("JBD2: Cannot enable fast commits.\n");
>  			return 0;
>  		}
>  	}
>  
> -	/* Load the checksum driver if necessary */
> -	if ((journal->j_chksum_driver == NULL) &&
> -	    INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3)) {
> -		journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> -		if (IS_ERR(journal->j_chksum_driver)) {
> -			printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> -			journal->j_chksum_driver = NULL;
> -			return 0;
> -		}
> -		/* Precompute checksum seed for all metadata */
> +	/* Precompute checksum seed for all metadata */
> +	if (INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3))
>  		journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
>  						   sizeof(sb->s_uuid));
> -	}
>  
>  	lock_buffer(journal->j_sb_buffer);
>  
>  	/* If enabling v3 checksums, update superblock */
>  	if (INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3)) {
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 8aef9bb6ad57..33d25a3d15f1 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -26,11 +26,11 @@
>  #include <linux/mutex.h>
>  #include <linux/timer.h>
>  #include <linux/slab.h>
>  #include <linux/bit_spinlock.h>
>  #include <linux/blkdev.h>
> -#include <crypto/hash.h>
> +#include <linux/crc32c.h>
>  #endif
>  
>  #define journal_oom_retry 1
>  
>  /*
> @@ -1239,17 +1239,10 @@ struct journal_s
>  	 * An opaque pointer to fs-private information.  ext3 puts its
>  	 * superblock pointer here.
>  	 */
>  	void *j_private;
>  
> -	/**
> -	 * @j_chksum_driver:
> -	 *
> -	 * Reference to checksum algorithm driver via cryptoapi.
> -	 */
> -	struct crypto_shash *j_chksum_driver;
> -
>  	/**
>  	 * @j_csum_seed:
>  	 *
>  	 * Precomputed journal UUID checksum for seeding other checksums.
>  	 */
> @@ -1748,14 +1741,11 @@ static inline bool jbd2_journal_has_csum_v2or3_feature(journal_t *j)
>  	return jbd2_has_feature_csum2(j) || jbd2_has_feature_csum3(j);
>  }
>  
>  static inline int jbd2_journal_has_csum_v2or3(journal_t *journal)
>  {
> -	WARN_ON_ONCE(jbd2_journal_has_csum_v2or3_feature(journal) &&
> -		     journal->j_chksum_driver == NULL);
> -
> -	return journal->j_chksum_driver != NULL;
> +	return jbd2_journal_has_csum_v2or3_feature(journal);

Same comment as my last reply about removing trivial helpers, but
otherwise
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

If you push this for 6.13 I'd be happy that the shash junk finally went
away.  The onstack struct desc thing in the _chksum() functions was by
far the most sketchy part of the ext4/jbd2 metadata csum project. :)

--D

>  }
>  
>  static inline int jbd2_journal_get_num_fc_blks(journal_superblock_t *jsb)
>  {
>  	int num_fc_blocks = be32_to_cpu(jsb->s_num_fc_blks);
> @@ -1794,26 +1784,11 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal)
>  #define JBD_MAX_CHECKSUM_SIZE 4
>  
>  static inline u32 jbd2_chksum(journal_t *journal, u32 crc,
>  			      const void *address, unsigned int length)
>  {
> -	struct {
> -		struct shash_desc shash;
> -		char ctx[JBD_MAX_CHECKSUM_SIZE];
> -	} desc;
> -	int err;
> -
> -	BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) >
> -		JBD_MAX_CHECKSUM_SIZE);
> -
> -	desc.shash.tfm = journal->j_chksum_driver;
> -	*(u32 *)desc.ctx = crc;
> -
> -	err = crypto_shash_update(&desc.shash, address, length);
> -	BUG_ON(err);
> -
> -	return *(u32 *)desc.ctx;
> +	return crc32c(crc, address, length);
>  }
>  
>  /* Return most recent uncommitted transaction */
>  static inline tid_t  jbd2_get_latest_transaction(journal_t *journal)
>  {
> -- 
> 2.47.0
> 
>