| Message ID | 1366189907-16942-1-git-send-email-sbehrens@giantdisaster.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
--
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
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 -- 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
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);
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(-)