[PATCH 0/2] btrfs: Improve logging for barrier-related operation

[PATCH 0/2] btrfs: Improve logging for barrier-related operation

[sawara04.o]

From: Kyoji Ogasawara <sawara04.o@gmail.com>

This patch series improves logging related to barrier operations.

The first patch raises the log level for the "nobarrier" to better
reflect the associated risk.

The second patch fixes an incorrect log message that could cause
confusion.

Kyoji Ogasawara (2):
  btrfs: Raise nobarrier log level to warn
  btrfs: Fix incorrect log message related barrier

 fs/btrfs/super.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.49.0

[PATCH 1/2] btrfs: Raise nobarrier log level to warn

[sawara04.o]

From: Kyoji Ogasawara <sawara04.o@gmail.com>

The nobarrier option disables barriers, improving performance at the cost
of potential data loss during crashes or power failures especially on devices
without reliable write-back caching.
To better reflect this risk, the log level has been raised to warn.

Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
---
 fs/btrfs/super.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 7310e2fa8526..012b63a07ab1 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		}
 		break;
 	case Opt_barrier:
-		if (result.negated)
+		if (result.negated) {
 			btrfs_set_opt(ctx->mount_opt, NOBARRIER);
-		else
+			btrfs_warn(NULL, "turning off barriers, use with care");
+		} else {
 			btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
+		}
 		break;
 	case Opt_thread_pool:
 		if (result.uint_32 == 0) {
@@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
 	btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
 	btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
 	btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
-	btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");
 	btrfs_info_if_set(info, old, NOTREELOG, "disabling tree log");
 	btrfs_info_if_set(info, old, NOLOGREPLAY, "disabling log replay at mount time");
 	btrfs_info_if_set(info, old, FLUSHONCOMMIT, "turning on flush-on-commit");
-- 
2.49.0

[PATCH 2/2] btrfs: Fix incorrect log message related barrier

[sawara04.o]

From: Kyoji Ogasawara <sawara04.o@gmail.com>

Fix a wrong log message that appears when the "nobarrier" mount
option is unset.
When "nobarrier" is unset, barrier is actually enabled. However,
the log incorrectly stated "turning off barriers".

Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
---
 fs/btrfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 012b63a07ab1..0e8e978519d8 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1462,7 +1462,7 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
 	btrfs_info_if_unset(info, old, NODATACOW, "setting datacow");
 	btrfs_info_if_unset(info, old, SSD, "not using ssd optimizations");
 	btrfs_info_if_unset(info, old, SSD_SPREAD, "not using spread ssd allocation scheme");
-	btrfs_info_if_unset(info, old, NOBARRIER, "turning off barriers");
+	btrfs_info_if_unset(info, old, NOBARRIER, "turning on barriers");
 	btrfs_info_if_unset(info, old, NOTREELOG, "enabling tree log");
 	btrfs_info_if_unset(info, old, SPACE_CACHE, "disabling disk space caching");
 	btrfs_info_if_unset(info, old, FREE_SPACE_TREE, "disabling free space tree");
-- 
2.49.0

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Qu Wenruo]

在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> From: Kyoji Ogasawara <sawara04.o@gmail.com>
> 
> The nobarrier option disables barriers, improving performance at the cost
> of potential data loss during crashes or power failures especially on devices
> without reliable write-back caching.
> To better reflect this risk, the log level has been raised to warn.
> 
> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
> ---
>   fs/btrfs/super.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7310e2fa8526..012b63a07ab1 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   		}
>   		break;
>   	case Opt_barrier:
> -		if (result.negated)
> +		if (result.negated) {
>   			btrfs_set_opt(ctx->mount_opt, NOBARRIER);
> -		else
> +			btrfs_warn(NULL, "turning off barriers, use with care");
> +		} else {
>   			btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
> +		}
>   		break;
>   	case Opt_thread_pool:
>   		if (result.uint_32 == 0) {
> @@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
>   	btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
>   	btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
>   	btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
> -	btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");

I know you want to avoid duplicated messages about the nobarrier.

But it is better to use btrfs_emit_options() to add the warning.

The problem of showing the error in btrfs_parse_param() is, it can be 
triggered multiple times.

E.g. mount -o nobarrier,nobarrier,nobarrier $dev $mnt

Then there will be 3 lines of "turning of barrier, use with care".
But inside btrfs_emit_options() it will be only once.

In fact this also affect the warning for excessive commit mount option, 
but there is no better solution for that option, but we can do better here.

Thanks,
Qu

Re: [PATCH 2/2] btrfs: Fix incorrect log message related barrier

[Qu Wenruo]

在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> From: Kyoji Ogasawara <sawara04.o@gmail.com>
> 
> Fix a wrong log message that appears when the "nobarrier" mount
> option is unset.
> When "nobarrier" is unset, barrier is actually enabled. However,
> the log incorrectly stated "turning off barriers".
> 
> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

This is definitely a copy-paste-error.

And we can add a fixes tag when merging:

Fixes: eddb1a433f26 ("btrfs: add reconfigure callback for fs_context")

Thanks,
Qu

> ---
>   fs/btrfs/super.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 012b63a07ab1..0e8e978519d8 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1462,7 +1462,7 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
>   	btrfs_info_if_unset(info, old, NODATACOW, "setting datacow");
>   	btrfs_info_if_unset(info, old, SSD, "not using ssd optimizations");
>   	btrfs_info_if_unset(info, old, SSD_SPREAD, "not using spread ssd allocation scheme");
> -	btrfs_info_if_unset(info, old, NOBARRIER, "turning off barriers");
> +	btrfs_info_if_unset(info, old, NOBARRIER, "turning on barriers");
>   	btrfs_info_if_unset(info, old, NOTREELOG, "enabling tree log");
>   	btrfs_info_if_unset(info, old, SPACE_CACHE, "disabling disk space caching");
>   	btrfs_info_if_unset(info, old, FREE_SPACE_TREE, "disabling free space tree");

Re: [PATCH 2/2] btrfs: Fix incorrect log message related barrier

[Kyoji Ogasawara]

Thanks for the review.

Regarding the Fixes tag, would you prefer I resubmit the patch with
Fixes: eddb1a433f26 ("btrfs: add reconfigure callback for fs_context")?

2025年5月21日(水) 13:15 Qu Wenruo <quwenruo.btrfs@gmx.com>:
>
>
>
> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> > From: Kyoji Ogasawara <sawara04.o@gmail.com>
> >
> > Fix a wrong log message that appears when the "nobarrier" mount
> > option is unset.
> > When "nobarrier" is unset, barrier is actually enabled. However,
> > the log incorrectly stated "turning off barriers".
> >
> > Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> This is definitely a copy-paste-error.
>
> And we can add a fixes tag when merging:
>
> Fixes: eddb1a433f26 ("btrfs: add reconfigure callback for fs_context")
>
> Thanks,
> Qu
>
> > ---
> >   fs/btrfs/super.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 012b63a07ab1..0e8e978519d8 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -1462,7 +1462,7 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
> >       btrfs_info_if_unset(info, old, NODATACOW, "setting datacow");
> >       btrfs_info_if_unset(info, old, SSD, "not using ssd optimizations");
> >       btrfs_info_if_unset(info, old, SSD_SPREAD, "not using spread ssd allocation scheme");
> > -     btrfs_info_if_unset(info, old, NOBARRIER, "turning off barriers");
> > +     btrfs_info_if_unset(info, old, NOBARRIER, "turning on barriers");
> >       btrfs_info_if_unset(info, old, NOTREELOG, "enabling tree log");
> >       btrfs_info_if_unset(info, old, SPACE_CACHE, "disabling disk space caching");
> >       btrfs_info_if_unset(info, old, FREE_SPACE_TREE, "disabling free space tree");
>

Re: [PATCH 2/2] btrfs: Fix incorrect log message related barrier

[Qu Wenruo]

在 2025/5/24 11:30, Kyoji Ogasawara 写道:
> Thanks for the review.
> 
> Regarding the Fixes tag, would you prefer I resubmit the patch with
> Fixes: eddb1a433f26 ("btrfs: add reconfigure callback for fs_context")?

We will handle the tag when merging the patches into for-next branch.

So you don't need to do anything.

Thanks,
Qu

> 
> 2025年5月21日(水) 13:15 Qu Wenruo <quwenruo.btrfs@gmx.com>:
>>
>>
>>
>> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
>>> From: Kyoji Ogasawara <sawara04.o@gmail.com>
>>>
>>> Fix a wrong log message that appears when the "nobarrier" mount
>>> option is unset.
>>> When "nobarrier" is unset, barrier is actually enabled. However,
>>> the log incorrectly stated "turning off barriers".
>>>
>>> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> This is definitely a copy-paste-error.
>>
>> And we can add a fixes tag when merging:
>>
>> Fixes: eddb1a433f26 ("btrfs: add reconfigure callback for fs_context")
>>
>> Thanks,
>> Qu
>>
>>> ---
>>>    fs/btrfs/super.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 012b63a07ab1..0e8e978519d8 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -1462,7 +1462,7 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
>>>        btrfs_info_if_unset(info, old, NODATACOW, "setting datacow");
>>>        btrfs_info_if_unset(info, old, SSD, "not using ssd optimizations");
>>>        btrfs_info_if_unset(info, old, SSD_SPREAD, "not using spread ssd allocation scheme");
>>> -     btrfs_info_if_unset(info, old, NOBARRIER, "turning off barriers");
>>> +     btrfs_info_if_unset(info, old, NOBARRIER, "turning on barriers");
>>>        btrfs_info_if_unset(info, old, NOTREELOG, "enabling tree log");
>>>        btrfs_info_if_unset(info, old, SPACE_CACHE, "disabling disk space caching");
>>>        btrfs_info_if_unset(info, old, FREE_SPACE_TREE, "disabling free space tree");
>>
> 

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Kyoji Ogasawara]

Thanks for the explanation. I understand the issue with btrfs_parse_param()
being triggered multiple times.

If I move the log into btrfs_parse_param(), it would currently use
btrfs_info_if_set(),
resulting in an info level log.

Is an info level acceptable for this warning, or would you prefer a
warn level log?

If so, should I create a new helper function like btrfs_warn_if_set()?

2025年5月21日(水) 13:13 Qu Wenruo <quwenruo.btrfs@gmx.com>:
>
>
>
> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> > From: Kyoji Ogasawara <sawara04.o@gmail.com>
> >
> > The nobarrier option disables barriers, improving performance at the cost
> > of potential data loss during crashes or power failures especially on devices
> > without reliable write-back caching.
> > To better reflect this risk, the log level has been raised to warn.
> >
> > Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
> > ---
> >   fs/btrfs/super.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 7310e2fa8526..012b63a07ab1 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >               }
> >               break;
> >       case Opt_barrier:
> > -             if (result.negated)
> > +             if (result.negated) {
> >                       btrfs_set_opt(ctx->mount_opt, NOBARRIER);
> > -             else
> > +                     btrfs_warn(NULL, "turning off barriers, use with care");
> > +             } else {
> >                       btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
> > +             }
> >               break;
> >       case Opt_thread_pool:
> >               if (result.uint_32 == 0) {
> > @@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
> >       btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
> >       btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
> >       btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
> > -     btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");
>
> I know you want to avoid duplicated messages about the nobarrier.
>
> But it is better to use btrfs_emit_options() to add the warning.
>
> The problem of showing the error in btrfs_parse_param() is, it can be
> triggered multiple times.
>
> E.g. mount -o nobarrier,nobarrier,nobarrier $dev $mnt
>
> Then there will be 3 lines of "turning of barrier, use with care".
> But inside btrfs_emit_options() it will be only once.
>
> In fact this also affect the warning for excessive commit mount option,
> but there is no better solution for that option, but we can do better here.
>
> Thanks,
> Qu

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Qu Wenruo]

在 2025/5/24 11:51, Kyoji Ogasawara 写道:
> Thanks for the explanation. I understand the issue with btrfs_parse_param()
> being triggered multiple times.
> 
> If I move the log into btrfs_parse_param(), it would currently use
> btrfs_info_if_set(),
> resulting in an info level log.
> 
> Is an info level acceptable for this warning, or would you prefer a
> warn level log?

I think info level is good enough.

As the main purpose of that message line is still just to show we're 
using barrier or not, the extra "use with care" is just something good 
to have.

Thus no need to go warning IMHO.

Thanks,
Qu


> 
> If so, should I create a new helper function like btrfs_warn_if_set()?
> 
> 2025年5月21日(水) 13:13 Qu Wenruo <quwenruo.btrfs@gmx.com>:
>>
>>
>>
>> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
>>> From: Kyoji Ogasawara <sawara04.o@gmail.com>
>>>
>>> The nobarrier option disables barriers, improving performance at the cost
>>> of potential data loss during crashes or power failures especially on devices
>>> without reliable write-back caching.
>>> To better reflect this risk, the log level has been raised to warn.
>>>
>>> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
>>> ---
>>>    fs/btrfs/super.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 7310e2fa8526..012b63a07ab1 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>>                }
>>>                break;
>>>        case Opt_barrier:
>>> -             if (result.negated)
>>> +             if (result.negated) {
>>>                        btrfs_set_opt(ctx->mount_opt, NOBARRIER);
>>> -             else
>>> +                     btrfs_warn(NULL, "turning off barriers, use with care");
>>> +             } else {
>>>                        btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
>>> +             }
>>>                break;
>>>        case Opt_thread_pool:
>>>                if (result.uint_32 == 0) {
>>> @@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
>>>        btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
>>>        btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
>>>        btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
>>> -     btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");
>>
>> I know you want to avoid duplicated messages about the nobarrier.
>>
>> But it is better to use btrfs_emit_options() to add the warning.
>>
>> The problem of showing the error in btrfs_parse_param() is, it can be
>> triggered multiple times.
>>
>> E.g. mount -o nobarrier,nobarrier,nobarrier $dev $mnt
>>
>> Then there will be 3 lines of "turning of barrier, use with care".
>> But inside btrfs_emit_options() it will be only once.
>>
>> In fact this also affect the warning for excessive commit mount option,
>> but there is no better solution for that option, but we can do better here.
>>
>> Thanks,
>> Qu
> 

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Kyoji Ogasawara]

Got it, thanks.

I'll add the "turning off barriers, use with care" log at the info
level in btrfs_emit_options().

Since this is part of a patch set and "[2/2] btrfs: Fix incorrect log
message related barrier"
doesn't need a Fixes tag, should I just resubmit the corrected version
of this patch?

Thanks,
Kyoji

2025年5月24日(土) 12:49 Qu Wenruo <wqu@suse.com>:
>
>
>
> 在 2025/5/24 11:51, Kyoji Ogasawara 写道:
> > Thanks for the explanation. I understand the issue with btrfs_parse_param()
> > being triggered multiple times.
> >
> > If I move the log into btrfs_parse_param(), it would currently use
> > btrfs_info_if_set(),
> > resulting in an info level log.
> >
> > Is an info level acceptable for this warning, or would you prefer a
> > warn level log?
>
> I think info level is good enough.
>
> As the main purpose of that message line is still just to show we're
> using barrier or not, the extra "use with care" is just something good
> to have.
>
> Thus no need to go warning IMHO.
>
> Thanks,
> Qu
>
>
> >
> > If so, should I create a new helper function like btrfs_warn_if_set()?
> >
> > 2025年5月21日(水) 13:13 Qu Wenruo <quwenruo.btrfs@gmx.com>:
> >>
> >>
> >>
> >> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> >>> From: Kyoji Ogasawara <sawara04.o@gmail.com>
> >>>
> >>> The nobarrier option disables barriers, improving performance at the cost
> >>> of potential data loss during crashes or power failures especially on devices
> >>> without reliable write-back caching.
> >>> To better reflect this risk, the log level has been raised to warn.
> >>>
> >>> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
> >>> ---
> >>>    fs/btrfs/super.c | 7 ++++---
> >>>    1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> >>> index 7310e2fa8526..012b63a07ab1 100644
> >>> --- a/fs/btrfs/super.c
> >>> +++ b/fs/btrfs/super.c
> >>> @@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >>>                }
> >>>                break;
> >>>        case Opt_barrier:
> >>> -             if (result.negated)
> >>> +             if (result.negated) {
> >>>                        btrfs_set_opt(ctx->mount_opt, NOBARRIER);
> >>> -             else
> >>> +                     btrfs_warn(NULL, "turning off barriers, use with care");
> >>> +             } else {
> >>>                        btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
> >>> +             }
> >>>                break;
> >>>        case Opt_thread_pool:
> >>>                if (result.uint_32 == 0) {
> >>> @@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
> >>>        btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
> >>>        btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
> >>>        btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
> >>> -     btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");
> >>
> >> I know you want to avoid duplicated messages about the nobarrier.
> >>
> >> But it is better to use btrfs_emit_options() to add the warning.
> >>
> >> The problem of showing the error in btrfs_parse_param() is, it can be
> >> triggered multiple times.
> >>
> >> E.g. mount -o nobarrier,nobarrier,nobarrier $dev $mnt
> >>
> >> Then there will be 3 lines of "turning of barrier, use with care".
> >> But inside btrfs_emit_options() it will be only once.
> >>
> >> In fact this also affect the warning for excessive commit mount option,
> >> but there is no better solution for that option, but we can do better here.
> >>
> >> Thanks,
> >> Qu
> >
>

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Kyoji Ogasawara]

Sorry for sending piecemeal updates.

I've realized that if the log goes into btrfs_emit_options(), it'll
only show up during remount operations.

So, I was thinking of adding the following code to open_ctree(), right
after the btrfs_check_options() call.

       if (btrfs_test_opt(fs_info, NOBARRIER))
               btrfs_info(fs_info, "turning off barriers, use with care");

What do you think of this approach?

We could also set the log level to warning if that's preferred.

2025年5月25日(日) 11:32 Kyoji Ogasawara <sawara04.o@gmail.com>:
>
> Got it, thanks.
>
> I'll add the "turning off barriers, use with care" log at the info
> level in btrfs_emit_options().
>
> Since this is part of a patch set and "[2/2] btrfs: Fix incorrect log
> message related barrier"
> doesn't need a Fixes tag, should I just resubmit the corrected version
> of this patch?
>
> Thanks,
> Kyoji
>
> 2025年5月24日(土) 12:49 Qu Wenruo <wqu@suse.com>:
> >
> >
> >
> > 在 2025/5/24 11:51, Kyoji Ogasawara 写道:
> > > Thanks for the explanation. I understand the issue with btrfs_parse_param()
> > > being triggered multiple times.
> > >
> > > If I move the log into btrfs_parse_param(), it would currently use
> > > btrfs_info_if_set(),
> > > resulting in an info level log.
> > >
> > > Is an info level acceptable for this warning, or would you prefer a
> > > warn level log?
> >
> > I think info level is good enough.
> >
> > As the main purpose of that message line is still just to show we're
> > using barrier or not, the extra "use with care" is just something good
> > to have.
> >
> > Thus no need to go warning IMHO.
> >
> > Thanks,
> > Qu
> >
> >
> > >
> > > If so, should I create a new helper function like btrfs_warn_if_set()?
> > >
> > > 2025年5月21日(水) 13:13 Qu Wenruo <quwenruo.btrfs@gmx.com>:
> > >>
> > >>
> > >>
> > >> 在 2025/5/21 12:57, sawara04.o@gmail.com 写道:
> > >>> From: Kyoji Ogasawara <sawara04.o@gmail.com>
> > >>>
> > >>> The nobarrier option disables barriers, improving performance at the cost
> > >>> of potential data loss during crashes or power failures especially on devices
> > >>> without reliable write-back caching.
> > >>> To better reflect this risk, the log level has been raised to warn.
> > >>>
> > >>> Signed-off-by: Kyoji Ogasawara <sawara04.o@gmail.com>
> > >>> ---
> > >>>    fs/btrfs/super.c | 7 ++++---
> > >>>    1 file changed, 4 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > >>> index 7310e2fa8526..012b63a07ab1 100644
> > >>> --- a/fs/btrfs/super.c
> > >>> +++ b/fs/btrfs/super.c
> > >>> @@ -407,10 +407,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > >>>                }
> > >>>                break;
> > >>>        case Opt_barrier:
> > >>> -             if (result.negated)
> > >>> +             if (result.negated) {
> > >>>                        btrfs_set_opt(ctx->mount_opt, NOBARRIER);
> > >>> -             else
> > >>> +                     btrfs_warn(NULL, "turning off barriers, use with care");
> > >>> +             } else {
> > >>>                        btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
> > >>> +             }
> > >>>                break;
> > >>>        case Opt_thread_pool:
> > >>>                if (result.uint_32 == 0) {
> > >>> @@ -1439,7 +1441,6 @@ static void btrfs_emit_options(struct btrfs_fs_info *info,
> > >>>        btrfs_info_if_set(info, old, NODATASUM, "setting nodatasum");
> > >>>        btrfs_info_if_set(info, old, SSD, "enabling ssd optimizations");
> > >>>        btrfs_info_if_set(info, old, SSD_SPREAD, "using spread ssd allocation scheme");
> > >>> -     btrfs_info_if_set(info, old, NOBARRIER, "turning off barriers");
> > >>
> > >> I know you want to avoid duplicated messages about the nobarrier.
> > >>
> > >> But it is better to use btrfs_emit_options() to add the warning.
> > >>
> > >> The problem of showing the error in btrfs_parse_param() is, it can be
> > >> triggered multiple times.
> > >>
> > >> E.g. mount -o nobarrier,nobarrier,nobarrier $dev $mnt
> > >>
> > >> Then there will be 3 lines of "turning of barrier, use with care".
> > >> But inside btrfs_emit_options() it will be only once.
> > >>
> > >> In fact this also affect the warning for excessive commit mount option,
> > >> but there is no better solution for that option, but we can do better here.
> > >>
> > >> Thanks,
> > >> Qu
> > >
> >

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Qu Wenruo]

在 2025/5/26 03:15, Kyoji Ogasawara 写道:
> Sorry for sending piecemeal updates.
> 
> I've realized that if the log goes into btrfs_emit_options(), it'll
> only show up during remount operations.
> 
> So, I was thinking of adding the following code to open_ctree(), right
> after the btrfs_check_options() call.
> 
>         if (btrfs_test_opt(fs_info, NOBARRIER))
>                 btrfs_info(fs_info, "turning off barriers, use with care");
> 
> What do you think of this approach?
> 
> We could also set the log level to warning if that's preferred.

It turns out to be a more deeply involved bug.

The root cause is, we no longer use btrfs_parse_options() to output the 
mount option changes.

Before the fsconfig change, we have btrfs_parse_options() function, 
which will output an info line for each triggered option.
That function is utilized by both mount (open_ctree()) and remount 
(btrfs_remount())

But with the fsconfig migration, we use parse_param() helper, which now 
only handles the feature setting, no more messaging.
And btrfs_emit_options() to emit the message line.

The btrfs_emit_options() can handle empty @old context, thus it is 
designed to handle both mount and remount.

Unfortunately at mount time we do not call btrfs_emit_options(), this 
means we lost all the previous mount messages.

I'm not sure if this is intended or not, the fact we didn't notice the 
change until now means, most of the info lines are not that useful and 
no one is really noticing them.

So @josef, is it intended to skip all the mount option related info 
messages? Or it's just a bug?

Thanks,
Qu

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[David Sterba]

On Sat, May 24, 2025 at 01:18:58PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/5/24 11:51, Kyoji Ogasawara 写道:
> > Thanks for the explanation. I understand the issue with btrfs_parse_param()
> > being triggered multiple times.
> > 
> > If I move the log into btrfs_parse_param(), it would currently use
> > btrfs_info_if_set(),
> > resulting in an info level log.
> > 
> > Is an info level acceptable for this warning, or would you prefer a
> > warn level log?
> 
> I think info level is good enough.
> 
> As the main purpose of that message line is still just to show we're 
> using barrier or not, the extra "use with care" is just something good 
> to have.
> 
> Thus no need to go warning IMHO.

Agreed, info level is the right one. Warn messages are for user/admin
attention and some action may be needed. If somebody intentionally sets
the slightly dangerous options then info is to log that and "you get
what you ask for".

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[David Sterba]

On Mon, May 26, 2025 at 02:24:54PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/5/26 03:15, Kyoji Ogasawara 写道:
> > Sorry for sending piecemeal updates.
> > 
> > I've realized that if the log goes into btrfs_emit_options(), it'll
> > only show up during remount operations.
> > 
> > So, I was thinking of adding the following code to open_ctree(), right
> > after the btrfs_check_options() call.
> > 
> >         if (btrfs_test_opt(fs_info, NOBARRIER))
> >                 btrfs_info(fs_info, "turning off barriers, use with care");
> > 
> > What do you think of this approach?
> > 
> > We could also set the log level to warning if that's preferred.
> 
> It turns out to be a more deeply involved bug.
> 
> The root cause is, we no longer use btrfs_parse_options() to output the 
> mount option changes.
> 
> Before the fsconfig change, we have btrfs_parse_options() function, 
> which will output an info line for each triggered option.
> That function is utilized by both mount (open_ctree()) and remount 
> (btrfs_remount())
> 
> But with the fsconfig migration, we use parse_param() helper, which now 
> only handles the feature setting, no more messaging.
> And btrfs_emit_options() to emit the message line.
> 
> The btrfs_emit_options() can handle empty @old context, thus it is 
> designed to handle both mount and remount.
> 
> Unfortunately at mount time we do not call btrfs_emit_options(), this 
> means we lost all the previous mount messages.
> 
> I'm not sure if this is intended or not, the fact we didn't notice the 
> change until now means, most of the info lines are not that useful and 
> no one is really noticing them.
> 
> So @josef, is it intended to skip all the mount option related info 
> messages? Or it's just a bug?

I don't think this was intentional and it looks like a usability
regression. If it's just changed timing when the options are printed
during mount it may not be serious.

Comparing old and new logs from my VMs, I see the difference now. It
may not be easy to spot as it's the compression messages and seeing at
least some mount messages I don't know what could be missing there.

We should probably reinstate the messages, possibly also removing some
that are not that interesting like 'has skinny extents' or 'checking
UUID tree'.

Re: [PATCH 1/2] btrfs: Raise nobarrier log level to warn

[Kyoji Ogasawara]

This is becoming a different discussion than the original patch
content, what should we do about this?

Should this patch only output logs about “no barrier” as a temporary
fix, and another patch will restore the log output?

I also think that at least the “[2/2] btrfs: Fix incorrect log message
related barrier” patch should be applied.

Re: [PATCH 2/2] btrfs: Fix incorrect log message related barrier

[Kyoji Ogasawara]

Since the discussion appears to have stalled at 'btrfs: Raise
nobarrier log level to warn', I plan to drop this patch and submit it
again separately.

Thanks,
Kyoji