Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M

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

On 10/18/19 12:32 AM, David Sterba wrote:
> On Thu, Sep 12, 2019 at 08:45:50AM +0800, Anand Jain wrote:
>> In case of vm guest images copied from the golden image there is no
>> physical device or loop device or nbd device until its configured on
>> the host when required, so check for duplicate fsid at the time of
>> btrfstune -M is not convincing or a very limited approach.
>>
>>>> - As I said before, its a genuine use-case here where the user wants to
>>>>      revert the fsid change, so that btrfs fs root image can be booted.
>>>>      Its up-to the user if fsid is duplicate in the user space, as btrfs
>>>>      kernel rightly fails the mount if its duplicate fsid anyways.
>>>
>>> Reverting the uuid is fine
>>
>> ok thanks.
>>
>>> and requiring the uuids to be unique is
>>> preventing the users doing stupid things unknowingly.
>>
>> Right it should be done. But..
>> btrfstune -M is a wrong place.
> 
> No it's not, it's exactly the right place because that's the moment when
> user is doing an action that has consequences and if there's room for
> mistakes, it should not be too easy. If the usecase is valid, it should
> be possible though.


>> Because it can't avoid all the
>> cases of fsid getting duplicated.
> 
> But this does not matter. We're not protecting an image against external
> changes, but by accidental change by a concrete user action at a
> specific time.


>> Even after btrfstune -M, the fsid can be duplicated by the user.
> 
> Yes of course, eg. by doing 'echo UUID | dd of=image seek=... bs=...',
> there's no way we can prevent users from doing that. But that command is
> up to user to check for the target device and we have no responsibility
> for the damage.

>> So what's the point in restricting the btrfstune -M and fail to
>> undo the changed fsid.
> 
> The point is to prevent accidental damage.

> For the same reasons we have
> 'mkfs.btrfs -f' or 'btrfd device add -f' or even 'btrfstune -f -S 0'.
> I'm surprised and slightly disappointed that we have to go through these
> points, this is 101 of user interfaces.

  David,
  I understand the -f warning part we can have that too here, but that
  wasn't review comments so far.
  Per this thread's comments so far, it was only to keep undo fsid part
  blocked and reasoning was arbitrary, neither the code or patch which
  blocked it explains. It only sounded like punish the expert users
  so that we can keep the naive users safe.

>>> You seem to have a
>>> usecase where even duplicate uuids are required but I'm afraid it's not
>>> all clear how is it supposed to work. A few more examples or commands
>>> would be helpful.
>>
>> In the use case here, even the host is also running a copy of the golden
>> image (same fsid as vm guest) and because of duplicate fsid you can
>> only mount a vm guest image on the host after the btrfstune -m command
>> on the vm guest image. But after you have done that, as the vm guest
>> fsid is changed, it fails to boot, unfortunately changed fsid can not
>> be undone without this patch.
> 
> I can't say I have a clear picture yet, can you please describe it in
> some more desriptive way, like
> 
> host1: create image1-uuid1
> 
> host2: copy image1-uuid1 to image1-uuid2
> host2: use image1-uuid2
> host2: change image1-uuid2 back to uuid1     <-- I want this to work
  From the bug as I received.
     create btrfs root-image for the vm use.
     copy root-image to root-image1
     copy root-image to root-image2
     start vm1 using root-image1
     (when root-image1 has issues; shutdown vm1)
     start vm2 using root-image2 with root-image1 accessible.
     login to vm2
       (change fsid so that root-image1 can be mounted)
       btrsftune -m remote/root-image1
       mount -o loop remote/root-image1 /mnt
         analyze, collect logs, fix remote/root-image1
       umount /mnt
       (Revert the changed fsid so that vm2 can boot) <<<< Usecase wants 
this to work
       btrfstune -M $(btrfs in dump-super remote/root-image2 | \
                      grep metadata_uuid | awk '{print $2}') \
                      remote/root-image2
     logout from vm2
     start vm1 using root-image1

>> The fsid can be duplicate by many different other ways anyways. So in
>> this case how does it matter if btrfstune -M tries to ensure that fsid
>> is unique among the blkid known set of devices, which may change any
>> time after btrfstune -M as well (just copy a vm guest and map it to
>> a loop device). So btrfstune -M should be free from this check and
>> help the use case as above.
>>
>> And if we are concerned about the duplicate fsid as I asked Nikolay
>> before, we need to know what are problems in specifies, so that it can
>> be fixed in separate patches, but definitely not in btrfstune -M as
>> it can't fix the duplicate fsid problem completely as vm images can
>> be copied and mapped to a loop/nbd device anytime even after
>> btrfstune -M.
> 
> This only reiterates and was aswered above.
> 
> The usecase was not explained at the beginning,

  The change log explains the usecase. Looks like it wasn't sufficient
  to bring the whole context correctly. My bad.

> so it got a NAK because
> it brought a potentially dangerous change. The next step is usecase
> clarification so we understand if there's a way to make it work for the
> common and your usecase.
> 
> And we're almost there, but instead of handwaving that we can't prevent
> users doing lots of things, a simple 'so let's allow duplicate uuids
> with -f with a big warning and a paragraph in documentation, and btw
> here's a testcase' would do.  The patch could have been merged a month
> ago.
> 

  This usecase was using ext4 before. They aren't happy that btrfs needs
  btrfstune -m to mount a duplicate fsid and is still hoping that
  we would provide a better seamless solution like as below so that it
  to circumvent the duplicating fsid.

     mount -o loop,random_temp_fsid root-image2 /mnt

  where option random_tmp_fsid creates a kernel in-memory random-fsid
  to mount the duplicate fsid.

  If we are ok to support -o random_tmp_fsid (which I received a nack on
  the other channel), then I can drop this btrfs-progs patch altogether.
  I think we should allow the flexibility seamlessly. Also this kernel
  approach doesn't confuse the user space as perceived, as both
  metadata_uuid and fsid remains unchanged and it helps this use case
  much better.

Thanks, Anand