Re: [PATCH 4/5] btrfs: remove identified alien device in open_fs_devices

[Nikolay Borisov <nborisov@suse.com>]

On 7.10.19 г. 12:45 ч., Anand Jain wrote:
> Following test case explains it all, even though the degraded mount is
> successful the btrfs-progs fails to report the missing device.
> 
>  mkfs.btrfs -fq -draid1 -mraid1 /dev/sdc /dev/sdd && \
>  wipefs -a /dev/sdd && mount -o degraded /dev/sdc /btrfs && \
>  btrfs fi show -m /btrfs
> 
>  Label: none  uuid: 2b3b8d92-572b-4d37-b4ee-046d3a538495
> 	Total devices 2 FS bytes used 128.00KiB
> 	devid    1 size 1.09TiB used 2.01GiB path /dev/sdc
> 	devid    2 size 1.09TiB used 2.01GiB path /dev/sdd
> 
> This is because btrfs-progs does it fundamentally wrong way that
> it deduces the missing device status in the user land instead of
> refuting from the kernel.
> 
> At the same time in the kernel when we know that there is device
> with non-btrfs magic, then remove that device from the list so
> that btrfs-progs or someother userland utility won't be confused.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/disk-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 326d5281ad93..e05856432456 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3417,7 +3417,7 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>  	if (btrfs_super_bytenr(super) != bytenr ||
>  		    btrfs_super_magic(super) != BTRFS_MAGIC) {
>  		brelse(bh);
> -		return -EINVAL;
> +		return -EUCLEAN;

This is really non-obvious and you are propagating the special-meaning
of EUCLEAN waaaaaaaay beyond btrfs_open_one_device. In fact what this
patch does is make the following call chain return EUCLAN:

btrfs_open_one_device <-- finally removing the device in this function
 btrfs_get_bdev_and_sb <-- propagating it to here
  btrfs_read_dev_super
    btrfs_read_dev_one_super <-- you return the EUCLEAN


And your commit log doesn't mention anything about that. EUCLEAN
warrants a comment in this case since it changes behavior in
higher-level layers.

>  	}
>  
>  	*bh_ret = bh;
>