[PATCH] Btrfs: clear received_uuid field for new writable snapshots

[PATCH] Btrfs: clear received_uuid field for new writable snapshots

[Stefan Behrens]

For created snapshots, the full root_item is copied from the source
root and afterwards selectively modified. The current code forgets
to clear the field received_uuid. The only problem is that it is
confusing when you look at it with 'btrfs subv list', since for
writable snapshots, the contents of the snapshot can be completely
unrelated to the previously received snapshot.
The receiver ignores such snapshots anyway because he also checks
the field stransid in the root_item and that value used to be reset
to zero for all created snapshots.

This commit changes two things:
- clear the received_uuid field for new writable snapshots.
- don't clear the send/receive related information like the stransid
  for read-only snapshots (which makes them useable as a parent for
  the automatic selection of parents in the receive code).

Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
---
 fs/btrfs/transaction.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index ffac232..94cbd10 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1170,13 +1170,17 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	memcpy(new_root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
 	memcpy(new_root_item->parent_uuid, root->root_item.uuid,
 			BTRFS_UUID_SIZE);
+	if (!(root_flags & BTRFS_ROOT_SUBVOL_RDONLY)) {
+		memset(new_root_item->received_uuid, 0,
+		       sizeof(new_root_item->received_uuid));
+		memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
+		memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
+		btrfs_set_root_stransid(new_root_item, 0);
+		btrfs_set_root_rtransid(new_root_item, 0);
+	}
 	new_root_item->otime.sec = cpu_to_le64(cur_time.tv_sec);
 	new_root_item->otime.nsec = cpu_to_le32(cur_time.tv_nsec);
 	btrfs_set_root_otransid(new_root_item, trans->transid);
-	memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
-	memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
-	btrfs_set_root_stransid(new_root_item, 0);
-	btrfs_set_root_rtransid(new_root_item, 0);
 
 	old = btrfs_lock_root_node(root);
 	ret = btrfs_cow_block(trans, root, old, NULL, 0, &old);
-- 
1.8.2.1

Re: [PATCH] Btrfs: clear received_uuid field for new writable snapshots

[Alex Lyakas]

Hi Stephan,
I fully understand the first part of your fix, and I believe it's
quite critical. Indeed, a writable snapshot should have no evidence
that it has an ancestor that was once "received".

Can you pls let me know that I understand the second part of your fix.
In btrfs-progs the following code in tree_search() would have
prevented us from mistakenly selecting such snapshot as a parent for
"receive":
        if (type == subvol_search_by_received_uuid) {
            entry = rb_entry(n, struct subvol_info,
                    rb_received_node);
            comp = memcmp(entry->received_uuid, uuid,
                    BTRFS_UUID_SIZE);
            if (!comp) {
                if (entry->stransid < stransid)
                    comp = -1;
                else if (entry->stransid > stransid)
                    comp = 1;
                else
                    comp = 0;
            }
The code checks both received_uuid (which would have been wrongly
equal to what we need), but also the stransid (which was the ctransid
on the send side), which would have been zero, so it wouldn't match.

Now after your fix, the stransid field becomes not needed, correct?
Because if we have a valid received_uuid, this means that either we
are the "received" snapshot, or our whole chain of ancestors are
read-only, and eventually there was an ancestor that was "received".
So we have valid data and can be used as a parent. Is it still needed
after your fix to check the stransid field ? (it doesn't hurt to check
it)

Clearring/Not clearing the rtransid - does it bring any value?
rtransid is the local transid of when we had completed the "receive"
process for this snap. Is there any interesting usage of this value?

Thanks,
Alex.


On Wed, Apr 17, 2013 at 12:11 PM, Stefan Behrens
<sbehrens@giantdisaster.de> wrote:
>
> For created snapshots, the full root_item is copied from the source
> root and afterwards selectively modified. The current code forgets
> to clear the field received_uuid. The only problem is that it is
> confusing when you look at it with 'btrfs subv list', since for
> writable snapshots, the contents of the snapshot can be completely
> unrelated to the previously received snapshot.
> The receiver ignores such snapshots anyway because he also checks
> the field stransid in the root_item and that value used to be reset
> to zero for all created snapshots.
>
> This commit changes two things:
> - clear the received_uuid field for new writable snapshots.
> - don't clear the send/receive related information like the stransid
>   for read-only snapshots (which makes them useable as a parent for
>   the automatic selection of parents in the receive code).
>
> Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
> ---
>  fs/btrfs/transaction.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index ffac232..94cbd10 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1170,13 +1170,17 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>         memcpy(new_root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
>         memcpy(new_root_item->parent_uuid, root->root_item.uuid,
>                         BTRFS_UUID_SIZE);
> +       if (!(root_flags & BTRFS_ROOT_SUBVOL_RDONLY)) {
> +               memset(new_root_item->received_uuid, 0,
> +                      sizeof(new_root_item->received_uuid));
> +               memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
> +               memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
> +               btrfs_set_root_stransid(new_root_item, 0);
> +               btrfs_set_root_rtransid(new_root_item, 0);
> +       }
>         new_root_item->otime.sec = cpu_to_le64(cur_time.tv_sec);
>         new_root_item->otime.nsec = cpu_to_le32(cur_time.tv_nsec);
>         btrfs_set_root_otransid(new_root_item, trans->transid);
> -       memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
> -       memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
> -       btrfs_set_root_stransid(new_root_item, 0);
> -       btrfs_set_root_rtransid(new_root_item, 0);
>
>         old = btrfs_lock_root_node(root);
>         ret = btrfs_cow_block(trans, root, old, NULL, 0, &old);
> --
> 1.8.2.1
>
> --
> 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] Btrfs: clear received_uuid field for new writable snapshots

[Stefan Behrens]

On Wed, 22 May 2013 13:37:15 +0300, Alex Lyakas wrote:
> Hi Stephan,
> I fully understand the first part of your fix, and I believe it's
> quite critical. Indeed, a writable snapshot should have no evidence
> that it has an ancestor that was once "received".
> 
> Can you pls let me know that I understand the second part of your fix.
> In btrfs-progs the following code in tree_search() would have
> prevented us from mistakenly selecting such snapshot as a parent for
> "receive":
>         if (type == subvol_search_by_received_uuid) {
>             entry = rb_entry(n, struct subvol_info,
>                     rb_received_node);
>             comp = memcmp(entry->received_uuid, uuid,
>                     BTRFS_UUID_SIZE);
>             if (!comp) {
>                 if (entry->stransid < stransid)
>                     comp = -1;
>                 else if (entry->stransid > stransid)
>                     comp = 1;
>                 else
>                     comp = 0;
>             }
> The code checks both received_uuid (which would have been wrongly
> equal to what we need), but also the stransid (which was the ctransid
> on the send side), which would have been zero, so it wouldn't match.
> 
> Now after your fix, the stransid field becomes not needed, correct?
> Because if we have a valid received_uuid, this means that either we
> are the "received" snapshot, or our whole chain of ancestors are
> read-only, and eventually there was an ancestor that was "received".
> So we have valid data and can be used as a parent. Is it still needed
> after your fix to check the stransid field ? (it doesn't hurt to check
> it)

Hi Alex,

Yes, the code in tree_search() that evaluates the stransid field can be
removed if compatibility of a new btrfs-progs release to an old kernel
is not a concern. And in the improved send/receive code (that makes use
of the UUID tree and the root tree to retrieve all the information it
needs "[PATCH v3 0/4] Btrfs-progs: speedup btrfs send/receive"), this
code is removed and stransid is not evaluated anymore. The evaluation
was only useful to fix the bug that the received_uuid field was not
cleared for writable snapshots.


> Clearring/Not clearing the rtransid - does it bring any value?
> rtransid is the local transid of when we had completed the "receive"
> process for this snap. Is there any interesting usage of this value?

There's no code that makes use of the rtransid field. But since a
read-only snapshot is identical to the parent, there is no need to clear
the field while creating a read-only snapshot. And since I changed this
for the stransid field (which is evaluated in the current btrfs-progs
code), I changed all related fields at the same time, even those that
are not evaluated anywhere.


> On Wed, Apr 17, 2013 at 12:11 PM, Stefan Behrens
> <sbehrens@giantdisaster.de> wrote:
>>
>> For created snapshots, the full root_item is copied from the source
>> root and afterwards selectively modified. The current code forgets
>> to clear the field received_uuid. The only problem is that it is
>> confusing when you look at it with 'btrfs subv list', since for
>> writable snapshots, the contents of the snapshot can be completely
>> unrelated to the previously received snapshot.
>> The receiver ignores such snapshots anyway because he also checks
>> the field stransid in the root_item and that value used to be reset
>> to zero for all created snapshots.
>>
>> This commit changes two things:
>> - clear the received_uuid field for new writable snapshots.
>> - don't clear the send/receive related information like the stransid
>>   for read-only snapshots (which makes them useable as a parent for
>>   the automatic selection of parents in the receive code).
>>
>> Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
>> ---
>>  fs/btrfs/transaction.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index ffac232..94cbd10 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1170,13 +1170,17 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>>         memcpy(new_root_item->uuid, new_uuid.b, BTRFS_UUID_SIZE);
>>         memcpy(new_root_item->parent_uuid, root->root_item.uuid,
>>                         BTRFS_UUID_SIZE);
>> +       if (!(root_flags & BTRFS_ROOT_SUBVOL_RDONLY)) {
>> +               memset(new_root_item->received_uuid, 0,
>> +                      sizeof(new_root_item->received_uuid));
>> +               memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
>> +               memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
>> +               btrfs_set_root_stransid(new_root_item, 0);
>> +               btrfs_set_root_rtransid(new_root_item, 0);
>> +       }
>>         new_root_item->otime.sec = cpu_to_le64(cur_time.tv_sec);
>>         new_root_item->otime.nsec = cpu_to_le32(cur_time.tv_nsec);
>>         btrfs_set_root_otransid(new_root_item, trans->transid);
>> -       memset(&new_root_item->stime, 0, sizeof(new_root_item->stime));
>> -       memset(&new_root_item->rtime, 0, sizeof(new_root_item->rtime));
>> -       btrfs_set_root_stransid(new_root_item, 0);
>> -       btrfs_set_root_rtransid(new_root_item, 0);
>>
>>         old = btrfs_lock_root_node(root);
>>         ret = btrfs_cow_block(trans, root, old, NULL, 0, &old);
>> --
>> 1.8.2.1