Replication functionality check-in. #4

satbirchhikara · 2018-03-01T09:14:04Z

Replication functionality.

Description

Replication related changes.

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

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.

jkryl · 2018-03-01T10:26:50Z

cmd/zrepl/zrepl

@@ -0,0 +1,228 @@
+#! /bin/bash


this file should be removed, it is automatically generated by libtool

Thanks Jan. I forgot that .....now done

jkryl · 2018-03-01T11:20:47Z

module/zfs/spa.c

@@ -3505,9 +3505,14 @@ spa_open_common(const char *pool, spa_t **spapp, void *tag, nvlist_t *nvpolicy,
 		mutex_exit(&spa_namespace_lock);
 	}

-	if (firstopen)
+	if (firstopen) {
+#ifndef _UZFS


here uzfs_zvol_create_cb is conditional based on _UZFS but the same function in zfs_ioctl.c is conditional based on _KERNEL. For consistency reasons it looks this one should be _KERNEL too.

Absolutely right. I fixed it. Consistency maintained.

jkryl · 2018-03-01T11:47:31Z

cmd/zrepl/zrepl.c

@@ -0,0 +1,976 @@
+
+#include </usr/include/arpa/inet.h>
+#include </usr/include/netdb.h>


/usr/include should be removed

jkryl · 2018-03-01T12:22:16Z

cmd/zrepl/zrepl.c

+ * Free zio command along with buffer.
+ */
+static void
+zio_cmd_free(zvol_io_cmd_t **cmd, zvol_op_code_t opcode)


isn't opcode stored in cmd?

Opcode is part of cmd->hdr, hdr is allocated from stack then we use hdr info to allocate cmd memory and buffer for read/write of size equal to hdr->len. Once we allocated cmd, we bcopy hdr to cmd->hdr.

ok, but if I understand correctly since hdr is part of cmd (no matter how it was constructed) it contains the opcode and we don't need to pass it explicitly as a parameter to this function.

Good point, I agree with you. I fixed it now.

jkryl · 2018-03-01T12:30:14Z

cmd/zrepl/zrepl.c

+			    hdr->offset, hdr->len, NULL);
+			break;
+
+		case ZVOL_OPCODE_SYNC:


shouldn't here be something what eventually calls zil_commit?

Vitta, has provided zil_commit API very recently. I will call that API here. This was not added because I need to add SYNC command in Replica which I deferred for my next sprint.

jkryl · 2018-03-01T13:25:15Z

cmd/zrepl/zrepl.c

+	/* The event loop */
+	while (1) {
+		int n, i;
+		n = epoll_wait(efd, events, MAXEVENTS, -1);


why do we bother with epoll if all we want to do is read data from one file descriptor?

Yes. I changed it. It was very hard for me to address this comment.

I hope I did not provide a misleading advice in this case :-)

No, absolutely not. It was constructive comment.

jkryl · 2018-03-01T13:28:02Z

cmd/zrepl/zrepl.c

+	}
+
+	strncpy(mgmt_ack.volname, name, strlen(name));
+	mgmt_ack.port = 3232;


the port number should be defined somewhere

jkryl · 2018-03-01T13:37:56Z

cmd/zrepl/zrepl.c

+ * exchanging info like IP add, port etc.
+ */
+static void
+uzfs_zvol_mgmt_thread(void *arg)


do I understand correctly that as time will go we will add more management commands on this connection? Or is the handshake command the only one?

Yup. You are right. There will be many more command, just to name few user/app triggered sync, snap creation, snap rollback/restrore, rebuild etc.......

jkryl · 2018-03-01T13:40:29Z

cmd/zrepl/zrepl.c

+	buf = kmem_alloc(1024, KM_SLEEP);
+	nbytes = fread(buf, sizeof (char), 1024, fp);
+
+	if (nbytes == 0) {


I think this should be nbytes <= 0

Absolutely. Done 👍

jkryl · 2018-03-01T13:54:58Z

cmd/zrepl/zrepl.c

+ * and spawn a new thread for each new connection req.
+ */
+static void
+uzfs_zvol_io_conn_acceptor(void)


I see a problem when this thread terminates for whatever reason it can be. the main thread does not know about it and the program will continue to run but will not accept any new connections. I think it would have been better if acceptor was run on behalf on the main thread instead of dedicating a separate thread to it, which makes the synchronization more difficult.

Jan, this is by design, we know that if uzfs_zvol_io_conn_acceptor thread die down because of any reason, replica will not be able to accept new IO connections. Same goes with uzfs_zvol_mgmt_thread. I would discuss it with team here and get back to you. I am keeping this as it is for now until we have better way of doing things.

One simple approach would be to kill main replica process if any of uzfs_zvol_mgmt_thread or uzfs_zvol_io_conn_acceptor goes down. That way container will reboot system back.

satbirchhikara

Jan,

I appreciate efforts and time you dedicated for this review. So many valuable comments which could be potential bugs in production. I am going to push the changes (comments addressed). Please have a look and let me know your views/comments.

Just a small request for time being, I do not want to remove printf/log statements before I push these changes to main repo, so you can ignore for time being. These extra debug statements help in verifying functionality a lot.

Again thanks a lot !!!

satbirchhikara · 2018-03-02T08:54:32Z

cmd/zrepl/zrepl.c

+		}
+
+		ZREPL_LOG("Count:%d Size: %ld\n", count, hdr.len);
+		if (hdr.opcode == ZVOL_OPCODE_HANDSHAKE) {


Yes Jan you are right, thats what our end goal is, for time being (during in house testing) ASSERT had been added "ASSERT(!zinfo->is_io_ack_sender_created)" to catch any such issue where we are sending multiple handshake. Will change code.

satbirchhikara · 2018-03-02T09:02:22Z

cmd/zrepl/zrepl.c

+			}
+			(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
+			continue;
+		}


Thanks Jan. Make sense to test that case. As of today, code will panic if another op_code sent before handshake. As part of handling it, I will turn down any such request/op_code which come before handshake.

satbirchhikara · 2018-03-02T09:12:59Z

cmd/zrepl/zrepl.c

+	}
+
+	strncpy(mgmt_ack.volname, name, strlen(name));
+	mgmt_ack.port = 3232;


satbirchhikara · 2018-03-02T09:16:44Z

cmd/zrepl/zrepl.c

+	buf = kmem_alloc(1024, KM_SLEEP);
+	nbytes = fread(buf, sizeof (char), 1024, fp);
+
+	if (nbytes == 0) {


Absolutely. Done 👍

satbirchhikara · 2018-03-02T09:22:21Z

cmd/zrepl/zrepl.c

+ * exchanging info like IP add, port etc.
+ */
+static void
+uzfs_zvol_mgmt_thread(void *arg)


Yup. You are right. There will be many more command, just to name few user/app triggered sync, snap creation, snap rollback/restrore, rebuild etc.......

satbirchhikara · 2018-03-02T13:41:27Z

cmd/zrepl/zrepl.c

+
+	zfs_arc_max = (512 << 20);
+	zfs_arc_min = (256 << 20);
+	printf("zarcmax: %lu zarcmin:%lu\n", zfs_arc_max, zfs_arc_min);


Jan to be candid, I have thought abt these values. I have inherited this code from one of file in uzfs. Will see what would be optimal value that we can place here. As of now I am adding TODO comments here.

satbirchhikara · 2018-03-02T13:47:27Z

cmd/zrepl/zrepl.c

+	}
+
+	uzfs_zrepl_walk_pool_directory();
+	sleep(5);


Just a random value. This delays is mainly because I want to load/open all datasets incore before someone start establishing connection and pumping data to it. We do little heavy task
loading/opening data set, which may take longer time. I know 5Sec is too much but since this is one time delay which is in boot path so I think it is okay to have some delay(if not 5sec). Please thoughts around ......

I have added this based on UT where I was starting replica and at same time started tgt, where tgt was little faster and it requested LUN/VOL which exist but replica unable to found it because replica was still busy loading/opening it.

satbirchhikara · 2018-03-02T13:51:17Z

cmd/zrepl/zrepl.c

+
+	while (1) {
+		sleep(5);
+	}


I agree with you. Focus was mainly on connections and IOs. These things like memory limit, signal handling etc ...left as it is. But these are equally important. Will fix these issues via separate bug.

satbirchhikara · 2018-03-02T13:54:38Z

cmd/zrepl/zrepl.c

+initialize_error:
+	uzfs_zrepl_close_log();
+	uzfs_fini();
+	return (0);


satbirchhikara · 2018-03-02T13:58:41Z

module/zfs/spa.c

@@ -3505,9 +3505,14 @@ spa_open_common(const char *pool, spa_t **spapp, void *tag, nvlist_t *nvpolicy,
 		mutex_exit(&spa_namespace_lock);
 	}

-	if (firstopen)
+	if (firstopen) {
+#ifndef _UZFS


Absolutely right. I fixed it. Consistency maintained.

jkryl

Hi Satbir, thank you for addressing review comments from previous round. I'm fine with the changes. Just one thing which I would appreciate as writer of fio engine (and iscsit could benefit from it as well): if we could have one include file which does not depend on any other non-standard (zfs) header files which would contain definitions strictly related to over-the-wire protocol (various defines and structures). That way anyone implementing the replica protocol can simply include it and use the definitions. Also to avoid different layout of structures due to compiler differences, all protocol structures should be defined with packed attribute and avoiding types which are architecture dependent. And one nit, I have noticed void pointer in one of the structures going over the wire - that should not be the case.

jkryl · 2018-03-02T17:05:56Z

cmd/zrepl/zrepl.c

+ * Free zio command along with buffer.
+ */
+static void
+zio_cmd_free(zvol_io_cmd_t **cmd, zvol_op_code_t opcode)


ok, but if I understand correctly since hdr is part of cmd (no matter how it was constructed) it contains the opcode and we don't need to pass it explicitly as a parameter to this function.

jkryl · 2018-03-02T17:07:19Z

cmd/zrepl/zrepl.c

+	char *p = buf;
+	ZREPL_ERRLOG("Trying to read nbytes: %lu\n", nbytes);
+	while (nbytes) {
+		count = read(fd, (void *)p, nbytes);


if it is blocking read then I think that the code looks good as it is

jkryl · 2018-03-02T17:07:34Z

cmd/zrepl/zrepl.c

+			    hdr->offset, hdr->len, NULL);
+			break;
+
+		case ZVOL_OPCODE_SYNC:


jkryl · 2018-03-02T17:11:58Z

cmd/zrepl/zrepl.c

+	/* The event loop */
+	while (1) {
+		int n, i;
+		n = epoll_wait(efd, events, MAXEVENTS, -1);


I hope I did not provide a misleading advice in this case :-)

jkryl · 2018-03-02T17:12:59Z

cmd/zrepl/zrepl.c

+ * and spawn a new thread for each new connection req.
+ */
+static void
+uzfs_zvol_io_conn_acceptor(void)


jkryl · 2018-03-02T17:13:40Z

cmd/zrepl/zrepl.c

+
+	zfs_arc_max = (512 << 20);
+	zfs_arc_min = (256 << 20);
+	printf("zarcmax: %lu zarcmin:%lu\n", zfs_arc_max, zfs_arc_min);


jkryl · 2018-03-02T17:16:06Z

cmd/zrepl/zrepl.c

+	}
+
+	uzfs_zrepl_walk_pool_directory();
+	sleep(5);


Thanks for explaining. I think it would be good to add a brief comment explaining the reason here.

jkryl · 2018-03-02T17:16:35Z

cmd/zrepl/zrepl.c

+
+	while (1) {
+		sleep(5);
+	}


satbirchhikara

Thanks Jan for your comments. This reply is specific to comments that you have sent recently, there are few action items pending from my side(which I need to discuss with team today and get back to you ....ofcourse with fix if needed) from your earlier comments.

Regarding sending void pointer over wire: To be candid I do not know implications of sending void pointer over wire though I googled but did not found useful. Would be great if you share some thoughts around it. Other thing is that this pointer is sent by iSCSI tgt controller, after receiving ack from replica, controller need to know in which cmd wait queue it need to look for to send ack back to client. So all said, this pointer is pointing to one of the queue at controller side. I will see if I can inherit definition to make this pointer explicit but if there is dependency of "Inclusion" then I would love to leave it as it is(unless there is some big issue with sending void pointer over wire).
W.r.t your suggestion about replica code to be independent, I tried my best of doing so but there may be glitch because this replica code is splitted into two parts a) Loading zinfo during mount path/destroying zinfo during export b) Steady state IO acceptance and acknowledgement. So part "b" is something I would say independent.

satbirchhikara · 2018-03-05T03:47:31Z

cmd/zrepl/zrepl.c

+ * Free zio command along with buffer.
+ */
+static void
+zio_cmd_free(zvol_io_cmd_t **cmd, zvol_op_code_t opcode)


Good point, I agree with you. I fixed it now.

satbirchhikara · 2018-03-05T04:02:20Z

cmd/zrepl/zrepl.c

+	/* The event loop */
+	while (1) {
+		int n, i;
+		n = epoll_wait(efd, events, MAXEVENTS, -1);


No, absolutely not. It was constructive comment.

vishnuitta · 2018-03-05T13:03:43Z

cmd/zrepl/zrepl.c

+		if ((hdr.opcode == ZVOL_OPCODE_WRITE) ||
+		    (hdr.opcode == ZVOL_OPCODE_HANDSHAKE)) {
+			count = uzfs_zvol_socket_read(fd, zio_cmd->buf,
+			    (sizeof (char) * hdr.len));


sizeof (char) can be removed

Yes, it can be removed but IMHO it is not a good practice. Moreover there is no performance impact of it. Compiler replace it with 1 and since compiler know that multiplication with 1 yield nothing so compiler keep hdr.len and discard other stuff. It make code little readable.

vishnuitta · 2018-03-05T13:25:40Z

cmd/zrepl/zrepl.c

+				zinfo->io_ack_waiting = 0;
+				if ((zinfo->state == ZVOL_INFO_STATE_OFFLINE) ||
+				    (zinfo->conn_closed == true)) {
+					(void) pthread_mutex_unlock(


this unlock can be moved to the end of 'exit'

As of today we have 2 exit in this function and in one the exit path we have lock held and in other exit path we do not have lock. Considering that we can not make it generic to release lock at exit. W.r.t to correctness/race, this thread would not be here unless Producer set conn_closed to true so we are okay to free zio_cmd without lock protection but if you insist I can take lock again and free zio_cmd(Completion Queue) here.

vishnuitta · 2018-03-05T14:05:30Z

cmd/zrepl/zrepl.c

+	char *buf = NULL;
+
+
+	sfd = create_and_bind(mgmt_port);


'bind' is not required.

I can understand we do not need to explicitly bind this since connect anyway bind socket before establishing connection to iSCSI controller.

vishnuitta · 2018-03-05T14:34:21Z

cmd/zrepl/zrepl.c

+		goto exit;
+	}
+
+	packet = kmem_alloc((sizeof (mgmt_ack_t) + sizeof (*hdr)) *


packet need to be freed.

👍 ....done

vishnuitta · 2018-03-05T14:43:35Z

lib/libzpool/uzfs_mgmt.c

@@ -385,6 +386,101 @@ uzfs_create_dataset(spa_t *spa, char *ds_name, uint64_t vol_size,
 	return (error);
 }

+/* uZFS Zvol create call back function */
+int
+uzfs_zvol_create_cb(const char *ds_name, void *arg)


this seems to be duplicated code, can be merged with other function.

True. The other API which we are talking about has been developed very recently and it will take time for me to soak those change. Since this code has wider impact on export/create/destroy i.e we have to take care of refcount increments and decrements on dataset as part of create and destroy respectively and since this code is already tested and very much stable, I am little apprehensive to merge both of them right away. Will open an issue and fix it. I hope that is fine.

satbirchhikara

Hi Vitta,

I honestly tried to address your comments. I would like to treat merging comments through separate issue. I hope that is fine. Please have a look and let me know.

Thanks & Regards,
Satbir Singh

satbirchhikara · 2018-03-06T04:52:47Z

cmd/zrepl/zrepl.c

+		if ((hdr.opcode == ZVOL_OPCODE_WRITE) ||
+		    (hdr.opcode == ZVOL_OPCODE_HANDSHAKE)) {
+			count = uzfs_zvol_socket_read(fd, zio_cmd->buf,
+			    (sizeof (char) * hdr.len));


Yes, it can be removed but IMHO it is not a good practice. Moreover there is no performance impact of it. Compiler replace it with 1 and since compiler know that multiplication with 1 yield nothing so compiler keep hdr.len and discard other stuff. It make code little readable.

satbirchhikara · 2018-03-06T04:54:15Z

cmd/zrepl/zrepl.c

+		goto exit;
+	}
+
+	packet = kmem_alloc((sizeof (mgmt_ack_t) + sizeof (*hdr)) *


👍 ....done

satbirchhikara · 2018-03-06T05:05:19Z

cmd/zrepl/zrepl.c

+	char *buf = NULL;
+
+
+	sfd = create_and_bind(mgmt_port);


I can understand we do not need to explicitly bind this since connect anyway bind socket before establishing connection to iSCSI controller.

satbirchhikara · 2018-03-06T05:13:08Z

cmd/zrepl/zrepl.c

+				zinfo->io_ack_waiting = 0;
+				if ((zinfo->state == ZVOL_INFO_STATE_OFFLINE) ||
+				    (zinfo->conn_closed == true)) {
+					(void) pthread_mutex_unlock(


As of today we have 2 exit in this function and in one the exit path we have lock held and in other exit path we do not have lock. Considering that we can not make it generic to release lock at exit. W.r.t to correctness/race, this thread would not be here unless Producer set conn_closed to true so we are okay to free zio_cmd without lock protection but if you insist I can take lock again and free zio_cmd(Completion Queue) here.

satbirchhikara · 2018-03-06T05:28:04Z

lib/libzpool/uzfs_mgmt.c

@@ -385,6 +386,101 @@ uzfs_create_dataset(spa_t *spa, char *ds_name, uint64_t vol_size,
 	return (error);
 }

+/* uZFS Zvol create call back function */
+int
+uzfs_zvol_create_cb(const char *ds_name, void *arg)


True. The other API which we are talking about has been developed very recently and it will take time for me to soak those change. Since this code has wider impact on export/create/destroy i.e we have to take care of refcount increments and decrements on dataset as part of create and destroy respectively and since this code is already tested and very much stable, I am little apprehensive to merge both of them right away. Will open an issue and fix it. I hope that is fine.

vishnuitta

changes looks good.

… replication

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

[ZoL#62] Replication functionality check-in Signed-off-by: satbir <[email protected]>

satbirchhikara added 2 commits March 1, 2018 14:38

Replication functionality check-in.

595e1b8

Travis issue fixed.

b738c36

jkryl reviewed Mar 1, 2018

View reviewed changes

Removing "cmd/zrepl/zrepl" file.

46f3197

satbirchhikara commented Mar 2, 2018

View reviewed changes

Fixed review comments from Jan.

62b5466

jkryl reviewed Mar 2, 2018

View reviewed changes

satbirchhikara commented Mar 5, 2018

View reviewed changes

satbirchhikara added 2 commits March 5, 2018 10:10

zio_cmd_free should not have opcode passed explici.

ee24979

Added SIGPIPE singal handling.

db34311

jkryl approved these changes Mar 5, 2018

View reviewed changes

vishnuitta reviewed Mar 5, 2018

View reviewed changes

satbirchhikara commented Mar 6, 2018

View reviewed changes

Make bind optional for client.

6eb7f42

vishnuitta approved these changes Mar 6, 2018

View reviewed changes

satbirchhikara and others added 9 commits March 6, 2018 13:58

Refcount drop need to be done before check.

6403b34

Merge branch 'zfs-0.7-release' into replication

7b6d316

Resolving compilation which happened because of merge

a52c788

Resolved conflicts

82cf27a

mgmt_ack need to be initialized properly

4ffb1a0

Code and test script has been changed so that test passed.

6e86140

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

970046b

… replication

Sycned and resolved some code merge issues.

c84d346

Resolved merge issue

2403f53

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

vishnuitta merged commit 1d06753 into mayadata-io:zfs-0.7-release Mar 7, 2018

jkryl pushed a commit that referenced this pull request Jun 26, 2018

Replication functionality check-in. (#4)

2bbf2cc

[ZoL#62] Replication functionality check-in Signed-off-by: satbir <[email protected]>

		@@ -0,0 +1,976 @@

		#include </usr/include/arpa/inet.h>
		#include </usr/include/netdb.h>

Replication functionality check-in. #4

Replication functionality check-in. #4

Conversation

satbirchhikara commented Mar 1, 2018

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

satbirchhikara Mar 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satbirchhikara left a 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

jkryl left a 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

satbirchhikara left a 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

satbirchhikara left a 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

vishnuitta left a comment

satbirchhikara Mar 2, 2018 •

edited

Loading