Re: [PATCH V3] Btrfs: device_list_add() should not update list when mounted

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

-------- Original Message --------
Subject: Re: [PATCH V3] Btrfs: device_list_add() should not update list 
when mounted
From: Anand Jain <Anand.Jain@oracle.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
Date: 2014年06月09日 10:19
>
> Hi Qu,
>
>> Thanks for your patch.
>>
>> The patch works fine,
>
>
>
>> but unfortunately the solution seems not so generic.
>> The patch just avoid the dirty work to distinguish devices with same 
>> uuid,
>> This will fix the bug with dev scan, but when you umount the fs and
>> mount it again, it will use the old device again, since 
>> device_list_add()
> > still can't distinguish different devices with same dev uuid.
>
>  As mentioned in the other thread.. we expect user to check the devices
>  before / after mount and wipefs the disks which should not belong to
>  the fsid. as below.
>
> ----------------
>  Yes. some challenges to get that based on the generation number.
>  too many limitations. and patch created didn't pass all the tests.
>  so I didn't send that patch.
>  But I was talking about this patch (sorry to confuse you).
>
>    Btrfs: device_list_add() should not update list when mounted
>
>  And as of now when its unmounted we expect user to wipe SB
>  of the disk which should not belong to the fsid. which will
>  solve the problem as well. but a bit of hard work though.
>  (there is a chance to notice the _actual_ disks being used
>  after the fs is mounted)
> ----------------
>
>  Above patch will avoid the serious problem that happens
>  when disk reappears (on a SElinux)
>
>  when disk reappears on SElinux the device_list_add() is called
>  automatically.  And that will replace the disk of a mounted FS.
>  This patch will fix that.
>
>
>
>
>
>> IMO *current* root fix should add the ablity to distinguish different
>> deivces even they share
>> same uuid.
>> And further more, *long term* fix should be not reusing dev uuid and use
>> above fix(ablity to distinguish)
>> as a fallback.
>>
>> About the method, I still think Wang's suggestion, using generation to
>> distinguish, is good enough for this
>> bug.
>> It would be quite kind for you to provide any other ideas.
>
>  generation check does not solve the following test case.
>     (missing devices are very common incidence at the data centers).
>     - replace a missing disk (replace-source) with replace-target
>     - unmount
>     - now replace-target disappears
>     - replace-source which was missing now reappears
>     - mount (mounts with replace-source since replace-target is not
>       present and so now the generation is greatest on the
>       replace-source)
>     - unmount
>     - now all disks reappears
>     - mount would pick replace-source instead of replace-target
>
>
>  As you mention replace should not clone uuid is the best approach
>  for all these issues. I had the same idea when I first notice this
>  issue.
>
>  But as of now the above workaround patch is simple and safe.
>
>
>  Let me know. your thoughts.
>
> Thanks , Anand
>
>
Thanks for your reply.

Did you tried the following test case?
- mount with degraded mode (missing device is called A)
- replace device A with device B.
- umount fs
- device A reappeared and reboot the system
- mount fs again.

I think the newly mounted fs will still use device A not device B with 
your patch,
even though device B has a larger generation.

For the case you mentioned, I think the behavior is OK,
always use the device with *current* largest generation is a acceptable
strategy, since the only things we can depend on the devices we 
*current* see...

P.S. Even btrfs changes to not duplicate dev uuid, for backward 
compatibility,
we still need to face the problem...

Thanks,
Qu.