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

[David Sterba <dsterba@suse.cz>]

On Mon, Apr 28, 2025 at 03:06:14PM +0930, Qu Wenruo wrote:
> 在 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"?

We used to have the warning there before the 6.8 mount option parser
rewrite and it got removed in 6941823cc87812 ("btrfs: remove old mount
API code") without being properly implemented in the new API.

The wording of the message was not alarming or scarying,
"excessive commit interval %u". The consequences of the large interval
could be data loss but this may not be suitable for the error message as
this is a corner case. I agree that 'use with care' (and read
documentation) would be reasonable.

Regarding nobarrier, there's a message "turning off barriers" printed in
btrfs_emit_options(), we can possibly enhance that too.