[PATCH 0/2] btrfs-progs: add educational warning for flipping RO to RW on recevied subvolumes

[PATCH 0/2] btrfs-progs: add educational warning for flipping RO to RW on recevied subvolumes

[Qu Wenruo]

Since we're going to change how kernel handles received subvolume
RO->RW flipping, it is going to cause kernel behavior change for some
incremental send users.

Add extra educational warning to both "btrfs prop set" command and man
page of "btrfs prop".

Qu Wenruo (2):
  btrfs-progs: do extra warning when setting ro false on received
    subvolume
  btrfs-progs: doc: add extra note on flipping read-only on received
    subvolumes

 Documentation/btrfs-property.asciidoc |  6 ++++++
 props.c                               | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+)

-- 
2.33.0

[PATCH 1/2] btrfs-progs: do extra warning when setting ro false on received subvolume

[Qu Wenruo]

When flipping received subvolume from RO to RW, any new write to the
subvolume could cause later incremental receive to fail or corrupt data.

Thus we're trying to clear received UUID when doing such RO->RW flip, to
prevent data corruption.

But unfortunately the old (and wrong) behavior has been there for a
while, and changing the kernel behavior will make existing users
confused.

Thus here we enhance subvolume read-only prop to do extra warning on
users to educate them about both the new behavior and the problems of
old behaviors.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 props.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/props.c b/props.c
index 81509e48cd16..b86ecc61b5b3 100644
--- a/props.c
+++ b/props.c
@@ -39,6 +39,15 @@
 #define ENOATTR ENODATA
 #endif
 
+static void do_warn_about_rorw_flip(const char *path)
+{
+	warning("Flipping subvolume %s from RO to RW will cause either:", path);
+	warning("- No more incremental receive based on this subvolume");
+	warning("  Newer kernels will remove the recevied UUID to prevent corruption");
+	warning("- Data corruption or receive corruption doing incremental receive");
+	warning("  Older kernels can't detect the modification, and cause corruption or receive failure");
+}
+
 static int prop_read_only(enum prop_object_type type,
 			  const char *object,
 			  const char *name,
@@ -48,6 +57,9 @@ static int prop_read_only(enum prop_object_type type,
 	bool read_only;
 
 	if (value) {
+		struct btrfs_util_subvolume_info subvol;
+		u8 empty_uuid[BTRFS_UUID_SIZE] = { 0 };
+
 		if (!strcmp(value, "true")) {
 			read_only = true;
 		} else if (!strcmp(value, "false")) {
@@ -57,6 +69,15 @@ static int prop_read_only(enum prop_object_type type,
 			return -EINVAL;
 		}
 
+		err = btrfs_util_subvolume_info(object, 0, &subvol);
+		if (err) {
+			warning("unable to get subvolume info for path %s",
+				object);
+		} else if (!read_only && memcmp(empty_uuid, subvol.received_uuid,
+				   BTRFS_UUID_SIZE)){
+			do_warn_about_rorw_flip(object);
+		}
+
 		err = btrfs_util_set_subvolume_read_only(object, read_only);
 		if (err) {
 			error_btrfs_util(err);
-- 
2.33.0

Re: [PATCH 1/2] btrfs-progs: do extra warning when setting ro false on received subvolume

[Nikolay Borisov]

On 10.09.21 г. 9:03, Qu Wenruo wrote:
> When flipping received subvolume from RO to RW, any new write to the
> subvolume could cause later incremental receive to fail or corrupt data.
> 
> Thus we're trying to clear received UUID when doing such RO->RW flip, to
> prevent data corruption.
> 
> But unfortunately the old (and wrong) behavior has been there for a
> while, and changing the kernel behavior will make existing users
> confused.
> 
> Thus here we enhance subvolume read-only prop to do extra warning on
> users to educate them about both the new behavior and the problems of
> old behaviors.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  props.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/props.c b/props.c
> index 81509e48cd16..b86ecc61b5b3 100644
> --- a/props.c
> +++ b/props.c
> @@ -39,6 +39,15 @@
>  #define ENOATTR ENODATA
>  #endif
>  
> +static void do_warn_about_rorw_flip(const char *path)
> +{
> +	warning("Flipping subvolume %s from RO to RW will cause either:", path);
> +	warning("- No more incremental receive based on this subvolume");
> +	warning("  Newer kernels will remove the recevied UUID to prevent corruption");
> +	warning("- Data corruption or receive corruption doing incremental receive");
> +	warning("  Older kernels can't detect the modification, and cause corruption or receive failure");

This is a bad format for a warning, let's keep it simple - just state
that flipping ro->rw would break incremental send and that's that.

> +}
> +
>  static int prop_read_only(enum prop_object_type type,
>  			  const char *object,
>  			  const char *name,
> @@ -48,6 +57,9 @@ static int prop_read_only(enum prop_object_type type,
>  	bool read_only;
>  
>  	if (value) {
> +		struct btrfs_util_subvolume_info subvol;
> +		u8 empty_uuid[BTRFS_UUID_SIZE] = { 0 };
> +
>  		if (!strcmp(value, "true")) {
>  			read_only = true;
>  		} else if (!strcmp(value, "false")) {
> @@ -57,6 +69,15 @@ static int prop_read_only(enum prop_object_type type,
>  			return -EINVAL;
>  		}
>  
> +		err = btrfs_util_subvolume_info(object, 0, &subvol);
> +		if (err) {
> +			warning("unable to get subvolume info for path %s",
> +				object);
> +		} else if (!read_only && memcmp(empty_uuid, subvol.received_uuid,
> +				   BTRFS_UUID_SIZE)){
> +			do_warn_about_rorw_flip(object);
> +		}
> +
>  		err = btrfs_util_set_subvolume_read_only(object, read_only);
>  		if (err) {
>  			error_btrfs_util(err);
> 

Re: [PATCH 1/2] btrfs-progs: do extra warning when setting ro false on received subvolume

[Qu Wenruo]

On 2021/9/10 下午2:36, Nikolay Borisov wrote:
> 
> 
> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>> When flipping received subvolume from RO to RW, any new write to the
>> subvolume could cause later incremental receive to fail or corrupt data.
>>
>> Thus we're trying to clear received UUID when doing such RO->RW flip, to
>> prevent data corruption.
>>
>> But unfortunately the old (and wrong) behavior has been there for a
>> while, and changing the kernel behavior will make existing users
>> confused.
>>
>> Thus here we enhance subvolume read-only prop to do extra warning on
>> users to educate them about both the new behavior and the problems of
>> old behaviors.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   props.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/props.c b/props.c
>> index 81509e48cd16..b86ecc61b5b3 100644
>> --- a/props.c
>> +++ b/props.c
>> @@ -39,6 +39,15 @@
>>   #define ENOATTR ENODATA
>>   #endif
>>   
>> +static void do_warn_about_rorw_flip(const char *path)
>> +{
>> +	warning("Flipping subvolume %s from RO to RW will cause either:", path);
>> +	warning("- No more incremental receive based on this subvolume");
>> +	warning("  Newer kernels will remove the recevied UUID to prevent corruption");
>> +	warning("- Data corruption or receive corruption doing incremental receive");
>> +	warning("  Older kernels can't detect the modification, and cause corruption or receive failure");
> 
> This is a bad format for a warning, let's keep it simple - just state
> that flipping ro->rw would break incremental send and that's that.

But won't this on older kernels cause more confusion?

The old kernels will still allow incremental send using that flipped 
subvolume without problem, just corrupting the data..

(Well, that's definitely "broken", but still a different behavior)

Thanks,
Qu
> 
>> +}
>> +
>>   static int prop_read_only(enum prop_object_type type,
>>   			  const char *object,
>>   			  const char *name,
>> @@ -48,6 +57,9 @@ static int prop_read_only(enum prop_object_type type,
>>   	bool read_only;
>>   
>>   	if (value) {
>> +		struct btrfs_util_subvolume_info subvol;
>> +		u8 empty_uuid[BTRFS_UUID_SIZE] = { 0 };
>> +
>>   		if (!strcmp(value, "true")) {
>>   			read_only = true;
>>   		} else if (!strcmp(value, "false")) {
>> @@ -57,6 +69,15 @@ static int prop_read_only(enum prop_object_type type,
>>   			return -EINVAL;
>>   		}
>>   
>> +		err = btrfs_util_subvolume_info(object, 0, &subvol);
>> +		if (err) {
>> +			warning("unable to get subvolume info for path %s",
>> +				object);
>> +		} else if (!read_only && memcmp(empty_uuid, subvol.received_uuid,
>> +				   BTRFS_UUID_SIZE)){
>> +			do_warn_about_rorw_flip(object);
>> +		}
>> +
>>   		err = btrfs_util_set_subvolume_read_only(object, read_only);
>>   		if (err) {
>>   			error_btrfs_util(err);
>>
> 

Re: [PATCH 1/2] btrfs-progs: do extra warning when setting ro false on received subvolume

[Graham Cobb]

On 10/09/2021 08:28, Qu Wenruo wrote:
> 
> 
> On 2021/9/10 下午2:36, Nikolay Borisov wrote:
>>
>>
>> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>>> When flipping received subvolume from RO to RW, any new write to the
>>> subvolume could cause later incremental receive to fail or corrupt data.
>>>
>>> Thus we're trying to clear received UUID when doing such RO->RW flip, to
>>> prevent data corruption.
>>>
>>> But unfortunately the old (and wrong) behavior has been there for a
>>> while, and changing the kernel behavior will make existing users
>>> confused.
>>>
>>> Thus here we enhance subvolume read-only prop to do extra warning on
>>> users to educate them about both the new behavior and the problems of
>>> old behaviors.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   props.c | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/props.c b/props.c
>>> index 81509e48cd16..b86ecc61b5b3 100644
>>> --- a/props.c
>>> +++ b/props.c
>>> @@ -39,6 +39,15 @@
>>>   #define ENOATTR ENODATA
>>>   #endif
>>>   +static void do_warn_about_rorw_flip(const char *path)
>>> +{
>>> +    warning("Flipping subvolume %s from RO to RW will cause
>>> either:", path);
>>> +    warning("- No more incremental receive based on this subvolume");
>>> +    warning("  Newer kernels will remove the recevied UUID to
>>> prevent corruption");
>>> +    warning("- Data corruption or receive corruption doing
>>> incremental receive");
>>> +    warning("  Older kernels can't detect the modification, and
>>> cause corruption or receive failure");
>>
>> This is a bad format for a warning, let's keep it simple - just state
>> that flipping ro->rw would break incremental send and that's that.
> 
> But won't this on older kernels cause more confusion?
> 
> The old kernels will still allow incremental send using that flipped
> subvolume without problem, just corrupting the data..
> 
> (Well, that's definitely "broken", but still a different behavior)

Exactly. I agree with Nikolay - keep the warning simple. Let the man
page explain in more detail.

[PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-property.asciidoc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/btrfs-property.asciidoc b/Documentation/btrfs-property.asciidoc
index 4796083378e4..8949ea22edae 100644
--- a/Documentation/btrfs-property.asciidoc
+++ b/Documentation/btrfs-property.asciidoc
@@ -42,6 +42,12 @@ the following:
 
 ro::::
 read-only flag of subvolume: true or false
++
+NOTE: For recevied subvolumes, flipping from read-only to read-write will
+either remove the recevied UUID and prevent future incremental receive
+(on newer kernels), or cause future data corruption and recevie failure
+(on older kernels).
+
 label::::
 label of the filesystem. For an unmounted filesystem, provide a path to a block
 device as object. For a mounted filesystem, specify a mount point.
-- 
2.33.0

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Nikolay Borisov]

On 10.09.21 г. 9:03, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  Documentation/btrfs-property.asciidoc | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/btrfs-property.asciidoc b/Documentation/btrfs-property.asciidoc
> index 4796083378e4..8949ea22edae 100644
> --- a/Documentation/btrfs-property.asciidoc
> +++ b/Documentation/btrfs-property.asciidoc
> @@ -42,6 +42,12 @@ the following:
>  
>  ro::::
>  read-only flag of subvolume: true or false
> ++
> +NOTE: For recevied subvolumes, flipping from read-only to read-write will
> +either remove the recevied UUID and prevent future incremental receive
> +(on newer kernels), or cause future data corruption and recevie failure
> +(on older kernels).

Hang on a minute, flipping RO->RW won't cause corruption by itself. So
flipping will just break incremental sends which is completely fine.

> +
>  label::::
>  label of the filesystem. For an unmounted filesystem, provide a path to a block
>  device as object. For a mounted filesystem, specify a mount point.
> 

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Qu Wenruo]

On 2021/9/10 下午2:33, Nikolay Borisov wrote:
> 
> 
> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   Documentation/btrfs-property.asciidoc | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/btrfs-property.asciidoc b/Documentation/btrfs-property.asciidoc
>> index 4796083378e4..8949ea22edae 100644
>> --- a/Documentation/btrfs-property.asciidoc
>> +++ b/Documentation/btrfs-property.asciidoc
>> @@ -42,6 +42,12 @@ the following:
>>   
>>   ro::::
>>   read-only flag of subvolume: true or false
>> ++
>> +NOTE: For recevied subvolumes, flipping from read-only to read-write will
>> +either remove the recevied UUID and prevent future incremental receive
>> +(on newer kernels), or cause future data corruption and recevie failure
>> +(on older kernels).
> 
> Hang on a minute, flipping RO->RW won't cause corruption by itself.

It looks like the "future" part is not clear enough.

> So
> flipping will just break incremental sends which is completely fine.

The "breaking" part is more straightforward for newer kernel, but not 
older kernels (it's definitely breaking, but not that directly observable)

Thanks,
Qu

> 
>> +
>>   label::::
>>   label of the filesystem. For an unmounted filesystem, provide a path to a block
>>   device as object. For a mounted filesystem, specify a mount point.
>>
> 

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Graham Cobb]

On 10/09/2021 08:30, Qu Wenruo wrote:
> 
> 
> On 2021/9/10 下午2:33, Nikolay Borisov wrote:
>>
>>
>> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   Documentation/btrfs-property.asciidoc | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/btrfs-property.asciidoc
>>> b/Documentation/btrfs-property.asciidoc
>>> index 4796083378e4..8949ea22edae 100644
>>> --- a/Documentation/btrfs-property.asciidoc
>>> +++ b/Documentation/btrfs-property.asciidoc
>>> @@ -42,6 +42,12 @@ the following:
>>>     ro::::
>>>   read-only flag of subvolume: true or false
>>> ++
>>> +NOTE: For recevied subvolumes, flipping from read-only to read-write
>>> will
>>> +either remove the recevied UUID and prevent future incremental receive
>>> +(on newer kernels), or cause future data corruption and recevie failure
>>> +(on older kernels).
>>
>> Hang on a minute, flipping RO->RW won't cause corruption by itself.
> 
> It looks like the "future" part is not clear enough.
> 
>> So
>> flipping will just break incremental sends which is completely fine.
> 
> The "breaking" part is more straightforward for newer kernel, but not
> older kernels (it's definitely breaking, but not that directly observable)

How about...

NOTE: Changing a subvolume which has been received using btrfs receive
to be writable could silently corrupt future btrfs receive operations
using the subvolume. Recent kernels automatically remove the 'received
UUID' on the subvolume when the read-only flag is set to false in order
to protect against this corruption. This will mean that subsequent btrfs
receive commands which refer to this subvolume will fail. With earlier
kernels, similar btrfs receive commands could cause silent corruption.

> 
> Thanks,
> Qu
> 
>>
>>> +
>>>   label::::
>>>   label of the filesystem. For an unmounted filesystem, provide a
>>> path to a block
>>>   device as object. For a mounted filesystem, specify a mount point.
>>>
>>
> 

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[David Sterba]

On Fri, Sep 10, 2021 at 09:33:41AM +0300, Nikolay Borisov wrote:
> 
> 
> On 10.09.21 г. 9:03, Qu Wenruo wrote:
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >  Documentation/btrfs-property.asciidoc | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/btrfs-property.asciidoc b/Documentation/btrfs-property.asciidoc
> > index 4796083378e4..8949ea22edae 100644
> > --- a/Documentation/btrfs-property.asciidoc
> > +++ b/Documentation/btrfs-property.asciidoc
> > @@ -42,6 +42,12 @@ the following:
> >  
> >  ro::::
> >  read-only flag of subvolume: true or false
> > ++
> > +NOTE: For recevied subvolumes, flipping from read-only to read-write will
> > +either remove the recevied UUID and prevent future incremental receive
> > +(on newer kernels), or cause future data corruption and recevie failure
> > +(on older kernels).
> 
> Hang on a minute, flipping RO->RW won't cause corruption by itself. So
> flipping will just break incremental sends which is completely fine.

I'm still not decided if it's 'completely fine' to break incremental
send so easily.

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Qu Wenruo]

On 2021/9/10 下午5:45, David Sterba wrote:
> On Fri, Sep 10, 2021 at 09:33:41AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   Documentation/btrfs-property.asciidoc | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/btrfs-property.asciidoc b/Documentation/btrfs-property.asciidoc
>>> index 4796083378e4..8949ea22edae 100644
>>> --- a/Documentation/btrfs-property.asciidoc
>>> +++ b/Documentation/btrfs-property.asciidoc
>>> @@ -42,6 +42,12 @@ the following:
>>>
>>>   ro::::
>>>   read-only flag of subvolume: true or false
>>> ++
>>> +NOTE: For recevied subvolumes, flipping from read-only to read-write will
>>> +either remove the recevied UUID and prevent future incremental receive
>>> +(on newer kernels), or cause future data corruption and recevie failure
>>> +(on older kernels).
>>
>> Hang on a minute, flipping RO->RW won't cause corruption by itself. So
>> flipping will just break incremental sends which is completely fine.
>
> I'm still not decided if it's 'completely fine' to break incremental
> send so easily.
>

Then even we just keep the existing behavior, we still need some
educational warning here.

In that keep-recevied-uuid case, here we just need to warn the users
about that, later incremental receive may fail and the recevied data may
not be correct, and call it a day (without any kernel modification).

In that case, it's all users' fault and except the corrupted data and
receive failure, everything else should be fine.

Kernel won't crash, users get their "expected" corrupted data, and we
won't need to bother the kernel behavior change.

But I don't think that would be any better than a sudden behavior change...

Thanks,
Qu

Re: [PATCH 2/2] btrfs-progs: doc: add extra note on flipping read-only on received subvolumes

[Nikolay Borisov]

On 10.09.21 г. 12:59, Qu Wenruo wrote:
> 
> 
> On 2021/9/10 下午5:45, David Sterba wrote:
>> On Fri, Sep 10, 2021 at 09:33:41AM +0300, Nikolay Borisov wrote:
>>>
>>>
>>> On 10.09.21 г. 9:03, Qu Wenruo wrote:
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>   Documentation/btrfs-property.asciidoc | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/btrfs-property.asciidoc
>>>> b/Documentation/btrfs-property.asciidoc
>>>> index 4796083378e4..8949ea22edae 100644
>>>> --- a/Documentation/btrfs-property.asciidoc
>>>> +++ b/Documentation/btrfs-property.asciidoc
>>>> @@ -42,6 +42,12 @@ the following:
>>>>
>>>>   ro::::
>>>>   read-only flag of subvolume: true or false
>>>> ++
>>>> +NOTE: For recevied subvolumes, flipping from read-only to
>>>> read-write will
>>>> +either remove the recevied UUID and prevent future incremental receive
>>>> +(on newer kernels), or cause future data corruption and recevie
>>>> failure
>>>> +(on older kernels).
>>>
>>> Hang on a minute, flipping RO->RW won't cause corruption by itself. So
>>> flipping will just break incremental sends which is completely fine.
>>
>> I'm still not decided if it's 'completely fine' to break incremental
>> send so easily.
>>
> 
> Then even we just keep the existing behavior, we still need some
> educational warning here.
> 
> In that keep-recevied-uuid case, here we just need to warn the users
> about that, later incremental receive may fail and the recevied data may
> not be correct, and call it a day (without any kernel modification).
> 
> In that case, it's all users' fault and except the corrupted data and
> receive failure, everything else should be fine.
> 
> Kernel won't crash, users get their "expected" corrupted data, and we
> won't need to bother the kernel behavior change.
> 
> But I don't think that would be any better than a sudden behavior change...

I wholeheartedly disagree. If we do a behavior change then users must
just adjust which really means they have to make 1 full send. If we keep
the current behavior we are bound to be getting reports of corrupted
data/failed receive of subvolumes. And in the end this would end up
being a huge time sink for the person who ends up investigating this...

> 
> Thanks,
> Qu
>