Re: [PATCH] btrfs: Implement warning for commit values exceeding 300

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2025/4/28 14:16, sawara04.o@gmail.com 写道:
> From: Kyoji Ogasawara <sawara04.o@gmail.com>
> 
> The Btrfs documentation states that if the commit value is greater than 300
> a warning should be issued. This commit implements that functionality.
> For more details, visit:
> https://btrfs.readthedocs.io/en/latest/Administration.html#btrfs-specific-mount-options
> 
> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
> ---
>   fs/btrfs/fs.h    | 1 +
>   fs/btrfs/super.c | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index b572d6b9730b..f46fba127caa 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -285,6 +285,7 @@ enum {
>   #define BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR		0ULL
>   
>   #define BTRFS_DEFAULT_COMMIT_INTERVAL	(30)
> +#define BTRFS_WARNING_COMMIT_INTERVAL	(300)
>   #define BTRFS_DEFAULT_MAX_INLINE	(2048)
>   
>   struct btrfs_dev_replace {
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index dc4fee519ca6..c6911e9f17f2 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -569,6 +569,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   		break;
>   	case Opt_commit_interval:
>   		ctx->commit_interval = result.uint_32;
> +		if (ctx->commit_interval > BTRFS_WARNING_COMMIT_INTERVAL) {
> +			btrfs_warn(NULL,
> +"commit=%u is considerably high (> %u). Large amount of data can be lost when the system crashes.",

I'd say the large commit value is not really going to cause a lot of 
data on crash.
It's really depending on the workload, e.g. if there no fs activity at 
all for over 300s, there will be no data loss at all.

Furthermore, we do not really to scare users to use a super low value, 
which will cause tons of unnecessary transactions and fragments the 
filesystem with too many small extents (if data cow is enabled).

Another thing is, we do not even warn about more dangerous mount 
options, like nobarrier, thus I'm not sure if we really want such a warning.


At least I'd prefer a less scary warning, maybe just "Use with care"?

Thanks,
Qu

> +				ctx->commit_interval,
> +				BTRFS_WARNING_COMMIT_INTERVAL);
> +		}
>   		if (ctx->commit_interval == 0)
>   			ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
>   		break;