Patchwork [BUG:1922] cli, mgmt/glusterd: add op-errstr to gluster rebalance

login
register
Submitter Pranith K
Date 2011-01-17 05:05:37
Message ID <20110117050537.GA17892@dev.gluster.com>
Download mbox | patch
Permalink /patch/5988/
State Changes Requested
Delegated to: Amar Tumballi
Headers show

Comments

Pranith K - 2011-01-17 05:05:37
Signed-off-by: Pranith Kumar K <pranithk@gluster.com>
---
 cli/src/cli3_1-cops.c                          |   36 ++-
 rpc/xdr/src/cli1-xdr.c                         |    2 +
 rpc/xdr/src/cli1-xdr.h                         |    1 +
 rpc/xdr/src/cli1.x                             |    1 +
 xlators/mgmt/glusterd/src/glusterd-rebalance.c |  301 ++++++++++++++++--------
 5 files changed, 227 insertions(+), 114 deletions(-)
Amar Tumballi - 2011-01-17 06:00:35
I am coming out with a document on RPC/XDR related changes. For now, I will
say we hold this patch as the XDR is changed for rebalance response. Ideally
we should add another program version to solve this.

Other than that, logic wise, patch is fine.

On Mon, Jan 17, 2011 at 10:35 AM, Pranith Kumar K <pranithk@gluster.com>wrote:

>
>
> Signed-off-by: Pranith Kumar K <pranithk@gluster.com>
> ---
>  cli/src/cli3_1-cops.c                          |   36 ++-
>  rpc/xdr/src/cli1-xdr.c                         |    2 +
>  rpc/xdr/src/cli1-xdr.h                         |    1 +
>  rpc/xdr/src/cli1.x                             |    1 +
>  xlators/mgmt/glusterd/src/glusterd-rebalance.c |  301
> ++++++++++++++++--------
>  5 files changed, 227 insertions(+), 114 deletions(-)
>
> diff --git a/cli/src/cli3_1-cops.c b/cli/src/cli3_1-cops.c
> index a988150..e289aed 100644
> --- a/cli/src/cli3_1-cops.c
> +++ b/cli/src/cli3_1-cops.c
> @@ -802,24 +802,34 @@ gf_cli3_1_defrag_volume_cbk (struct rpc_req *req,
> struct iovec *iov,
>                 cmd = local->u.defrag_vol.cmd;
>         }
>         if (cmd == GF_DEFRAG_CMD_START) {
> -                cli_out ("starting rebalance on volume %s has been %s",
> volname,
> -                         (rsp.op_ret) ? "unsuccessful": "successful");
> -                if (rsp.op_ret && rsp.op_errno == EEXIST)
> -                       cli_out ("Rebalance already started on volume %s",
> -                                volname);
> +                if (rsp.op_ret && strcmp (rsp.op_errstr, ""))
> +                        cli_out (rsp.op_errstr);
> +                else
> +                        cli_out ("starting rebalance on volume %s has been
> %s",
> +                                 volname, (rsp.op_ret) ? "unsuccessful":
> +                                 "successful");
>         }
>         if (cmd == GF_DEFRAG_CMD_STOP) {
> -                if (rsp.op_ret == -1)
> -                        cli_out ("rebalance volume %s stop failed",
> volname);
> -                else
> +                if (rsp.op_ret == -1) {
> +                        if (strcmp (rsp.op_errstr, ""))
> +                                cli_out (rsp.op_errstr);
> +                        else
> +                                cli_out ("rebalance volume %s stop
> failed",
> +                                         volname);
> +                } else {
>                         cli_out ("stopped rebalance process of volume %s
> \n"
>                                  "(after rebalancing %"PRId64" files
> totaling "
>                                  "%"PRId64" bytes)", volname, rsp.files,
> rsp.size);
> +                }
>         }
>         if (cmd == GF_DEFRAG_CMD_STATUS) {
> -                if (rsp.op_ret == -1)
> -                        cli_out ("failed to get the status of rebalance
> process");
> -                else {
> +                if (rsp.op_ret == -1) {
> +                        if (strcmp (rsp.op_errstr, ""))
> +                                cli_out (rsp.op_errstr);
> +                        else
> +                                cli_out ("failed to get the status of "
> +                                         "rebalance process");
> +                } else {
>                         char *status = "unknown";
>                         if (rsp.op_errno == 0)
>                                 status = "not started";
> @@ -854,6 +864,10 @@ gf_cli3_1_defrag_volume_cbk (struct rpc_req *req,
> struct iovec *iov,
>         ret = rsp.op_ret;
>
>  out:
> +        if (rsp.op_errstr)
> +                free (rsp.op_errstr); //malloced by xdr
> +        if (rsp.volname)
> +                free (rsp.volname); //malloced by xdr
>         cli_cmd_broadcast_response (ret);
>         return ret;
>  }
> diff --git a/rpc/xdr/src/cli1-xdr.c b/rpc/xdr/src/cli1-xdr.c
> index 084aa8b..7a86097 100644
> --- a/rpc/xdr/src/cli1-xdr.c
> +++ b/rpc/xdr/src/cli1-xdr.c
> @@ -367,6 +367,8 @@ xdr_gf1_cli_defrag_vol_rsp (XDR *xdrs,
> gf1_cli_defrag_vol_rsp *objp)
>                 return FALSE;
>         if (!xdr_int (xdrs, &objp->op_errno))
>                 return FALSE;
> +        if (!xdr_string (xdrs, &objp->op_errstr, ~0))
> +                return FALSE;
>         if (!xdr_string (xdrs, &objp->volname, ~0))
>                 return FALSE;
>         if (!xdr_u_quad_t (xdrs, &objp->files))
> diff --git a/rpc/xdr/src/cli1-xdr.h b/rpc/xdr/src/cli1-xdr.h
> index c2ccb32..5415e5a 100644
> --- a/rpc/xdr/src/cli1-xdr.h
> +++ b/rpc/xdr/src/cli1-xdr.h
> @@ -221,6 +221,7 @@ typedef struct gf1_cli_defrag_vol_req
> gf1_cli_defrag_vol_req;
>  struct gf1_cli_defrag_vol_rsp {
>        int op_ret;
>        int op_errno;
> +       char *op_errstr;
>        char *volname;
>        u_quad_t files;
>        u_quad_t size;
> diff --git a/rpc/xdr/src/cli1.x b/rpc/xdr/src/cli1.x
> index 4a0bd71..0ab5ed4 100644
> --- a/rpc/xdr/src/cli1.x
> +++ b/rpc/xdr/src/cli1.x
> @@ -148,6 +148,7 @@ struct gf1_cli_get_vol_rsp {
>  struct gf1_cli_defrag_vol_rsp {
>         int     op_ret;
>         int     op_errno;
> +        string  op_errstr<>;
>         string  volname<>;
>         unsigned hyper   files;
>         unsigned hyper   size;
> diff --git a/xlators/mgmt/glusterd/src/glusterd-rebalance.c
> b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
> index 3e4b4f5..fb9d9b2 100644
> --- a/xlators/mgmt/glusterd/src/glusterd-rebalance.c
> +++ b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
> @@ -315,25 +315,53 @@ out:
>  }
>
>  int
> -glusterd_defrag_stop (glusterd_volinfo_t *volinfo,
> -                      gf1_cli_defrag_vol_rsp *rsp)
> +glusterd_defrag_stop_validate (glusterd_volinfo_t *volinfo,
> +                               char *op_errstr, size_t len)
> +{
> +        int     ret = -1;
> +        if (glusterd_is_defrag_on (volinfo) == 0) {
> +                snprintf (op_errstr, len, "Rebalance on %s is either
> Completed "
> +                          "or not yet started", volinfo->volname);
> +                goto out;
> +        }
> +        ret = 0;
> +out:
> +        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
> +        return ret;
> +}
> +validate_xlator_volume_options
> +int
> +glusterd_defrag_stop (glusterd_volinfo_t *volinfo, u_quad_t *files,
> +                      u_quad_t *size, char *op_errstr, size_t len)
>  {
>         /* TODO: set a variaeble 'stop_defrag' here, it should be checked
>            in defrag loop */
> -        if (!volinfo || !volinfo->defrag)
> +        int     ret = -1;
> +        GF_ASSERT (volinfo);
> +        GF_ASSERT (files);
> +        GF_ASSERT (size);
> +        GF_ASSERT (op_errstr);
> +
> +        ret = glusterd_defrag_stop_validate (volinfo, op_errstr, len);
> +        if (ret)
> +                goto out;
> +        if (!volinfo || !volinfo->defrag) {
> +                ret = -1;
>                 goto out;
> +        }
>
>         LOCK (&volinfo->defrag->lock);
>         {
>                 volinfo->defrag_status = GF_DEFRAG_STATUS_STOPED;
> -                rsp->files = volinfo->defrag->total_files;
> -                rsp->size = volinfo->defrag->total_data;
> +                *files = volinfo->defrag->total_files;
> +                *size = volinfo->defrag->total_data;
>         }
>         UNLOCK (&volinfo->defrag->lock);
>
> -        rsp->op_ret = 0;
> +        ret = 0;
>  out:
> -        return 0;
> +        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
> +        return ret;
>  }
>
>  int
> @@ -363,120 +391,195 @@ out:
>         return 0;
>  }
>
> -int
> -glusterd_handle_defrag_volume (rpcsvc_request_t *req)
> +void
> +glusterd_rebalance_cmd_attempted_log (int cmd, char *volname)
>  {
> -        int32_t                ret           = -1;
> -        gf1_cli_defrag_vol_req cli_req       = {0,};
> -        glusterd_conf_t         *priv = NULL;
> -        char                   cmd_str[4096] = {0,};
> -        glusterd_volinfo_t      *volinfo = NULL;
> -        glusterd_defrag_info_t *defrag =  NULL;
> -        gf1_cli_defrag_vol_rsp rsp = {0,};
> -
> -        GF_ASSERT (req);
> -
> -        priv    = THIS->private;
> -        if (!gf_xdr_to_cli_defrag_vol_req (req->msg[0], &cli_req)) {
> -                //failed to decode msg;
> -                req->rpc_err = GARBAGE_ARGS;
> -                goto out;
> -        }
> -
> -        switch (cli_req.cmd) {
> +        switch (cmd) {
>                 case GF_DEFRAG_CMD_START:
>                         gf_cmd_log ("Volume rebalance"," on volname: %s "
> -                                    "cmd: start, attempted",
> cli_req.volname);
> +                                    "cmd: start, attempted", volname);
>                         break;
>                 case GF_DEFRAG_CMD_STOP:
>                         gf_cmd_log ("Volume rebalance"," on volname: %s "
> -                                    "cmd: stop, attempted",
> cli_req.volname);
> +                                    "cmd: stop, attempted", volname);
>                         break;
>                 default:
>                         break;
>         }
>         gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance volume %s on
> %s",
> -                (cli_req.cmd == GF_DEFRAG_CMD_START)?"start":(cli_req.cmd
> == GF_DEFRAG_CMD_STOP)?"stop":"status"
> -                , cli_req.volname);
> +                (cmd == GF_DEFRAG_CMD_START)?"start":(cmd ==
> GF_DEFRAG_CMD_STOP)?"stop":"status"
> +                , volname);
> +}
>
> -        rsp.volname = cli_req.volname;
> -        rsp.op_ret = -1;
> -        if (glusterd_volinfo_find(cli_req.volname, &volinfo)) {
> -                gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance on
> invalid"
> -                        " volname %s", cli_req.volname);
> +void
> +glusterd_rebalance_cmd_log (int cmd, char *volname, int status)
> +{
> +        if (cmd != GF_DEFRAG_CMD_STATUS) {
> +                gf_cmd_log ("volume rebalance"," on volname: %s %d %s",
> +                            volname, cmd, ((status)?"FAILED":"SUCCESS"));
> +        }
> +}
> +
> +int
> +glusterd_defrag_start_validate (glusterd_volinfo_t *volinfo, char
> *op_errstr,
> +                                size_t len)
> +{
> +        int     ret = -1;
> +
> +        if (glusterd_is_defrag_on (volinfo)) {
> +                gf_log ("glusterd", GF_LOG_DEBUG,
> +                        "rebalance on volume %s already started",
> +                        volinfo->volname);
> +                snprintf (op_errstr, len, "Rebalance on %s is already
> started",
> +                          volinfo->volname);
>                 goto out;
>         }
>
> -        if (volinfo->status != GLUSTERD_STATUS_STARTED) {
> -                gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance on
> stopped"
> -                        " volname %s", cli_req.volname);
> +        if (glusterd_is_rb_started (volinfo) ||
> +            glusterd_is_rb_paused (volinfo)) {
> +                gf_log ("glusterd", GF_LOG_DEBUG,
> +                        "Replace brick is in progress on volume %s",
> +                        volinfo->volname);
> +                snprintf (op_errstr, len, "Replace brick is in progress on
> "
> +                          "volume %s", volinfo->volname);
>                 goto out;
>         }
> +        ret = 0;
> +out:
> +        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
> +        return ret;
> +}
>
> -        switch (cli_req.cmd) {
> -        case GF_DEFRAG_CMD_START:
> -        {
> -                if (volinfo->defrag) {
> -                        gf_log ("glusterd", GF_LOG_DEBUG,
> -                                "rebalance on volume %s already started",
> -                                cli_req.volname);
> -                        rsp.op_errno = EEXIST;
> -                        goto out;
> -                }
> +int
> +glusterd_handle_defrag_start (glusterd_volinfo_t *volinfo, char
> *op_errstr,
> +                              size_t len)
> +{
> +        int                    ret = -1;
> +        glusterd_defrag_info_t *defrag =  NULL;
> +        char                   cmd_str[4096] = {0,};
> +        glusterd_conf_t        *priv = NULL;
>
> -                if (glusterd_is_rb_started (volinfo) ||
> -                    glusterd_is_rb_paused (volinfo)) {
> -                        gf_log ("glusterd", GF_LOG_DEBUG,
> -                                "Replace brick is in progress on volume
> %s",
> -                                cli_req.volname);
> -                        goto out;
> -                }
> -                volinfo->defrag = GF_CALLOC (1, sizeof
> (glusterd_defrag_info_t),
> -                                             gf_gld_mt_defrag_info);
> -                if (!volinfo->defrag)
> -                        goto out;
> +        priv    = THIS->private;
>
> -                defrag = volinfo->defrag;
> +        GF_ASSERT (volinfo);
> +        GF_ASSERT (op_errstr);
>
> -                LOCK_INIT (&defrag->lock);
> -                snprintf (defrag->mount, 1024, "%s/mount/%s",
> -                          priv->workdir, cli_req.volname);
> -                /* Create a directory, mount glusterfs over it, start
> glusterfs-defrag */
> -                snprintf (cmd_str, 4096, "mkdir -p %s", defrag->mount);
> -                ret = system (cmd_str);
> +        ret = glusterd_defrag_start_validate (volinfo, op_errstr, len);
> +        if (ret)
> +                goto out;
> +        volinfo->defrag = GF_CALLOC (1, sizeof (glusterd_defrag_info_t),
> +                                     gf_gld_mt_defrag_info);
> +        if (!volinfo->defrag)
> +                goto out;
>
> -                if (ret) {
> -                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s
> failed", cmd_str);
> -                        goto out;
> -                }
> +        defrag = volinfo->defrag;
>
> -                snprintf (cmd_str, 4096, "%s/sbin/glusterfs -s localhost "
> -                          "--volfile-id %s --volume-name %s-quick-read "
> -                          "--xlator-option *dht.unhashed-sticky-bit=yes "
> -                          "--xlator-option *dht.use-readdirp=yes "
> -                          "--xlator-option *dht.lookup-unhashed=yes %s",
> -                          GFS_PREFIX, cli_req.volname, cli_req.volname,
> -                          defrag->mount);
> -                ret = gf_system (cmd_str);
> -                if (ret) {
> -                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s
> failed", cmd_str);
> -                        goto out;
> -                }
> +        LOCK_INIT (&defrag->lock);
> +        snprintf (defrag->mount, 1024, "%s/mount/%s",
> +                  priv->workdir, volinfo->volname);
> +        /* Create a directory, mount glusterfs over it, start
> glusterfs-defrag */
> +        snprintf (cmd_str, sizeof (cmd_str), "mkdir -p %s",
> defrag->mount);
> +        ret = system (cmd_str);
>
> -                volinfo->defrag_status = GF_DEFRAG_STATUS_STARTED;
> -                rsp.op_ret = 0;
> +        if (ret) {
> +                gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed",
> cmd_str);
> +                goto out;
> +        }
>
> -                ret = pthread_create (&defrag->th, NULL,
> glusterd_defrag_start,
> -                                      volinfo);
> -                if (ret) {
> -                        snprintf (cmd_str, 1024, "umount -l %s",
> defrag->mount);
> -                        ret = system (cmd_str);
> -                        rsp.op_ret = -1;
> -                }
> -                break;
> +        snprintf (cmd_str, sizeof (cmd_str), "%s/sbin/glusterfs -s
> localhost "
> +                  "--volfile-id %s --volume-name %s-quick-read "
> +                  "--xlator-option *dht.unhashed-sticky-bit=yes "
> +                  "--xlator-option *dht.use-readdirp=yes "
> +                  "--xlator-option *dht.lookup-unhashed=yes %s",
> +                  GFS_PREFIX, volinfo->volname, volinfo->volname,
> +                  defrag->mount);
> +        ret = gf_system (cmd_str);
> +        if (ret) {
> +                gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed",
> cmd_str);
> +                goto out;
> +        }
> +
> +        volinfo->defrag_status = GF_DEFRAG_STATUS_STARTED;
> +
> +        ret = pthread_create (&defrag->th, NULL, glusterd_defrag_start,
> +                              volinfo);
> +        if (ret) {
> +                snprintf (cmd_str, sizeof (cmd_str), "umount -l %s",
> defrag->mount);
> +                if (system (cmd_str))
> +                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s "
> +                               "failed", cmd_str);
>         }
> +out:
> +        gf_log ("", GF_LOG_DEBUG, "Returning %d", ret);
> +        return ret;
> +}
> +
> +int
> +glusterd_rebalance_cmd_validate (int cmd, char *volname,
> +                                 glusterd_volinfo_t **volinfo,
> +                                 char *op_errstr, size_t len)
> +{
> +        int ret = -1;
> +
> +        if (glusterd_volinfo_find(volname, volinfo)) {
> +                gf_log ("glusterd", GF_LOG_ERROR, "Received rebalance on
> invalid"
> +                        " volname %s", volname);
> +                snprintf (op_errstr, len, "Volume %s does not exist",
> +                          volname);
> +                goto out;
> +        }
> +
> +        if ((*volinfo)->status != GLUSTERD_STATUS_STARTED) {
> +                gf_log ("glusterd", GF_LOG_ERROR, "Received rebalance on
> stopped"
> +                        " volname %s", volname);
> +                snprintf (op_errstr, len, "Volume %s needs to "
> +                          "be started to perform rebalance", volname);
> +                goto out;
> +        }
> +        ret = 0;
> +out:
> +        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
> +        return ret;
> +}
> +
> +int
> +glusterd_handle_defrag_volume (rpcsvc_request_t *req)
> +{
> +        int32_t                ret           = -1;
> +        gf1_cli_defrag_vol_req cli_req       = {0,};
> +        glusterd_volinfo_t      *volinfo = NULL;
> +        gf1_cli_defrag_vol_rsp rsp = {0,};
> +        char                   msg[2048] = {0};
> +        glusterd_conf_t         *priv = NULL;
> +
> +        GF_ASSERT (req);
> +
> +        priv    = THIS->private;
> +        if (!gf_xdr_to_cli_defrag_vol_req (req->msg[0], &cli_req)) {
> +                //failed to decode msg;
> +                req->rpc_err = GARBAGE_ARGS;
> +                goto out;
> +        }
> +
> +        glusterd_rebalance_cmd_attempted_log (cli_req.cmd,
> cli_req.volname);
> +
> +        rsp.volname = cli_req.volname;
> +        rsp.op_ret = -1;
> +        rsp.op_errstr = msg;
> +
> +        ret = glusterd_rebalance_cmd_validate (cli_req.cmd,
> cli_req.volname,
> +                                               &volinfo, msg, sizeof
> (msg));
> +        if (ret)
> +                goto out;
> +        switch (cli_req.cmd) {
> +        case GF_DEFRAG_CMD_START:
> +                ret = glusterd_handle_defrag_start (volinfo, msg, sizeof
> (msg));
> +                rsp.op_ret = ret;
> +                break;
>         case GF_DEFRAG_CMD_STOP:
> -                ret = glusterd_defrag_stop (volinfo, &rsp);
> +                ret = glusterd_defrag_stop (volinfo, &rsp.files,
> &rsp.size,
> +                                            msg, sizeof (msg));
> +                rsp.op_ret = ret;
>                 break;
>         case GF_DEFRAG_CMD_STATUS:
>                 ret = glusterd_defrag_status_get (volinfo, &rsp);
> @@ -484,15 +587,7 @@ glusterd_handle_defrag_volume (rpcsvc_request_t *req)
>         default:
>                 break;
>         }
> -        if (ret)
> -                gf_log("glusterd", GF_LOG_DEBUG, "command: %s
> failed",cmd_str);
> -
> -        if (cli_req.cmd != GF_DEFRAG_CMD_STATUS) {
> -                gf_cmd_log ("volume rebalance"," on volname: %s %d %s",
> -                            cli_req.volname,
> -                            cli_req.cmd, ((ret)?"FAILED":"SUCCESS"));
> -        }
> -
> +        glusterd_rebalance_cmd_log (cli_req.cmd, cli_req.volname,
> rsp.op_ret);
>  out:
>
>         ret = glusterd_submit_reply (req, &rsp, NULL, 0, NULL,
> --
> 1.7.0.4
>
>

Patch

diff --git a/cli/src/cli3_1-cops.c b/cli/src/cli3_1-cops.c
index a988150..e289aed 100644
--- a/cli/src/cli3_1-cops.c
+++ b/cli/src/cli3_1-cops.c
@@ -802,24 +802,34 @@  gf_cli3_1_defrag_volume_cbk (struct rpc_req *req, struct iovec *iov,
                 cmd = local->u.defrag_vol.cmd;
         }
         if (cmd == GF_DEFRAG_CMD_START) {
-                cli_out ("starting rebalance on volume %s has been %s", volname,
-                         (rsp.op_ret) ? "unsuccessful": "successful");
-                if (rsp.op_ret && rsp.op_errno == EEXIST)
-                       cli_out ("Rebalance already started on volume %s", 
-                                volname);
+                if (rsp.op_ret && strcmp (rsp.op_errstr, ""))
+                        cli_out (rsp.op_errstr);
+                else
+                        cli_out ("starting rebalance on volume %s has been %s",
+                                 volname, (rsp.op_ret) ? "unsuccessful":
+                                 "successful");
         }
         if (cmd == GF_DEFRAG_CMD_STOP) {
-                if (rsp.op_ret == -1)
-                        cli_out ("rebalance volume %s stop failed", volname);
-                else
+                if (rsp.op_ret == -1) {
+                        if (strcmp (rsp.op_errstr, ""))
+                                cli_out (rsp.op_errstr);
+                        else
+                                cli_out ("rebalance volume %s stop failed",
+                                         volname);
+                } else {
                         cli_out ("stopped rebalance process of volume %s \n"
                                  "(after rebalancing %"PRId64" files totaling "
                                  "%"PRId64" bytes)", volname, rsp.files, rsp.size);
+                }
         }
         if (cmd == GF_DEFRAG_CMD_STATUS) {
-                if (rsp.op_ret == -1)
-                        cli_out ("failed to get the status of rebalance process");
-                else {
+                if (rsp.op_ret == -1) {
+                        if (strcmp (rsp.op_errstr, ""))
+                                cli_out (rsp.op_errstr);
+                        else
+                                cli_out ("failed to get the status of "
+                                         "rebalance process");
+                } else {
                         char *status = "unknown";
                         if (rsp.op_errno == 0)
                                 status = "not started";
@@ -854,6 +864,10 @@  gf_cli3_1_defrag_volume_cbk (struct rpc_req *req, struct iovec *iov,
         ret = rsp.op_ret;
 
 out:
+        if (rsp.op_errstr)
+                free (rsp.op_errstr); //malloced by xdr
+        if (rsp.volname)
+                free (rsp.volname); //malloced by xdr
         cli_cmd_broadcast_response (ret);
         return ret;
 }
diff --git a/rpc/xdr/src/cli1-xdr.c b/rpc/xdr/src/cli1-xdr.c
index 084aa8b..7a86097 100644
--- a/rpc/xdr/src/cli1-xdr.c
+++ b/rpc/xdr/src/cli1-xdr.c
@@ -367,6 +367,8 @@  xdr_gf1_cli_defrag_vol_rsp (XDR *xdrs, gf1_cli_defrag_vol_rsp *objp)
 		 return FALSE;
 	 if (!xdr_int (xdrs, &objp->op_errno))
 		 return FALSE;
+	 if (!xdr_string (xdrs, &objp->op_errstr, ~0))
+		 return FALSE;
 	 if (!xdr_string (xdrs, &objp->volname, ~0))
 		 return FALSE;
 	 if (!xdr_u_quad_t (xdrs, &objp->files))
diff --git a/rpc/xdr/src/cli1-xdr.h b/rpc/xdr/src/cli1-xdr.h
index c2ccb32..5415e5a 100644
--- a/rpc/xdr/src/cli1-xdr.h
+++ b/rpc/xdr/src/cli1-xdr.h
@@ -221,6 +221,7 @@  typedef struct gf1_cli_defrag_vol_req gf1_cli_defrag_vol_req;
 struct gf1_cli_defrag_vol_rsp {
 	int op_ret;
 	int op_errno;
+	char *op_errstr;
 	char *volname;
 	u_quad_t files;
 	u_quad_t size;
diff --git a/rpc/xdr/src/cli1.x b/rpc/xdr/src/cli1.x
index 4a0bd71..0ab5ed4 100644
--- a/rpc/xdr/src/cli1.x
+++ b/rpc/xdr/src/cli1.x
@@ -148,6 +148,7 @@  struct gf1_cli_get_vol_rsp {
  struct gf1_cli_defrag_vol_rsp {
         int     op_ret;
         int     op_errno;
+        string  op_errstr<>;
         string  volname<>;
         unsigned hyper   files;
         unsigned hyper   size;
diff --git a/xlators/mgmt/glusterd/src/glusterd-rebalance.c b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
index 3e4b4f5..fb9d9b2 100644
--- a/xlators/mgmt/glusterd/src/glusterd-rebalance.c
+++ b/xlators/mgmt/glusterd/src/glusterd-rebalance.c
@@ -315,25 +315,53 @@  out:
 }
 
 int
-glusterd_defrag_stop (glusterd_volinfo_t *volinfo,
-                      gf1_cli_defrag_vol_rsp *rsp)
+glusterd_defrag_stop_validate (glusterd_volinfo_t *volinfo,
+                               char *op_errstr, size_t len)
+{
+        int     ret = -1;
+        if (glusterd_is_defrag_on (volinfo) == 0) {
+                snprintf (op_errstr, len, "Rebalance on %s is either Completed "
+                          "or not yet started", volinfo->volname);
+                goto out;
+        }
+        ret = 0;
+out:
+        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
+        return ret;
+}
+
+int
+glusterd_defrag_stop (glusterd_volinfo_t *volinfo, u_quad_t *files,
+                      u_quad_t *size, char *op_errstr, size_t len)
 {
         /* TODO: set a variaeble 'stop_defrag' here, it should be checked
            in defrag loop */
-        if (!volinfo || !volinfo->defrag)
+        int     ret = -1;
+        GF_ASSERT (volinfo);
+        GF_ASSERT (files);
+        GF_ASSERT (size);
+        GF_ASSERT (op_errstr);
+
+        ret = glusterd_defrag_stop_validate (volinfo, op_errstr, len);
+        if (ret)
+                goto out;
+        if (!volinfo || !volinfo->defrag) {
+                ret = -1;
                 goto out;
+        }
 
         LOCK (&volinfo->defrag->lock);
         {
                 volinfo->defrag_status = GF_DEFRAG_STATUS_STOPED;
-                rsp->files = volinfo->defrag->total_files;
-                rsp->size = volinfo->defrag->total_data;
+                *files = volinfo->defrag->total_files;
+                *size = volinfo->defrag->total_data;
         }
         UNLOCK (&volinfo->defrag->lock);
 
-        rsp->op_ret = 0;
+        ret = 0;
 out:
-        return 0;
+        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
+        return ret;
 }
 
 int
@@ -363,120 +391,195 @@  out:
         return 0;
 }
 
-int
-glusterd_handle_defrag_volume (rpcsvc_request_t *req)
+void
+glusterd_rebalance_cmd_attempted_log (int cmd, char *volname)
 {
-        int32_t                ret           = -1;
-        gf1_cli_defrag_vol_req cli_req       = {0,};
-        glusterd_conf_t         *priv = NULL;
-        char                   cmd_str[4096] = {0,};
-        glusterd_volinfo_t      *volinfo = NULL;
-        glusterd_defrag_info_t *defrag =  NULL;
-        gf1_cli_defrag_vol_rsp rsp = {0,};
-
-        GF_ASSERT (req);
-
-        priv    = THIS->private;
-        if (!gf_xdr_to_cli_defrag_vol_req (req->msg[0], &cli_req)) {
-                //failed to decode msg;
-                req->rpc_err = GARBAGE_ARGS;
-                goto out;
-        }
-
-        switch (cli_req.cmd) {
+        switch (cmd) {
                 case GF_DEFRAG_CMD_START:
                         gf_cmd_log ("Volume rebalance"," on volname: %s "
-                                    "cmd: start, attempted", cli_req.volname);
+                                    "cmd: start, attempted", volname);
                         break;
                 case GF_DEFRAG_CMD_STOP:
                         gf_cmd_log ("Volume rebalance"," on volname: %s "
-                                    "cmd: stop, attempted", cli_req.volname);
+                                    "cmd: stop, attempted", volname);
                         break;
                 default:
                         break;
         }
         gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance volume %s on %s",
-                (cli_req.cmd == GF_DEFRAG_CMD_START)?"start":(cli_req.cmd == GF_DEFRAG_CMD_STOP)?"stop":"status"
-                , cli_req.volname);
+                (cmd == GF_DEFRAG_CMD_START)?"start":(cmd == GF_DEFRAG_CMD_STOP)?"stop":"status"
+                , volname);
+}
 
-        rsp.volname = cli_req.volname;
-        rsp.op_ret = -1;
-        if (glusterd_volinfo_find(cli_req.volname, &volinfo)) {
-                gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance on invalid"
-                        " volname %s", cli_req.volname);
+void
+glusterd_rebalance_cmd_log (int cmd, char *volname, int status)
+{
+        if (cmd != GF_DEFRAG_CMD_STATUS) {
+                gf_cmd_log ("volume rebalance"," on volname: %s %d %s",
+                            volname, cmd, ((status)?"FAILED":"SUCCESS"));
+        }
+}
+
+int
+glusterd_defrag_start_validate (glusterd_volinfo_t *volinfo, char *op_errstr,
+                                size_t len)
+{
+        int     ret = -1;
+
+        if (glusterd_is_defrag_on (volinfo)) {
+                gf_log ("glusterd", GF_LOG_DEBUG,
+                        "rebalance on volume %s already started",
+                        volinfo->volname);
+                snprintf (op_errstr, len, "Rebalance on %s is already started",
+                          volinfo->volname);
                 goto out;
         }
 
-        if (volinfo->status != GLUSTERD_STATUS_STARTED) {
-                gf_log ("glusterd", GF_LOG_NORMAL, "Received rebalance on stopped"
-                        " volname %s", cli_req.volname);
+        if (glusterd_is_rb_started (volinfo) ||
+            glusterd_is_rb_paused (volinfo)) {
+                gf_log ("glusterd", GF_LOG_DEBUG,
+                        "Replace brick is in progress on volume %s",
+                        volinfo->volname);
+                snprintf (op_errstr, len, "Replace brick is in progress on "
+                          "volume %s", volinfo->volname);
                 goto out;
         }
+        ret = 0;
+out:
+        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
+        return ret;
+}
 
-        switch (cli_req.cmd) {
-        case GF_DEFRAG_CMD_START:
-        {
-                if (volinfo->defrag) {
-                        gf_log ("glusterd", GF_LOG_DEBUG,
-                                "rebalance on volume %s already started",
-                                cli_req.volname);
-                        rsp.op_errno = EEXIST;
-                        goto out;
-                }
+int
+glusterd_handle_defrag_start (glusterd_volinfo_t *volinfo, char *op_errstr,
+                              size_t len)
+{
+        int                    ret = -1;
+        glusterd_defrag_info_t *defrag =  NULL;
+        char                   cmd_str[4096] = {0,};
+        glusterd_conf_t        *priv = NULL;
 
-                if (glusterd_is_rb_started (volinfo) ||
-                    glusterd_is_rb_paused (volinfo)) {
-                        gf_log ("glusterd", GF_LOG_DEBUG,
-                                "Replace brick is in progress on volume %s",
-                                cli_req.volname);
-                        goto out;
-                }
-                volinfo->defrag = GF_CALLOC (1, sizeof (glusterd_defrag_info_t),
-                                             gf_gld_mt_defrag_info);
-                if (!volinfo->defrag)
-                        goto out;
+        priv    = THIS->private;
 
-                defrag = volinfo->defrag;
+        GF_ASSERT (volinfo);
+        GF_ASSERT (op_errstr);
 
-                LOCK_INIT (&defrag->lock);
-                snprintf (defrag->mount, 1024, "%s/mount/%s",
-                          priv->workdir, cli_req.volname);
-                /* Create a directory, mount glusterfs over it, start glusterfs-defrag */
-                snprintf (cmd_str, 4096, "mkdir -p %s", defrag->mount);
-                ret = system (cmd_str);
+        ret = glusterd_defrag_start_validate (volinfo, op_errstr, len);
+        if (ret)
+                goto out;
+        volinfo->defrag = GF_CALLOC (1, sizeof (glusterd_defrag_info_t),
+                                     gf_gld_mt_defrag_info);
+        if (!volinfo->defrag)
+                goto out;
 
-                if (ret) {
-                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed", cmd_str);
-                        goto out;
-                }
+        defrag = volinfo->defrag;
 
-                snprintf (cmd_str, 4096, "%s/sbin/glusterfs -s localhost "
-                          "--volfile-id %s --volume-name %s-quick-read "
-                          "--xlator-option *dht.unhashed-sticky-bit=yes "
-                          "--xlator-option *dht.use-readdirp=yes "
-                          "--xlator-option *dht.lookup-unhashed=yes %s",
-                          GFS_PREFIX, cli_req.volname, cli_req.volname,
-                          defrag->mount);
-                ret = gf_system (cmd_str);
-                if (ret) {
-                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed", cmd_str);
-                        goto out;
-                }
+        LOCK_INIT (&defrag->lock);
+        snprintf (defrag->mount, 1024, "%s/mount/%s",
+                  priv->workdir, volinfo->volname);
+        /* Create a directory, mount glusterfs over it, start glusterfs-defrag */
+        snprintf (cmd_str, sizeof (cmd_str), "mkdir -p %s", defrag->mount);
+        ret = system (cmd_str);
 
-                volinfo->defrag_status = GF_DEFRAG_STATUS_STARTED;
-                rsp.op_ret = 0;
+        if (ret) {
+                gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed", cmd_str);
+                goto out;
+        }
 
-                ret = pthread_create (&defrag->th, NULL, glusterd_defrag_start,
-                                      volinfo);
-                if (ret) {
-                        snprintf (cmd_str, 1024, "umount -l %s", defrag->mount);
-                        ret = system (cmd_str);
-                        rsp.op_ret = -1;
-                }
-                break;
+        snprintf (cmd_str, sizeof (cmd_str), "%s/sbin/glusterfs -s localhost "
+                  "--volfile-id %s --volume-name %s-quick-read "
+                  "--xlator-option *dht.unhashed-sticky-bit=yes "
+                  "--xlator-option *dht.use-readdirp=yes "
+                  "--xlator-option *dht.lookup-unhashed=yes %s",
+                  GFS_PREFIX, volinfo->volname, volinfo->volname,
+                  defrag->mount);
+        ret = gf_system (cmd_str);
+        if (ret) {
+                gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed", cmd_str);
+                goto out;
+        }
+
+        volinfo->defrag_status = GF_DEFRAG_STATUS_STARTED;
+
+        ret = pthread_create (&defrag->th, NULL, glusterd_defrag_start,
+                              volinfo);
+        if (ret) {
+                snprintf (cmd_str, sizeof (cmd_str), "umount -l %s", defrag->mount);
+                if (system (cmd_str))
+                        gf_log("glusterd", GF_LOG_DEBUG, "command: %s "
+                               "failed", cmd_str);
         }
+out:
+        gf_log ("", GF_LOG_DEBUG, "Returning %d", ret);
+        return ret;
+}
+
+int
+glusterd_rebalance_cmd_validate (int cmd, char *volname,
+                                 glusterd_volinfo_t **volinfo,
+                                 char *op_errstr, size_t len)
+{
+        int ret = -1;
+
+        if (glusterd_volinfo_find(volname, volinfo)) {
+                gf_log ("glusterd", GF_LOG_ERROR, "Received rebalance on invalid"
+                        " volname %s", volname);
+                snprintf (op_errstr, len, "Volume %s does not exist",
+                          volname);
+                goto out;
+        }
+
+        if ((*volinfo)->status != GLUSTERD_STATUS_STARTED) {
+                gf_log ("glusterd", GF_LOG_ERROR, "Received rebalance on stopped"
+                        " volname %s", volname);
+                snprintf (op_errstr, len, "Volume %s needs to "
+                          "be started to perform rebalance", volname);
+                goto out;
+        }
+        ret = 0;
+out:
+        gf_log ("glusterd", GF_LOG_DEBUG, "Returning %d", ret);
+        return ret;
+}
+
+int
+glusterd_handle_defrag_volume (rpcsvc_request_t *req)
+{
+        int32_t                ret           = -1;
+        gf1_cli_defrag_vol_req cli_req       = {0,};
+        glusterd_volinfo_t      *volinfo = NULL;
+        gf1_cli_defrag_vol_rsp rsp = {0,};
+        char                   msg[2048] = {0};
+        glusterd_conf_t         *priv = NULL;
+
+        GF_ASSERT (req);
+
+        priv    = THIS->private;
+        if (!gf_xdr_to_cli_defrag_vol_req (req->msg[0], &cli_req)) {
+                //failed to decode msg;
+                req->rpc_err = GARBAGE_ARGS;
+                goto out;
+        }
+
+        glusterd_rebalance_cmd_attempted_log (cli_req.cmd, cli_req.volname);
+
+        rsp.volname = cli_req.volname;
+        rsp.op_ret = -1;
+        rsp.op_errstr = msg;
+
+        ret = glusterd_rebalance_cmd_validate (cli_req.cmd, cli_req.volname,
+                                               &volinfo, msg, sizeof (msg));
+        if (ret)
+                goto out;
+        switch (cli_req.cmd) {
+        case GF_DEFRAG_CMD_START:
+                ret = glusterd_handle_defrag_start (volinfo, msg, sizeof (msg));
+                rsp.op_ret = ret;
+                break;
         case GF_DEFRAG_CMD_STOP:
-                ret = glusterd_defrag_stop (volinfo, &rsp);
+                ret = glusterd_defrag_stop (volinfo, &rsp.files, &rsp.size,
+                                            msg, sizeof (msg));
+                rsp.op_ret = ret;
                 break;
         case GF_DEFRAG_CMD_STATUS:
                 ret = glusterd_defrag_status_get (volinfo, &rsp);
@@ -484,15 +587,7 @@  glusterd_handle_defrag_volume (rpcsvc_request_t *req)
         default:
                 break;
         }
-        if (ret)
-                gf_log("glusterd", GF_LOG_DEBUG, "command: %s failed",cmd_str);
-
-        if (cli_req.cmd != GF_DEFRAG_CMD_STATUS) {
-                gf_cmd_log ("volume rebalance"," on volname: %s %d %s",
-                            cli_req.volname,
-                            cli_req.cmd, ((ret)?"FAILED":"SUCCESS"));
-        }
-
+        glusterd_rebalance_cmd_log (cli_req.cmd, cli_req.volname, rsp.op_ret);
 out:
 
         ret = glusterd_submit_reply (req, &rsp, NULL, 0, NULL,