[PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[Nikolay Borisov]

btrfs_update_root can fail for any number of reasons, however the only error
handling we do is to revert the modified flags, yet we do not abort the
transaction but proceed to commit it. Fix this by explicitly checking if the
update root routine has failed and abort the transaction.

Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d6715c2bcdc4..09fcd51f0e8c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
+	if (ret < 0)
+		btrfs_abort_transaction(trans, ret);
 
 	btrfs_commit_transaction(trans);
+
 out_reset:
 	if (ret)
 		btrfs_set_root_flags(&root->root_item, root_flags);
-- 
2.7.4

[RFC PATCH 2/2] btrfs: Remove received_uuid during received snapshot 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 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.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

This patch has been inspired by the problem described in 
https://www.spinics.net/lists/linux-btrfs/msg68879.html

 fs/btrfs/ioctl.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 09fcd51f0e8c..71fd28caefdd 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1811,6 +1811,17 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 		goto out_drop_sem;
 
 	root_flags = btrfs_root_flags(&root->root_item);
+
+	/*
+	 * 1 - root item
+	 * 1 - uuid item
+	 */
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_drop_sem;
+	}
+
 	if (flags & BTRFS_SUBVOL_RDONLY) {
 		btrfs_set_root_flags(&root->root_item,
 				     root_flags | BTRFS_ROOT_SUBVOL_RDONLY);
@@ -1824,30 +1835,38 @@ 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);
+			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);
+					goto out_end_trans;
+				}
+
+				memset(root->root_item.received_uuid, 0,
+				       BTRFS_UUID_SIZE);
+			}
 		} else {
 			spin_unlock(&root->root_item_lock);
 			btrfs_warn(fs_info,
 				   "Attempt to set subvolume %llu read-write during send",
 				   root->root_key.objectid);
 			ret = -EPERM;
-			goto out_drop_sem;
+			btrfs_abort_transaction(trans, ret);
+			goto out_end_trans;
 		}
 	}
 
-	trans = btrfs_start_transaction(root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		goto out_reset;
-	}
-
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
 	if (ret < 0)
 		btrfs_abort_transaction(trans, ret);
 
+out_end_trans:
 	btrfs_commit_transaction(trans);
-
-out_reset:
 	if (ret)
 		btrfs_set_root_flags(&root->root_item, root_flags);
 out_drop_sem:
-- 
2.7.4

[PATCH v2] btrfs: Remove received_uuid during received snapshot 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 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 <nborisov@suse.com>
---

Changes since v1: 
 * Dropped patch 1/2 which was doing a bit of refactoring w.r.t transaction 
 failure handling and instead folded the once change with made sense (
 ret = btrfs_commit_transaction) under the out_reset label.

 fs/btrfs/ioctl.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d6715c2bcdc4..2049da6a0698 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1811,6 +1811,17 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 		goto out_drop_sem;
 
 	root_flags = btrfs_root_flags(&root->root_item);
+
+	/*
+	 * 1 - root item
+	 * 1 - uuid item
+	 */
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_drop_sem;
+	}
+
 	if (flags & BTRFS_SUBVOL_RDONLY) {
 		btrfs_set_root_flags(&root->root_item,
 				     root_flags | BTRFS_ROOT_SUBVOL_RDONLY);
@@ -1824,27 +1835,36 @@ 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);
+			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);
+					goto out_reset;
+				}
+
+				memset(root->root_item.received_uuid, 0,
+				       BTRFS_UUID_SIZE);
+			}
 		} else {
 			spin_unlock(&root->root_item_lock);
 			btrfs_warn(fs_info,
 				   "Attempt to set subvolume %llu read-write during send",
 				   root->root_key.objectid);
 			ret = -EPERM;
-			goto out_drop_sem;
+			btrfs_abort_transaction(trans, ret);
+			goto out_reset;
 		}
 	}
 
-	trans = btrfs_start_transaction(root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		goto out_reset;
-	}
-
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
 
-	btrfs_commit_transaction(trans);
 out_reset:
+	ret = btrfs_commit_transaction(trans);
 	if (ret)
 		btrfs_set_root_flags(&root->root_item, root_flags);
 out_drop_sem:
-- 
2.7.4

Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[David Sterba]

On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
> btrfs_update_root can fail for any number of reasons, however the only error
> handling we do is to revert the modified flags, yet we do not abort the
> transaction but proceed to commit it.

AFAICS btrfs_update_root aborts the transaction itself, so it's not
needed in the caller.

> Fix this by explicitly checking if the
> update root routine has failed and abort the transaction.
> 
> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d6715c2bcdc4..09fcd51f0e8c 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>  
>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>  				&root->root_key, &root->root_item);
> +	if (ret < 0)
> +		btrfs_abort_transaction(trans, ret);
>  
>  	btrfs_commit_transaction(trans);

I think the error from commit_transaction should be returned as well if
we get here.

So:

ret = btrfs_update_root();

if (ret < 0) {
	end_transaction();
	goto out_reset;
}

ret = btrfs_commit_transaction();

out_reset:
/* and then as it is */
if (ret)
	btrfs_set_root_flags(...);

etc.



> +
>  out_reset:
>  	if (ret)
>  		btrfs_set_root_flags(&root->root_item, root_flags);
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[Nikolay Borisov]

On 26.09.2017 20:39, David Sterba wrote:
> On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
>> btrfs_update_root can fail for any number of reasons, however the only error
>> handling we do is to revert the modified flags, yet we do not abort the
>> transaction but proceed to commit it.
> 
> AFAICS btrfs_update_root aborts the transaction itself, so it's not
> needed in the caller.
> 
>> Fix this by explicitly checking if the
>> update root routine has failed and abort the transaction.
>>
>> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/ioctl.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index d6715c2bcdc4..09fcd51f0e8c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>  
>>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>>  				&root->root_key, &root->root_item);
>> +	if (ret < 0)
>> +		btrfs_abort_transaction(trans, ret);
>>  
>>  	btrfs_commit_transaction(trans);
> 
> I think the error from commit_transaction should be returned as well if
> we get here.
> 
> So:
> 
> ret = btrfs_update_root();
> 
> if (ret < 0) {
> 	end_transaction();
> 	goto out_reset;
> }
> 
> ret = btrfs_commit_transaction();
> 
> out_reset:
> /* and then as it is */
> if (ret)
> 	btrfs_set_root_flags(...);
> 
> etc.

I think even this is not needed, since btrfs_commit_transaction actually
handles aborted transaction by calling btrfs_end_transaction and
returning the error with which the trans was aborted. So I will just
drop this patch


> 
> 
> 
>> +
>>  out_reset:
>>  	if (ret)
>>  		btrfs_set_root_flags(&root->root_item, root_flags);
>> -- 
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[David Sterba]

On Wed, Sep 27, 2017 at 11:48:59AM +0300, Nikolay Borisov wrote:
> On 26.09.2017 20:39, David Sterba wrote:
> > On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
> >> btrfs_update_root can fail for any number of reasons, however the only error
> >> handling we do is to revert the modified flags, yet we do not abort the
> >> transaction but proceed to commit it.
> > 
> > AFAICS btrfs_update_root aborts the transaction itself, so it's not
> > needed in the caller.
> > 
> >> Fix this by explicitly checking if the
> >> update root routine has failed and abort the transaction.
> >>
> >> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>  fs/btrfs/ioctl.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> index d6715c2bcdc4..09fcd51f0e8c 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
> >>  
> >>  	ret = btrfs_update_root(trans, fs_info->tree_root,
> >>  				&root->root_key, &root->root_item);
> >> +	if (ret < 0)
> >> +		btrfs_abort_transaction(trans, ret);
> >>  
> >>  	btrfs_commit_transaction(trans);
> > 
> > I think the error from commit_transaction should be returned as well if
> > we get here.
> > 
> > So:
> > 
> > ret = btrfs_update_root();
> > 
> > if (ret < 0) {
> > 	end_transaction();
> > 	goto out_reset;
> > }
> > 
> > ret = btrfs_commit_transaction();
> > 
> > out_reset:
> > /* and then as it is */
> > if (ret)
> > 	btrfs_set_root_flags(...);
> > 
> > etc.
> 
> I think even this is not needed, since btrfs_commit_transaction actually
> handles aborted transaction by calling btrfs_end_transaction and
> returning the error with which the trans was aborted. So I will just
> drop this patch

>From API point of view, I'd like to see some pattern, where we don't
call commit after transaction abort IF we have enough information from
the context. Ie. the abort and commit would be in the same function.
Commit has to deal with an aborted transaction, that's right, but kind
of an implementation detail and last resort resolution.

Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[Nikolay Borisov]

On 27.09.2017 17:00, David Sterba wrote:
> On Wed, Sep 27, 2017 at 11:48:59AM +0300, Nikolay Borisov wrote:
>> On 26.09.2017 20:39, David Sterba wrote:
>>> On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
>>>> btrfs_update_root can fail for any number of reasons, however the only error
>>>> handling we do is to revert the modified flags, yet we do not abort the
>>>> transaction but proceed to commit it.
>>>
>>> AFAICS btrfs_update_root aborts the transaction itself, so it's not
>>> needed in the caller.
>>>
>>>> Fix this by explicitly checking if the
>>>> update root routine has failed and abort the transaction.
>>>>
>>>> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>> ---
>>>>  fs/btrfs/ioctl.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index d6715c2bcdc4..09fcd51f0e8c 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>>>  
>>>>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>>>>  				&root->root_key, &root->root_item);
>>>> +	if (ret < 0)
>>>> +		btrfs_abort_transaction(trans, ret);
>>>>  
>>>>  	btrfs_commit_transaction(trans);
>>>
>>> I think the error from commit_transaction should be returned as well if
>>> we get here.
>>>
>>> So:
>>>
>>> ret = btrfs_update_root();
>>>
>>> if (ret < 0) {
>>> 	end_transaction();
>>> 	goto out_reset;
>>> }
>>>
>>> ret = btrfs_commit_transaction();
>>>
>>> out_reset:
>>> /* and then as it is */
>>> if (ret)
>>> 	btrfs_set_root_flags(...);
>>>
>>> etc.
>>
>> I think even this is not needed, since btrfs_commit_transaction actually
>> handles aborted transaction by calling btrfs_end_transaction and
>> returning the error with which the trans was aborted. So I will just
>> drop this patch
> 
> From API point of view, I'd like to see some pattern, where we don't
> call commit after transaction abort IF we have enough information from
> the context. Ie. the abort and commit would be in the same function.
> Commit has to deal with an aborted transaction, that's right, but kind
> of an implementation detail and last resort resolution.

Fair point, I will respin the patch as well as some of my other
transaction error handling patches.


> 

[PATCH 1/2] btrfs: Explicitly handle btrfs_update_root failure

[Nikolay Borisov]

btrfs_udpate_root can fail and it aborts the transaction, the correct way to
handle an aborted transaction is to explicitly end with btrfs_end_transaction.
Even now the code is correct since btrfs_commit_transaction would handle an
aborted transaction but this is more of an implementation detail. So let's be
explicit in handling failure in btrfs_update_root.

Furthermore btrfs_commit_transaction can also fail and by ignoring it's return
value we could have left the in-memory copy of the root item in an inconsistent
state. So capture the error value which allows us to correctly revert the RO/RW
flags in case of commit failure.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d6715c2bcdc4..ee4ee7cbba72 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1842,8 +1842,13 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
+	if (ret < 0) {
+		btrfs_end_transaction(trans);
+		goto out_reset;
+	}
+
+	ret = btrfs_commit_transaction(trans);
 
-	btrfs_commit_transaction(trans);
 out_reset:
 	if (ret)
 		btrfs_set_root_flags(&root->root_item, root_flags);
-- 
2.7.4

[PATCH v3 2/2] btrfs: Remove received_uuid during received snapshot 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 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 <nborisov@suse.com>
---

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

 fs/btrfs/ioctl.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ee4ee7cbba72..c0374125cec2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1811,6 +1811,17 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
 		goto out_drop_sem;
 
 	root_flags = btrfs_root_flags(&root->root_item);
+
+	/*
+	 * 1 - root item
+	 * 1 - uuid item
+	 */
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_drop_sem;
+	}
+
 	if (flags & BTRFS_SUBVOL_RDONLY) {
 		btrfs_set_root_flags(&root->root_item,
 				     root_flags | BTRFS_ROOT_SUBVOL_RDONLY);
@@ -1824,22 +1835,33 @@ 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);
+			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);
+			}
 		} else {
 			spin_unlock(&root->root_item_lock);
 			btrfs_warn(fs_info,
 				   "Attempt to set subvolume %llu read-write during send",
 				   root->root_key.objectid);
 			ret = -EPERM;
-			goto out_drop_sem;
+			btrfs_abort_transaction(trans, ret);
+			btrfs_end_transaction(trans);
+			goto out_reset;
 		}
 	}
 
-	trans = btrfs_start_transaction(root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		goto out_reset;
-	}
-
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&root->root_key, &root->root_item);
 	if (ret < 0) {
-- 
2.7.4

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

[David Sterba]

On Thu, Sep 28, 2017 at 10:53:18AM +0300, 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 <nborisov@suse.com>
> ---
> 
> v3:
>  * Rework the patch considering latest feedback from David Sterba i.e. 
>  explicitly use btrfs_end_transaction 
> 
>  fs/btrfs/ioctl.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ee4ee7cbba72..c0374125cec2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1811,6 +1811,17 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>  		goto out_drop_sem;
>  
>  	root_flags = btrfs_root_flags(&root->root_item);
> +
> +	/*
> +	 * 1 - root item
> +	 * 1 - uuid item
> +	 */
> +	trans = btrfs_start_transaction(root, 2);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out_drop_sem;
> +	}
> +
>  	if (flags & BTRFS_SUBVOL_RDONLY) {
>  		btrfs_set_root_flags(&root->root_item,
>  				     root_flags | BTRFS_ROOT_SUBVOL_RDONLY);
> @@ -1824,22 +1835,33 @@ 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);
> +			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);
> +			}
>  		} else {
>  			spin_unlock(&root->root_item_lock);
>  			btrfs_warn(fs_info,
>  				   "Attempt to set subvolume %llu read-write during send",
>  				   root->root_key.objectid);
>  			ret = -EPERM;
> -			goto out_drop_sem;
> +			btrfs_abort_transaction(trans, ret);
> +			btrfs_end_transaction(trans);
> +			goto out_reset;

Adding the transaction before the "if (flags & BTRFS_SUBVOL_RDONLY)"
condition makes it much worse. The "is subvolume in send" test is
supposed to be lightweight and should not shoot down the whole
filesystem. The usecase is explained in 2c68653787f91c62f8.

Also the received_uuid must be changed under the root_item_lock.

I think it should be fine to keep the transaction start where it is,
change the received_uuid eventually and let it commit. You can set the
transaction units to 2 unconditionally.

>  		}
>  	}
>  
> -	trans = btrfs_start_transaction(root, 1);
> -	if (IS_ERR(trans)) {
> -		ret = PTR_ERR(trans);
> -		goto out_reset;
> -	}
> -
>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>  				&root->root_key, &root->root_item);
>  	if (ret < 0) {

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

[Nikolay Borisov]

On 29.09.2017 20:56, David Sterba wrote:
> On Thu, Sep 28, 2017 at 10:53:18AM +0300, 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 <nborisov@suse.com>
>> ---
>>
>> v3:
>>  * Rework the patch considering latest feedback from David Sterba i.e. 
>>  explicitly use btrfs_end_transaction 
>>
>>  fs/btrfs/ioctl.c | 36 +++++++++++++++++++++++++++++-------
>>  1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index ee4ee7cbba72..c0374125cec2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1811,6 +1811,17 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>  		goto out_drop_sem;
>>  
>>  	root_flags = btrfs_root_flags(&root->root_item);
>> +
>> +	/*
>> +	 * 1 - root item
>> +	 * 1 - uuid item
>> +	 */
>> +	trans = btrfs_start_transaction(root, 2);
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto out_drop_sem;
>> +	}
>> +
>>  	if (flags & BTRFS_SUBVOL_RDONLY) {
>>  		btrfs_set_root_flags(&root->root_item,
>>  				     root_flags | BTRFS_ROOT_SUBVOL_RDONLY);
>> @@ -1824,22 +1835,33 @@ 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);
>> +			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);
>> +			}
>>  		} else {
>>  			spin_unlock(&root->root_item_lock);
>>  			btrfs_warn(fs_info,
>>  				   "Attempt to set subvolume %llu read-write during send",
>>  				   root->root_key.objectid);
>>  			ret = -EPERM;
>> -			goto out_drop_sem;
>> +			btrfs_abort_transaction(trans, ret);
>> +			btrfs_end_transaction(trans);
>> +			goto out_reset;
> 
> Adding the transaction before the "if (flags & BTRFS_SUBVOL_RDONLY)"
> condition makes it much worse. The "is subvolume in send" test is
> supposed to be lightweight and should not shoot down the whole
> filesystem. The usecase is explained in 2c68653787f91c62f8.
> 
> Also the received_uuid must be changed under the root_item_lock.
> 
> I think it should be fine to keep the transaction start where it is,
> change the received_uuid eventually and let it commit. You can set the
> transaction units to 2 unconditionally.

So what you are suggesting is to not move the transaction start before
the if check? But then how would you structure the code to remove the
uuid only if we are switchin RO->RW and not in send without duplicating
the checks right before btrfs_update_root?


> 
>>  		}
>>  	}
>>  
>> -	trans = btrfs_start_transaction(root, 1);
>> -	if (IS_ERR(trans)) {
>> -		ret = PTR_ERR(trans);
>> -		goto out_reset;
>> -	}
>> -
>>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>>  				&root->root_key, &root->root_item);
>>  	if (ret < 0) {

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

[David Sterba]

On Fri, Sep 29, 2017 at 10:15:30PM +0300, Nikolay Borisov wrote:
> > Adding the transaction before the "if (flags & BTRFS_SUBVOL_RDONLY)"
> > condition makes it much worse. The "is subvolume in send" test is
> > supposed to be lightweight and should not shoot down the whole
> > filesystem. The usecase is explained in 2c68653787f91c62f8.
> > 
> > Also the received_uuid must be changed under the root_item_lock.
> > 
> > I think it should be fine to keep the transaction start where it is,
> > change the received_uuid eventually and let it commit. You can set the
> > transaction units to 2 unconditionally.
> 
> So what you are suggesting is to not move the transaction start before
> the if check? But then how would you structure the code to remove the
> uuid only if we are switchin RO->RW and not in send without duplicating
> the checks right before btrfs_update_root?

--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1772,6 +1772,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
        u64 root_flags;
        u64 flags;
        int ret = 0;
+       bool clear_received_uuid = false;

        if (!inode_owner_or_capable(inode))
                return -EPERM;
@@ -1820,6 +1821,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_uuid = true;
                } else {
                        spin_unlock(&root->root_item_lock);
                        btrfs_warn(fs_info,
@@ -1836,6 +1838,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->root_key, &root->root_item);

[PATCH v4] btrfs: Remove received_uuid during received snapshot 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 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 <nborisov@suse.com>
Suggested-by: David Sterba <dsterba@suse.cz>
---

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->root_item,
 				     root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
 			spin_unlock(&root->root_item_lock);
+			clear_received_uuid = true;
 		} else {
 			spin_unlock(&root->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->root_key, &root->root_item);
 	if (ret < 0) {
-- 
2.7.4

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 <nborisov@suse.com>
> Suggested-by: David Sterba <dsterba@suse.cz>
> ---
> 
> 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->root_item,
>   				     root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
>   			spin_unlock(&root->root_item_lock);
> +			clear_received_uuid = true;
>   		} else {
>   			spin_unlock(&root->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->root_key, &root->root_item);
>   	if (ret < 0) {
> 

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.

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

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

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

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

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 <nborisov@suse.com>
> Suggested-by: David Sterba <dsterba@suse.cz>
> ---
> 
> 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->root_item,
>  				     root_flags & ~BTRFS_ROOT_SUBVOL_RDONLY);
>  			spin_unlock(&root->root_item_lock);
> +			clear_received_uuid = true;
>  		} else {
>  			spin_unlock(&root->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->root_key, &root->root_item);
>  	if (ret < 0) {
> 


-- 
Hans van Kranenburg

Re: [PATCH 1/2] btrfs: Explicitly handle btrfs_update_root failure

[David Sterba]

On Thu, Sep 28, 2017 at 10:53:17AM +0300, Nikolay Borisov wrote:
> btrfs_udpate_root can fail and it aborts the transaction, the correct way to
> handle an aborted transaction is to explicitly end with btrfs_end_transaction.
> Even now the code is correct since btrfs_commit_transaction would handle an
> aborted transaction but this is more of an implementation detail. So let's be
> explicit in handling failure in btrfs_update_root.
> 
> Furthermore btrfs_commit_transaction can also fail and by ignoring it's return
> value we could have left the in-memory copy of the root item in an inconsistent
> state. So capture the error value which allows us to correctly revert the RO/RW
> flags in case of commit failure.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>