Skip to content
Permalink
Browse files

bookmark cloning: cloning redaction bookmarks produces non-redcation …

…bookmarks
  • Loading branch information
problame committed Jan 14, 2020
1 parent 81582ef commit fa99694ea55fb6a1477e2804a7e150bb31e3b84a
@@ -3615,6 +3615,10 @@ as the incremental source for a
.Nm zfs Cm send
command.
.Pp
When copying an existing redaction bookmark, the resulting bookmark is
.Sy not
a redaction bookmark.
.Pp
This feature must be enabled to be used.
See
.Xr zpool-features 5
@@ -56,6 +56,9 @@ dsl_bookmark_hold_ds(dsl_pool_t *dp, const char *fullname,
}

/*
* When reading BOOKMARK_V1 bookmarks, the BOOKMARK_V2 fields are guaranteed
* to be zeroed.
*
* Returns ESRCH if bookmark is not found.
* Note, we need to use the ZAP rather than the AVL to look up bookmarks
* by name, because only the ZAP honors the casesensitivity setting.
@@ -216,10 +219,6 @@ dsl_bookmark_create_check_impl_book(dsl_pool_t *dp,
if (error != 0)
return (error);

/* We only support bookmarks without redaction lists */
if (source_bm_phys.zbm_redaction_obj != 0)
return (EINVAL);

return (0);
}

@@ -510,7 +509,6 @@ dsl_bookmark_create_sync_impl_book(
dsl_dataset_t *bmark_fs_source, *bmark_fs_new;
char *source_shortname, *new_shortname;
zfs_bookmark_phys_t source_phys;
boolean_t source_redacted;

VERIFY0(dsl_bookmark_hold_ds(dp, source_name, &bmark_fs_source, FTAG,
&source_shortname));
@@ -519,48 +517,52 @@ dsl_bookmark_create_sync_impl_book(
/* Bookmarks must be on the same dataset */
VERIFY3U(bmark_fs_source->ds_object, ==, bmark_fs_new->ds_object);

/* make the copy */
/*
* create a copy of the source bookmark by coping most of its members
*
* Caveat: bookmarking a redaction bookmark yields a normal bookmark
* -----------------------------------------------------------------
* Reasoning:
* - The zbm_redaction_obj would be referred to by both source and new
* bookmark, but would be destroyed once either source or new is
* destroyed, resulting in use-after-free of the referrred object.
* - User expectation when issuing the `zfs bookmark` command is that
* a normal bookmark of the source is created
*
* Design Alternatives For Full Redaction Bookmoark Copying:
* - reference-count the redaction object => would require on-disk
* format change for existing redaction objects
* - Copy the redaction object => cannot be done in syncing context
* because the redaction object might be too large
*/

VERIFY0(dsl_bookmark_lookup_impl(bmark_fs_source, source_shortname,
&source_phys));
dsl_bookmark_node_t *new_dbn = dsl_bookmark_node_alloc(new_shortname);

memcpy(&new_dbn->dbn_phys, &source_phys, sizeof (source_phys));
source_redacted = new_dbn->dbn_phys.zbm_redaction_obj != 0;
if (source_redacted) {
/*
* FIXME: Design Decision. Options:
* A) copy zbm_redaction_obj and adjust
* new_dbn->dbn_phys.zbm_redaction_obj
* B) use reference counting on zbm_redaction_obj, under
* the assumption that the redaction object is immutable
* if the redaction process has completed (correct?)
* C) Do not support copying redaction bookmarks for now
* D) Cloning a redaction bookmark means creating a
* `v1-style bookmark`, i.e. a bookmark with
* zbm_redaction_obj=0.
* This might be arguable if the user expectation is that the
* `zfs bookmark` subcommand only creates v1-style bookmarks.
* `zfs bookmark fs#redactionbookmark fs#newbookmark`
*
* dsl_bookmark_create_check_impl_book currently returns EINVAL for
* source redaction bookmarks, thereby effecrtively implementing
* option C.
*/
}
new_dbn->dbn_phys.zbm_redaction_obj = 0;

/* update feature flags */
/* update feature counters */
if (new_dbn->dbn_phys.zbm_flags & ZBM_FLAG_HAS_FBN) {
spa_feature_incr(dp->dp_spa,
SPA_FEATURE_BOOKMARK_WRITTEN, tx);
}
if (source_redacted) {
spa_feature_incr(dp->dp_spa,
SPA_FEATURE_REDACTION_BOOKMARKS, tx);
/* SPA_FEATURE_BOOKMARK_V2 inc'ed in dsl_bookmark_node_add */
}
/* no need for redaction bookmark counter; nulled zbm_redaction_obj */
/* dsl_bookmark_node_add bumps bookmarks and v2-bookmarks counter */

/*
* write new bookmark
*
* Note that dsl_bookmark_lookup_impl guarantees that, if source is a
* v1 bookmark, the v2-only fields are zeroed.
* And dsl_bookmark_node_add writes back a v1-sized bookmark if
* v2 bookmarks are disabled and/or v2-only fields are zeroed.
* => bookmark copying works on pre-bookmark-v2 pools
*/
dsl_bookmark_node_add(bmark_fs_new, new_dbn, tx);

spa_history_log_internal_ds(bmark_fs_source, "bookmark_clone", tx,
spa_history_log_internal_ds(bmark_fs_source, "bookmark", tx,
"name=%s creation_txg=%llu source_guid=%llu",
new_shortname, (longlong_t)new_dbn->dbn_phys.zbm_creation_txg,
(longlong_t)source_phys.zbm_guid);
@@ -159,7 +159,7 @@ log_mustnot zfs bookmark "#" ""
log_mustnot zfs bookmark "" "#"
log_mustnot zfs bookmark "" ""

# Verify that copied 'normal' bookmarks are independent of the original bookmark
# Verify that copied 'normal' bookmarks are independent of the source bookmark
log_must zfs bookmark "$DATASET#$TESTBM" "$DATASET#$TESTBMCOPY"
log_must zfs destroy "$DATASET#$TESTBM"
log_must eval "zfs send $DATASET@$TESTSNAP > $TEST_BASE_DIR/zfstest_datastream.$$"
@@ -173,7 +173,7 @@ log_must eval "destroy_dataset $DATASET@$TESTSNAP2"
log_must zfs destroy "$DATASET#$TESTBMCOPY"
log_must zfs bookmark "$DATASET@$TESTSNAP" "$DATASET#$TESTBM"

# Verify that copied redaction bookmarks are independent of the original bookmark
# Verify that copied redaction bookmarks are independent of the source bookmark
## create redaction bookmark
log_must zfs destroy "$DATASET#$TESTBM"
log_must zfs destroy "$DATASET@$TESTSNAP"
@@ -183,20 +183,30 @@ log_must eval "echo redacted > $TESTDIR/secret"
log_must zfs snapshot "$DATASET@$TESTSNAP2" # TESTSNAP2 is the redaction snapshot
log_must zfs list -t all -o name,createtxg,guid,mountpoint,written
log_must zfs redact "$DATASET@$TESTSNAP" "$TESTBM" "$DATASET@$TESTSNAP2"
## copy the redaction bookmark, destroy original
# ensure our primitive for testing whether a bookmark is a redaction bookmark works
log_must eval "zfs get all $DATASET#$TESTBM | grep redact_snaps"
## copy the redaction bookmark
log_must zfs bookmark "$DATASET#$TESTBM" "#$TESTBMCOPY"
## if FUTURE_WORK; then # try to use the copy even after the source bookmark is destroyed
# log_must zfs destroy "$DATASET#$TESTBM"
# log_must eval "zfs send --redact "$TESTBMCOPY" $DATASET@$TESTSNAP > $TEST_BASE_DIR/zfstest_datastream.$$"
# log_must eval "destroy_dataset $TESTPOOL/$TESTFS/recv"
# log_must eval "zfs recv -o mountpoint=none $TESTPOOL/$TESTFS/recv < $TEST_BASE_DIR/zfstest_datastream.$$"
# log_must eval "zfs send -i @$TESTSNAP $DATASET@$TESTSNAP2 > $TEST_BASE_DIR/zfstest_datastream.$$"
# log_must eval "zfs recv $TESTPOOL/$TESTFS/recv < $TEST_BASE_DIR/zfstest_datastream.$$"
## else # make sure the copy is not a redaction bookmark
log_must eval "zfs send --redact "$TESTBMCOPY" -i $DATASET@$TESTSNAP $DATASET@$TESTSNAP2 2>&1 | head -n 100 | grep 'internal error: Invalid argument'"
log_mustnot eval "zfs get all $DATASET#$TESTBMCOPY | grep redact_snaps"
# try the above again after destroying the source bookmark, preventive measure for future work
log_must zfs destroy "$DATASET#$TESTBM"
log_must eval 'echo foo | grep bar'
## try to use the copy
log_must eval "zfs send --redact "$TESTBM" $DATASET@$TESTSNAP > $TEST_BASE_DIR/zfstest_datastream.$$"
log_must eval "destroy_dataset $TESTPOOL/$TESTFS/recv"
log_must eval "zfs recv -o mountpoint=none $TESTPOOL/$TESTFS/recv < $TEST_BASE_DIR/zfstest_datastream.$$"
log_must eval "zfs send -i @$TESTSNAP $DATASET@$TESTSNAP2 > $TEST_BASE_DIR/zfstest_datastream.$$"
log_must eval "zfs recv $TESTPOOL/$TESTFS/recv < $TEST_BASE_DIR/zfstest_datastream.$$"
log_must eval "zfs send --redact "$TESTBMCOPY" -i $DATASET@$TESTSNAP $DATASET@$TESTSNAP2 2>&1 | head -n 100 | grep 'internal error: Invalid argument'"
log_mustnot eval "zfs get all $DATASET#$TESTBMCOPY | grep redact_snaps"
## fi
## cleanup
log_must eval "destroy_dataset $DATASET@$TESTSNAP2"
log_must zfs destroy "$DATASET#$TESTBMCOPY"
log_must eval "destroy_dataset $DATASET@$TESTSNAP"
log_must zfs snapshot "$DATASET@$TESTSNAP"
log_must zfs bookmark "$DATASET@$TESTSNAP" "$DATASET#$TESTBM"

log_pass "'zfs bookmark' works as expected only when passed valid arguments."
log_pass "'zfs bookmark' works as expected"

0 comments on commit fa99694

Please sign in to comment.
You can’t perform that action at this time.