dht: send lookup on old name inside rename with bname and pargfid

Inside rename, a lookup is done on the source name to make sure that
the file is there. But we used to do a gfid based lookup and hence,
even if the source name was renamed to a new name from some other client,
lookup will be successful as server3_3_lookup will fetch the new path
based on the gfid.

So even if the source file does not exist any more rename will carry on,
and as server3_3_link(destination is hashed to a different brick other
than source cached scenario) also does gfid based resolve, it wont
detect that the source name does not exist and hardlink creation will be
successful (since gfid based resolve will get the new dentry).

To solve this problem, do a name based lookup inside rename. So that
rename will fail right away if the source does not exist.

Change-Id: Ieba8bdd6675088dbf18de90ed4622df043d163bd
BUG: 1412135
Signed-off-by: Susant Palai <spalai@redhat.com>
Reviewed-on: https://review.gluster.org/16375
Smoke: Gluster Build System <jenkins@build.gluster.org>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
Reviewed-by: N Balachandran <nbalacha@redhat.com>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Reviewed-by: Raghavendra G <rgowdapp@redhat.com>

2 files changed, 63 insertions, 10 deletions

diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c
index b1ed094e0ae..ec7cbe08ad3 100644
--- a/libglusterfs/src/xlator.c
+++ b/libglusterfs/src/xlator.c
@@ -939,7 +939,11 @@ loc_copy (loc_t *dst, loc_t *src)
         GF_VALIDATE_OR_GOTO ("xlator", dst, err);
         GF_VALIDATE_OR_GOTO ("xlator", src, err);
 
-        gf_uuid_copy (dst->gfid, src->gfid);
+        if (!gf_uuid_is_null (src->gfid))
+                gf_uuid_copy (dst->gfid, src->gfid);
+        else if (src->inode && !gf_uuid_is_null (src->inode->gfid))
+                gf_uuid_copy (dst->gfid, src->inode->gfid);
+
         gf_uuid_copy (dst->pargfid, src->pargfid);
 
         if (src->inode)
diff --git a/xlators/cluster/dht/src/dht-rename.c b/xlators/cluster/dht/src/dht-rename.c
index c24e6ea7aca..32806e911bb 100644
--- a/xlators/cluster/dht/src/dht-rename.c
+++ b/xlators/cluster/dht/src/dht-rename.c
@@ -1435,9 +1435,15 @@ dht_rename_lookup_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                        inode_t *inode, struct iatt *stbuf, dict_t *xattr,
                        struct iatt *postparent)
 {
-        dht_local_t *local                      = NULL;
-        int          call_cnt                   = 0;
-        dht_conf_t  *conf                       = NULL;
+        dht_local_t *local                              = NULL;
+        int          call_cnt                           = 0;
+        dht_conf_t  *conf                               = NULL;
+        char         gfid_local[GF_UUID_BUF_SIZE]       = {0};
+        char         gfid_server[GF_UUID_BUF_SIZE]      = {0};
+        int          child_index                        = -1;
+
+
+        child_index = (long)cookie;
 
         local = frame->local;
         conf = this->private;
@@ -1456,16 +1462,40 @@ dht_rename_lookup_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                  * that this is a linkfile and fail the rename operation.
                  */
                 local->is_linkfile = _gf_true;
+                local->op_errno = op_errno;
         } else if (xattr && check_is_linkfile (inode, stbuf, xattr,
                                                conf->link_xattr_name)) {
                 local->is_linkfile = _gf_true;
+                /* Found linkto file instead of data file, passdown ENOENT
+                 * based on the above comment */
+                local->op_errno = ENOENT;
+        }
+
+        if (!local->is_linkfile &&
+             gf_uuid_compare (local->lock[0].layout.parent_layout.locks[child_index]->loc.gfid,
+             stbuf->ia_gfid)) {
+                gf_uuid_unparse (local->lock[0].layout.parent_layout.locks[child_index]->loc.gfid,
+                                 gfid_local);
+                gf_uuid_unparse (stbuf->ia_gfid, gfid_server);
+
+                gf_msg (this->name, GF_LOG_WARNING, 0,
+                        DHT_MSG_GFID_MISMATCH,
+                        "path:%s, received a different gfid, local_gfid= %s"
+                        " server_gfid: %s",
+                        local->loc.path, gfid_local, gfid_server);
+
+                /* Will passdown ENOENT anyway since the file we sent on
+                 * rename is replaced with a different file */
+                local->op_errno = ENOENT;
+                /* Since local->is_linkfile is used here to detect failure,
+                 * marking this to true */
+                local->is_linkfile = _gf_true;
         }
 
         call_cnt = dht_frame_return (frame);
         if (is_last_call (call_cnt)) {
                 if (local->is_linkfile) {
                         local->op_ret = -1;
-                        local->op_errno = op_errno;
                         goto fail;
                 }
 
@@ -1529,12 +1559,31 @@ dht_rename_lock_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
 
         local->call_cnt = local->lock[0].layout.parent_layout.lk_count;
 
+        /* Why not use local->lock.locks[?].loc for lookup post lock phase
+         * ---------------------------------------------------------------
+         * "layout.parent_layout.locks[?].loc" does not have the name and pargfid
+         * populated.
+         * Reason: If we had populated the name and pargfid, server might
+         * resolve to a successful lookup even if there is a file with same name
+         * with a different gfid(unlink & create) as server does name based
+         * resolution on first priority. And this can result in operating on a
+         * different inode entirely.
+         *
+         * Now consider a scenario where source file was renamed by some other
+         * client to a new name just before this lock was granted. So if a
+         * lookup would be done on local->lock[0].layout.parent_layout.locks[?].loc,
+         * server will send success even if the entry was renamed (since server will
+         * do a gfid based resolution). So once a lock is granted, make sure the file
+         * exists with the name that the client requested with.
+         * */
+
         for (i = 0; i < local->lock[0].layout.parent_layout.lk_count; i++) {
-                STACK_WIND (frame, dht_rename_lookup_cbk,
-                            local->lock[0].layout.parent_layout.locks[i]->xl,
-                            local->lock[0].layout.parent_layout.locks[i]->xl->fops->lookup,
-                            &local->lock[0].layout.parent_layout.locks[i]->loc,
-                            xattr_req);
+                STACK_WIND_COOKIE (frame, dht_rename_lookup_cbk, (void *)(long)i,
+                                   local->lock[0].layout.parent_layout.locks[i]->xl,
+                                   local->lock[0].layout.parent_layout.locks[i]->xl->fops->lookup,
+                                   ((gf_uuid_compare (local->loc.gfid, \
+                                     local->lock[0].layout.parent_layout.locks[i]->loc.gfid) == 0) ?
+                                    &local->loc : &local->loc2), xattr_req);
         }
 
         dict_unref (xattr_req);