| Submitter | Pranith K |
|---|---|
| Date | 2010-09-24 10:08:36 |
| Message ID | <20100924100836.GA9885@dev.gluster.com> |
| Download | mbox | patch |
| Permalink | /patch/4956/ |
| State | Accepted |
| Headers | show |
Comments
On Fri, Sep 24, 2010 at 3:38 PM, Pranith Kumar K <pranithk@gluster.com>wrote: > > > Signed-off-by: Pranith Kumar K <pranithk@gluster.com> > --- > xlators/mgmt/glusterd/src/glusterd-utils.c | 37 > +++++++++++++++------------- > xlators/mgmt/glusterd/src/glusterd-utils.h | 2 +- > 2 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c > b/xlators/mgmt/glusterd/src/glusterd-utils.c > index a59a069..411d217 100644 > --- a/xlators/mgmt/glusterd/src/glusterd-utils.c > +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c > @@ -769,8 +769,8 @@ glusterd_volinfo_find (char *volname, > glusterd_volinfo_t **volinfo) > > > int32_t > -glusterd_service_stop(const char *service, char *pidfile, int sig, > - gf_boolean_t keep_pidfile) > +glusterd_service_stop (const char *service, char *pidfile, int sig, > + gf_boolean_t force_kill) > Pranith, here you are changing the semantics of the function parameters just to advocate a special behavior for one of its users! This is quite likely to effect the other users... ... as it does effect rb_kill_destination_brick(), which uses that parameter in the "keep the pidfile" sense. That behavior was added in 32041afe<http://git.gluster.com/?p=glusterfs.git;a=commitdiff;h=32041afe> by Pavan. Have you checked with him that it's OK to remove the pidfile? (Well sure yeah it can be that he kept the pidfile by mistake...). Csaba
hi Csaba,
This is the change I intended to do.
1) Remove keep_pid flag altogether, because when the process is killed
the file is cleanedup any way.
2) add force_kill flag, because some actions need the process to be
killed before carrying out subsequent action.
nfs process restart and replace-brick operations need this option.
so the flag is true for both of them and false in case of glusterfs
process stopping.
It so happened that keep_pidfile was true for replace-brick and false
for glusterfs process stop. So they were not showing up in the diff.
Thanks for keeping an eye on the commits :).
Thanks
Pranith.
On Friday 24 September 2010 08:55 PM, Csaba Henk wrote:
> On Fri, Sep 24, 2010 at 3:38 PM, Pranith Kumar K <pranithk@gluster.com
> <mailto:pranithk@gluster.com>> wrote:
>
>
>
> Signed-off-by: Pranith Kumar K <pranithk@gluster.com
> <mailto:pranithk@gluster.com>>
> ---
> xlators/mgmt/glusterd/src/glusterd-utils.c | 37
> +++++++++++++++-------------
> xlators/mgmt/glusterd/src/glusterd-utils.h | 2 +-
> 2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c
> b/xlators/mgmt/glusterd/src/glusterd-utils.c
> index a59a069..411d217 100644
> --- a/xlators/mgmt/glusterd/src/glusterd-utils.c
> +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
> @@ -769,8 +769,8 @@ glusterd_volinfo_find (char *volname,
> glusterd_volinfo_t **volinfo)
>
>
> int32_t
> -glusterd_service_stop(const char *service, char *pidfile, int sig,
> - gf_boolean_t keep_pidfile)
> +glusterd_service_stop (const char *service, char *pidfile, int sig,
> + gf_boolean_t force_kill)
>
>
> Pranith, here you are changing the semantics of the function
> parameters just to advocate a special behavior for one of its users!
> This is quite likely to effect the other users...
>
> ... as it does effect rb_kill_destination_brick(), which uses that
> parameter in the "keep the pidfile" sense. That behavior was added in
> 32041afe
> <http://git.gluster.com/?p=glusterfs.git;a=commitdiff;h=32041afe> by
> Pavan. Have you checked with him that it's OK to remove the pidfile?
> (Well sure yeah it can be that he kept the pidfile by mistake...).
>
> Csaba
>
On Fri, Sep 24, 2010 at 10:17 PM, Pranith Kumar K <pranithk@gluster.com>wrote: > This is the change I intended to do. > 1) Remove keep_pid flag altogether, because when the process is killed the > file is cleanedup any way. > 2) add force_kill flag, because some actions need the process to be killed > before carrying out subsequent action. > nfs process restart and replace-brick operations need this option. so > the flag is true for both of them and false in case of glusterfs process > stopping. > > It so happened that keep_pidfile was true for replace-brick and false for > glusterfs process stop. So they were not showing up in the diff. > OK dude so I'm not disappointed in you ;) > Thanks for keeping an eye on the commits :). > Rebase is what makes my day. Csaba
Patch
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c index a59a069..411d217 100644 --- a/xlators/mgmt/glusterd/src/glusterd-utils.c +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c @@ -769,8 +769,8 @@ glusterd_volinfo_find (char *volname, glusterd_volinfo_t **volinfo) int32_t -glusterd_service_stop(const char *service, char *pidfile, int sig, - gf_boolean_t keep_pidfile) +glusterd_service_stop (const char *service, char *pidfile, int sig, + gf_boolean_t force_kill) { int32_t ret = -1; pid_t pid = -1; @@ -806,20 +806,23 @@ glusterd_service_stop(const char *service, char *pidfile, int sig, ret = kill (pid, sig); - if (ret) { - gf_log ("", GF_LOG_ERROR, "Unable to kill pid %d", pid); - goto out; - } - - if (keep_pidfile) - goto out; - - ret = unlink (pidfile); - - if (ret && (ENOENT != errno)) { - gf_log ("", GF_LOG_ERROR, "Unable to unlink pidfile: %s", - pidfile); - goto out; + if (force_kill) { + sleep (1); + ret = access (pidfile, F_OK); + if (!ret) { + ret = kill (pid, SIGKILL); + if (ret) { + gf_log ("", GF_LOG_ERROR, "Unable to " + "kill pid %d", pid); + goto out; + } + ret = unlink (pidfile); + if (ret && (ENOENT != errno)) { + gf_log ("", GF_LOG_ERROR, "Unable to " + "unlink pidfile: %s", pidfile); + goto out; + } + } } ret = 0; @@ -1642,7 +1645,7 @@ glusterd_nfs_server_stop () GLUSTERD_GET_NFS_DIR(path, priv); GLUSTERD_GET_NFS_PIDFILE(pidfile); - return glusterd_service_stop ("nfsd", pidfile, SIGTERM, _gf_false); + return glusterd_service_stop ("nfsd", pidfile, SIGTERM, _gf_true); } int diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.h b/xlators/mgmt/glusterd/src/glusterd-utils.h index bc20c2d..3f49acf 100644 --- a/xlators/mgmt/glusterd/src/glusterd-utils.h +++ b/xlators/mgmt/glusterd/src/glusterd-utils.h @@ -92,7 +92,7 @@ glusterd_volinfo_find (char *volname, glusterd_volinfo_t **volinfo); int32_t glusterd_service_stop(const char *service, char *pidfile, int sig, - gf_boolean_t keep_pidfile); + gf_boolean_t force_kill); int32_t glusterd_resolve_brick (glusterd_brickinfo_t *brickinfo);
Signed-off-by: Pranith Kumar K <pranithk@gluster.com> --- xlators/mgmt/glusterd/src/glusterd-utils.c | 37 +++++++++++++++------------- xlators/mgmt/glusterd/src/glusterd-utils.h | 2 +- 2 files changed, 21 insertions(+), 18 deletions(-)