Re: [PATCH 4/5] btrfs: don't report success for a skipped log sync

[Filipe Manana]

On Fri, Oct 30, 2026 at 2:39 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> When a commit of the log root tree gives up, because the transaction
> needs a full commit or writing the log failed, btrfs_sync_log() still
> advances log_transid_committed and hands the result to the contexts on
> the list. Unlike a successful commit it does not advance log_transid, so
> the log transaction it just retired is still the one new syncs join.
>
> A sync that gets to the log root tree after that adds its context to
> the emptied list, sees log_transid_committed >= its log_transid and
> returns its log_ret, which is still 0:
>
>   fsync A (subvol 1)           rename       fsync B (subvol 2)
>
>                                             btrfs_need_log_full_commit()
>                                               -> false
>                                btrfs_set_log_full_commit()
>   btrfs_need_log_full_commit()
>     -> true
>   btrfs_remove_all_log_ctxs()
>   log_transid_committed = N
>   log_transid stays N
>                                             log_transid = N, log_ret = 0
>                                             log_transid_committed >= N
>                                             return 0
>
> btrfs_sync_file() then ends the transaction without committing it, and a
> crash before A's transaction commit loses what B was told is durable.
> This was reproduced with delays added to btrfs_sync_log().
>
> Fix by advancing log_transid when a commit of the log root tree gives
> up, so a later sync joins the next log transaction and either commits it
> or gives up itself. The check for an already committed log transaction
> can then no longer be true, so assert that it isn't, and still force a
> full commit if it is.
>
> Fixes: d1433debe7f4 ("Btrfs: just wait or commit our own log sub-transaction")
> CC: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
>  fs/btrfs/tree-log.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 1d9dfb63fc5a..b542b1d97989 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3437,11 +3437,20 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>                 goto out;
>         }
>
> -       if (log_root_tree->log_transid_committed >= root_log_ctx.log_transid) {
> +       /*
> +        * A commit of the log root tree moves log_transid on even when it
> +        * gives up, so the log transaction we just joined can't be done.
> +        */
> +       if (unlikely(log_root_tree->log_transid_committed >=
> +                    root_log_ctx.log_transid)) {
> +               ASSERT(0, "log_transid_committed=%d log_transid=%d",
> +                      log_root_tree->log_transid_committed,
> +                      root_log_ctx.log_transid);

Can we please stop using ASSERT() where we pass a false value and have
code to handle the unexpected case?
Some distros like SUSE ones enable assertions by default, so this is
effectively the same as BUG_ON().

As we have code to deal with this and avoid a crash, just use
WARN_ON() in the if statements expression with btrfs_warn(), or the
DEBUG_WARN() macro we have.
Both will trigger a stack trace and make fstests report it if we ever
hit it, while avoiding the unnecessary crash from the BUG() call done
by ASSERT().

Otherwise, it looks good.
With that change:

Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>

Thanks.



>                 blk_finish_plug(&plug);
>                 list_del_init(&root_log_ctx.list);
> +               btrfs_wait_tree_log_extents(log, mark);
>                 mutex_unlock(&log_root_tree->log_mutex);
> -               ret = root_log_ctx.log_ret;
> +               ret = BTRFS_LOG_FORCE_COMMIT;
>                 trace_btrfs_sync_log_exit(trans, root, ctx, ret);
>                 goto out;
>         }
> @@ -3575,6 +3584,13 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>         mutex_lock(&log_root_tree->log_mutex);
>         btrfs_remove_all_log_ctxs(log_root_tree, index2, ret);
>
> +       /*
> +        * If the commit gave up, log_transid still names the log transaction
> +        * we are retiring. Move it on, so that nobody joins a log transaction
> +        * whose result has already been handed out.
> +        */
> +       if (log_root_tree->log_transid == root_log_ctx.log_transid)
> +               log_root_tree->log_transid++;
>         log_root_tree->log_transid_committed++;
>         log_root_tree->log_commit[index2] = false;
>         mutex_unlock(&log_root_tree->log_mutex);
>
> --
> 2.55.0
>
>