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

Kyoji Ogasawara
2025-05-25T02:32:32Z

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