On Mon, Oct 07, 2019 at 09:37:49PM +0800, Qu Wenruo wrote:
>
>
> On 2019/10/7 下午9:30, Nikolay Borisov wrote:
> >
> >
> > 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@xxxxxxxxxx>
> >> ---
> >> 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.
>
>
> And, for most case, EUCLEAN should have a proper kernel message for
> what's going wrong, the value we hit and the value we expect.
>
> And for EUCLEAN, it's more like graceful error out for impossible thing.
> This is definitely not the case, and I believe the original EINVAL makes
> more sense than EUCLEAN.
I agree, EUCLEAN is wrong here.
