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

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