linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
@ 2019-09-06  0:50 Anand Jain
  2019-09-06  7:21 ` Nikolay Borisov
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-09-06  0:50 UTC (permalink / raw)
  To: linux-btrfs

It's common to copy/snapshot an OS image to run another instance of the OS.
A duplicate fsid can't be mounted on the same system unless the fsid is
changed by using btrfstune -m.

However in some circumstances the image needs to go back to the original
fsid /metadata_uuid.

As of now btrfstune -M fails if the specified uuid isn't unique, as show
below.

btrfstune -M $(btrfs in dump-super ./2-2g.img | grep metadata_uuid | \
					awk '{print $2}') ./2-2g.img
ERROR: fsid 87f8d9c5-a8b7-438e-a890-17bbe11c95e5 is not unique

But as we are changing the fsid of an unmounted image, so its ok to
leave it to the users choice if the fsid is not unique, so that the
image can be sent back the system where it was used with that fsid.

So this patch drops the check test_uuid_unique() for btrfstune -M|m.
Thanks.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 btrfstune.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/btrfstune.c b/btrfstune.c
index afa3aae35412..4befcadef8b1 100644
--- a/btrfstune.c
+++ b/btrfstune.c
@@ -570,10 +570,6 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 			error("could not parse UUID: %s", new_fsid_str);
 			return 1;
 		}
-		if (!test_uuid_unique(new_fsid_str)) {
-			error("fsid %s is not unique", new_fsid_str);
-			return 1;
-		}
 	}
 
 	fd = open(device, O_RDWR);
-- 
2.21.0 (Apple Git-120)


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-06  0:50 [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M Anand Jain
@ 2019-09-06  7:21 ` Nikolay Borisov
  2019-09-06  9:27   ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: Nikolay Borisov @ 2019-09-06  7:21 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



On 6.09.19 г. 3:50 ч., Anand Jain wrote:
> It's common to copy/snapshot an OS image to run another instance of the OS.
> A duplicate fsid can't be mounted on the same system unless the fsid is
> changed by using btrfstune -m.
> 
> However in some circumstances the image needs to go back to the original
> fsid /metadata_uuid.
> 
> As of now btrfstune -M fails if the specified uuid isn't unique, as show
> below.
> 
> btrfstune -M $(btrfs in dump-super ./2-2g.img | grep metadata_uuid | \
> 					awk '{print $2}') ./2-2g.img
> ERROR: fsid 87f8d9c5-a8b7-438e-a890-17bbe11c95e5 is not unique

NAK.

This is intended. Otherwise it's an open avenue for the user to shoot
themselves in the foot. If you know what you are doing and are
absolutely sure the original fs is no longer present - then just flush
libblkid cache and you'll be able to set the FSID back to the original one.




> 
> But as we are changing the fsid of an unmounted image, so its ok to
> leave it to the users choice if the fsid is not unique, so that the
> image can be sent back the system where it was used with that fsid.
> 
> So this patch drops the check test_uuid_unique() for btrfstune -M|m.
> Thanks.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  btrfstune.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/btrfstune.c b/btrfstune.c
> index afa3aae35412..4befcadef8b1 100644
> --- a/btrfstune.c
> +++ b/btrfstune.c
> @@ -570,10 +570,6 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
>  			error("could not parse UUID: %s", new_fsid_str);
>  			return 1;
>  		}
> -		if (!test_uuid_unique(new_fsid_str)) {
> -			error("fsid %s is not unique", new_fsid_str);
> -			return 1;
> -		}
>  	}
>  
>  	fd = open(device, O_RDWR);
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-06  7:21 ` Nikolay Borisov
@ 2019-09-06  9:27   ` Anand Jain
  2019-09-09 11:40     ` Nikolay Borisov
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-09-06  9:27 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs


<snip>

> This is intended. Otherwise it's an open avenue for the user to shoot
> themselves in the foot.

  I don't understand how?

> If you know what you are doing and are
> absolutely sure the original fs is no longer present 
> - then just flush
> libblkid cache and you'll be able to set the FSID back to the original one.
> 
<snip>

  No no its not about the stale cache holding the original fsid. The use
  case is - a golden copy of the bootable OS image is being used and
  shares the same fsid on multiple hosts.
  Now if you want to mount another copy it for some changes, you need to
  btrfstune -m on the copy. And later if you want to boot it
  successfully, it needs its original fsid back.

HTH, Anand

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-06  9:27   ` Anand Jain
@ 2019-09-09 11:40     ` Nikolay Borisov
  2019-09-10  5:12       ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: Nikolay Borisov @ 2019-09-09 11:40 UTC (permalink / raw)
  To: Anand Jain, Nikolay Borisov, linux-btrfs



On 6.09.19 г. 12:27 ч., Anand Jain wrote:
> 
> <snip>
> 
>> This is intended. Otherwise it's an open avenue for the user to shoot
>> themselves in the foot.
> 
>  I don't understand how?
> 
>> If you know what you are doing and are
>> absolutely sure the original fs is no longer present - then just flush
>> libblkid cache and you'll be able to set the FSID back to the original
>> one.
>>
> <snip>
> 
>  No no its not about the stale cache holding the original fsid. The use

Then this is worst. UUID, by definition, are Unique. What you want to is
to toss the Unique part, meaning we'll really be left with some sort of
ID. What's more - the UUID is used by libblkid to populate the available
devices so not making it unique can cause subtle bugs.

>  case is - a golden copy of the bootable OS image is being used and
>  shares the same fsid on multiple hosts.
>  Now if you want to mount another copy it for some changes, you need to
>  btrfstune -m on the copy. And later if you want to boot it
>  successfully, it needs its original fsid back.
> 
> HTH, Anand

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-09 11:40     ` Nikolay Borisov
@ 2019-09-10  5:12       ` Anand Jain
  2019-09-11 17:01         ` David Sterba
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-09-10  5:12 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs


Nikolay,

>>> This is intended. Otherwise it's an open avenue for the user to shoot
>>> themselves in the foot.
>>
>>   I don't understand how?

  Again. Any idea how? Is there any test case?

  <snip>

> UUID, by definition, are Unique. What you want to is
> to toss the Unique part, meaning we'll really be left with some sort of
> ID. What's more - the UUID is used by libblkid to populate the available
> devices so not making it unique can cause subtle bugs.

- Irrespective of the fstype, when fs image file is copied/snapshot-ed
   the fsid is indeed going to be duplicate.

- xfs and btrfs kernel doesn't allow duplicate fsid to be mounted which
   is fair and accepted. (I remember fixing the duplicate fsid bugs in
   the kernel, just fyi if you are concerned about them).

- ext4 kernel does allow duplicate fsid to be mounted.

- btrfstume -M <uuid> isn't the place to check if the fsid is a
   duplicate. Because, libblkid will be unaware of the complete list of
   fs images with its fsid.

- 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.

Thanks, Anand

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-10  5:12       ` Anand Jain
@ 2019-09-11 17:01         ` David Sterba
  2019-09-12  0:45           ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: David Sterba @ 2019-09-11 17:01 UTC (permalink / raw)
  To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs

On Tue, Sep 10, 2019 at 01:12:43PM +0800, Anand Jain wrote:
> 
> Nikolay,
> 
> >>> This is intended. Otherwise it's an open avenue for the user to shoot
> >>> themselves in the foot.
> >>
> >>   I don't understand how?
> 
>   Again. Any idea how? Is there any test case?
> 
>   <snip>
> 
> > UUID, by definition, are Unique. What you want to is
> > to toss the Unique part, meaning we'll really be left with some sort of
> > ID. What's more - the UUID is used by libblkid to populate the available
> > devices so not making it unique can cause subtle bugs.
> 
> - Irrespective of the fstype, when fs image file is copied/snapshot-ed
>    the fsid is indeed going to be duplicate.
> 
> - xfs and btrfs kernel doesn't allow duplicate fsid to be mounted which
>    is fair and accepted. (I remember fixing the duplicate fsid bugs in
>    the kernel, just fyi if you are concerned about them).
> 
> - ext4 kernel does allow duplicate fsid to be mounted.
> 
> - btrfstume -M <uuid> isn't the place to check if the fsid is a
>    duplicate. Because, libblkid will be unaware of the complete list of
>    fs images with its fsid.

I don't understand this part. Blkid tracks the device iformation, like
the uuid, and the cache gets updated. So what does 'will be unaware of
the complete list' mean?

If it's on the same host it's a matter of keeping the cache in sync with
the actual devices.

> - 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 and requiring the uuids to be unique is
preventing the users doing stupid things unknowingly. 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-11 17:01         ` David Sterba
@ 2019-09-12  0:45           ` Anand Jain
  2019-09-24 11:20             ` Anand Jain
  2019-10-17 16:32             ` David Sterba
  0 siblings, 2 replies; 12+ messages in thread
From: Anand Jain @ 2019-09-12  0:45 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs


thanks for the comments, more inline below.

>> - btrfstume -M <uuid> isn't the place to check if the fsid is a
>>     duplicate. Because, libblkid will be unaware of the complete list of
>>     fs images with its fsid.
> 
> I don't understand this part. Blkid tracks the device iformation, like
> the uuid, and the cache gets updated. So what does 'will be unaware of
> the complete list' mean?
> 
> If it's on the same host it's a matter of keeping the cache in sync with
> the actual devices.

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. Because it can't avoid all the
cases of fsid getting duplicated.
Even after btrfstune -M, the fsid can be duplicated by the user.
So what's the point in restricting the btrfstune -M and fail to
undo the changed fsid.

> 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.

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.

Thanks, Anand

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-12  0:45           ` Anand Jain
@ 2019-09-24 11:20             ` Anand Jain
  2019-10-01  8:08               ` Anand Jain
  2019-10-17 16:32             ` David Sterba
  1 sibling, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-09-24 11:20 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs


David, ping on this patch.



On 9/12/19 8:45 AM, Anand Jain wrote:
> 
> thanks for the comments, more inline below.
> 
>>> - btrfstume -M <uuid> isn't the place to check if the fsid is a
>>>     duplicate. Because, libblkid will be unaware of the complete list of
>>>     fs images with its fsid.
>>
>> I don't understand this part. Blkid tracks the device iformation, like
>> the uuid, and the cache gets updated. So what does 'will be unaware of
>> the complete list' mean?
>>
>> If it's on the same host it's a matter of keeping the cache in sync with
>> the actual devices.
> 
> 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. Because it can't avoid all the
> cases of fsid getting duplicated.
> Even after btrfstune -M, the fsid can be duplicated by the user.
> So what's the point in restricting the btrfstune -M and fail to
> undo the changed fsid.
> 
>> 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.
> 
> 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.
> 
> Thanks, Anand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-24 11:20             ` Anand Jain
@ 2019-10-01  8:08               ` Anand Jain
  0 siblings, 0 replies; 12+ messages in thread
From: Anand Jain @ 2019-10-01  8:08 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs


Ping.

Just found that- an image with metadata_uuid can't be a seed device - 
does not make any sense to me as to why.

And test_uuid_unique() blocks undo of metadata_uuid on certain systems.

Moreover the reason to use test_uuid_unique() in the first place
is arbitrary.

Thanks,
Anand



On 9/24/19 7:20 PM, Anand Jain wrote:
> 
> David, ping on this patch.
> 
> 
> 
> On 9/12/19 8:45 AM, Anand Jain wrote:
>>
>> thanks for the comments, more inline below.
>>
>>>> - btrfstume -M <uuid> isn't the place to check if the fsid is a
>>>>     duplicate. Because, libblkid will be unaware of the complete 
>>>> list of
>>>>     fs images with its fsid.
>>>
>>> I don't understand this part. Blkid tracks the device iformation, like
>>> the uuid, and the cache gets updated. So what does 'will be unaware of
>>> the complete list' mean?
>>>
>>> If it's on the same host it's a matter of keeping the cache in sync with
>>> the actual devices.
>>
>> 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. Because it can't avoid all the
>> cases of fsid getting duplicated.
>> Even after btrfstune -M, the fsid can be duplicated by the user.
>> So what's the point in restricting the btrfstune -M and fail to
>> undo the changed fsid.
>>
>>> 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.
>>
>> 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.
>>
>> Thanks, Anand
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-09-12  0:45           ` Anand Jain
  2019-09-24 11:20             ` Anand Jain
@ 2019-10-17 16:32             ` David Sterba
  2019-10-18  8:52               ` Anand Jain
  1 sibling, 1 reply; 12+ messages in thread
From: David Sterba @ 2019-10-17 16:32 UTC (permalink / raw)
  To: Anand Jain; +Cc: dsterba, Nikolay Borisov, linux-btrfs

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.

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

> 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, 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-10-17 16:32             ` David Sterba
@ 2019-10-18  8:52               ` Anand Jain
  2020-05-20 10:44                 ` Anand Jain
  0 siblings, 1 reply; 12+ messages in thread
From: Anand Jain @ 2019-10-18  8:52 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M
  2019-10-18  8:52               ` Anand Jain
@ 2020-05-20 10:44                 ` Anand Jain
  0 siblings, 0 replies; 12+ messages in thread
From: Anand Jain @ 2020-05-20 10:44 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov, linux-btrfs


David,

  We let the fsid to change but restrict if the user decides to undo.
  This bug is like a one-way trap.

  Any resolution on the issue below?

Thanks, Anand

 >> On 10/18/19 12:32 AM, David Sterba wrote:

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-05-20 10:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06  0:50 [PATCH] btrfs-progs: drop unique uuid test for btrfstune -M Anand Jain
2019-09-06  7:21 ` Nikolay Borisov
2019-09-06  9:27   ` Anand Jain
2019-09-09 11:40     ` Nikolay Borisov
2019-09-10  5:12       ` Anand Jain
2019-09-11 17:01         ` David Sterba
2019-09-12  0:45           ` Anand Jain
2019-09-24 11:20             ` Anand Jain
2019-10-01  8:08               ` Anand Jain
2019-10-17 16:32             ` David Sterba
2019-10-18  8:52               ` Anand Jain
2020-05-20 10:44                 ` Anand Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).