Re: [PATCH]btrfs: Fix fstest case btrfs/219

[Anand Jain <anand.jain@oracle.com>]

On 27/07/2022 02:38, David Sterba wrote:
> On Fri, Jul 22, 2022 at 01:34:11AM -0400, hmsjwzb wrote:
>> On 7/21/22 09:37, Nikolay Borisov wrote:
>>> On 21.07.22 г. 11:36 ч., Flint.Wang wrote:
>>>> Hi,
>>>> fstest btrfs/291 failed.
>>>>
>>>> [How to reproduce]
>>>> mkdir -p /mnt/test/219.mnt
>>>> xfs_io -f -c "truncate 256m" /mnt/test/219.img1
>>>> mkfs.btrfs /mnt/test/219.img1
>>>> cp /mnt/test/219.img1 /mnt/test/219.img2
>>>> mount -o loop /mnt/test/219.img1 /mnt/test/219.mnt
>>>> umount /mnt/test/219.mnt
>>>> losetup -f --show /mnt/test/219.img1 dev
>>>> mount /dev/loop0 /mnt/test/219.mnt
>>>> umount /mnt/test/219.mnt
>>>> mount -o loop /mnt/test/219.img2 /mnt/test/219.mnt
>>>>
>>>> [Root cause]
>>>> if (fs_devices->opened && found_transid < device->generation) {
>>>>      /*
>>>>       * That is if the FS is _not_ mounted and if you
>>>>       * are here, that means there is more than one
>>>>       * disk with same uuid and devid.We keep the one
>>>>       * with larger generation number or the last-in if
>>>>       * generation are equal.
>>>>       */
>>>>      mutex_unlock(&fs_devices->device_list_mutex);
>>>>      return ERR_PTR(-EEXIST);
>>>> }
>>>>
>>>> [Personal opinion]
>>>> User might back up a block device to another. I think it is improper
>>>> to forbid user from mounting it.
>>>>
>>>> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
>>>
>>>
>>> This lacks any explanation whatsoever so it's not possible to judge whether the fix is correct or not.
>>>
>>>> ---
>>>>    fs/btrfs/volumes.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 6aa6bc769569a..76af32032ac85 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -900,7 +900,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>>>>             * tracking a problem where systems fail mount by subvolume id
>>>>             * when we reject replacement on a mounted FS.
>>>>             */
>>>> -        if (!fs_devices->opened && found_transid < device->generation) {
>>>> +        if (fs_devices->opened && found_transid < device->generation) {
>>>>                /*
>>>>                 * That is if the FS is _not_ mounted and if you
>>>>                 * are here, that means there is more than one
>>
>> Hi Nikolay,
>>
>> It seems the failure of btrfs/219 needs some explanation.
>>
>> Here is the thing.
>>          1. A storage device A with btrfs filesystem is running on a host.
>>          2. For example, we backup the device A to an exactly some device B.
>>          3. The device A continue to run for a while so the device->generation is getting bigger.
>>          4. Then you umount the device A and try to mount device B.
>>          5. Kernel find that device A has the same UUID as device B and has bigger device->generation.
>>             So the mount request of device B will be rejected.
> 
> That's on purpose, devices are matched by UUIDs and making block copies
> of the same filesystem is known "don't do that" and discouraged.
> 
> If you must store the block copies then you can change the UUID by
> btrfstune, there are two ways (fast metadata_uuid, and slow rewriting
> all metadata uuids in all blocks).
> 
>>
>>              if (!fs_devices->opened && found_transid < device->generation) {
>>                   /*
>>                    * That is if the FS is _not_ mounted and if you
>>                    * are here, that means there is more than one
>>                    * disk with same uuid and devid.We keep the one
>>                    * with larger generation number or the last-in if
>>                    * generation are equal.
>>                    */
>>                    mutex_unlock(&fs_devices->device_list_mutex);
>>                    return ERR_PTR(-EEXIST);
>>              }
>>
>> I think it is improper to reject that request. Because device A is not in open state.
> 
> But this would prevent mounting A. There should really be some way to
> distiguish the filesystems, the block device is not a stable identifier,
> the UUID is. Imagine having 10 copies of the same filesystem identified
> by the same UUID and device UUID, but with different generations and
> data. That's asking for problems.
> 
> There's not much the filesystem driver can do than to avoid using old
> devices and giving preference to the highest generation device. All
> devices with btrfs signature are registered in memory and this is the
> primary source when mounting the devices, not the block device itself.


David,

  The unintegrated patch [1] also used the same use case.

   [1]
     [PATCH v2][RESEND] btrfs: allow single disk devices to mount with 
older generations

  IMO device-copy and mount (without changing the UUID) can be allowed
  for a single device btrfs volume only. We even have a fstest case
  btrfs/219, which tests single device duplicate UUIDs.

  Please integrate [1].

-Anand