[TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. #123

satbirchhikara · 2018-09-27T07:14:56Z

This PR is to flush all outstanding IOs before create snapshot on rebuild clone. This way we are making sure that all IOs will be flushed to clone and we are not missing any IO.
Signed-off-by: satbir [email protected]

Signed-off-by: satbir <[email protected]>

mynktl · 2018-09-27T07:51:29Z

lib/libzrepl/data_conn.c

+#endif
+			while (1) {
+				if (!zinfo->quiesce_done)
+					sleep(1);


I have a doubt. If replica gets disconnected from the target during rebuilding and quiescing was requested then will this thread exit?

current zfs takes care that - if either mgmt or data conn breaks, any rebuilding related threads also will be exited.
With current quiescing code also, I don't think there will be any issue, except that, it may be delayed to break rebuild threads.

Yes, I is fixed now.

Signed-off-by: satbir <[email protected]>

vishnuitta · 2018-09-27T09:09:10Z

lib/libzrepl/data_conn.c


 		/* IO to clone should be sent only when it is from app */
-		if (!is_rebuild && (zinfo->clone_zv != NULL)) {
+		if (!is_rebuild && !ZVOL_IS_HEALTHY(zinfo->main_zv) &&
+		    (zinfo->clone_zv != NULL)) {


we can remove check for clone_zv

vishnuitta · 2018-09-27T09:10:47Z

lib/libzrepl/data_conn.c

@@ -334,6 +341,12 @@ uzfs_zvol_worker(void *arg)
 				}
 			}

+			/* App IOs should go to cloen_zv */


we can make this as 'else' for above 'if' without NULL check for clone_zv

vishnuitta · 2018-09-27T09:43:40Z

lib/libzrepl/data_conn.c

+			if (inject_error.delay.rebuid_io_quiesce_check_by_pass)
+				continue;
+#endif
+			while (1) {


this may need to be executed only in the thread where we had set these flags

I think after discussion this code have no side effect.

Signed-off-by: satbir <[email protected]>

pawanpraka1 · 2018-09-27T10:53:08Z

lib/libzrepl/data_conn.c

+			if (!rebuild_cmd_req &&
+			    !ZVOL_IS_HEALTHY(zinfo->main_zv) &&
+			    (zinfo->clone_zv != NULL))
+				read_zv = zinfo->clone_zv;


we should do the same for the sync also (ZVOL_OPCODE_SYNC) ?

Signed-off-by: satbir <[email protected]>

vishnuitta · 2018-09-27T12:50:54Z

lib/libzpool/taskq.c

@@ -213,6 +213,19 @@ taskq_wait_outstanding(taskq_t *tq, taskqid_t id)
 	taskq_wait(tq);
 }

+int


comment related to function

Sir I think this is not latest code. ....I have renamed it to taskq_check_active_ios(). Please do let me know if you meant else

just few lines of comment explaining the i/p and o/p of this function

vishnuitta · 2018-09-27T12:55:36Z

include/zrepl_prot.h

@@ -169,6 +169,7 @@ enum zvol_status {

 typedef enum zvol_status zvol_status_t;

+#define	ZVOL_IS_HEALTHY(zv) (zv->zv_status == ZVOL_STATUS_HEALTHY)


lets us uzfs_zinfo_get_status(zinfo) getter function

There are some more macros which are also not as per guidelines.
#define ZINFO_IS_DEGRADED(zinfo) (ZVOL_IS_DEGRADED(zinfo->main_zv))
#define ZVOL_IS_DEGRADED(zv) (zv->zv_status == ZVOL_STATUS_DEGRADED)
Let me fix them with next PR. This PR seems to be little heavy weight.

pawanpraka1 · 2018-09-27T13:04:53Z

lib/libzrepl/data_conn.c

@@ -348,6 +356,8 @@ uzfs_zvol_worker(void *arg)

 		case ZVOL_OPCODE_SYNC:
 			uzfs_flush_data(zinfo->main_zv);
+			if (zinfo->clone_zv)


lets check !ZVOL_IS_HEALTHY(zinfo->main_zv) here also instead of checking for zinfo->clone_zv).

pawanpraka1 · 2018-09-27T13:10:07Z

lib/libzrepl/data_conn.c

+				if (zinfo->quiesce_done ||
+				    !taskq_check_active_ios(
+				    zinfo->uzfs_zvol_taskq)) {
+					zinfo->quiesce_done = 1;


do we need to set zinfo->quiesce_done to one? if condition already make sure of it.

never mind, saw the || condition. looks ok.

vishnuitta · 2018-09-27T13:19:31Z

include/zrepl_mgmt.h

@@ -158,6 +158,11 @@ typedef struct zvol_info_s {

 	/* Will be used to singal ack-sender to exit */
 	uint8_t		conn_closed;
+
+	/* Rebuild flags to quiesce IOs */
+	uint8_t		quiesce_requested;


can we use these two as bits in 'flags' variable?

and create setter/getter macros around these variables

ok.. we can't set them as bits, as there is no lock in reading these variables

pawanpraka1 · 2018-09-27T13:59:16Z

lib/libzrepl/data_conn.c

@@ -348,6 +356,8 @@ uzfs_zvol_worker(void *arg)

 		case ZVOL_OPCODE_SYNC:
 			uzfs_flush_data(zinfo->main_zv);


also we should flush to main_zv if (ZVOL_IS_REBUILDING_AFS(zinfo->main_zv) || ZVOL_IS_HEALTHY(zinfo->main_zv).

… TA3151 Signed-off-by: Satbir <[email protected]>

codecov-io · 2018-09-27T15:23:01Z

Codecov Report

Merging #123 into zfs-0.7-release will decrease coverage by 0.44%.
The diff coverage is 80%.

@@                 Coverage Diff                 @@
##           zfs-0.7-release     #123      +/-   ##
===================================================
- Coverage             52.4%   51.96%   -0.45%     
===================================================
  Files                  241      241              
  Lines                77442    77437       -5     
===================================================
- Hits                 40587    40240     -347     
- Misses               36855    37197     +342

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1086c95...8d7e1d0. Read the comment docs.

vishnuitta

changes looks good

Flush outstanding IOs before taking snapshot on rebuild clone.

0b95fd2

Signed-off-by: satbir <[email protected]>

satbirchhikara requested review from pawanpraka1, vishnuitta and mynktl September 27, 2018 07:14

satbirchhikara changed the title ~~Flush outstanding IOs before taking snapshot on rebuild clone.~~ [TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. Sep 27, 2018

mynktl requested changes Sep 27, 2018

View reviewed changes

Read should happen on dataset based on zinfo state.

acff067

Signed-off-by: satbir <[email protected]>

vishnuitta reviewed Sep 27, 2018

View reviewed changes

Addressed some of the comments

03b762c

Signed-off-by: satbir <[email protected]>

pawanpraka1 reviewed Sep 27, 2018

View reviewed changes

mynktl previously approved these changes Sep 27, 2018

View reviewed changes

Addressed few more review comments.

963ad74

Signed-off-by: satbir <[email protected]>

satbirchhikara dismissed mynktl’s stale review via 963ad74 September 27, 2018 11:49

Sync should happen on clone_zv as well if it is non-null

bdd090b

Signed-off-by: satbir <[email protected]>

pawanpraka1 mentioned this pull request Sep 27, 2018

[TA2546] feat(snaprebuild) IOs on the cloned volume if it exists #117

Closed

vishnuitta reviewed Sep 27, 2018

View reviewed changes

pawanpraka1 reviewed Sep 27, 2018

View reviewed changes

vishnuitta reviewed Sep 27, 2018

View reviewed changes

pawanpraka1 reviewed Sep 27, 2018

View reviewed changes

Merge branch 'zfs-0.7-release' of https://github.com/openebs/zfs into…

8d7e1d0

… TA3151 Signed-off-by: Satbir <[email protected]>

vishnuitta approved these changes Sep 27, 2018

View reviewed changes

vishnuitta merged commit d0f0edd into mayadata-io:zfs-0.7-release Sep 27, 2018

satbirchhikara deleted the TA3151 branch September 27, 2018 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. #123

[TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. #123

satbirchhikara commented Sep 27, 2018 •

edited

Loading

mynktl Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018 •

edited

Loading

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

pawanpraka1 Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

pawanpraka1 Sep 27, 2018

satbirchhikara Sep 27, 2018

pawanpraka1 Sep 27, 2018

pawanpraka1 Sep 27, 2018

satbirchhikara Sep 27, 2018

vishnuitta Sep 27, 2018

vishnuitta Sep 27, 2018

vishnuitta Sep 27, 2018

satbirchhikara Sep 27, 2018

pawanpraka1 Sep 27, 2018

satbirchhikara Sep 27, 2018

codecov-io commented Sep 27, 2018

vishnuitta left a comment

		@@ -169,6 +169,7 @@ enum zvol_status {

		typedef enum zvol_status zvol_status_t;

		#define ZVOL_IS_HEALTHY(zv) (zv->zv_status == ZVOL_STATUS_HEALTHY)

		@@ -348,6 +356,8 @@ uzfs_zvol_worker(void *arg)

		case ZVOL_OPCODE_SYNC:
		uzfs_flush_data(zinfo->main_zv);

[TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. #123

[TA3151]feat(snap_rebuild)Flush outstanding IOs before taking snapshot on rebuild clone. #123

Conversation

satbirchhikara commented Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 27, 2018

Codecov Report

vishnuitta left a comment

Choose a reason for hiding this comment

satbirchhikara commented Sep 27, 2018 •

edited

Loading

vishnuitta Sep 27, 2018 •

edited

Loading