Re: sanlock: shutdown reply

[Nir Soffer]

>  commit 93885cbfc6271ecb5007601369ad29cee3df6b99
>  Author: David Teigland <teigland(a)redhat.com&gt;
>  Date:   Fri Apr 11 14:10:25 2014 -0500
>  
>      sanlock: shutdown reply
>      
>      A wait option (-w 1) can be added to sanlock shutdown.
>      The shutdown command will wait for a result from the
>      daemon indicating success (exiting), or failure (not
>      exiting because lockspaces exist).
>       
This option seems useful for vdsm.

>      Signed-off-by: David Teigland <teigland(a)redhat.com&gt;
>  
>  diff --git a/src/client_cmd.c b/src/client_cmd.c
>  index 7723f88..15c26a1 100644
>  --- a/src/client_cmd.c
>  +++ b/src/client_cmd.c
>  @@ -636,15 +636,40 @@ int sanlock_log_dump(int max_size)
>   	return rv;
>   }
>   
>  -int sanlock_shutdown(uint32_t force)
>  +int sanlock_shutdown(uint32_t force, int wait_result)
>   {
>  +	struct sm_header h;
>  +	int cmd;
>   	int fd;
>  +	int rv = 0;
>  +
>  +	if (wait_result)
>  +		cmd = SM_CMD_SHUTDOWN_WAIT;
>  +	else
>  +		cmd = SM_CMD_SHUTDOWN;
>   
>  -	fd = send_command(SM_CMD_SHUTDOWN, force);
>  +	fd = send_command(cmd, force);
>   	if (fd < 0)
>   		return fd; 
What you mean is return -1 - right?

>   
>  +	if (cmd != SM_CMD_SHUTDOWN_WAIT)
>  +		goto out;
>  +
>  +	memset(&h, 0, sizeof(h));
>  +
>  +	rv = recv(fd, &h, sizeof(h), MSG_WAITALL);
>  +	if (rv < 0) {
>  +		rv = -errno;
>  +		goto out;
>  +	}
>  +	if (rv != sizeof(h)) {
>  +		rv = -1;
>  +		goto out;
>  +	}
>  +
>  +	rv = h.data;
>  + out:
>   	close(fd);
>  -	return 0;
>  +	return rv;
>   }
>   
>  diff --git a/src/client_cmd.h b/src/client_cmd.h
>  index 35f82d9..f7c69c1 100644
>  --- a/src/client_cmd.h
>  +++ b/src/client_cmd.h
>  @@ -12,6 +12,6 @@
>   int sanlock_status(int debug, char sort_arg);
>   int sanlock_host_status(int debug, char *lockspace_name);
>   int sanlock_log_dump(int max_size);
>  -int sanlock_shutdown(uint32_t force);
>  +int sanlock_shutdown(uint32_t force, int wait_result);
>   
>   #endif
>  diff --git a/src/cmd.c b/src/cmd.c
>  index f19e8fb..a9a1eef 100644
>  --- a/src/cmd.c
>  +++ b/src/cmd.c
>  @@ -1159,6 +1159,74 @@ static void cmd_get_lvb(struct task *task GNUC_UNUSED,
>  struct cmd_args *ca)
>   	client_resume(ca->ci_in);
>   }
>   
>  +static int shutdown_reply_ci = -1;
>  +static int shutdown_reply_fd = -1;
>  +
>  +static int daemon_shutdown_start(int ci, int fd, int force)
>  +{
>  +	int rv;
>  +
>  +	if (force) {
>  +		shutdown_reply_ci = ci;
>  +		shutdown_reply_fd = fd;
>  +		external_shutdown = 2;
>  +		return 0;
>  +	}
>  +
>  +	pthread_mutex_lock(&spaces_mutex);
>  +	if (list_empty(&spaces) &&
>  +	    list_empty(&spaces_rem) &&
>  +	    list_empty(&spaces_add)) {
>  +		shutdown_reply_ci = ci;
>  +		shutdown_reply_fd = fd;
>  +		external_shutdown = 1;
>  +		rv = 0;
>  +	} else {
>  +		rv = -EBUSY;
>  +	}
>  +	pthread_mutex_unlock(&spaces_mutex);
>  +
>  +	return rv;
>  +}
>  +
>  +static void cmd_shutdown_wait(struct task *task GNUC_UNUSED, struct cmd_args
>  *ca)
>  +{
>  +	int fd, result;
>  +
>  +	fd = client[ca->ci_in].fd;
>  +
>  +	result = daemon_shutdown_start(ca->ci_in, fd, ca->header.data);
>  +
>  +	/*
>  +	 * daemon_shutdown_reply will send the result at the
>  +	 * end of main_loop.
>  +	 */
>  +	if (!result)
>  +		return; 
It will be more clear to do:

    if (result == -EBUSY)
        return;

>  +
>  +	send_result(fd, &ca->header, result);
>  +	client_resume(ca->ci_in);
>  +}
>  +
>  +void daemon_shutdown_reply(void)
>  +{
>  +	struct sm_header h;
>  +
>  +	/* shutdown wait was not used */
>  +	if (shutdown_reply_fd == -1)
>  +		return;
>  +
>  +	memset(&h, 0, sizeof(h));
>  +	h.magic = SM_MAGIC;
>  +	h.version = SM_PROTO;
>  +	h.length = sizeof(h);
>  +
>  +	send(shutdown_reply_fd, &h, sizeof(h), MSG_NOSIGNAL);
>  +	close(shutdown_reply_fd);
>  +
>  +	client_resume(shutdown_reply_ci);
>  +}
>  +
>   static void cmd_add_lockspace(struct cmd_args *ca)
>   {
>   	struct sanlk_lockspace lockspace;
>  @@ -1908,6 +1976,9 @@ void call_cmd_thread(struct task *task, struct cmd_args
>  *ca)
>   	case SM_CMD_GET_LVB:
>   		cmd_get_lvb(task, ca);
>   		break;
>  +	case SM_CMD_SHUTDOWN_WAIT:
>  +		cmd_shutdown_wait(task, ca);
>  +		break;
>   	};
>   }
>   
>  diff --git a/src/cmd.h b/src/cmd.h
>  index 3f31283..30c19e6 100644
>  --- a/src/cmd.h
>  +++ b/src/cmd.h
>  @@ -24,4 +24,6 @@ void call_cmd_thread(struct task *task, struct cmd_args
>  *ca);
>   /* cmds processed by main loop */
>   void call_cmd_daemon(int ci, struct sm_header *h_recv, int client_maxi);
>   
>  +void daemon_shutdown_reply(void);
>  +
>   #endif
>  diff --git a/src/main.c b/src/main.c
>  index 5f3c6ed..dbb6da8 100644
>  --- a/src/main.c
>  +++ b/src/main.c
>  @@ -840,6 +840,8 @@ static int main_loop(void)
>   
>   	free_lockspaces(1);
>   
>  +	daemon_shutdown_reply();
>  +
>   	return 0;
>   }
>   
>  @@ -1194,6 +1196,7 @@ static void process_connection(int ci)
>   	case SM_CMD_READ_RESOURCE_OWNERS:
>   	case SM_CMD_SET_LVB:
>   	case SM_CMD_GET_LVB:
>  +	case SM_CMD_SHUTDOWN_WAIT:
>   		rv = client_suspend(ci);
>   		if (rv < 0)
>   			return;
>  @@ -1814,7 +1817,7 @@ static void print_usage(void)
>   	printf("sanlock client gets [-h 0|1]\n");
>   	printf("sanlock client host_status -s LOCKSPACE [-D]\n");
>   	printf("sanlock client log_dump\n");
>  -	printf("sanlock client shutdown [-f 0|1]\n");
>  +	printf("sanlock client shutdown [-f 0|1] [-w 0|1]\n"); 
For boolean option, it is more user friendly to use -w without a value:

    sanlock client shutdown -w

But it seems that this is already use like this in other commands :-(

>   	printf("sanlock client init -s LOCKSPACE | -r
> RESOURCE\n");
>   	printf("sanlock client read -s LOCKSPACE | -r RESOURCE\n");
>   	printf("sanlock client align -s LOCKSPACE\n");
>  @@ -2047,6 +2050,7 @@ static int read_command_line(int argc, char *argv[])
>   			break;
>   		case 'w':
>   			com.use_watchdog = atoi(optionarg);
>  +			com.wait = atoi(optionarg);
>   			break;
>   		case 'h':
>   			if (com.action == ACT_GETS || com.action == ACT_CLIENT_READ)
>  @@ -2399,8 +2403,8 @@ static int do_client(void)
>   		break;
>   
>   	case ACT_SHUTDOWN:
>  -		log_tool("shutdown");
>  -		rv = sanlock_shutdown(com.force_mode);
>  +		log_tool("shutdown force %d wait %d", com.force_mode, com.wait);
>  +		rv = sanlock_shutdown(com.force_mode, com.wait);
>   		log_tool("shutdown done %d", rv);
>   		break;
>   
>  diff --git a/src/sanlock.8 b/src/sanlock.8
>  index 0d2b63c..7b92329 100644
>  --- a/src/sanlock.8
>  +++ b/src/sanlock.8
>  @@ -273,7 +273,10 @@ Print the sanlock daemon internal debug log.
>   Ask the sanlock daemon to exit.  Without the force option (-f 0), the
>   command will be ignored if any lockspaces exist.  With the force option
>   (-f 1), any registered processes will be killed, their resource leases
>  -released, and lockspaces removed.
>  +released, and lockspaces removed.  With the wait option (-w 1), the
>  +command will wait for a result from the daemon indicating that it has
>  +shut down and is exiting, or cannot shut down because lockspaces
>  +exist (command fails).
>   
>   .BR "sanlock client init -s" " LOCKSPACE"
>   
>  diff --git a/src/sanlock_internal.h b/src/sanlock_internal.h
>  index c3fa574..a62135c 100644
>  --- a/src/sanlock_internal.h
>  +++ b/src/sanlock_internal.h
>  @@ -265,6 +265,7 @@ struct command_line {
>   	int debug;
>   	int debug_renew;
>   	int quiet_fail;
>  +	int wait;
>   	int use_watchdog;
>   	int high_priority;		/* -h */
>   	int get_hosts;			/* -h */
>  diff --git a/src/sanlock_sock.h b/src/sanlock_sock.h
>  index c5c1ec9..8e8f5e2 100644
>  --- a/src/sanlock_sock.h
>  +++ b/src/sanlock_sock.h
>  @@ -47,6 +47,7 @@ enum {
>   	SM_CMD_GET_LVB		 = 26,
>   	SM_CMD_CONVERT		 = 27,
>   	SM_CMD_VERSION		 = 28,
>  +	SM_CMD_SHUTDOWN_WAIT     = 29,
>   };
>   
>   struct sm_header { 
How much time it takes to get a reply from the daemon?

We certainly like this feature in next release, but it is not a must.

Thanks,
Nir