[RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch

[RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch

[Nikolay Borisov]

Currently when a read-only snapshot is received and subsequently its
ro property is set to false i.e. switched to rw-mode the
received_uuid/stime/rtime/stransid/rtransid of that subvol remains
intact. However, once the received volume is switched to RW mode we
cannot guaranteee that it contains the same data, so it makes sense
to remove those fields which indicate this volume was ever
send/received. Additionally, sending such volume can cause conflicts
due to the presence of received_uuid.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Suggested-by: David Sterba <dsterba@suse.cz>
---

I've been carrying this patch in my tree for around 2.5 years. It stems from
multiple reports on the mailing list about people changing the RO->RW mode on
a received snapshot and getting unexpected behavior. AFAIR this patch resolved
that.


 fs/btrfs/ioctl.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 40b729dce91c..39840b654600 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1873,6 +1873,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 	struct btrfs_trans_handle *trans;
 	u64 root_flags;
 	u64 flags;
+	bool clear_received_state = false;
 	int ret = 0;

 	if (!inode_owner_or_capable(inode))
@@ -1917,6 +1918,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 			btrfs_set_root_flags(&root->root_item,
 				     root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
 			spin_unlock(&root->root_item_lock);
+			clear_received_state = true;
 		} else {
 			spin_unlock(&root->root_item_lock);
 			btrfs_warn(fs_info,
@@ -1933,6 +1935,31 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 		goto out_reset;
 	}

+	if (clear_received_state) {
+	        if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {
+			struct btrfs_root_item *root_item = &root->root_item;
+
+	                ret = btrfs_uuid_tree_remove(trans,
+	                                root->root_item.received_uuid,
+	                                BTRFS_UUID_KEY_RECEIVED_SUBVOL,
+	                                root->root_key.objectid);
+
+	                if (ret && ret != -ENOENT) {
+	                        btrfs_abort_transaction(trans, ret);
+	                        btrfs_end_transaction(trans);
+	                        goto out_reset;
+	                }
+
+	                memset(root_item->received_uuid, 0, BTRFS_UUID_SIZE);
+			btrfs_set_root_stransid(root_item, 0);
+			btrfs_set_root_rtransid(root_item, 0);
+			btrfs_set_stack_timespec_sec(&root_item->stime, 0);
+			btrfs_set_stack_timespec_nsec(&root_item->stime, 0);
+			btrfs_set_stack_timespec_sec(&root_item->rtime, 0);
+			btrfs_set_stack_timespec_nsec(&root_item->rtime, 0);
+	        }
+	}
+
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
 	if (ret < 0) {
--
2.17.1

Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch

[Josef Bacik]

On 4/3/20 9:44 AM, Nikolay Borisov wrote:
> Currently when a read-only snapshot is received and subsequently its
> ro property is set to false i.e. switched to rw-mode the
> received_uuid/stime/rtime/stransid/rtransid of that subvol remains
> intact. However, once the received volume is switched to RW mode we
> cannot guaranteee that it contains the same data, so it makes sense
> to remove those fields which indicate this volume was ever
> send/received. Additionally, sending such volume can cause conflicts
> due to the presence of received_uuid.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Suggested-by: David Sterba <dsterba@suse.cz>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch

[Filipe Manana]

On Fri, Apr 3, 2020 at 2:45 PM Nikolay Borisov <nborisov@suse.com> wrote:
>
> Currently when a read-only snapshot is received and subsequently its
> ro property is set to false i.e. switched to rw-mode the
> received_uuid/stime/rtime/stransid/rtransid of that subvol remains
> intact. However, once the received volume is switched to RW mode we
> cannot guaranteee that it contains the same data, so it makes sense
> to remove those fields which indicate this volume was ever
> send/received. Additionally, sending such volume can cause conflicts
> due to the presence of received_uuid.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Suggested-by: David Sterba <dsterba@suse.cz>
> ---
>
> I've been carrying this patch in my tree for around 2.5 years. It stems from
> multiple reports on the mailing list about people changing the RO->RW mode on
> a received snapshot and getting unexpected behavior. AFAIR this patch resolved
> that.

I agree with it, and this sporadically causes problems that get
reported on the list.
I spent a bunch of time back and forth with a user that ran into a
problem caused by switching snapshots from RO to RW.

So I would like to add the following paragraphs and Link tag to the changelog:

"
Preserving the received_uuid (and related fields) after transitioning the
snapshot from RO to RW and then changing the snapshot has a potential for
causing send to fail in many unexpected ways if we later turn it back to
RO and use it for an incremental send operation.

A recent example, in the thread to which the Link tag below points to, we
had a user with a filesystem that had multiple snapshots with the same
received_uuid but with different content due to a transition from RO to RW
and then back to RO. When an incremental send was attempted using two of
those snapshots, it resulted in send emitting a clone operation that was
intended to clone from the parent root to the send root - however because
both roots had the same received_uuid, the receiver ended up picking the
send root as the source root for the clone operation, which happened to
result in the clone operation to fail with -EINVAL, due to the source and
destination offsets being the same (and on the same root and file). In this
particular case there was no harm, but we could end up in a case where the
clone operation succeeds but clones wrong data due to picking up an
incorrect root - as in the case of multiple snapshots with the same
received_uuid, it has no way to know which one is the correct one if they
have different content.

Link: https://lore.kernel.org/linux-btrfs/CAOaVUnV3L6RpcqJ5gaqzNXWXK0VMkEVXCdihawH1PgS6TiMchQ@mail.gmail.com/
"


A few comments below as well.

>
>
>  fs/btrfs/ioctl.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 40b729dce91c..39840b654600 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1873,6 +1873,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>         struct btrfs_trans_handle *trans;
>         u64 root_flags;
>         u64 flags;
> +       bool clear_received_state = false;
>         int ret = 0;
>
>         if (!inode_owner_or_capable(inode))
> @@ -1917,6 +1918,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>                         btrfs_set_root_flags(&root->root_item,
>                                      root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
>                         spin_unlock(&root->root_item_lock);
> +                       clear_received_state = true;
>                 } else {
>                         spin_unlock(&root->root_item_lock);
>                         btrfs_warn(fs_info,
> @@ -1933,6 +1935,31 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>                 goto out_reset;
>         }
>
> +       if (clear_received_state) {
> +               if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {

This could be combined into a single if to reduce indentation:   if
(clear_received_state && !btrfs_is_empty_uuid(...))

Also, there's here indentation weirdness due to using spaces instead
of tabs, and checkpatch complains about it:

ERROR: code indent should use tabs where possible
#80: FILE: fs/btrfs/ioctl.c:1996:
+^I        if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {$

ERROR: code indent should use tabs where possible
#83: FILE: fs/btrfs/ioctl.c:1999:
+^I                ret = btrfs_uuid_tree_remove(trans,$

ERROR: code indent should use tabs where possible
#84: FILE: fs/btrfs/ioctl.c:2000:
+^I                                root->root_item.received_uuid,$
(...)

> +                       struct btrfs_root_item *root_item = &root->root_item;
> +
> +                       ret = btrfs_uuid_tree_remove(trans,
> +                                       root->root_item.received_uuid,
> +                                       BTRFS_UUID_KEY_RECEIVED_SUBVOL,
> +                                       root->root_key.objectid);

If we reach here, we should have reserved one additional metadata item
when starting the transaction.
And add a comment about what each item is used for - 1 for updating
the root item in the root tree, another 1 for removing an item from
the uuid tree.

Also, we can use root_item there instead of root->root_item.

Other than that, it looks fine to me.

Thanks.

> +
> +                       if (ret && ret != -ENOENT) {
> +                               btrfs_abort_transaction(trans, ret);
> +                               btrfs_end_transaction(trans);
> +                               goto out_reset;
> +                       }
> +
> +                       memset(root_item->received_uuid, 0, BTRFS_UUID_SIZE);
> +                       btrfs_set_root_stransid(root_item, 0);
> +                       btrfs_set_root_rtransid(root_item, 0);
> +                       btrfs_set_stack_timespec_sec(&root_item->stime, 0);
> +                       btrfs_set_stack_timespec_nsec(&root_item->stime, 0);
> +                       btrfs_set_stack_timespec_sec(&root_item->rtime, 0);
> +                       btrfs_set_stack_timespec_nsec(&root_item->rtime, 0);
> +               }
> +       }
> +
>         ret = btrfs_update_root(trans, fs_info->tree_root,
>                                 &root->root_key, &root->root_item);
>         if (ret < 0) {
> --
> 2.17.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch

[Graham Cobb]

On 08/09/2021 12:32, Filipe Manana wrote:
> On Fri, Apr 3, 2020 at 2:45 PM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> Currently when a read-only snapshot is received and subsequently its
>> ro property is set to false i.e. switched to rw-mode the
>> received_uuid/stime/rtime/stransid/rtransid of that subvol remains
>> intact. However, once the received volume is switched to RW mode we
>> cannot guaranteee that it contains the same data, so it makes sense
>> to remove those fields which indicate this volume was ever
>> send/received. Additionally, sending such volume can cause conflicts
>> due to the presence of received_uuid.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Suggested-by: David Sterba <dsterba@suse.cz>
>> ---
>>
>> I've been carrying this patch in my tree for around 2.5 years. It stems from
>> multiple reports on the mailing list about people changing the RO->RW mode on
>> a received snapshot and getting unexpected behavior. AFAIR this patch resolved
>> that.
> 
> I agree with it, and this sporadically causes problems that get
> reported on the list.
> I spent a bunch of time back and forth with a user that ran into a
> problem caused by switching snapshots from RO to RW.
> 
> So I would like to add the following paragraphs and Link tag to the changelog:
> 
> "
> Preserving the received_uuid (and related fields) after transitioning the
> snapshot from RO to RW and then changing the snapshot has a potential for
> causing send to fail in many unexpected ways if we later turn it back to
> RO and use it for an incremental send operation.
> 
> A recent example, in the thread to which the Link tag below points to, we
> had a user with a filesystem that had multiple snapshots with the same
> received_uuid but with different content due to a transition from RO to RW
> and then back to RO. 

Should btrfs-check be enhanced to detect subvolumes with duplicated
received_uuid? Presumably the repair would be to change all of them to
new UUIDs (or maybe keep the oldest one unchanged?).

Graham

When an incremental send was attempted using two of
> those snapshots, it resulted in send emitting a clone operation that was
> intended to clone from the parent root to the send root - however because
> both roots had the same received_uuid, the receiver ended up picking the
> send root as the source root for the clone operation, which happened to
> result in the clone operation to fail with -EINVAL, due to the source and
> destination offsets being the same (and on the same root and file). In this
> particular case there was no harm, but we could end up in a case where the
> clone operation succeeds but clones wrong data due to picking up an
> incorrect root - as in the case of multiple snapshots with the same
> received_uuid, it has no way to know which one is the correct one if they
> have different content.
> 
> Link: https://lore.kernel.org/linux-btrfs/CAOaVUnV3L6RpcqJ5gaqzNXWXK0VMkEVXCdihawH1PgS6TiMchQ@mail.gmail.com/
> "
> 
> 
> A few comments below as well.
> 
>>
>>
>>  fs/btrfs/ioctl.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 40b729dce91c..39840b654600 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1873,6 +1873,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>         struct btrfs_trans_handle *trans;
>>         u64 root_flags;
>>         u64 flags;
>> +       bool clear_received_state = false;
>>         int ret = 0;
>>
>>         if (!inode_owner_or_capable(inode))
>> @@ -1917,6 +1918,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>                         btrfs_set_root_flags(&root->root_item,
>>                                      root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
>>                         spin_unlock(&root->root_item_lock);
>> +                       clear_received_state = true;
>>                 } else {
>>                         spin_unlock(&root->root_item_lock);
>>                         btrfs_warn(fs_info,
>> @@ -1933,6 +1935,31 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>                 goto out_reset;
>>         }
>>
>> +       if (clear_received_state) {
>> +               if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {
> 
> This could be combined into a single if to reduce indentation:   if
> (clear_received_state && !btrfs_is_empty_uuid(...))
> 
> Also, there's here indentation weirdness due to using spaces instead
> of tabs, and checkpatch complains about it:
> 
> ERROR: code indent should use tabs where possible
> #80: FILE: fs/btrfs/ioctl.c:1996:
> +^I        if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {$
> 
> ERROR: code indent should use tabs where possible
> #83: FILE: fs/btrfs/ioctl.c:1999:
> +^I                ret = btrfs_uuid_tree_remove(trans,$
> 
> ERROR: code indent should use tabs where possible
> #84: FILE: fs/btrfs/ioctl.c:2000:
> +^I                                root->root_item.received_uuid,$
> (...)
> 
>> +                       struct btrfs_root_item *root_item = &root->root_item;
>> +
>> +                       ret = btrfs_uuid_tree_remove(trans,
>> +                                       root->root_item.received_uuid,
>> +                                       BTRFS_UUID_KEY_RECEIVED_SUBVOL,
>> +                                       root->root_key.objectid);
> 
> If we reach here, we should have reserved one additional metadata item
> when starting the transaction.
> And add a comment about what each item is used for - 1 for updating
> the root item in the root tree, another 1 for removing an item from
> the uuid tree.
> 
> Also, we can use root_item there instead of root->root_item.
> 
> Other than that, it looks fine to me.
> 
> Thanks.
> 
>> +
>> +                       if (ret && ret != -ENOENT) {
>> +                               btrfs_abort_transaction(trans, ret);
>> +                               btrfs_end_transaction(trans);
>> +                               goto out_reset;
>> +                       }
>> +
>> +                       memset(root_item->received_uuid, 0, BTRFS_UUID_SIZE);
>> +                       btrfs_set_root_stransid(root_item, 0);
>> +                       btrfs_set_root_rtransid(root_item, 0);
>> +                       btrfs_set_stack_timespec_sec(&root_item->stime, 0);
>> +                       btrfs_set_stack_timespec_nsec(&root_item->stime, 0);
>> +                       btrfs_set_stack_timespec_sec(&root_item->rtime, 0);
>> +                       btrfs_set_stack_timespec_nsec(&root_item->rtime, 0);
>> +               }
>> +       }
>> +
>>         ret = btrfs_update_root(trans, fs_info->tree_root,
>>                                 &root->root_key, &root->root_item);
>>         if (ret < 0) {
>> --
>> 2.17.1
>>
> 
>