Re: [PATCH 1/1] gssd: move read of upcall into main thread

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 05/05/2016 12:50 PM, Olga Kornievskaia wrote:
> This patch moves reading of the upcall information from the child thread
> into the main thread. It removes the need to synchronize between the
> parent and child thread before processing upcall. Also it creates the thread
> in a detached state.
> 
> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
Committed... 

steved.

> ---
>  utils/gssd/gssd.c      | 93 +++++++++++++++++++++++++++++++++++---------------
>  utils/gssd/gssd.h      | 13 +++++--
>  utils/gssd/gssd_proc.c | 73 ++++++++++++---------------------------
>  3 files changed, 96 insertions(+), 83 deletions(-)
> 
> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> index 9bf7917..2179e15 100644
> --- a/utils/gssd/gssd.c
> +++ b/utils/gssd/gssd.c
> @@ -363,54 +363,91 @@ gssd_destroy_client(struct clnt_info *clp)
>  
>  static void gssd_scan(void);
>  
> -static inline void 
> -wait_for_child_and_detach(pthread_t th)
> +static int
> +start_upcall_thread(void (*func)(struct clnt_upcall_info *), void *info)
>  {
> -	pthread_mutex_lock(&pmutex);
> -	while (!thread_started)
> -		pthread_cond_wait(&pcond, &pmutex);
> -	thread_started = false;
> -	pthread_mutex_unlock(&pmutex);
> -	pthread_detach(th);
> +	pthread_attr_t attr;
> +	pthread_t th;
> +	int ret;
> +
> +	ret = pthread_attr_init(&attr);
> +	if (ret != 0) {
> +		printerr(0, "ERROR: failed to init pthread attr: ret %d: %s\n",
> +			 ret, strerror(errno));
> +		return ret;
> +	}
> +	ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
> +	if (ret != 0) {
> +		printerr(0, "ERROR: failed to create pthread attr: ret %d: "
> +			 "%s\n", ret, strerror(errno));
> +		return ret;
> +	}
> +
> +	ret = pthread_create(&th, &attr, (void *)func, (void *)info);
> +	if (ret != 0)
> +		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> +			 ret, strerror(errno));
> +	return ret;
> +}
> +
> +static struct clnt_upcall_info *alloc_upcall_info(struct clnt_info *clp)
> +{
> +	struct clnt_upcall_info *info;
> +
> +	info = malloc(sizeof(struct clnt_upcall_info));
> +	if (info == NULL)
> +		return NULL;
> +	info->clp = clp;
> +
> +	return info;
>  }
>  
> -/* For each upcall create a thread, detach from the main process so that
> - * resources are released back into the system without the need for a join.
> - * We need to wait for the child thread to start and consume the event from
> - * the file descriptor.
> +/* For each upcall read the upcall info into the buffer, then create a
> + * thread in a detached state so that resources are released back into
> + * the system without the need for a join.
>   */
>  static void
>  gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
>  {
>  	struct clnt_info *clp = data;
> -	pthread_t th;
> -	int ret;
> +	struct clnt_upcall_info *info;
>  
> -	ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
> -				(void *)clp);
> -	if (ret != 0) {
> -		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> -			 ret, strerror(errno));
> +	info = alloc_upcall_info(clp);
> +	if (info == NULL)
> +		return;
> +
> +	info->lbuflen = read(clp->gssd_fd, info->lbuf, sizeof(info->lbuf));
> +	if (info->lbuflen <= 0 || info->lbuf[info->lbuflen-1] != '\n') {
> +		printerr(0, "WARNING: %s: failed reading request\n", __func__);
> +		free(info);
>  		return;
>  	}
> -	wait_for_child_and_detach(th);
> +	info->lbuf[info->lbuflen-1] = 0;
> +
> +	if (start_upcall_thread(handle_gssd_upcall, info))
> +		free(info);
>  }
>  
>  static void
>  gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
>  {
>  	struct clnt_info *clp = data;
> -	pthread_t th;
> -	int ret;
> +	struct clnt_upcall_info *info;
>  
> -	ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
> -				(void *)clp);
> -	if (ret != 0) {
> -		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> -			 ret, strerror(errno));
> +	info = alloc_upcall_info(clp);
> +	if (info == NULL)
> +		return;
> +
> +	if (read(clp->krb5_fd, &info->uid,
> +			sizeof(info->uid)) < (ssize_t)sizeof(info->uid)) {
> +		printerr(0, "WARNING: %s: failed reading uid from krb5 "
> +			 "upcall pipe: %s\n", __func__, strerror(errno));
> +		free(info);
>  		return;
>  	}
> -	wait_for_child_and_detach(th);
> +
> +	if (start_upcall_thread(handle_krb5_upcall, info))
> +		free(info);
>  }
>  
>  static struct clnt_info *
> diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> index 565bce3..f4f5975 100644
> --- a/utils/gssd/gssd.h
> +++ b/utils/gssd/gssd.h
> @@ -49,7 +49,7 @@
>  #define GSSD_DEFAULT_MACHINE_CRED_SUFFIX	"machine"
>  #define GSSD_DEFAULT_KEYTAB_FILE		"/etc/krb5.keytab"
>  #define GSSD_SERVICE_NAME			"nfs"
> -
> +#define RPC_CHAN_BUF_SIZE			32768
>  /*
>   * The gss mechanisms that we can handle
>   */
> @@ -85,8 +85,15 @@ struct clnt_info {
>  	struct			sockaddr_storage addr;
>  };
>  
> -void handle_krb5_upcall(struct clnt_info *clp);
> -void handle_gssd_upcall(struct clnt_info *clp);
> +struct clnt_upcall_info {
> +	struct clnt_info 	*clp;
> +	char			lbuf[RPC_CHAN_BUF_SIZE];
> +	int			lbuflen;
> +	uid_t			uid;
> +};
> +
> +void handle_krb5_upcall(struct clnt_upcall_info *clp);
> +void handle_gssd_upcall(struct clnt_upcall_info *clp);
>  
>  
>  #endif /* _RPC_GSSD_H_ */
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index afaaf9e..d74d372 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -79,7 +79,6 @@
>  #include "nfsrpc.h"
>  #include "nfslib.h"
>  #include "gss_names.h"
> -#include "misc.h"
>  
>  /* Encryption types supported by the kernel rpcsec_gss code */
>  int num_krb5_enctypes = 0;
> @@ -708,45 +707,22 @@ out_return_error:
>  	goto out;
>  }
>  
> -/* signal to the parent thread that we have read from the file descriptor.
> - * it should allow the parent to proceed to poll on the descriptor for
> - * the next upcall from the kernel.
> - */
> -static inline void
> -signal_parent_event_consumed(void)
> -{
> -	pthread_mutex_lock(&pmutex);
> -	thread_started = true;
> -	pthread_cond_signal(&pcond);
> -	pthread_mutex_unlock(&pmutex);
> -}
> -
>  void
> -handle_krb5_upcall(struct clnt_info *clp)
> +handle_krb5_upcall(struct clnt_upcall_info *info)
>  {
> -	uid_t			uid;
> -	int 			status;
> +	struct clnt_info *clp = info->clp;
>  
> -	status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
> -	signal_parent_event_consumed();
> +	printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath);
>  
> -	if (status) {
> -		printerr(0, "WARNING: failed reading uid from krb5 "
> -			    "upcall pipe: %s\n", strerror(errno));
> -		return;
> -	}
> -
> -	printerr(2, "\n%s: uid %d (%s)\n", __func__, uid, clp->relpath);
> -
> -	process_krb5_upcall(clp, uid, clp->krb5_fd, NULL, NULL);
> +	process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL);
> +	free(info);
>  }
>  
>  void
> -handle_gssd_upcall(struct clnt_info *clp)
> +handle_gssd_upcall(struct clnt_upcall_info *info)
>  {
> +	struct clnt_info	*clp = info->clp;
>  	uid_t			uid;
> -	char			lbuf[RPC_CHAN_BUF_SIZE];
> -	int			lbuflen = 0;
>  	char			*p;
>  	char			*mech = NULL;
>  	char			*uidstr = NULL;
> @@ -754,19 +730,9 @@ handle_gssd_upcall(struct clnt_info *clp)
>  	char			*service = NULL;
>  	char			*enctypes = NULL;
>  
> -	lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
> -	signal_parent_event_consumed();
> +	printerr(2, "\n%s: '%s' (%s)\n", __func__, info->lbuf, clp->relpath);
>  
> -	if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
> -		printerr(0, "WARNING: handle_gssd_upcall: "
> -			    "failed reading request\n");
> -		return;
> -	}
> -	lbuf[lbuflen-1] = 0;
> -
> -	printerr(2, "\n%s: '%s' (%s)\n", __func__, lbuf, clp->relpath);
> -
> -	for (p = strtok(lbuf, " "); p; p = strtok(NULL, " ")) {
> +	for (p = strtok(info->lbuf, " "); p; p = strtok(NULL, " ")) {
>  		if (!strncmp(p, "mech=", strlen("mech=")))
>  			mech = p + strlen("mech=");
>  		else if (!strncmp(p, "uid=", strlen("uid=")))
> @@ -782,8 +748,8 @@ handle_gssd_upcall(struct clnt_info *clp)
>  	if (!mech || strlen(mech) < 1) {
>  		printerr(0, "WARNING: handle_gssd_upcall: "
>  			    "failed to find gss mechanism name "
> -			    "in upcall string '%s'\n", lbuf);
> -		return;
> +			    "in upcall string '%s'\n", info->lbuf);
> +		goto out;
>  	}
>  
>  	if (uidstr) {
> @@ -795,21 +761,21 @@ handle_gssd_upcall(struct clnt_info *clp)
>  	if (!uidstr) {
>  		printerr(0, "WARNING: handle_gssd_upcall: "
>  			    "failed to find uid "
> -			    "in upcall string '%s'\n", lbuf);
> -		return;
> +			    "in upcall string '%s'\n", info->lbuf);
> +		goto out;
>  	}
>  
>  	if (enctypes && parse_enctypes(enctypes) != 0) {
>  		printerr(0, "WARNING: handle_gssd_upcall: "
>  			 "parsing encryption types failed: errno %d\n", errno);
> -		return;
> +		goto out;
>  	}
>  
>  	if (target && strlen(target) < 1) {
>  		printerr(0, "WARNING: handle_gssd_upcall: "
>  			 "failed to parse target name "
> -			 "in upcall string '%s'\n", lbuf);
> -		return;
> +			 "in upcall string '%s'\n", info->lbuf);
> +		goto out;
>  	}
>  
>  	/*
> @@ -823,8 +789,8 @@ handle_gssd_upcall(struct clnt_info *clp)
>  	if (service && strlen(service) < 1) {
>  		printerr(0, "WARNING: handle_gssd_upcall: "
>  			 "failed to parse service type "
> -			 "in upcall string '%s'\n", lbuf);
> -		return;
> +			 "in upcall string '%s'\n", info->lbuf);
> +		goto out;
>  	}
>  
>  	if (strcmp(mech, "krb5") == 0 && clp->servername)
> @@ -835,5 +801,8 @@ handle_gssd_upcall(struct clnt_info *clp)
>  				 "received unknown gss mech '%s'\n", mech);
>  		do_error_downcall(clp->gssd_fd, uid, -EACCES);
>  	}
> +out:
> +	free(info);
> +	return;
>  }
>  
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux