Upcall: Read gfid from iatt in case of invalid inode

soumyakoduri · nixpanic · commit ffe80877a0eb · 2015-12-01T02:32:50.000-08:00
When any file/dir is looked upon for the first time, inode created shall be invalid till it gets linked to the inode table. In such cases, read the gfid from the iatt structure returned as part of such fops for UPCALL processing. Change-Id: Ie5eb2f3be18c34cf7ef172e126c9db5ef7a8512b BUG: 1283983 Signed-off-by: Soumya Koduri <skoduri@redhat.com> Reviewed-on: http://review.gluster.org/12773 Reviewed-by: Kaleb KEITHLEY <kkeithle@redhat.com> Tested-by: NetBSD Build System <jenkins@build.gluster.org> Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Niels de Vos <ndevos@redhat.com>
diff --git a/api/src/glfs-handleops.c b/api/src/glfs-handleops.c
@@ -1288,6 +1288,10 @@ pub_glfs_h_create_from_handle (struct glfs *fs, unsigned char *handle, int len,
 
         memcpy (loc.gfid, handle, GFAPI_HANDLE_LENGTH);
 
+        /* make sure the gfid received is valid */
+        GF_VALIDATE_OR_GOTO ("glfs_h_create_from_handle",
+                             !(gf_uuid_is_null (loc.gfid)), out);
+
         newinode = inode_find (subvol->itable, loc.gfid);
         if (newinode) {
                 if (!stat) /* No need of lookup */
diff --git a/tests/basic/gfapi/bug1283983.c b/tests/basic/gfapi/bug1283983.c
@@ -0,0 +1,123 @@
+#include <fcntl.h>
+#include <unistd.h>
+#include <time.h>
+#include <limits.h>
+#include <alloca.h>
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <glusterfs/api/glfs.h>
+#include <glusterfs/api/glfs-handles.h>
+int gfapi = 1;
+
+#define LOG_ERR(func, ret) do { \
+        if (ret != 0) {            \
+                fprintf (stderr, "%s : returned error ret(%d), errno(%d)\n", \
+                         func, ret, errno); \
+                exit(1); \
+        } else { \
+                fprintf (stderr, "%s : returned %d\n", func, ret); \
+        } \
+        } while (0)
+#define LOG_IF_NO_ERR(func, ret) do { \
+        if (ret == 0) {            \
+                fprintf (stderr, "%s : hasn't returned error %d\n", \
+                         func, ret); \
+                exit(1); \
+        } else { \
+                fprintf (stderr, "%s : returned %d\n", func, ret); \
+        } \
+        } while (0)
+int
+main (int argc, char *argv[])
+{
+        glfs_t    *fs = NULL;
+        int        ret = 0, i;
+        glfs_fd_t *fd = NULL;
+        char      *filename = "/a1";
+        char      *filename2 = "/a2";
+        struct     stat sb = {0, };
+        struct    callback_arg cbk;
+        char      *logfile = NULL;
+        char      *volname = NULL;
+        int       cnt = 1;
+        struct callback_inode_arg *in_arg = NULL;
+        struct glfs_object *root = NULL, *leaf = NULL;
+
+        cbk.reason = 0;
+
+        fprintf (stderr, "Starting libgfapi_fini\n");
+        if (argc != 3) {
+                fprintf (stderr, "Invalid argument\n");
+                exit(1);
+        }
+
+        volname = argv[1];
+        logfile = argv[2];
+
+
+        fs = glfs_new (volname);
+        if (!fs) {
+                fprintf (stderr, "glfs_new: returned NULL\n");
+                return 1;
+        }
+
+        ret = glfs_set_volfile_server (fs, "tcp", "localhost", 24007);
+        LOG_ERR("glfs_set_volfile_server", ret);
+
+        ret = glfs_set_logging (fs, logfile, 7);
+        LOG_ERR("glfs_set_logging", ret);
+
+        ret = glfs_init (fs);
+        LOG_ERR("glfs_init", ret);
+
+        sleep (2);
+        root = glfs_h_lookupat (fs, NULL, "/", &sb, 0);
+        if (!root) {
+                ret = -1;
+                LOG_ERR ("glfs_h_lookupat root", ret);
+        }
+        leaf = glfs_h_lookupat (fs, root, filename, &sb, 0);
+        if (!leaf) {
+                ret = -1;
+                LOG_IF_NO_ERR ("glfs_h_lookupat leaf", ret);
+        }
+
+        leaf = glfs_h_creat (fs, root, filename, O_RDWR, 0644, &sb);
+        if (!leaf) {
+                ret = -1;
+                LOG_ERR ("glfs_h_lookupat leaf", ret);
+        }
+        fprintf (stderr, "glfs_h_create leaf - %p\n", leaf);
+
+        leaf = glfs_h_lookupat (fs, root, filename2, &sb, 0);
+        if (!leaf) {
+                ret = -1;
+                LOG_IF_NO_ERR ("glfs_h_lookupat leaf", ret);
+        }
+
+        ret = glfs_h_rename (fs, root, filename, root, filename2);
+        LOG_ERR("glfs_rename", ret);
+
+        while (cnt++ < 5) {
+                ret = glfs_h_poll_upcall(fs, &cbk);
+                LOG_ERR ("glfs_h_poll_upcall", ret);
+
+                /* There should not be any upcalls sent */
+                if (cbk.reason != GFAPI_CBK_EVENT_NULL) {
+                        fprintf (stderr, "Error: Upcall received(%d)\n",
+                                 cbk.reason);
+                        exit (1);
+                }
+        }
+
+        ret = glfs_fini(fs);
+        LOG_ERR("glfs_fini", ret);
+
+        fprintf (stderr, "End of libgfapi_fini\n");
+
+        exit(0);
+}
+
+
diff --git a/tests/basic/gfapi/bug1283983.sh b/tests/basic/gfapi/bug1283983.sh
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+cleanup;
+
+TEST glusterd
+
+TEST $CLI volume create $V0 localhost:$B0/brick1;
+EXPECT 'Created' volinfo_field $V0 'Status';
+
+TEST $CLI volume start $V0;
+EXPECT 'Started' volinfo_field $V0 'Status';
+
+logdir=`gluster --print-logdir`
+
+## Enable Upcall cache-invalidation feature
+TEST $CLI volume set $V0 features.cache-invalidation on;
+
+build_tester $(dirname $0)/bug1283983.c -lgfapi -o $(dirname $0)/bug1283983
+
+TEST ./$(dirname $0)/bug1283983 $V0  $logdir/bug1283983.log
+
+## There shouldn't be any NULL gfid messages logged
+TEST ! cat $logdir/bug1283983.log | grep "upcall" | grep "00000000-0000-0000-0000-000000000000"
+
+cleanup_tester $(dirname $0)/bug1283983
+
+TEST $CLI volume stop $V0
+TEST $CLI volume delete $V0
+
+cleanup;
diff --git a/xlators/features/upcall/src/upcall-internal.c b/xlators/features/upcall/src/upcall-internal.c
@@ -88,16 +88,14 @@ get_cache_invalidation_timeout(xlator_t *this) {
  * Allocate and add a new client entry to the given upcall entry
  */
 upcall_client_t*
-add_upcall_client (call_frame_t *frame, uuid_t gfid,
-                   client_t *client,
+add_upcall_client (call_frame_t *frame, client_t *client,
                    upcall_inode_ctx_t *up_inode_ctx)
 {
         upcall_client_t *up_client_entry = NULL;
 
         pthread_mutex_lock (&up_inode_ctx->client_list_lock);
         {
                 up_client_entry = __add_upcall_client (frame,
-                                                       gfid,
                                                        client,
                                                        up_inode_ctx);
         }
@@ -107,8 +105,7 @@ add_upcall_client (call_frame_t *frame, uuid_t gfid,
 }
 
 upcall_client_t*
-__add_upcall_client (call_frame_t *frame, uuid_t gfid,
-                     client_t *client,
+__add_upcall_client (call_frame_t *frame, client_t *client,
                      upcall_inode_ctx_t *up_inode_ctx)
 {
         upcall_client_t *up_client_entry = NULL;
@@ -137,11 +134,11 @@ __add_upcall_client (call_frame_t *frame, uuid_t gfid,
 }
 
 /*
- * Given gfid and client->uid, retrieve the corresponding upcall client entry.
+ * Given client->uid, retrieve the corresponding upcall client entry.
  * If none found, create a new entry.
  */
 upcall_client_t*
-__get_upcall_client (call_frame_t *frame, uuid_t gfid, client_t *client,
+__get_upcall_client (call_frame_t *frame, client_t *client,
                      upcall_inode_ctx_t *up_inode_ctx)
 {
         upcall_client_t *up_client_entry = NULL;
@@ -165,7 +162,7 @@ __get_upcall_client (call_frame_t *frame, uuid_t gfid, client_t *client,
         }
 
         if (!found_client) { /* create one */
-                up_client_entry = __add_upcall_client (frame, gfid, client,
+                up_client_entry = __add_upcall_client (frame, client,
                                                        up_inode_ctx);
         }
 
@@ -200,6 +197,7 @@ __upcall_inode_ctx_set (inode_t *inode, xlator_t *this)
         INIT_LIST_HEAD (&inode_ctx->inode_ctx_list);
         INIT_LIST_HEAD (&inode_ctx->client_list);
         inode_ctx->destroy = 0;
+        gf_uuid_copy (inode_ctx->gfid, inode->gfid);
 
         ctx = (long) inode_ctx;
         ret = __inode_ctx_set (inode, this, &ctx);
@@ -458,7 +456,7 @@ upcall_reaper_thread_init (xlator_t *this)
 }
 
 /*
- * Given a gfid, client, first fetch upcall_entry_t based on gfid.
+ * Given a client, first fetch upcall_entry_t from the inode_ctx client list.
  * Later traverse through the client list of that upcall entry. If this client
  * is not present in the list, create one client entry with this client info.
  * Also check if there are other clients which need to be notified of this
@@ -502,16 +500,31 @@ upcall_cache_invalidate (call_frame_t *frame, xlator_t *this, client_t *client,
                 return;
         }
 
+        /* In case of LOOKUP, if first time, inode created shall be
+         * invalid till it gets linked to inode table. Read gfid from
+         * the stat returned in such cases.
+         */
+        if (gf_uuid_is_null (up_inode_ctx->gfid)) {
+                /* That means inode must have been invalid when this inode_ctx
+                 * is created. Copy the gfid value from stbuf instead.
+                 */
+                gf_uuid_copy (up_inode_ctx->gfid, stbuf->ia_gfid);
+        }
+
+        GF_VALIDATE_OR_GOTO ("upcall_cache_invalidate",
+                             !(gf_uuid_is_null (up_inode_ctx->gfid)), out);
         pthread_mutex_lock (&up_inode_ctx->client_list_lock);
         {
                 list_for_each_entry_safe (up_client_entry, tmp,
                                           &up_inode_ctx->client_list,
                                           client_list) {
 
+                        /* Do not send UPCALL event if same client. */
                         if (!strcmp(client->client_uid,
                                    up_client_entry->client_uid)) {
                                 up_client_entry->access_time = time(NULL);
                                 found = _gf_true;
+                                continue;
                         }
 
                         /*
@@ -532,20 +545,21 @@ upcall_cache_invalidate (call_frame_t *frame, xlator_t *this, client_t *client,
                          *  expire_time_attr to 0.
                          */
                         upcall_client_cache_invalidate(this,
-                                                       inode->gfid,
+                                                       up_inode_ctx->gfid,
                                                        up_client_entry,
                                                        flags, stbuf,
                                                        p_stbuf, oldp_stbuf);
                 }
 
                 if (!found) {
                         up_client_entry = __add_upcall_client (frame,
-                                                               inode->gfid,
                                                                client,
                                                                up_inode_ctx);
                 }
         }
         pthread_mutex_unlock (&up_inode_ctx->client_list_lock);
+out:
+        return;
 }
 
 /*
@@ -565,6 +579,8 @@ upcall_client_cache_invalidate (xlator_t *this, uuid_t gfid,
         int                                 ret     = -1;
         time_t t_expired = time(NULL) - up_client_entry->access_time;
 
+        GF_VALIDATE_OR_GOTO ("upcall_client_cache_invalidate",
+                             !(gf_uuid_is_null (gfid)), out);
         timeout = get_cache_invalidation_timeout(this);
 
         if (t_expired < timeout) {
@@ -609,6 +625,8 @@ upcall_client_cache_invalidate (xlator_t *this, uuid_t gfid,
                         __upcall_cleanup_client_entry (up_client_entry);
                 }
         }
+out:
+        return;
 }
 
 /*
@@ -640,7 +658,7 @@ upcall_cache_forget (xlator_t *this, inode_t *inode, upcall_inode_ctx_t *up_inod
                         up_client_entry->access_time = time(NULL);
 
                         upcall_client_cache_invalidate(this,
-                                                       inode->gfid,
+                                                       up_inode_ctx->gfid,
                                                        up_client_entry,
                                                        flags, NULL,
                                                        NULL, NULL);
diff --git a/xlators/features/upcall/src/upcall.c b/xlators/features/upcall/src/upcall.c
@@ -161,7 +161,7 @@ up_readv_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         }
         flags = UP_UPDATE_CLIENT;
         upcall_cache_invalidate (frame, this, client, local->inode, flags,
-                                 NULL, NULL, NULL);
+                                 stbuf, NULL, NULL);
 
 out:
         UPCALL_STACK_UNWIND (readv, frame, op_ret, op_errno, vector,
@@ -747,7 +747,7 @@ up_lookup_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         }
         flags = UP_UPDATE_CLIENT;
         upcall_cache_invalidate (frame, this, client, local->inode, flags,
-                                 NULL, NULL, NULL);
+                                 stbuf, NULL, NULL);
 
 out:
         UPCALL_STACK_UNWIND (lookup, frame, op_ret, op_errno, inode, stbuf,
@@ -804,7 +804,7 @@ up_stat_cbk (call_frame_t *frame, void *cookie,
         }
         flags = UP_UPDATE_CLIENT;
         upcall_cache_invalidate (frame, this, client, local->inode, flags,
-                                 NULL, NULL, NULL);
+                                 buf, NULL, NULL);
 
 out:
         UPCALL_STACK_UNWIND (stat, frame, op_ret, op_errno, buf,
@@ -970,7 +970,7 @@ up_readlink_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         }
         flags = UP_UPDATE_CLIENT;
         upcall_cache_invalidate (frame, this, client, local->inode, flags,
-                                 NULL, NULL, NULL);
+                                 stbuf, NULL, NULL);
 
 out:
         UPCALL_STACK_UNWIND (readlink, frame, op_ret, op_errno, path, stbuf,
diff --git a/xlators/features/upcall/src/upcall.h b/xlators/features/upcall/src/upcall.h
@@ -72,6 +72,7 @@ struct _upcall_inode_ctx_t {
         pthread_mutex_t client_list_lock; /* mutex for clients list
                                              of this upcall entry */
         int destroy;
+        uuid_t   gfid; /* gfid of the entry */
 };
 typedef struct _upcall_inode_ctx_t upcall_inode_ctx_t;
 
@@ -89,14 +90,11 @@ typedef struct upcall_local upcall_local_t;
 void upcall_local_wipe (xlator_t *this, upcall_local_t *local);
 upcall_local_t *upcall_local_init (call_frame_t *frame, xlator_t *this, inode_t *inode);
 
-upcall_client_t *add_upcall_client (call_frame_t *frame, uuid_t gfid,
-                                    client_t *client,
+upcall_client_t *add_upcall_client (call_frame_t *frame, client_t *client,
                                     upcall_inode_ctx_t *up_inode_ctx);
-upcall_client_t *__add_upcall_client (call_frame_t *frame, uuid_t gfid,
-                                      client_t *client,
+upcall_client_t *__add_upcall_client (call_frame_t *frame, client_t *client,
                                       upcall_inode_ctx_t *up_inode_ctx);
-upcall_client_t *__get_upcall_client (call_frame_t *frame, uuid_t gfid,
-                                      client_t *client,
+upcall_client_t *__get_upcall_client (call_frame_t *frame, client_t *client,
                                       upcall_inode_ctx_t *up_inode_ctx);
 int __upcall_cleanup_client_entry (upcall_client_t *up_client);
 int upcall_cleanup_expired_clients (xlator_t *this,
