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

[Anand Jain]

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@xxxxxxxx>
> > >
> > >
> > > 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