gNFS: More clean up for Gluster NFS

1) Fix the typo in NFS default ACL
   The typo was introduced as part of the Fix to BZ 1009210 i.e.
   http://review.gluster.org/5980. The user ACL xattr structure
   was passed to default ACL xattr.

2) Clean up NFS code to avoid unnecessary SEGV in
   rpcsvc_drc_reconfigure() which was not validating the
   svc->drc. Add a routine rpcsvc_drc_deinit() to handle
   the clean up of DRC specific data structures. For init(),
   use rpcsvc_drc_init().

3) nfs_init_state() was returning wrong value even if the
   registration with portmapper failed, causing the NFS
   server process to hang around. As a result it used to
   get SEGV during rpcsvc_drc_reconfigure().

4) Clean up memfactor usage across nfs.c nfs3.c.

Change-Id: I5cea26cb68dd8a822ec0ae104952f67fe63fa703
BUG: 1009210
Signed-off-by: Santosh Kumar Pradhan <spradhan@redhat.com>
Reviewed-on: http://review.gluster.org/6329
Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Vijay Bellur <vbellur@redhat.com>

6 files changed, 116 insertions, 76 deletions

diff --git a/rpc/rpc-lib/src/rpc-drc.c b/rpc/rpc-lib/src/rpc-drc.c
index 8181e6aeed8..e7ba114dd27 100644
--- a/rpc/rpc-lib/src/rpc-drc.c
+++ b/rpc/rpc-lib/src/rpc-drc.c
@@ -708,15 +708,10 @@ rpcsvc_drc_init (rpcsvc_t *svc, dict_t *options)
         uint32_t                    drc_size       = 0;
         uint32_t                    drc_factor     = 0;
         rpcsvc_drc_globals_t       *drc            = NULL;
-        static gf_boolean_t         drc_inited     = _gf_false;
 
         GF_ASSERT (svc);
         GF_ASSERT (options);
 
-        /* Already inited */
-        if (drc_inited)
-                return 0;
-
         if (!svc->drc) {
                 drc = GF_CALLOC (1, sizeof (rpcsvc_drc_globals_t),
                                  gf_common_mt_drc_globals_t);
@@ -742,11 +737,10 @@ rpcsvc_drc_init (rpcsvc_t *svc, dict_t *options)
                 gf_log (GF_RPCSVC, GF_LOG_INFO, "drc user options need second look");
                 ret = _gf_true;
         }
-        drc->enable_drc = ret;
 
         if (ret == _gf_false) {
                 /* drc off */
-                gf_log (GF_RPCSVC, GF_LOG_DEBUG, "DRC is off");
+                gf_log (GF_RPCSVC, GF_LOG_INFO, "DRC is manually turned OFF");
                 ret = 0;
                 goto out;
         }
@@ -802,8 +796,6 @@ rpcsvc_drc_init (rpcsvc_t *svc, dict_t *options)
 
         gf_log (GF_RPCSVC, GF_LOG_DEBUG, "drc init successful");
         drc->status = DRC_INITIATED;
-        drc_inited = _gf_true;
-
  out:
         UNLOCK (&drc->lock);
         if (ret == -1) {
@@ -818,6 +810,32 @@ rpcsvc_drc_init (rpcsvc_t *svc, dict_t *options)
 }
 
 int
+rpcsvc_drc_deinit (rpcsvc_t *svc)
+{
+        rpcsvc_drc_globals_t *drc  = NULL;
+
+        if (!svc)
+                return (-1);
+
+        drc = svc->drc;
+        if (!drc)
+                return (0);
+
+        LOCK (&drc->lock);
+        (void) rpcsvc_unregister_notify (svc, rpcsvc_drc_notify, THIS);
+        if (drc->mempool) {
+                mem_pool_destroy (drc->mempool);
+                drc->mempool = NULL;
+        }
+        UNLOCK (&drc->lock);
+
+        GF_FREE (drc);
+        svc->drc = NULL;
+
+        return (0);
+}
+
+int
 rpcsvc_drc_reconfigure (rpcsvc_t *svc, dict_t *options)
 {
         int                     ret        = -1;
@@ -825,48 +843,58 @@ rpcsvc_drc_reconfigure (rpcsvc_t *svc, dict_t *options)
         rpcsvc_drc_globals_t    *drc       = NULL;
         uint32_t                drc_size   = 0;
 
+        /* Input sanitization */
         if ((!svc) || (!options))
                 return (-1);
 
+        /* If DRC was not enabled before, Let rpcsvc_drc_init() to
+         * take care of DRC initialization part.
+         */
         drc = svc->drc;
-        /* reconfig for drc-size */
-        if (dict_get_uint32 (options, "nfs.drc-size", &drc_size))
-                drc_size = DRC_DEFAULT_CACHE_SIZE;
-
-        if (drc->global_cache_size != drc_size) {
-                gf_log (GF_RPCSVC, GF_LOG_DEBUG, "nfs.drc-size size can not "
-                        "be reconfigured without NFS server restart.");
-                return (-1);
+        if (!drc) {
+                return rpcsvc_drc_init(svc, options);
         }
 
-        /* reconfig for nfs.drc */
+        /* DRC was already enabled before. Going to be reconfigured. Check
+         * if reconfigured options contain "nfs.drc" and "nfs.drc-size".
+         *
+         * NB: If DRC is "OFF", "drc-size" has no role to play.
+         *     So, "drc-size" gets evaluated IFF DRC is "ON".
+         *
+         * If DRC is reconfigured,
+         *     case 1: DRC is "ON"
+         *         sub-case 1: drc-size remains same
+         *              ACTION: Nothing to do.
+         *         sub-case 2: drc-size just changed
+         *              ACTION: rpcsvc_drc_deinit() followed by
+         *                      rpcsvc_drc_init().
+         *
+         *     case 2: DRC is "OFF"
+         *         ACTION: rpcsvc_drc_deinit()
+         */
         ret = dict_get_str_boolean (options, "nfs.drc", _gf_true);
         if (ret < 0) {
-                ret = _gf_true;
+                enable_drc = _gf_true;
+        } else {
+                enable_drc = ret;
         }
-        enable_drc = ret;
-
-        if (drc->enable_drc == enable_drc)
-                return 0;
 
-        drc->enable_drc = enable_drc;
+        /* case 1: DRC is "ON"*/
         if (enable_drc) {
-                if (drc == NULL)
-                        return rpcsvc_drc_init(svc, options);
-        } else {
-                if (drc == NULL)
+                /* Fetch drc-size if reconfigured */
+                if (dict_get_uint32 (options, "nfs.drc-size", &drc_size))
+                        drc_size = DRC_DEFAULT_CACHE_SIZE;
+
+                /* case 1: sub-case 1*/
+                if (drc->global_cache_size == drc_size)
                         return (0);
 
-                LOCK (&drc->lock);
-                (void) rpcsvc_unregister_notify (svc, rpcsvc_drc_notify, THIS);
-                if (drc->mempool) {
-                        mem_pool_destroy (drc->mempool);
-                        drc->mempool = NULL;
-                }
-                UNLOCK (&drc->lock);
-                GF_FREE (drc);
-                svc->drc = NULL;
+                /* case 1: sub-case 2*/
+                (void) rpcsvc_drc_deinit (svc);
+                return rpcsvc_drc_init (svc, options);
         }
 
-        return (0);
+        /* case 2: DRC is "OFF" */
+        gf_log (GF_RPCSVC, GF_LOG_INFO, "DRC is manually turned OFF");
+        return rpcsvc_drc_deinit (svc);
 }
diff --git a/rpc/rpc-lib/src/rpc-drc.h b/rpc/rpc-lib/src/rpc-drc.h
index 7dfaef9783c..c42c2a2c2fe 100644
--- a/rpc/rpc-lib/src/rpc-drc.h
+++ b/rpc/rpc-lib/src/rpc-drc.h
@@ -71,7 +71,6 @@ struct drc_globals {
         struct list_head          cache_head;
         uint32_t                  client_count;
         struct list_head          clients_head;
-        gf_boolean_t              enable_drc;
 };
 
 int
@@ -99,6 +98,9 @@ int
 rpcsvc_drc_init (rpcsvc_t *svc, dict_t *options);
 
 int
+rpcsvc_drc_deinit (rpcsvc_t *svc);
+
+int
 rpcsvc_drc_reconfigure (rpcsvc_t *svc, dict_t *options);
 
 #endif /* RPC_DRC_H */
diff --git a/xlators/nfs/server/src/acl3.c b/xlators/nfs/server/src/acl3.c
index 08b099b4e60..a9843c7108b 100644
--- a/xlators/nfs/server/src/acl3.c
+++ b/xlators/nfs/server/src/acl3.c
@@ -562,7 +562,7 @@ acl3svc_setacl (rpcsvc_request_t *req)
         }
 
         /* Populate xattr buffer for Default ACL */
-        bufheader = (struct posix_acl_xattr_header *)(cs->aclxattr);
+        bufheader = (struct posix_acl_xattr_header *)(cs->daclxattr);
         bufheader->version = htole32(POSIX_ACL_VERSION);
         bufentry  = bufheader->entries;
         for (i = 0; i < cs->daclcount; i++) {
diff --git a/xlators/nfs/server/src/nfs.c b/xlators/nfs/server/src/nfs.c
index 75c8fe44eaf..8c895c66d92 100644
--- a/xlators/nfs/server/src/nfs.c
+++ b/xlators/nfs/server/src/nfs.c
@@ -959,7 +959,8 @@ nfs_init_state (xlator_t *this)
                 nfs->enable_nlm = _gf_false;
         }
 
-        nfs->rpcsvc =  rpcsvc_init (this, this->ctx, this->options, 0);
+        nfs->rpcsvc =  rpcsvc_init (this, this->ctx,
+                                    this->options, fopspoolsize);
         if (!nfs->rpcsvc) {
                 ret = -1;
                 gf_log (GF_NFS, GF_LOG_ERROR, "RPC service init failed");
@@ -995,6 +996,9 @@ nfs_drc_init (xlator_t *this)
         int       ret     = -1;
         rpcsvc_t *svc     = NULL;
 
+        GF_VALIDATE_OR_GOTO (GF_NFS, this, out);
+        GF_VALIDATE_OR_GOTO (GF_NFS, this->private, out);
+
         svc = ((struct nfs_state *)(this->private))->rpcsvc;
         if (!svc)
                 goto out;
@@ -1227,6 +1231,23 @@ reconfigure (xlator_t *this, dict_t *options)
         return (0);
 }
 
+/* Main init() routine for NFS server xlator. It inits NFS v3 protocol
+ * and its dependent protocols e.g. ACL, MOUNT v3 (mount3), NLM and
+ * DRC.
+ *
+ * Usage: glusterfsd:
+ *            glusterfs_process_volfp() =>
+ *              glusterfs_graph_activate() =>
+ *                glusterfs_graph_init() =>
+ *                  xlator_init () => NFS init() routine
+ *
+ * If init() routine fails, the glusterfsd cleans up the NFS process
+ * by invoking cleanup_and_exit().
+ *
+ * RETURN:
+ *       0 (SUCCESS) if all protocol specific inits PASS.
+ *      -1 (FAILURE) if any of them FAILS.
+ */
 int
 init (xlator_t *this) {
 
@@ -1234,59 +1255,52 @@ init (xlator_t *this) {
         int                     ret = -1;
 
         if (!this)
-                return -1;
+                return (-1);
 
         nfs = nfs_init_state (this);
         if (!nfs) {
                 gf_log (GF_NFS, GF_LOG_ERROR, "Failed to init nfs option");
-                return -1;
+                return (-1);
         }
 
         ret = nfs_add_all_initiators (nfs);
-        if (ret == -1) {
+        if (ret) {
                 gf_log (GF_NFS, GF_LOG_ERROR, "Failed to add initiators");
-                goto err;
+                return (-1);
         }
 
         ret = nfs_init_subvolumes (nfs, this->children);
-        if (ret == -1) {
-                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init NFS "
-                        "exports");
-                goto err;
+        if (ret) {
+                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init NFS exports");
+                return (-1);
         }
 
         ret = mount_init_state (this);
-        if (ret == -1) {
-                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init Mount"
-                        "state");
-                goto err;
+        if (ret) {
+                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init Mount state");
+                return (-1);
         }
 
         ret = nlm4_init_state (this);
-        if (ret == -1) {
-                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init NLM"
-                        "state");
-                goto err;
+        if (ret) {
+                gf_log (GF_NFS, GF_LOG_CRITICAL, "Failed to init NLM state");
+                return (-1);
         }
 
         ret = nfs_init_versions (nfs, this);
-        if (ret == -1) {
-                gf_log (GF_NFS, GF_LOG_ERROR, "Failed to initialize "
-                        "protocols");
-                /* Do not return an error on this. If we dont return
-                 * an error, the process keeps running and it helps
-                 * to point out where the log is by doing ps ax|grep gluster.
-                 */
-                ret = 0;
-                goto err;
+        if (ret) {
+                gf_log (GF_NFS, GF_LOG_ERROR, "Failed to initialize protocols");
+                return (-1);
         }
 
         ret = nfs_drc_init (this);
-        if (ret == 0)
-                gf_log (GF_NFS, GF_LOG_INFO, "NFS service started");
-err:
+        if (ret) {
+                gf_log (GF_NFS, GF_LOG_ERROR, "Failed to initialize DRC");
+                return (-1);
+        }
 
-        return ret;
+        gf_log (GF_NFS, GF_LOG_INFO, "NFS service started");
+        return (0); /* SUCCESS */
 }
 
 
diff --git a/xlators/nfs/server/src/nfs3.c b/xlators/nfs/server/src/nfs3.c
index f914c31937d..0fea135c784 100644
--- a/xlators/nfs/server/src/nfs3.c
+++ b/xlators/nfs/server/src/nfs3.c
@@ -5256,8 +5256,6 @@ nfs3_init_options (struct nfs3_state *nfs3, dict_t *options)
          * accommodate the NFS headers also in the same buffer. */
         nfs3->iobsize = nfs3->iobsize * 2;
 
-        /* mem-factor */
-        nfs3->memfactor = GF_NFS3_DEFAULT_MEMFACTOR;
         ret = 0;
 err:
         return ret;
@@ -5507,7 +5505,7 @@ nfs3_init_state (xlator_t *nfsx)
         unsigned int            localpool = 0;
         struct nfs_state        *nfs = NULL;
 
-        if (!nfsx)
+        if ((!nfsx) || (!nfsx->private))
                 return NULL;
 
         nfs3 = (struct nfs3_state *)GF_CALLOC (1, sizeof (*nfs3),
@@ -5526,7 +5524,7 @@ nfs3_init_state (xlator_t *nfsx)
 
         nfs3->iobpool = nfsx->ctx->iobuf_pool;
 
-        localpool = nfs3->memfactor * GF_NFS_CONCURRENT_OPS_MULT;
+        localpool = nfs->memfactor * GF_NFS_CONCURRENT_OPS_MULT;
         gf_log (GF_NFS3, GF_LOG_TRACE, "local pool: %d", localpool);
         nfs3->localpool = mem_pool_new (nfs3_call_state_t, localpool);
         if (!nfs3->localpool) {
diff --git a/xlators/nfs/server/src/nfs3.h b/xlators/nfs/server/src/nfs3.h
index 0c35445a471..023b394cf5a 100644
--- a/xlators/nfs/server/src/nfs3.h
+++ b/xlators/nfs/server/src/nfs3.h
@@ -143,8 +143,6 @@ typedef struct nfs3_state {
          */
         uint64_t                iobsize;
 
-        unsigned int            memfactor;
-
         struct list_head        fdlru;
         gf_lock_t               fdlrulock;
         int                     fdcount;