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
[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -537,6 +537,11 @@ uzfs_zvol_destroy_snapshot_clone(zvol_state_t *zv, zvol_state_t *snap_zv, | |
int ret1 = 0; | ||
char *clonename; | ||
|
||
if (snap_zv == NULL) { | ||
VERIFY(clone_zv != NULL); | ||
return (0); | ||
} | ||
|
||
clonename = kmem_asprintf("%s/%s_%s", spa_name(zv->zv_spa), | ||
strchr(zv->zv_name, '/') + 1, | ||
REBUILD_SNAPSHOT_CLONENAME); | ||
|
@@ -545,12 +550,16 @@ uzfs_zvol_destroy_snapshot_clone(zvol_state_t *zv, zvol_state_t *snap_zv, | |
clone_zv->zv_name, clonename, zv->zv_name); | ||
|
||
/* Destroy clone's snapshot */ | ||
ret = uzfs_destroy_internal_all_snap(clone_zv); | ||
ret = uzfs_destroy_all_internal_snapshots(clone_zv); | ||
if (ret != 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should break if ret is nonzero. Any reason why we are not breaking from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant - return if ret is nonzero There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. it should return to caller if |
||
LOG_ERR("Rebuild_clone snap destroy failed on:%s" | ||
" with err:%d", clone_zv->zv_name, ret); | ||
" with err:%d", zv->zv_name, ret); | ||
} | ||
|
||
/* | ||
* We need to release the snapshot zv so that next hold | ||
* on dataset doesn't fail | ||
*/ | ||
uzfs_zvol_release_internal_clone(zv, snap_zv, clone_zv); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if clone_zv is NULL, there can be panic in this release_internal_clone. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all the callers of uzfs_zvol_release_internal_clone are setting snapshot_zv and clone_zv to NULL. So, release need to happen. Lets add this as comment for later usage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even if all snapshots deletion fails, we need to release and exit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
|
||
// try_clone_delete_again: | ||
|
@@ -607,3 +616,54 @@ uzfs_zinfo_destroy_internal_clone(zvol_info_t *zinfo) | |
clone_zv); | ||
return (ret); | ||
} | ||
|
||
/* | ||
* This API is used to delete stale | ||
* cloned volume and backing snapshot. | ||
*/ | ||
int | ||
uzfs_zinfo_destroy_stale_clone(zvol_info_t *zinfo) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if open_dataset or hold_dataset of clone fails, its better to retry next time by releasing snap_zv and returning error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. |
||
{ | ||
int ret = 0; | ||
char *clone_subname = NULL; | ||
zvol_state_t *l_snap_zv = NULL, *l_clone_zv = NULL; | ||
zvol_state_t *zv; | ||
|
||
if (!zinfo->main_zv) | ||
return (0); | ||
|
||
zv = zinfo->main_zv; | ||
|
||
ret = get_snapshot_zv(zv, REBUILD_SNAPSHOT_SNAPNAME, | ||
&l_snap_zv, B_FALSE, B_TRUE); | ||
if (ret != 0) { | ||
LOG_ERR("Failed to get info about %s@%s", | ||
zv->zv_name, REBUILD_SNAPSHOT_SNAPNAME); | ||
return (ret); | ||
} | ||
|
||
clone_subname = kmem_asprintf("%s_%s", strchr(zv->zv_name, '/') + 1, | ||
REBUILD_SNAPSHOT_CLONENAME); | ||
|
||
ret = uzfs_open_dataset(zv->zv_spa, clone_subname, &l_clone_zv); | ||
if (ret == 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should handle error here. Any reason why we are not handling error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I will update this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 'else' of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
/* | ||
* If hold on clone dataset fails then we will | ||
* try to delete the clone after sometime. | ||
*/ | ||
ret = uzfs_hold_dataset(l_clone_zv); | ||
if (ret != 0) { | ||
LOG_ERR("Failed to hold clone: %d", ret); | ||
uzfs_close_dataset(l_clone_zv); | ||
uzfs_close_dataset(l_snap_zv); | ||
return (ret); | ||
} | ||
} | ||
|
||
if (!uzfs_zvol_destroy_snapshot_clone(zv, l_snap_zv, l_clone_zv)) | ||
zv->rebuild_info.stale_clone_exist = 0; | ||
|
||
strfree(clone_subname); | ||
|
||
return (ret); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -894,12 +894,21 @@ uzfs_zvol_rebuild_dw_replica(void *arg) | |
close(sfd); | ||
} | ||
|
||
if (wquiesce) | ||
uzfs_zinfo_destroy_internal_clone(zinfo); | ||
#ifdef DEBUG | ||
if (inject_error.delay.rebuild_complete == 1) | ||
sleep(10); | ||
#endif | ||
|
||
if (wquiesce) { | ||
if (uzfs_zinfo_destroy_internal_clone(zinfo)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in else, better to set the stale_clone_exist to 0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. |
||
mutex_enter(&zinfo->main_zv->rebuild_mtx); | ||
zinfo->main_zv->rebuild_info.stale_clone_exist++; | ||
mutex_exit(&zinfo->main_zv->rebuild_mtx); | ||
} | ||
} | ||
|
||
/* Parent thread have taken refcount, drop it now */ | ||
uzfs_zinfo_drop_refcnt(zinfo); | ||
|
||
zk_thread_exit(); | ||
} | ||
|
||
|
@@ -963,6 +972,10 @@ uzfs_zvol_timer_thread(void) | |
next_check = now + | ||
zinfo->update_ionum_interval; | ||
} | ||
|
||
if (ZVOL_HAS_STALE_CLONE(zinfo->main_zv)) { | ||
uzfs_zinfo_destroy_stale_clone(zinfo); | ||
} | ||
} else if (uzfs_zvol_get_status(zinfo->main_zv) == | ||
ZVOL_STATUS_DEGRADED && | ||
zinfo->main_zv->zv_objset) { | ||
|
@@ -1014,6 +1027,7 @@ uzfs_zvol_timer_thread(void) | |
node_next); | ||
kmem_free(n_zinfo, sizeof (*n_zinfo)); | ||
} | ||
zk_thread_exit(); | ||
} | ||
|
||
/* | ||
|
@@ -1515,7 +1529,6 @@ uzfs_zvol_rebuild_scanner(void *arg) | |
} | ||
if (ZINFO_IS_DEGRADED(zinfo)) | ||
zv = zinfo->clone_zv; | ||
|
||
rc = uzfs_zvol_create_internal_snapshot(zv, | ||
&snap_zv, metadata.io_num); | ||
if (rc != 0) { | ||
|
@@ -1652,6 +1665,7 @@ reinitialize_zv_state(zvol_state_t *zv) | |
|
||
uzfs_zvol_set_status(zv, ZVOL_STATUS_DEGRADED); | ||
uzfs_zvol_set_rebuild_status(zv, ZVOL_REBUILDING_INIT); | ||
zv->rebuild_info.stale_clone_exist = 0; | ||
} | ||
|
||
/* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well, we are using clone_zv and snap_zv.. so, these two can't be NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. In this function, there are no checks regarding non-null of clone and snap zv. Error checking should be added to this function. I will update this function with error checking of all zv.