From a19fa252942938a308ffa655fca3814d0660c6e2 Mon Sep 17 00:00:00 2001

From: Vinayakswami Hariharmath <vharihar@redhat.com>

Date: Wed, 3 Jun 2020 18:58:56 +0530

Subject: [PATCH 563/584] features/shard: Use fd lookup post file open

Issue:

When a process has the open fd and the same file is

unlinked in middle of the operations, then file based

lookup fails with ENOENT or stale file

Solution:

When the file already open and fd is available, use fstat

to get the file attributes

Backport of:

> Upstream-patch: https://review.gluster.org/#/c/glusterfs/+/24528/

> Change-Id: I0e83aee9f11b616dcfe13769ebfcda6742e4e0f4

> Fixes: #1281

> Signed-off-by: Vinayakswami Hariharmath <vharihar@redhat.com>

BUG: 1925425

Change-Id: I0e83aee9f11b616dcfe13769ebfcda6742e4e0f4

Signed-off-by: Vinayakswami Hariharmath <vharihar@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/c/rhs-glusterfs/+/244957

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 tests/bugs/shard/issue-1281.t      |  34 +++++++++++

 xlators/features/shard/src/shard.c | 119 +++++++++++++++++++++++--------------

 2 files changed, 110 insertions(+), 43 deletions(-)

 create mode 100644 tests/bugs/shard/issue-1281.t

diff --git a/tests/bugs/shard/issue-1281.t b/tests/bugs/shard/issue-1281.t

new file mode 100644

index 0000000..9704caa

--- /dev/null

+++ b/tests/bugs/shard/issue-1281.t

@@ -0,0 +1,34 @@

+#!/bin/bash

+

+. $(dirname $0)/../../include.rc

+

+cleanup;

+

+TEST glusterd

+TEST pidof glusterd

+TEST $CLI volume create $V0 $H0:$B0/${V0}{0,1}

+TEST $CLI volume set $V0 features.shard on

+TEST $CLI volume set $V0 performance.quick-read off

+TEST $CLI volume set $V0 performance.io-cache off

+TEST $CLI volume set $V0 performance.read-ahead off

+TEST $CLI volume set $V0 performance.write-behind off

+TEST $CLI volume start $V0

+

+TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0

+

+#Open a file and store descriptor in fd = 5

+exec 5>$M0/foo

+

+#Unlink the same file which is opened in prev step

+TEST unlink $M0/foo

+

+#Write something on the file using the open fd = 5

+echo "issue-1281" >&5

+

+#Write on the descriptor should be succesful

+EXPECT 0 echo $?

+

+#Close the fd = 5

+exec 5>&-

+

+cleanup

diff --git a/xlators/features/shard/src/shard.c b/xlators/features/shard/src/shard.c

index c5cc224..2ba4528 100644

--- a/xlators/features/shard/src/shard.c

+++ b/xlators/features/shard/src/shard.c

@@ -1653,26 +1653,24 @@ err:

 }

 int

-shard_lookup_base_file_cbk(call_frame_t *frame, void *cookie, xlator_t *this,

-                           int32_t op_ret, int32_t op_errno, inode_t *inode,

-                           struct iatt *buf, dict_t *xdata,

-                           struct iatt *postparent)

+shard_set_iattr_invoke_post_handler(call_frame_t *frame, xlator_t *this,

+                                    inode_t *inode, int32_t op_ret,

+                                    int32_t op_errno, struct iatt *buf,

+                                    dict_t *xdata)

 {

     int ret = -1;

     int32_t mask = SHARD_INODE_WRITE_MASK;

-    shard_local_t *local = NULL;

+    shard_local_t *local = frame->local;

     shard_inode_ctx_t ctx = {

         0,

     };

-    local = frame->local;

-

     if (op_ret < 0) {

         gf_msg(this->name, GF_LOG_ERROR, op_errno,

                SHARD_MSG_BASE_FILE_LOOKUP_FAILED,

                "Lookup on base file"

                " failed : %s",

-               loc_gfid_utoa(&(local->loc)));

+               uuid_utoa(inode->gfid));

         local->op_ret = op_ret;

         local->op_errno = op_errno;

         goto unwind;

@@ -1706,18 +1704,57 @@ unwind:

 }

 int

-shard_lookup_base_file(call_frame_t *frame, xlator_t *this, loc_t *loc,

-                       shard_post_fop_handler_t handler)

+shard_fstat_base_file_cbk(call_frame_t *frame, void *cookie, xlator_t *this,

+                          int32_t op_ret, int32_t op_errno, struct iatt *buf,

+                          dict_t *xdata)

+{

+    shard_local_t *local = frame->local;

+

+    shard_set_iattr_invoke_post_handler(frame, this, local->fd->inode, op_ret,

+                                        op_errno, buf, xdata);

+    return 0;

+}

+

+int

+shard_lookup_base_file_cbk(call_frame_t *frame, void *cookie, xlator_t *this,

+                           int32_t op_ret, int32_t op_errno, inode_t *inode,

+                           struct iatt *buf, dict_t *xdata,

+                           struct iatt *postparent)

+{

+    /* In case of op_ret < 0, inode passed to this function will be NULL

+       ex: in case of op_errno = ENOENT. So refer prefilled inode data

+       which is part of local.

+       Note: Reassigning/overriding the inode passed to this cbk with inode

+       which is part of *struct shard_local_t* won't cause any issue as

+       both inodes have same reference/address as of the inode passed */

+    inode = ((shard_local_t *)frame->local)->loc.inode;

+

+    shard_set_iattr_invoke_post_handler(frame, this, inode, op_ret, op_errno,

+                                        buf, xdata);

+    return 0;

+}

+

+/* This function decides whether to make file based lookup or

+ * fd based lookup (fstat) depending on the 3rd and 4th arg.

+ * If fd != NULL and loc == NULL then call is for fstat

+ * If fd == NULL and loc != NULL then call is for file based

+ * lookup. Please pass args based on the requirement.

+ */

+int

+shard_refresh_base_file(call_frame_t *frame, xlator_t *this, loc_t *loc,

+                        fd_t *fd, shard_post_fop_handler_t handler)

 {

     int ret = -1;

+    inode_t *inode = NULL;

     shard_local_t *local = NULL;

     dict_t *xattr_req = NULL;

     gf_boolean_t need_refresh = _gf_false;

     local = frame->local;

     local->handler = handler;

+    inode = fd ? fd->inode : loc->inode;

-    ret = shard_inode_ctx_fill_iatt_from_cache(loc->inode, this, &local->prebuf,

+    ret = shard_inode_ctx_fill_iatt_from_cache(inode, this, &local->prebuf,

                                                &need_refresh);

     /* By this time, inode ctx should have been created either in create,

      * mknod, readdirp or lookup. If not it is a bug!

@@ -1726,7 +1763,7 @@ shard_lookup_base_file(call_frame_t *frame, xlator_t *this, loc_t *loc,

         gf_msg_debug(this->name, 0,

                      "Skipping lookup on base file: %s"

                      "Serving prebuf off the inode ctx cache",

-                     uuid_utoa(loc->gfid));

+                     uuid_utoa(inode->gfid));

         goto out;

     }

@@ -1737,10 +1774,14 @@ shard_lookup_base_file(call_frame_t *frame, xlator_t *this, loc_t *loc,

         goto out;

     }

-    SHARD_MD_READ_FOP_INIT_REQ_DICT(this, xattr_req, loc->gfid, local, out);

+    SHARD_MD_READ_FOP_INIT_REQ_DICT(this, xattr_req, inode->gfid, local, out);

-    STACK_WIND(frame, shard_lookup_base_file_cbk, FIRST_CHILD(this),

-               FIRST_CHILD(this)->fops->lookup, loc, xattr_req);

+    if (fd)

+        STACK_WIND(frame, shard_fstat_base_file_cbk, FIRST_CHILD(this),

+                   FIRST_CHILD(this)->fops->fstat, fd, xattr_req);

+    else

+        STACK_WIND(frame, shard_lookup_base_file_cbk, FIRST_CHILD(this),

+                   FIRST_CHILD(this)->fops->lookup, loc, xattr_req);

     dict_unref(xattr_req);

     return 0;

@@ -2718,8 +2759,8 @@ shard_truncate(call_frame_t *frame, xlator_t *this, loc_t *loc, off_t offset,

     local->resolver_base_inode = loc->inode;

     GF_ATOMIC_INIT(local->delta_blocks, 0);

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_truncate_handler);

+    shard_refresh_base_file(frame, this, &local->loc, NULL,

+                            shard_post_lookup_truncate_handler);

     return 0;

 err:

@@ -2774,8 +2815,8 @@ shard_ftruncate(call_frame_t *frame, xlator_t *this, fd_t *fd, off_t offset,

     local->resolver_base_inode = fd->inode;

     GF_ATOMIC_INIT(local->delta_blocks, 0);

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_truncate_handler);

+    shard_refresh_base_file(frame, this, NULL, fd,

+                            shard_post_lookup_truncate_handler);

     return 0;

 err:

     shard_common_failure_unwind(GF_FOP_FTRUNCATE, frame, -1, ENOMEM);

@@ -2919,8 +2960,8 @@ shard_link(call_frame_t *frame, xlator_t *this, loc_t *oldloc, loc_t *newloc,

     if (!local->xattr_req)

         goto err;

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_link_handler);

+    shard_refresh_base_file(frame, this, &local->loc, NULL,

+                            shard_post_lookup_link_handler);

     return 0;

 err:

     shard_common_failure_unwind(GF_FOP_LINK, frame, -1, ENOMEM);

@@ -4249,8 +4290,8 @@ shard_post_inodelk_fop_handler(call_frame_t *frame, xlator_t *this)

     switch (local->fop) {

         case GF_FOP_UNLINK:

         case GF_FOP_RENAME:

-            shard_lookup_base_file(frame, this, &local->int_inodelk.loc,

-                                   shard_post_lookup_base_shard_rm_handler);

+            shard_refresh_base_file(frame, this, &local->int_inodelk.loc, NULL,

+                                    shard_post_lookup_base_shard_rm_handler);

             break;

         default:

             gf_msg(this->name, GF_LOG_WARNING, 0, SHARD_MSG_INVALID_FOP,

@@ -4505,8 +4546,8 @@ shard_rename_src_cbk(call_frame_t *frame, void *cookie, xlator_t *this,

     if (local->block_size) {

         local->tmp_loc.inode = inode_new(this->itable);

         gf_uuid_copy(local->tmp_loc.gfid, (local->loc.inode)->gfid);

-        shard_lookup_base_file(frame, this, &local->tmp_loc,

-                               shard_post_rename_lookup_handler);

+        shard_refresh_base_file(frame, this, &local->tmp_loc, NULL,

+                                shard_post_rename_lookup_handler);

     } else {

         shard_rename_cbk(frame, this);

     }

@@ -5242,8 +5283,8 @@ shard_readv(call_frame_t *frame, xlator_t *this, fd_t *fd, size_t size,

     local->loc.inode = inode_ref(fd->inode);

     gf_uuid_copy(local->loc.gfid, fd->inode->gfid);

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_readv_handler);

+    shard_refresh_base_file(frame, this, NULL, fd,

+                            shard_post_lookup_readv_handler);

     return 0;

 err:

     shard_common_failure_unwind(GF_FOP_READ, frame, -1, ENOMEM);

@@ -6046,8 +6087,8 @@ shard_fsync(call_frame_t *frame, xlator_t *this, fd_t *fd, int32_t datasync,

     local->loc.inode = inode_ref(fd->inode);

     gf_uuid_copy(local->loc.gfid, fd->inode->gfid);

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_fsync_handler);

+    shard_refresh_base_file(frame, this, NULL, fd,

+                            shard_post_lookup_fsync_handler);

     return 0;

 err:

     shard_common_failure_unwind(GF_FOP_FSYNC, frame, -1, ENOMEM);

@@ -6420,12 +6461,8 @@ shard_common_remove_xattr(call_frame_t *frame, xlator_t *this,

     if (xdata)

         local->xattr_req = dict_ref(xdata);

-    /* To-Do: Switch from LOOKUP which is path-based, to FSTAT if the fop is

-     * on an fd. This comes under a generic class of bugs in shard tracked by

-     * bz #1782428.

-     */

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_remove_xattr_handler);

+    shard_refresh_base_file(frame, this, loc, fd,

+                            shard_post_lookup_remove_xattr_handler);

     return 0;

 err:

     shard_common_failure_unwind(fop, frame, -1, op_errno);

@@ -6662,12 +6699,8 @@ shard_common_set_xattr(call_frame_t *frame, xlator_t *this, glusterfs_fop_t fop,

     if (xdata)

         local->xattr_rsp = dict_ref(xdata);

-    /* To-Do: Switch from LOOKUP which is path-based, to FSTAT if the fop is

-     * on an fd. This comes under a generic class of bugs in shard tracked by

-     * bz #1782428.

-     */

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_post_lookup_set_xattr_handler);

+    shard_refresh_base_file(frame, this, loc, fd,

+                            shard_post_lookup_set_xattr_handler);

     return 0;

 err:

     shard_common_failure_unwind(fop, frame, -1, op_errno);

@@ -6951,8 +6984,8 @@ shard_common_inode_write_begin(call_frame_t *frame, xlator_t *this,

     local->loc.inode = inode_ref(fd->inode);

     gf_uuid_copy(local->loc.gfid, fd->inode->gfid);

-    shard_lookup_base_file(frame, this, &local->loc,

-                           shard_common_inode_write_post_lookup_handler);

+    shard_refresh_base_file(frame, this, NULL, fd,

+                            shard_common_inode_write_post_lookup_handler);

     return 0;

 out:

     shard_common_failure_unwind(fop, frame, -1, ENOMEM);

-- 

1.8.3.1