Re: [PATCH 4/4] btrfs: validate device_list at scan for stray free

[Boris Burkov <boris@bur.io>]

On Sat, Mar 09, 2024 at 07:14:31PM +0530, Anand Jain wrote:
> Tempfsid assumes all registered single devices in the fs_devicies list are
> to be mounted; otherwise, they won't be in the btrfs_device list.
> 
> We recently fixed a related bug caused by leaving failed-open device in
> the list. This triggered tempfsid activation upon subsequent mounts of the
> same fsid wrongly.
> 
> To prevent this, scan the entire device list at mount for any stray
> device and free them in btrfs_scan_one_device().

Is this an additional precaution on top of maintaining an invariant on
every umount/failed mount that we have freed stale devices of single
device fs-es? Or is it fundamentally impossible for us to enforce that
invariant?

It feels like overkill to hack up free_stale_devices in this way,
compared to just ensuring that we manage cleaning up single devices
fs-es correctly when we are in a cleanup context. If this is practically
the best way to ensure we don't get caught with our pants down by a
random stale device, then I suppose it's fine.

A total aside I just thought of:
I think it might also make sense to consider adding logic to look for
single device fs-es with a device->bdev that is stale from the block
layer's perspective, and somehow marking those in a way that tempfsid
cares about. That would help with things that like that case where we
delete the block dev out from under a mounted fs and mount it a second
time with tempfsid after it's recreated. Not a huge deal, as we've
already discussed, though.

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 60d848392cd0..bb0857cfbef2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1382,6 +1382,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>  
>  	lockdep_assert_held(&uuid_mutex);
>  
> +	btrfs_free_stale_devices(0, NULL, true);
> +
>  	/*
>  	 * we would like to check all the supers, but that would make
>  	 * a btrfs mount succeed after a mkfs from a different FS.
> -- 
> 2.38.1
>