Re: [PATCH 6/7] btrfs: remove max_mirrors argument from write_all_supers()

[Chris Mason <clm@meta.com>]

fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's no need to pass max_mirrors to write_all_supers() since from the
> given transaction handle we can infer if we are in a transaction commit
> or fsync context, so we can determine how many mirrors we need to use.
> 
> So remove the max_mirror argument from write_all_supers() and stop
> adjusting it in the callees write_dev_supers() and wait_dev_supers(),
> simplifying them besides the parameter list for write_all_supers().

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit a30345d606f40c870c69fb77195b3516a6dd6504
Author: Filipe Manana <fdmanana@suse.com>

btrfs: remove max_mirrors argument from write_all_supers()

This commit removes the max_mirrors parameter from write_all_supers()
and determines the mirror count from the transaction handle's in_fsync
flag, claiming this simplifies the code while maintaining correctness.

> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 19f7927a000d9..cf4ab067be72c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c

[ ... ]

> @@ -4018,13 +4011,14 @@ int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags)
>  	return min_tolerated;
>  }
>
> -int write_all_supers(struct btrfs_trans_handle *trans, int max_mirrors)
> +int write_all_supers(struct btrfs_trans_handle *trans)
>  {
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  	struct list_head *head;
>  	struct btrfs_device *dev;
>  	struct btrfs_super_block *sb;
>  	struct btrfs_dev_item *dev_item;
> +	int max_mirrors;
>  	int ret;
>  	int do_barriers;
>  	int max_errors;
> @@ -4033,13 +4027,13 @@ int write_all_supers(struct btrfs_trans_handle *trans, int max_mirrors)
>
>  	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
>
> -	/*
> -	 * max_mirrors == 0 indicates we're from commit_transaction,
> -	 * not from fsync where the tree roots in fs_info have not
> -	 * been consistent on disk.
> -	 */
> -	if (max_mirrors == 0)
> +	if (trans->in_fsync) {
> +		max_mirrors = 1;
> +	} else {
> +		/* We are called from transaction commit. */
> +		max_mirrors = BTRFS_SUPER_MIRROR_MAX;
>  		backup_super_roots(fs_info);
> +	}

Can this logic handle the case where btrfs_sync_file() falls through to
btrfs_commit_transaction()? Looking at fs/btrfs/file.c, when a full_sync
encounters a log sync failure or BTRFS_LOG_FORCE_COMMIT, the code flow is:

btrfs_sync_file()
   trans->in_fsync = true;  /* Set at file.c:1728 */
   ...
   btrfs_sync_log()  /* May fail or return BTRFS_LOG_FORCE_COMMIT */
   ...
   btrfs_commit_transaction(trans)  /* Fallthrough at file.c:1787-1816 */
      write_all_supers(trans)
         if (trans->in_fsync)  /* Still true here */
            max_mirrors = 1;  /* But we're in transaction commit */

The OLD code always passed max_mirrors=0 from btrfs_commit_transaction()
(transaction.c:2560), ensuring full superblock redundancy with
BTRFS_SUPER_MIRROR_MAX=3 and calling backup_super_roots() regardless of
how the transaction commit was triggered.

The NEW code uses trans->in_fsync which remains true during the
fsync-to-commit fallthrough path. This causes write_all_supers() to set
max_mirrors=1 and skip backup_super_roots(), reducing superblock
redundancy from 3 copies to 1 and skipping tree root backups during these
commits.

Is the commit message claim that "from the given transaction handle we can
infer if we are in a transaction commit or fsync context" accurate for
this case?