Re: [PATCH v2] btrfs: introduce sync_csum_mode to tweak sync checksum behavior

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 31, 2024 at 04:13:45PM +0900, Naohiro Aota wrote:
> We disable offloading checksum to workqueues and do it synchronously when
> the checksum algorithm is fast. However, as reported in the link below,
> RAID0 with multiple devices may suffer from the sync checksum, because
> "fast checksum" is still not fast enough to catch up RAID0 writing.
> 
> To measure the effectiveness of sync checksum for developers, it would be
> better to have a switch for the sync checksum under CONFIG_BTRFS_DEBUG
> hood.
> 
> This commit introduces fs_devices->sync_csum_mode for CONFIG_BTRFS_DEBUG,

Please rename it to offload_checksums, this also inverts the logic but
is IMHO clear what it does.

> so that a btrfs developer can change the behavior by writing to
> /sys/fs/btrfs/<uuid>/sync_csum. The default is "auto" which is the same as
> the previous behavior. Or, you can set "on" or "off" to always/never use
> sync checksum.
> 
> More benchmark should be collected with this knob to implement a proper
> criteria to enable/disable sync checksum.
> 
> Link: https://lore.kernel.org/linux-btrfs/20230731152223.4EFB.409509F4@e16-tech.com/
> Link: https://lore.kernel.org/linux-btrfs/p3vo3g7pqn664mhmdhlotu5dzcna6vjtcoc2hb2lsgo2fwct7k@xzaxclba5tae/
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
> v2:
> - Call it "sync checksum" properly
> - Removed a patch to automatically change checksum behavior
> - Hide the sysfs interface under CONFIG_BTRFS_DEBUG
> ---
>  fs/btrfs/bio.c     | 13 ++++++++++++-
>  fs/btrfs/sysfs.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/volumes.h | 23 +++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 960b81718e29..c896d3cd792b 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -608,8 +608,19 @@ static void run_one_async_done(struct btrfs_work *work, bool do_free)
>  
>  static bool should_async_write(struct btrfs_bio *bbio)
>  {
> +	bool auto_csum_mode = true;
> +
> +#ifdef CONFIG_BTRFS_DEBUG
> +	struct btrfs_fs_devices *fs_devices = bbio->fs_info->fs_devices;
> +
> +	if (fs_devices->sync_csum_mode == BTRFS_SYNC_CSUM_FORCE_ON)
> +		return false;
> +
> +	auto_csum_mode = fs_devices->sync_csum_mode == BTRFS_SYNC_CSUM_AUTO;
> +#endif
> +
>  	/* Submit synchronously if the checksum implementation is fast. */
> -	if (test_bit(BTRFS_FS_CSUM_IMPL_FAST, &bbio->fs_info->flags))
> +	if (auto_csum_mode && test_bit(BTRFS_FS_CSUM_IMPL_FAST, &bbio->fs_info->flags))
>  		return false;
>  
>  	/*
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 84c05246ffd8..ea1e54149ef4 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1306,6 +1306,46 @@ static ssize_t btrfs_bg_reclaim_threshold_store(struct kobject *kobj,
>  BTRFS_ATTR_RW(, bg_reclaim_threshold, btrfs_bg_reclaim_threshold_show,
>  	      btrfs_bg_reclaim_threshold_store);
>  
> +#ifdef CONFIG_BTRFS_DEBUG
> +static ssize_t btrfs_sync_csum_show(struct kobject *kobj,
> +				    struct kobj_attribute *a, char *buf)
> +{
> +	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
> +
> +	switch (fs_devices->sync_csum_mode) {
> +	case BTRFS_SYNC_CSUM_AUTO:
> +		return sysfs_emit(buf, "auto\n");
> +	case BTRFS_SYNC_CSUM_FORCE_ON:
> +		return sysfs_emit(buf, "on\n");
> +	case BTRFS_SYNC_CSUM_FORCE_OFF:
> +		return sysfs_emit(buf, "off\n");

We're using numeric indicators for on/off in other sysfs files, though
here it's a bit more readable.

> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t btrfs_sync_csum_store(struct kobject *kobj,
> +				     struct kobj_attribute *a, const char *buf,
> +				     size_t len)
> +{
> +	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
> +
> +	if (sysfs_streq(buf, "auto"))

Please use kstrobool, it accepts awide range of "yes/no" values and
check for "auto" only after it returns -EINVAL.

> +		fs_devices->sync_csum_mode = BTRFS_SYNC_CSUM_AUTO;
> +	else if (sysfs_streq(buf, "on"))
> +		fs_devices->sync_csum_mode = BTRFS_SYNC_CSUM_FORCE_ON;
> +	else if (sysfs_streq(buf, "off"))
> +		fs_devices->sync_csum_mode = BTRFS_SYNC_CSUM_FORCE_OFF;
> +	else
> +		return -EINVAL;
> +
> +	return len;
> +	return -EINVAL;
> +}