[PATCH] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[PATCH] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[Gris Ge]

Issue:
    When multipathd is starting up, it will reply "timeout\n" immediatly
    when got any IPC command from socket. The expected way is to wait
    uxsock_timeout/1000 seconds.

Root cause:
    pthread_mutex_timedlock() are expecting a CLOCK_REALTIME time.

Fix:
    Use CLOCK_REALTIME for pthread_mutex_timedlock() and
    pthread_cond_timedwait().

Signed-off-by: Gris Ge <fge@redhat.com>
---
 multipathd/cli.c  | 2 +-
 multipathd/main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index 32d49766..002abe61 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -475,7 +475,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 	/*
 	 * execute handler
 	 */
-	if (clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
+	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
 		tmo.tv_sec += timeout;
 	} else {
 		tmo.tv_sec = 0;
diff --git a/multipathd/main.c b/multipathd/main.c
index 4be2c579..67997d08 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -198,7 +198,7 @@ int set_config_state(enum daemon_status state)
 		if (running_state != DAEMON_IDLE) {
 			struct timespec ts;
 
-			clock_gettime(CLOCK_MONOTONIC, &ts);
+			clock_gettime(CLOCK_REALTIME, &ts);
 			ts.tv_sec += 1;
 			rc = pthread_cond_timedwait(&config_cond,
 						    &config_lock, &ts);
-- 
2.14.0

Re: [PATCH] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[Bart Van Assche]

On Wed, 2017-08-09 at 21:22 +0800, Gris Ge wrote:
> diff --git a/multipathd/cli.c b/multipathd/cli.c
> index 32d49766..002abe61 100644
> --- a/multipathd/cli.c
> +++ b/multipathd/cli.c
> @@ -475,7 +475,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
>  	/*
>  	 * execute handler
>  	 */
> -	if (clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
> +	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
>  		tmo.tv_sec += timeout;
>  	} else {
>  		tmo.tv_sec = 0;

Hello Gris,

This is a good catch. Thanks for this fix.

> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4be2c579..67997d08 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -198,7 +198,7 @@ int set_config_state(enum daemon_status state)
>  		if (running_state != DAEMON_IDLE) {
>  			struct timespec ts;
>  
> -			clock_gettime(CLOCK_MONOTONIC, &ts);
> +			clock_gettime(CLOCK_REALTIME, &ts);
>  			ts.tv_sec += 1;
>  			rc = pthread_cond_timedwait(&config_cond,
>  						    &config_lock, &ts);

But this change looks wrong to me. Have you noticed that
pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) is used to make
config_cond use the monotonic clock? See also libmultipath/time-util.c.

Bart.

Re: [PATCH] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[Gris Ge]

[-- Attachment #1.1: Type: text/plain, Size: 913 bytes --]

On Wed, Aug 09, 2017 at 03:03:00PM +0000, Bart Van Assche wrote:
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 4be2c579..67997d08 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -198,7 +198,7 @@ int set_config_state(enum daemon_status state)
> >  		if (running_state != DAEMON_IDLE) {
> >  			struct timespec ts;
> >  
> > -			clock_gettime(CLOCK_MONOTONIC, &ts);
> > +			clock_gettime(CLOCK_REALTIME, &ts);
> >  			ts.tv_sec += 1;
> >  			rc = pthread_cond_timedwait(&config_cond,
> >  						    &config_lock, &ts);
> 
> But this change looks wrong to me. Have you noticed that
> pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) is used to make
> config_cond use the monotonic clock? See also libmultipath/time-util.c.
> 
> Bart.
Hi Bart,

I missed the pthread_cond_init_mono().
Will send V2 patch without this change.

Thanks.

-- 
Gris Ge

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH V2] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[Gris Ge]

Issue:
    When multipathd is starting up, it will reply "timeout\n" immediatly
    when got any IPC command from socket. The expected way is to wait
    uxsock_timeout/1000 seconds.

Root cause:
    pthread_mutex_timedlock() is expecting a CLOCK_REALTIME time.

Fix:
    Use CLOCK_REALTIME for pthread_mutex_timedlock().

Signed-off-by: Gris Ge <fge@redhat.com>
---
 multipathd/cli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index 32d49766..002abe61 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -475,7 +475,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 	/*
 	 * execute handler
 	 */
-	if (clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
+	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
 		tmo.tv_sec += timeout;
 	} else {
 		tmo.tv_sec = 0;
-- 
2.14.0

Re: [PATCH V2] multipathd daemon: Fix incorrect use of CLOCK_MONOTONIC in pthread

[Bart Van Assche]

On Thu, 2017-08-10 at 00:21 +0800, Gris Ge wrote:
> diff --git a/multipathd/cli.c b/multipathd/cli.c
> index 32d49766..002abe61 100644
> --- a/multipathd/cli.c
> +++ b/multipathd/cli.c
> @@ -475,7 +475,7 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
>  	/*
>  	 * execute handler
>  	 */
> -	if (clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
> +	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
>  		tmo.tv_sec += timeout;
>  	} else {
>  		tmo.tv_sec = 0;

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>