Re: [PATCH] btrfs: derive f_fsid with dev_t only when temp_fsid is active

[Anand Suveer Jain <asj@kernel.org>]

This issue and its current fix (below) have been on my mind, as I wasn't
fully satisfied with the approach even though it works.

Now, I think I have a better idea.

The core issue isn't that the new f_fsid derivation is wrong (recap:
f_fsid derivation changed from f(UUID) to f(UUID + dev_t)). Rather, the
problem is that f_fsid changes unexpectedly after an upgrade.

So we need to address the upgrade behavior, not how f_fsid itself is
derived.

My new proposed solution:

 . Put the new f_fsid derivation behind a compile-time config flag (an
incompat/opt-in flag).
 . For stable kernels, this flag will keep the new f_fsid derivation
disabled by default.

The only drawback I see with this approach is that it adds yet another
incompat flag, making the incompat feature list even longer.

Please let me know what you think. I can send out v2 with these changes
on Tuesday.

Thanks, Anand


On 13/9/26 02:06, Anand Jain wrote:
> Commit c2a74ed0494c ("btrfs: derive f_fsid from on-disk fsid and dev_t")
> mixed dev_t into f_fsid for all single-device setups to avoid f_fsid
> collisions with cloned filesystems.
> 
> However, doing this unconditionally breaks backward compatibility.
> statfs(2) f_fsid changes after a kernel upgrade, and also can shift
> across reboots or dev re-attaches as dev_t values change.
> 
> Fix this by only mixing dev_t when temp_fsid is active.
> This means for non-temp_fsid setups or the original mount, we use the
> old method of deriving fsid based on the UUID.
> 
> So in the case of a cloned Btrfs filesystem, we won't be able to
> maintain the same fsid across mount recycle if the mount order
> changes.
> 
> Fixes: c2a74ed0494c ("btrfs: derive f_fsid from on-disk fsid and dev_t")
> Reported-by: Dave Hansen <dave.hansen@intel.com>
> Closes: https://lore.kernel.org/linux-btrfs/be0c08f5-2f31-40f5-8a3b-f2f58b3e00ff@intel.com
> Signed-off-by: Anand Jain <asj@kernel.org>
> ---
>  Dave (Hansen), I wonder if you could verify whether this fixes the issue
>  on your end. I have run some limited test cases from fstests as of now,
>  and they passed.
> 
>  fs/btrfs/super.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 464129b1b0d4..ddb620ac241b 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1836,8 +1836,12 @@ static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	f_fsid.val[0] ^= btrfs_root_id(BTRFS_I(d_inode(dentry))->root) >> 32;
>  	f_fsid.val[1] ^= btrfs_root_id(BTRFS_I(d_inode(dentry))->root);
>  
> -	/* Hash dev_t to avoid f_fsid collision with cloned filesystems. */
> -	if (fs_info->fs_devices->total_devices == 1) {
> +	/*
> +	 * Hash dev_t to avoid f_fsid collisions with cloned filesystems.
> +	 * Only do this when a clone is present so the original filesystem
> +	 * (mounted first) maintains backward-compatible f_fsid behavior.
> +	 */
> +	if (fs_info->fs_devices->temp_fsid) {
>  		__kernel_fsid_t dev_fsid =
>  			u64_to_fsid(huge_encode_dev(fs_info->fs_devices->latest_dev->bdev->bd_dev));
>