* [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch @ 2020-04-03 13:44 Nikolay Borisov 2020-04-10 19:23 ` Josef Bacik 2021-09-08 11:32 ` Filipe Manana 0 siblings, 2 replies; 4+ messages in thread From: Nikolay Borisov @ 2020-04-03 13:44 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch 2020-04-03 13:44 [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch Nikolay Borisov @ 2020-04-10 19:23 ` Josef Bacik 2021-09-08 11:32 ` Filipe Manana 1 sibling, 0 replies; 4+ messages in thread From: Josef Bacik @ 2020-04-10 19:23 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch 2020-04-03 13:44 [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch Nikolay Borisov 2020-04-10 19:23 ` Josef Bacik @ 2021-09-08 11:32 ` Filipe Manana 2021-09-08 12:01 ` Graham Cobb 1 sibling, 1 reply; 4+ messages in thread From: Filipe Manana @ 2021-09-08 11:32 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs 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.” ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch 2021-09-08 11:32 ` Filipe Manana @ 2021-09-08 12:01 ` Graham Cobb 0 siblings, 0 replies; 4+ messages in thread From: Graham Cobb @ 2021-09-08 12:01 UTC (permalink / raw) To: fdmanana, Nikolay Borisov; +Cc: linux-btrfs 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 >> > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-08 12:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-03 13:44 [RESEND PATCH] btrfs: Remove received information from snapshot on ro->rw switch Nikolay Borisov 2020-04-10 19:23 ` Josef Bacik 2021-09-08 11:32 ` Filipe Manana 2021-09-08 12:01 ` Graham Cobb
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.