Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Hans van Kranenburg]

On 10/05/2017 10:22 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 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 the
> received uuid. The presence of the received_uuid can also cause problems when
> the volume is being send.
> 
> Signed-off-by: Nikolay Borisov 
> Suggested-by: David Sterba 
> ---
> 
> v4: 
>  * Put the uuid tree removal code after the lightweight 'send in progress' 
>  check and don't move btrfs_start_transaction as suggested by David
>  
> v3:
>  * Rework the patch considering latest feedback from David Sterba i.e. 
>   explicitly use btrfs_end_transaction 
> 
>  fs/btrfs/ioctl.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ee4ee7cbba72..9328c091854b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1775,6 +1775,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
> file *file,
>   struct btrfs_trans_handle *trans;
>   u64 root_flags;
>   u64 flags;
> + bool clear_received_uuid = false;
>   int ret = 0;
>  
>   if (!inode_owner_or_capable(inode))
> @@ -1824,6 +1825,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
> file *file,
>   btrfs_set_root_flags(>root_item,
>root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
>   spin_unlock(>root_item_lock);
> + clear_received_uuid = true;
>   } else {
>   spin_unlock(>root_item_lock);
>   btrfs_warn(fs_info,
> @@ -1840,6 +1842,24 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
> file *file,
>   goto out_reset;
>   }
>  
> + if (clear_received_uuid) {
> + if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {
> + ret = btrfs_uuid_tree_rem(trans, fs_info,
> + 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->root_item.received_uuid, 0,
> + BTRFS_UUID_SIZE);

Shouldn't we also wipe the other related fields here, like stime, rtime,
stransid, rtransid?

> + }
> + }
> +
>   ret = btrfs_update_root(trans, fs_info->tree_root,
>   >root_key, >root_item);
>   if (ret < 0) {
> 


-- 
Hans van Kranenburg
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Andrei Borzenkov]

07.10.2017 00:27, Hans van Kranenburg пишет:
> On 10/06/2017 10:07 PM, Andrei Borzenkov wrote:
>>
>> What is reason behind allowing change from ro to rw in the first place?
>> What is the use case?
> 
> I think this is a case of "well, nobody actually has been thinking of
> the use cases ever, we just did something yolo"
> 
> Btrfs does not make a difference between snapshots and clones. Other
> systems like netapp and zfs do. Btrfs cloud also do that, and just not
> expose the ro/rw flag to the outside.
> 

Current pure user-level implementation of btrfs receive requires
possibility to switch from rw to ro. So it is not possible to completely
hide it. This is different from both NetApp and ZFS. On NetApp
destination volume/qtree are always read-only for client access. ZFS
explicitly disallows any access to destination until transfer is complete.

It was already mentioned that in btrfs destination may be changed before
subvolume is changed to ro without anyone noticing it. Ideally btrfs
receive needs exclusive access to subvolume with some sort of automatic
cleanup if receive fails for any reason. This will ensure atomic (from
end user PoV) transfer.

> Personally, I would like btrfs to go into that direction, because it
> just makes things more clear. This is a snapshot, you cannot touch it.
> If you want to make changes, you have to make a rw clone of the snapshot.
>
> The nice thing for btrfs is that you can remove the snapshot after you
> made the rw clone, which you cannot do on a NetApp filer. :o)
> 
>>> Even if it wouldn't make sense for some reason, it's a nice thought
>>> experiment. :)
> 
> There we go :)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Hans van Kranenburg]

On 10/06/2017 10:07 PM, Andrei Borzenkov wrote:
> 06.10.2017 20:49, Hans van Kranenburg пишет:
>> On 10/06/2017 07:24 PM, David Sterba wrote:
>>> On Thu, Oct 05, 2017 at 05:03:47PM +0800, Anand Jain wrote:
 On 10/05/2017 04:22 PM, 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 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 the
> received uuid. The presence of the received_uuid can also cause problems 
> when
> the volume is being send.
>>
>> Are the 'can cause problems when being send' explained somewhere?
>>
> 
> If received_uuid is present, btrfs send will use it instead of subvolume
> uuid. It means btrfs receive may find wrong volume as differential
> stream base. Example that was demonstrated earlier
> 
> 1. A -> B on remote system S. B now has received_uui == A
> 2. A -> C on local system. C now has received_uuid == A
> 3. C is made read-write and changed.
> 4. Create snapshot D from C and do "btrfs send -p C D" to system S. Now
> btrfs receive on S will get base uuid of A and will find B. So any
> changes between B and C are silently lost.

Ah ok, yes, so the 'also' in that sentence just needs to go away. When
there are any modifications, it CAN NOT keep the received_uuid or bad
things will happen. and that's why this whole thread was started.


 Wonder if this [1] approach was considered
 [1]
   - set a flag on the subvolume to indicate its dirtied so that 
 received_uuid can be kept forever just in case if user needs it for some 
 reference at a later point of time.
>>>
>>> Yeah, we need to be careful here. There are more items related to the
>>> recived subvolume, besides received_uuid there's rtransid and rtime so
>>> they might need to be cleared as well.
>>>
>>> I don't remember all the details how the send/receive and uuids
>>> interact. Switching from ro->rw needs to affect the 'received' status,
>>> but I don't know how. The problem is that some information is being lost
>>> although it may be quite important to the user/administrator. In such
>>> cases it would be convenient to request a confirmation via a --force
>>> flag or something like that.
>>
>> On IRC I think we generally recommends users to never do this, and as a
>> best practice always clone the snapshot to a rw subvolume in a different
>> location if someone wants to proceed working with the contents and
>> changing them as opposed to messing with the ro/rw attributes.
>>
>> So, what about option [2]:
>>
>> [2] if a subvolume has a received_uuid, then just do not allow changing
>> it to rw.
>>
> 
> What is reason behind allowing change from ro to rw in the first place?
> What is the use case?

I think this is a case of "well, nobody actually has been thinking of
the use cases ever, we just did something yolo"

Btrfs does not make a difference between snapshots and clones. Other
systems like netapp and zfs do. Btrfs cloud also do that, and just not
expose the ro/rw flag to the outside.

Personally, I would like btrfs to go into that direction, because it
just makes things more clear. This is a snapshot, you cannot touch it.
If you want to make changes, you have to make a rw clone of the snapshot.

The nice thing for btrfs is that you can remove the snapshot after you
made the rw clone, which you cannot do on a NetApp filer. :o)

>> Even if it wouldn't make sense for some reason, it's a nice thought
>> experiment. :)

There we go :)

-- 
Hans van Kranenburg
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Andrei Borzenkov]

06.10.2017 20:49, Hans van Kranenburg пишет:
> On 10/06/2017 07:24 PM, David Sterba wrote:
>> On Thu, Oct 05, 2017 at 05:03:47PM +0800, Anand Jain wrote:
>>> On 10/05/2017 04:22 PM, 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 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 the
 received uuid. The presence of the received_uuid can also cause problems 
 when
 the volume is being send.
> 
> Are the 'can cause problems when being send' explained somewhere?
> 

If received_uuid is present, btrfs send will use it instead of subvolume
uuid. It means btrfs receive may find wrong volume as differential
stream base. Example that was demonstrated earlier

1. A -> B on remote system S. B now has received_uui == A
2. A -> C on local system. C now has received_uuid == A
3. C is made read-write and changed.
4. Create snapshot D from C and do "btrfs send -p C D" to system S. Now
btrfs receive on S will get base uuid of A and will find B. So any
changes between B and C are silently lost.

>>>
>>> Wonder if this [1] approach was considered
>>> [1]
>>>   - set a flag on the subvolume to indicate its dirtied so that 
>>> received_uuid can be kept forever just in case if user needs it for some 
>>> reference at a later point of time.
>>
>> Yeah, we need to be careful here. There are more items related to the
>> recived subvolume, besides received_uuid there's rtransid and rtime so
>> they might need to be cleared as well.
>>
>> I don't remember all the details how the send/receive and uuids
>> interact. Switching from ro->rw needs to affect the 'received' status,
>> but I don't know how. The problem is that some information is being lost
>> although it may be quite important to the user/administrator. In such
>> cases it would be convenient to request a confirmation via a --force
>> flag or something like that.
> 
> On IRC I think we generally recommends users to never do this, and as a
> best practice always clone the snapshot to a rw subvolume in a different
> location if someone wants to proceed working with the contents and
> changing them as opposed to messing with the ro/rw attributes.
> 
> So, what about option [2]:
> 
> [2] if a subvolume has a received_uuid, then just do not allow changing
> it to rw.
> 

What is reason behind allowing change from ro to rw in the first place?
What is the use case?

> Even if it wouldn't make sense for some reason, it's a nice thought
> experiment. :)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Hans van Kranenburg]

On 10/06/2017 07:24 PM, David Sterba wrote:
> On Thu, Oct 05, 2017 at 05:03:47PM +0800, Anand Jain wrote:
>> On 10/05/2017 04:22 PM, 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 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 the
>>> received uuid. The presence of the received_uuid can also cause problems 
>>> when
>>> the volume is being send.

Are the 'can cause problems when being send' explained somewhere?

>>
>> Wonder if this [1] approach was considered
>> [1]
>>   - set a flag on the subvolume to indicate its dirtied so that 
>> received_uuid can be kept forever just in case if user needs it for some 
>> reference at a later point of time.
> 
> Yeah, we need to be careful here. There are more items related to the
> recived subvolume, besides received_uuid there's rtransid and rtime so
> they might need to be cleared as well.
> 
> I don't remember all the details how the send/receive and uuids
> interact. Switching from ro->rw needs to affect the 'received' status,
> but I don't know how. The problem is that some information is being lost
> although it may be quite important to the user/administrator. In such
> cases it would be convenient to request a confirmation via a --force
> flag or something like that.

On IRC I think we generally recommends users to never do this, and as a
best practice always clone the snapshot to a rw subvolume in a different
location if someone wants to proceed working with the contents and
changing them as opposed to messing with the ro/rw attributes.

So, what about option [2]:

[2] if a subvolume has a received_uuid, then just do not allow changing
it to rw.

Even if it wouldn't make sense for some reason, it's a nice thought
experiment. :)

-- 
Hans van Kranenburg
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[David Sterba]

On Thu, Oct 05, 2017 at 05:03:47PM +0800, Anand Jain wrote:
> On 10/05/2017 04:22 PM, 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 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 the
> > received uuid. The presence of the received_uuid can also cause problems 
> > when
> > the volume is being send.
> 
> Wonder if this [1] approach was considered
> [1]
>   - set a flag on the subvolume to indicate its dirtied so that 
> received_uuid can be kept forever just in case if user needs it for some 
> reference at a later point of time.

Yeah, we need to be careful here. There are more items related to the
recived subvolume, besides received_uuid there's rtransid and rtime so
they might need to be cleared as well.

I don't remember all the details how the send/receive and uuids
interact. Switching from ro->rw needs to affect the 'received' status,
but I don't know how. The problem is that some information is being lost
although it may be quite important to the user/administrator. In such
cases it would be convenient to request a confirmation via a --force
flag or something like that.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] btrfs: Remove received_uuid during received snapshot ro->rw switch

[Anand Jain]

On 10/05/2017 04:22 PM, 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 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 the
received uuid. The presence of the received_uuid can also cause problems when
the volume is being send.



Wonder if this [1] approach was considered
[1]
 - set a flag on the subvolume to indicate its dirtied so that 
received_uuid can be kept forever just in case if user needs it for some 
reference at a later point of time.


Thanks, Anand



Signed-off-by: Nikolay Borisov 
Suggested-by: David Sterba 
---

v4:
  * Put the uuid tree removal code after the lightweight 'send in progress'
  check and don't move btrfs_start_transaction as suggested by David
  
v3:

  * Rework the patch considering latest feedback from David Sterba i.e.
   explicitly use btrfs_end_transaction

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ee4ee7cbba72..9328c091854b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1775,6 +1775,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
file *file,
struct btrfs_trans_handle *trans;
u64 root_flags;
u64 flags;
+   bool clear_received_uuid = false;
int ret = 0;
  
  	if (!inode_owner_or_capable(inode))

@@ -1824,6 +1825,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
file *file,
btrfs_set_root_flags(>root_item,
 root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
spin_unlock(>root_item_lock);
+   clear_received_uuid = true;
} else {
spin_unlock(>root_item_lock);
btrfs_warn(fs_info,
@@ -1840,6 +1842,24 @@ static noinline int btrfs_ioctl_subvol_setflags(struct 
file *file,
goto out_reset;
}
  
+	if (clear_received_uuid) {

+   if (!btrfs_is_empty_uuid(root->root_item.received_uuid)) {
+   ret = btrfs_uuid_tree_rem(trans, fs_info,
+   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->root_item.received_uuid, 0,
+   BTRFS_UUID_SIZE);
+   }
+   }
+
ret = btrfs_update_root(trans, fs_info->tree_root,
>root_key, >root_item);
if (ret < 0) {


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html