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

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

[sawara04.o]

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.",
+				ctx->commit_interval,
+				BTRFS_WARNING_COMMIT_INTERVAL);
+		}
 		if (ctx->commit_interval == 0)
 			ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
 		break;
-- 
2.47.0

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

[Qu Wenruo]

在 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;

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

[David Sterba]

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.

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

[Qu Wenruo]

在 2025/4/29 07:45, 小笠原 共志 写道:
> Thank you very much for your review and the helpful feedback.
> 
> I will revise the patch according to your suggestions, replacing the 
> warning message with something less alarming such as "Use with care."
> 
> Additionally, would it be okay if I also update the "nobarrier" message 
> to make it more informative?

Sure, that sounds good to me.

Thanks,
Qu

> 
> 2025年4月29日(火) 0:13 David Sterba <dsterba@suse.cz 
> <mailto: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
>     <mailto:sawara04.o@gmail.com> 写道:
>      > > From: Kyoji Ogasawara <sawara04.o@gmail.com
>     <mailto: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 <https://
>     btrfs.readthedocs.io/en/latest/Administration.html#btrfs-specific-
>     mount-options>
>      > >
>      > > Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com
>     <mailto: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.
> 

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

[小笠原 共志]

> Sure, that sounds good to me.

Thank you.

I would like to confirm whether the warning message
"excessive commit interval %u, use with care" is acceptable.
Please let me know if you have any concerns or suggestions
for improvement.

2025年4月29日(火) 7:19 Qu Wenruo <quwenruo.btrfs@gmx.com>:
>
>
>
> 在 2025/4/29 07:45, 小笠原 共志 写道:
> > Thank you very much for your review and the helpful feedback.
> >
> > I will revise the patch according to your suggestions, replacing the
> > warning message with something less alarming such as "Use with care."
> >
> > Additionally, would it be okay if I also update the "nobarrier" message
> > to make it more informative?
>
> Sure, that sounds good to me.
>
> Thanks,
> Qu
>
> >
> > 2025年4月29日(火) 0:13 David Sterba <dsterba@suse.cz
> > <mailto: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
> >     <mailto:sawara04.o@gmail.com> 写道:
> >      > > From: Kyoji Ogasawara <sawara04.o@gmail.com
> >     <mailto: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 <https://
> >     btrfs.readthedocs.io/en/latest/Administration.html#btrfs-specific-
> >     mount-options>
> >      > >
> >      > > Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com
> >     <mailto: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.
> >
>

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

[David Sterba]

On Thu, May 01, 2025 at 12:33:32AM +0900, 小笠原 共志 wrote:
> > Sure, that sounds good to me.
> 
> Thank you.
> 
> I would like to confirm whether the warning message
> "excessive commit interval %u, use with care" is acceptable.
> Please let me know if you have any concerns or suggestions
> for improvement.

Yes, this is OK, thanks.