Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API to get condensed tree of modified blocks between two txg #6

Merged
merged 6 commits into from
Mar 7, 2018

Conversation

mynktl
Copy link
Member

@mynktl mynktl commented Mar 2, 2018

Description

API to get condense tree of modified blocks between two txg
Changes in ZVOL ZAP update/read API regarding microzap

Added uzfs_txg_block_diff API, which will return condensed tree of changed blocks
between two txg.
In ZVOL ZAP update/read API, value changed to 64bit unsigned integer instead of char *.

How Has This Been Tested?

Unit test are added to test this change with uzfs_test.
To test this change :

  • ./cmd/uzfs_test/uzfs_test -T 3 -t 10 -n 5
  • ./cmd/uzfs_test/uzfs_test -T 4 -t 10

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

- changes in zvol zap update/read API for microzap checks

Signed-off-by: mayank <[email protected]>
Signed-off-by: mayank <[email protected]>
@mynktl mynktl changed the title - API to get condensed tree of modified blocks between two txg API to get condensed tree of modified blocks between two txg Mar 2, 2018
@@ -61,8 +73,8 @@ uzfs_update_zap_entries(void *zvol, const uzfs_zap_kv_t **array,

for (i = 0; i < count; i++) {
kv = array[i];
VERIFY0(zap_update(os, ZVOL_ZAP_OBJ, kv->key, 1, kv->size,
kv->value, tx));
VERIFY0(zap_update(os, ZVOL_ZAP_OBJ, kv->key, kv->size, 1,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand our kv->size is 8 currently. But, its better to be (.., 1, kv->size,..) @mynktl incase if kv->size becomes greater than 8 later on. Same goes to below also.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept it this way to avoid fat zap upgrade.

@@ -50,6 +51,17 @@ uzfs_update_zap_entries(void *zvol, const uzfs_zap_kv_t **array,
int err;
int i = 0;

/*
* check if key length is greater than MZAP_NAME_LEN.
* key with MZAP_NAME_LEN+ length will convert microzap

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mynktl for these changes, so that, it won't become fat zap. Comment need to be fixed at this line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

};

void
add_to_mblktree(avl_tree_t *tree, uint64_t boffset, uint64_t blen)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why tree called as mblktree..? also, add comments at each logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it contains modified block's details, so i kept it as mblktree. I will add comments at each tree operation/logic.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_to_txg_diff_tree

snprintf(snapname, sizeof (snapname), "%s@%s%llu", zv->zv_name,
TXG_DIFF_SNAPNAME, now);

error = dsl_pool_hold(snapname, FTAG, &dp);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we use dmu_objset_own rather than dsl_pool_hold & dsl_dataset_hold?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we are traversing snapshot only, so we can avoid dmu_objset_own. Regarding snapshot destruction while we are traversing snapshot, we can use dsl_dataset_long_hold.

} wblkinfo_t;

int
del_from_mblktree(avl_tree_t *tree, uint64_t offset, uint64_t len)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operations on tree can go into one file in lib/libzpool/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in production code, we are not using this API. So, i didn't keep it in lib/libzpool. Once we need this, we will move this to lib/libzpool.

Copy link

@vishnuitta vishnuitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes looks good @mynktl .. but, we need to change data structures and functions names. Also, some more unit testing needed to see whether the traversal is returning correct values.



int
uzfs_txg_block_diff(zvol_state_t *zv, uint64_t start_txg, uint64_t end_txg,
Copy link

@vishnuitta vishnuitta Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_get_txg_diff_tree


#define TXG_DIFF_SNAPNAME "tsnap"

struct diff_txg_blk {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_txg_diff_cb_args_t

}

int
uzfs_changed_block_cb(spa_t *spa, zilog_t *zillog, const blkptr_t *bp,
Copy link

@vishnuitta vishnuitta Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_txg_diff_cb

#define TXG_DIFF_SNAPNAME "tsnap"

struct diff_txg_blk {
avl_tree_t *tree;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_txg_diff_tree

}

static int
zvol_blk_off_cmpr(const void *arg1, const void *arg2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_txg_diff_tree_compare

}

void
uzfs_zvol_txg_diff_blk_test(void *arg)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uzfs_txg_diff_test

@mynktl
Copy link
Member Author

mynktl commented Mar 6, 2018

@vishnuitta I have updated PR with review comments.
I have made following changes:

  1. changed functions and data structures name.
  2. changes in functionality test for txg_diff_tree test, increased io_count from 5 to 5-100.
  3. changes in zap_update API regarding zap_entry's value check which was missing earlier.

io_num++;
blk_offset = uzfs_random(vol_blocks - 16);
/*
* make sure offset is aligned to block size
*/
offset = ((blk_offset * blksz + block_size) /

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offset can be just blk_offset * block_size

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, have a test case with less active size (probably for 30K or 50K blocks), so that, collisions/merges of tree can be verified

offset_matched = B_TRUE;
break;
}
if (temp_blk->offset + temp_blk->len ==

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this condition is not required at all

@@ -166,16 +186,37 @@ uzfs_zvol_txg_diff_blk_test(void *arg)

while (i++ < test_iterations) {
count = 0;
max_io = MAX(uzfs_random(100), 5);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it as 10K..

Copy link

@vishnuitta vishnuitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes looks good Mayank. Just take care of those minor comments.

mayank and others added 2 commits March 7, 2018 16:43
	modified blocks between two txg
- fix for active_size/vol_size argument parsing in uzfs_test

Signed-off-by: mayank <[email protected]>
@vishnuitta vishnuitta merged commit 399d4f2 into mayadata-io:zfs-0.7-release Mar 7, 2018
jkryl pushed a commit that referenced this pull request Jun 26, 2018
[CCS-68] travis integration with ZoL
jkryl pushed a commit that referenced this pull request Jun 26, 2018
- API to get condensed tree of modified blocks between two txg
Signed-off-by: mayank <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants