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

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

[Naohiro Aota]

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,
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");
+	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"))
+		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;
+}
+BTRFS_ATTR_RW(, sync_csum, btrfs_sync_csum_show, btrfs_sync_csum_store);
+#endif
+
 /*
  * Per-filesystem information and stats.
  *
@@ -1325,6 +1365,9 @@ static const struct attribute *btrfs_attrs[] = {
 	BTRFS_ATTR_PTR(, bg_reclaim_threshold),
 	BTRFS_ATTR_PTR(, commit_stats),
 	BTRFS_ATTR_PTR(, temp_fsid),
+#ifdef CONFIG_BTRFS_DEBUG
+	BTRFS_ATTR_PTR(, sync_csum),
+#endif
 	NULL,
 };
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 53f87f398da7..9b821677aeb3 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -276,6 +276,24 @@ enum btrfs_read_policy {
 	BTRFS_NR_READ_POLICY,
 };
 
+#ifdef CONFIG_BTRFS_DEBUG
+/*
+ * Checksum mode - do it synchronously in btrfs_submit_chunk() or offload it.
+ */
+enum btrfs_sync_csum_mode {
+	/*
+	 * Choose sync checksum or offloading automatically. Do it
+	 * synchronously if the checksum is fast, or offload to workqueues
+	 * otherwise.
+	 */
+	BTRFS_SYNC_CSUM_AUTO,
+	/* Never offload checksum to workqueues. */
+	BTRFS_SYNC_CSUM_FORCE_ON,
+	/* Always offload checksum to workqueues. */
+	BTRFS_SYNC_CSUM_FORCE_OFF,
+};
+#endif
+
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
 
@@ -380,6 +398,11 @@ struct btrfs_fs_devices {
 
 	/* Policy used to read the mirrored stripes. */
 	enum btrfs_read_policy read_policy;
+
+#ifdef CONFIG_BTRFS_DEBUG
+	/* Checksum mode - do it synchronously or offload it. */
+	enum btrfs_sync_csum_mode sync_csum_mode;
+#endif
 };
 
 #define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
-- 
2.43.0

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

[Johannes Thumshirn]

On 31.01.24 08:15, 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,
> 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>

As access to sysfs and fs_info can happen concurrently, should the 
read/write of fs_devices->sync_csum_mode be guarded by a 
READ_ONCE()/WRITE_ONCE()?

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

[David Sterba]

On Wed, Jan 31, 2024 at 02:15:32PM +0000, Johannes Thumshirn wrote:
> On 31.01.24 08:15, 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,
> > 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>
> 
> As access to sysfs and fs_info can happen concurrently, should the 
> read/write of fs_devices->sync_csum_mode be guarded by a 
> READ_ONCE()/WRITE_ONCE()?

Yes we use the *_ONCE helpers for values set from sysfs in cases where
it's without bad effects to race and change the value in the middle.
Here it would only skip one checksum offload/sync. It's not really a
guard but a note that there's something special about the value.

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

[Naohiro Aota]

On Wed, Jan 31, 2024 at 07:58:43PM +0100, David Sterba wrote:
> On Wed, Jan 31, 2024 at 02:15:32PM +0000, Johannes Thumshirn wrote:
> > On 31.01.24 08:15, 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,
> > > 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>
> > 
> > As access to sysfs and fs_info can happen concurrently, should the 
> > read/write of fs_devices->sync_csum_mode be guarded by a 
> > READ_ONCE()/WRITE_ONCE()?
> 
> Yes we use the *_ONCE helpers for values set from sysfs in cases where
> it's without bad effects to race and change the value in the middle.
> Here it would only skip one checksum offload/sync. It's not really a
> guard but a note that there's something special about the value.

Sure. I'll use it just in case.

Maybe, we want to convert fs_devices->read_policy as well, to prepare for
the feature we have more read policies implemented?

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

[David Sterba]

On Thu, Feb 01, 2024 at 01:16:40AM +0000, Naohiro Aota wrote:
> On Wed, Jan 31, 2024 at 07:58:43PM +0100, David Sterba wrote:
> > On Wed, Jan 31, 2024 at 02:15:32PM +0000, Johannes Thumshirn wrote:
> > > On 31.01.24 08:15, Naohiro Aota wrote:
> > Yes we use the *_ONCE helpers for values set from sysfs in cases where
> > it's without bad effects to race and change the value in the middle.
> > Here it would only skip one checksum offload/sync. It's not really a
> > guard but a note that there's something special about the value.
> 
> Sure. I'll use it just in case.
> 
> Maybe, we want to convert fs_devices->read_policy as well, to prepare for
> the feature we have more read policies implemented?

Yes indeed, good catch.

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

[David Sterba]

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;
> +}

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

[Naohiro Aota]

On Wed, Jan 31, 2024 at 08:04:59PM +0100, David Sterba wrote:
> 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.

Sure. I'll do so.

> > 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.

But, numeric indicators (0/1) cannot indicate tripe values well. Should I
represent it as e.g, "auto" => 0, "on" => 1, "off" => -1?

> 
> > +	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.

Sure.

> > +		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;
> > +}

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

[David Sterba]

On Thu, Feb 01, 2024 at 01:28:49AM +0000, Naohiro Aota wrote:
> On Wed, Jan 31, 2024 at 08:04:59PM +0100, David Sterba wrote:
> > On Wed, Jan 31, 2024 at 04:13:45PM +0900, Naohiro Aota wrote:
> > > +#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.
> 
> But, numeric indicators (0/1) cannot indicate tripe values well. Should I
> represent it as e.g, "auto" => 0, "on" => 1, "off" => -1?

We can do 0, 1, auto, the same values should work as the input. The
parsing will do a special case for auto.