ca3909
From 546f412c155dd5aca2b3cd4202f80c9977b215dc Mon Sep 17 00:00:00 2001
ca3909
From: Pranith Kumar K <pkarampu@redhat.com>
ca3909
Date: Wed, 4 Sep 2019 12:06:34 +0530
ca3909
Subject: [PATCH 287/297] cluster/ec: Fail fsync/flush for files on update
ca3909
 size/version failure
ca3909
ca3909
Problem:
ca3909
If update size/version is not successful on the file, updates on the
ca3909
same stripe could lead to data corruptions if the earlier un-aligned
ca3909
write is not successful on all the bricks. Application won't have
ca3909
any knowledge of this because update size/version happens in the
ca3909
background.
ca3909
ca3909
Fix:
ca3909
Fail fsync/flush on fds that are opened before update-size-version
ca3909
went bad.
ca3909
ca3909
Upstream-patch: https://review.gluster.org/c/glusterfs/+/23355
ca3909
fixes: bz#1745107
ca3909
Change-Id: I9d323eddcda703bd27d55f340c4079d76e06e492
ca3909
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
ca3909
Reviewed-on: https://code.engineering.redhat.com/gerrit/180672
ca3909
Tested-by: RHGS Build Bot <nigelb@redhat.com>
ca3909
Reviewed-by: Ashish Pandey <aspandey@redhat.com>
ca3909
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>
ca3909
---
ca3909
 tests/basic/ec/ec-badfd.c            | 124 +++++++++++++++++++++++++++++++++++
ca3909
 tests/basic/ec/ec-badfd.t            |  26 ++++++++
ca3909
 xlators/cluster/ec/src/ec-common.c   |  23 +++++++
ca3909
 xlators/cluster/ec/src/ec-generic.c  |  47 +++++++++++++
ca3909
 xlators/cluster/ec/src/ec-helpers.c  |   7 ++
ca3909
 xlators/cluster/ec/src/ec-messages.h |   2 +-
ca3909
 xlators/cluster/ec/src/ec-types.h    |   2 +
ca3909
 7 files changed, 230 insertions(+), 1 deletion(-)
ca3909
 create mode 100644 tests/basic/ec/ec-badfd.c
ca3909
 create mode 100755 tests/basic/ec/ec-badfd.t
ca3909
ca3909
diff --git a/tests/basic/ec/ec-badfd.c b/tests/basic/ec/ec-badfd.c
ca3909
new file mode 100644
ca3909
index 0000000..8be23c1
ca3909
--- /dev/null
ca3909
+++ b/tests/basic/ec/ec-badfd.c
ca3909
@@ -0,0 +1,124 @@
ca3909
+#include <stdio.h>
ca3909
+#include <fcntl.h>
ca3909
+#include <unistd.h>
ca3909
+#include <time.h>
ca3909
+#include <limits.h>
ca3909
+#include <string.h>
ca3909
+#include <stdlib.h>
ca3909
+#include <errno.h>
ca3909
+#include <glusterfs/api/glfs.h>
ca3909
+#include <glusterfs/api/glfs-handles.h>
ca3909
+
ca3909
+int
ca3909
+fill_iov(struct iovec *iov, char fillchar, int count)
ca3909
+{
ca3909
+    int ret = -1;
ca3909
+
ca3909
+    iov->iov_base = malloc(count + 1);
ca3909
+    if (iov->iov_base == NULL) {
ca3909
+        return ret;
ca3909
+    } else {
ca3909
+        iov->iov_len = count;
ca3909
+        ret = 0;
ca3909
+    }
ca3909
+    memset(iov->iov_base, fillchar, count);
ca3909
+    memset(iov->iov_base + count, '\0', 1);
ca3909
+
ca3909
+    return ret;
ca3909
+}
ca3909
+
ca3909
+int
ca3909
+write_sync(glfs_t *fs, glfs_fd_t *glfd, int char_count)
ca3909
+{
ca3909
+    ssize_t ret = -1;
ca3909
+    int flags = O_RDWR;
ca3909
+    struct iovec iov = {0};
ca3909
+
ca3909
+    ret = fill_iov(&iov, 'a', char_count);
ca3909
+    if (ret) {
ca3909
+        fprintf(stderr, "failed to create iov");
ca3909
+        goto out;
ca3909
+    }
ca3909
+
ca3909
+    ret = glfs_pwritev(glfd, &iov, 1, 0, flags);
ca3909
+out:
ca3909
+    if (ret < 0) {
ca3909
+        fprintf(stderr, "glfs_pwritev failed, %d", errno);
ca3909
+    }
ca3909
+    return ret;
ca3909
+}
ca3909
+
ca3909
+int
ca3909
+main(int argc, char *argv[])
ca3909
+{
ca3909
+    glfs_t *fs = NULL;
ca3909
+    glfs_fd_t *fd = NULL;
ca3909
+    int ret = 1;
ca3909
+    char volume_cmd[4096] = {0};
ca3909
+
ca3909
+    if (argc != 4) {
ca3909
+        fprintf(stderr, "Syntax: %s <host> <volname> <file>\n", argv[0]);
ca3909
+        return 1;
ca3909
+    }
ca3909
+
ca3909
+    fs = glfs_new(argv[2]);
ca3909
+    if (!fs) {
ca3909
+        fprintf(stderr, "glfs_new: returned NULL\n");
ca3909
+        return 1;
ca3909
+    }
ca3909
+
ca3909
+    ret = glfs_set_volfile_server(fs, "tcp", argv[1], 24007);
ca3909
+    if (ret != 0) {
ca3909
+        fprintf(stderr, "glfs_set_volfile_server: returned %d\n", ret);
ca3909
+        goto out;
ca3909
+    }
ca3909
+    ret = glfs_set_logging(fs, "/tmp/ec-badfd.log", 7);
ca3909
+    if (ret != 0) {
ca3909
+        fprintf(stderr, "glfs_set_logging: returned %d\n", ret);
ca3909
+        goto out;
ca3909
+    }
ca3909
+    ret = glfs_init(fs);
ca3909
+    if (ret != 0) {
ca3909
+        fprintf(stderr, "glfs_init: returned %d\n", ret);
ca3909
+        goto out;
ca3909
+    }
ca3909
+
ca3909
+    fd = glfs_open(fs, argv[3], O_RDWR);
ca3909
+    if (fd == NULL) {
ca3909
+        fprintf(stderr, "glfs_open: returned NULL\n");
ca3909
+        goto out;
ca3909
+    }
ca3909
+
ca3909
+    ret = write_sync(fs, fd, 16);
ca3909
+    if (ret < 0) {
ca3909
+        fprintf(stderr, "write_sync failed\n");
ca3909
+    }
ca3909
+
ca3909
+    snprintf(volume_cmd, sizeof(volume_cmd),
ca3909
+             "gluster --mode=script volume stop %s", argv[2]);
ca3909
+    /*Stop the volume so that update-size-version fails*/
ca3909
+    system(volume_cmd);
ca3909
+    sleep(8); /* 3 seconds more than eager-lock-timeout*/
ca3909
+    snprintf(volume_cmd, sizeof(volume_cmd),
ca3909
+             "gluster --mode=script volume start %s", argv[2]);
ca3909
+    system(volume_cmd);
ca3909
+    sleep(8); /*wait for bricks to come up*/
ca3909
+    ret = glfs_fsync(fd, NULL, NULL);
ca3909
+    if (ret == 0) {
ca3909
+        fprintf(stderr, "fsync succeeded on a BADFD\n");
ca3909
+        exit(1);
ca3909
+    }
ca3909
+
ca3909
+    ret = glfs_close(fd);
ca3909
+    if (ret == 0) {
ca3909
+        fprintf(stderr, "flush succeeded on a BADFD\n");
ca3909
+        exit(1);
ca3909
+    }
ca3909
+    ret = 0;
ca3909
+
ca3909
+out:
ca3909
+    unlink("/tmp/ec-badfd.log");
ca3909
+    glfs_fini(fs);
ca3909
+
ca3909
+    return ret;
ca3909
+}
ca3909
diff --git a/tests/basic/ec/ec-badfd.t b/tests/basic/ec/ec-badfd.t
ca3909
new file mode 100755
ca3909
index 0000000..56feb47
ca3909
--- /dev/null
ca3909
+++ b/tests/basic/ec/ec-badfd.t
ca3909
@@ -0,0 +1,26 @@
ca3909
+#!/bin/bash
ca3909
+
ca3909
+. $(dirname $0)/../../include.rc
ca3909
+. $(dirname $0)/../../volume.rc
ca3909
+
ca3909
+cleanup;
ca3909
+
ca3909
+TEST glusterd
ca3909
+TEST pidof glusterd
ca3909
+
ca3909
+TEST $CLI volume create $V0 disperse 6 redundancy 2 $H0:$B0/${V0}{1..6}
ca3909
+TEST $CLI volume set $V0 performance.write-behind off
ca3909
+TEST $CLI volume set $V0 disperse.eager-lock-timeout 5
ca3909
+
ca3909
+TEST $CLI volume start $V0
ca3909
+EXPECT 'Started' volinfo_field $V0 'Status'
ca3909
+
ca3909
+TEST $GFS -s $H0 --volfile-id $V0 $M0
ca3909
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "6" ec_child_up_count $V0 0
ca3909
+TEST touch $M0/file
ca3909
+
ca3909
+TEST build_tester $(dirname $0)/ec-badfd.c -lgfapi -Wall -O2
ca3909
+TEST $(dirname $0)/ec-badfd $H0 $V0 /file
ca3909
+cleanup_tester $(dirname ${0})/ec-badfd
ca3909
+
ca3909
+cleanup;
ca3909
diff --git a/xlators/cluster/ec/src/ec-common.c b/xlators/cluster/ec/src/ec-common.c
ca3909
index 5fb4610..92d4e5d 100644
ca3909
--- a/xlators/cluster/ec/src/ec-common.c
ca3909
+++ b/xlators/cluster/ec/src/ec-common.c
ca3909
@@ -2255,6 +2255,23 @@ ec_unlock_lock(ec_lock_link_t *link)
ca3909
     }
ca3909
 }
ca3909
 
ca3909
+void
ca3909
+ec_inode_bad_inc(inode_t *inode, xlator_t *xl)
ca3909
+{
ca3909
+    ec_inode_t *ctx = NULL;
ca3909
+
ca3909
+    LOCK(&inode->lock);
ca3909
+    {
ca3909
+        ctx = __ec_inode_get(inode, xl);
ca3909
+        if (ctx == NULL) {
ca3909
+            goto unlock;
ca3909
+        }
ca3909
+        ctx->bad_version++;
ca3909
+    }
ca3909
+unlock:
ca3909
+    UNLOCK(&inode->lock);
ca3909
+}
ca3909
+
ca3909
 int32_t
ca3909
 ec_update_size_version_done(call_frame_t *frame, void *cookie, xlator_t *this,
ca3909
                             int32_t op_ret, int32_t op_errno, dict_t *xattr,
ca3909
@@ -2270,6 +2287,12 @@ ec_update_size_version_done(call_frame_t *frame, void *cookie, xlator_t *this,
ca3909
     ctx = lock->ctx;
ca3909
 
ca3909
     if (op_ret < 0) {
ca3909
+        if (link->lock->fd == NULL) {
ca3909
+            ec_inode_bad_inc(link->lock->loc.inode, this);
ca3909
+        } else {
ca3909
+            ec_inode_bad_inc(link->lock->fd->inode, this);
ca3909
+        }
ca3909
+
ca3909
         gf_msg(fop->xl->name, fop_log_level(fop->id, op_errno), op_errno,
ca3909
                EC_MSG_SIZE_VERS_UPDATE_FAIL,
ca3909
                "Failed to update version and size. %s", ec_msg_str(fop));
ca3909
diff --git a/xlators/cluster/ec/src/ec-generic.c b/xlators/cluster/ec/src/ec-generic.c
ca3909
index acc16b5..b019050 100644
ca3909
--- a/xlators/cluster/ec/src/ec-generic.c
ca3909
+++ b/xlators/cluster/ec/src/ec-generic.c
ca3909
@@ -150,6 +150,37 @@ ec_manager_flush(ec_fop_data_t *fop, int32_t state)
ca3909
     }
ca3909
 }
ca3909
 
ca3909
+static int32_t
ca3909
+ec_validate_fd(fd_t *fd, xlator_t *xl)
ca3909
+{
ca3909
+    uint64_t iversion = 0;
ca3909
+    uint64_t fversion = 0;
ca3909
+    ec_inode_t *inode_ctx = NULL;
ca3909
+    ec_fd_t *fd_ctx = NULL;
ca3909
+
ca3909
+    LOCK(&fd->lock);
ca3909
+    {
ca3909
+        fd_ctx = __ec_fd_get(fd, xl);
ca3909
+        if (fd_ctx) {
ca3909
+            fversion = fd_ctx->bad_version;
ca3909
+        }
ca3909
+    }
ca3909
+    UNLOCK(&fd->lock);
ca3909
+
ca3909
+    LOCK(&fd->inode->lock);
ca3909
+    {
ca3909
+        inode_ctx = __ec_inode_get(fd->inode, xl);
ca3909
+        if (inode_ctx) {
ca3909
+            iversion = inode_ctx->bad_version;
ca3909
+        }
ca3909
+    }
ca3909
+    UNLOCK(&fd->inode->lock);
ca3909
+    if (fversion < iversion) {
ca3909
+        return EBADF;
ca3909
+    }
ca3909
+    return 0;
ca3909
+}
ca3909
+
ca3909
 void
ca3909
 ec_flush(call_frame_t *frame, xlator_t *this, uintptr_t target,
ca3909
          uint32_t fop_flags, fop_flush_cbk_t func, void *data, fd_t *fd,
ca3909
@@ -165,6 +196,14 @@ ec_flush(call_frame_t *frame, xlator_t *this, uintptr_t target,
ca3909
     GF_VALIDATE_OR_GOTO(this->name, frame, out);
ca3909
     GF_VALIDATE_OR_GOTO(this->name, this->private, out);
ca3909
 
ca3909
+    error = ec_validate_fd(fd, this);
ca3909
+    if (error) {
ca3909
+        gf_msg(this->name, GF_LOG_ERROR, EBADF, EC_MSG_FD_BAD,
ca3909
+               "Failing %s on %s", gf_fop_list[GF_FOP_FLUSH],
ca3909
+               fd->inode ? uuid_utoa(fd->inode->gfid) : "");
ca3909
+        goto out;
ca3909
+    }
ca3909
+
ca3909
     fop = ec_fop_data_allocate(frame, this, GF_FOP_FLUSH, 0, target, fop_flags,
ca3909
                                ec_wind_flush, ec_manager_flush, callback, data);
ca3909
     if (fop == NULL) {
ca3909
@@ -381,6 +420,14 @@ ec_fsync(call_frame_t *frame, xlator_t *this, uintptr_t target,
ca3909
     GF_VALIDATE_OR_GOTO(this->name, frame, out);
ca3909
     GF_VALIDATE_OR_GOTO(this->name, this->private, out);
ca3909
 
ca3909
+    error = ec_validate_fd(fd, this);
ca3909
+    if (error) {
ca3909
+        gf_msg(this->name, GF_LOG_ERROR, EBADF, EC_MSG_FD_BAD,
ca3909
+               "Failing %s on %s", gf_fop_list[GF_FOP_FSYNC],
ca3909
+               fd->inode ? uuid_utoa(fd->inode->gfid) : "");
ca3909
+        goto out;
ca3909
+    }
ca3909
+
ca3909
     fop = ec_fop_data_allocate(frame, this, GF_FOP_FSYNC, 0, target, fop_flags,
ca3909
                                ec_wind_fsync, ec_manager_fsync, callback, data);
ca3909
     if (fop == NULL) {
ca3909
diff --git a/xlators/cluster/ec/src/ec-helpers.c b/xlators/cluster/ec/src/ec-helpers.c
ca3909
index 43f6e3b..baac001 100644
ca3909
--- a/xlators/cluster/ec/src/ec-helpers.c
ca3909
+++ b/xlators/cluster/ec/src/ec-helpers.c
ca3909
@@ -753,6 +753,7 @@ __ec_fd_get(fd_t *fd, xlator_t *xl)
ca3909
 {
ca3909
     int i = 0;
ca3909
     ec_fd_t *ctx = NULL;
ca3909
+    ec_inode_t *ictx = NULL;
ca3909
     uint64_t value = 0;
ca3909
     ec_t *ec = xl->private;
ca3909
 
ca3909
@@ -775,6 +776,12 @@ __ec_fd_get(fd_t *fd, xlator_t *xl)
ca3909
                 GF_FREE(ctx);
ca3909
                 return NULL;
ca3909
             }
ca3909
+            /* Only refering bad-version so no need for lock
ca3909
+             * */
ca3909
+            ictx = __ec_inode_get(fd->inode, xl);
ca3909
+            if (ictx) {
ca3909
+                ctx->bad_version = ictx->bad_version;
ca3909
+            }
ca3909
         }
ca3909
     } else {
ca3909
         ctx = (ec_fd_t *)(uintptr_t)value;
ca3909
diff --git a/xlators/cluster/ec/src/ec-messages.h b/xlators/cluster/ec/src/ec-messages.h
ca3909
index 7c28808..be86b37 100644
ca3909
--- a/xlators/cluster/ec/src/ec-messages.h
ca3909
+++ b/xlators/cluster/ec/src/ec-messages.h
ca3909
@@ -55,6 +55,6 @@ GLFS_MSGID(EC, EC_MSG_INVALID_CONFIG, EC_MSG_HEAL_FAIL,
ca3909
            EC_MSG_CONFIG_XATTR_INVALID, EC_MSG_EXTENSION, EC_MSG_EXTENSION_NONE,
ca3909
            EC_MSG_EXTENSION_UNKNOWN, EC_MSG_EXTENSION_UNSUPPORTED,
ca3909
            EC_MSG_EXTENSION_FAILED, EC_MSG_NO_GF, EC_MSG_MATRIX_FAILED,
ca3909
-           EC_MSG_DYN_CREATE_FAILED, EC_MSG_DYN_CODEGEN_FAILED);
ca3909
+           EC_MSG_DYN_CREATE_FAILED, EC_MSG_DYN_CODEGEN_FAILED, EC_MSG_FD_BAD);
ca3909
 
ca3909
 #endif /* !_EC_MESSAGES_H_ */
ca3909
diff --git a/xlators/cluster/ec/src/ec-types.h b/xlators/cluster/ec/src/ec-types.h
ca3909
index 1c295c0..f27f2ec 100644
ca3909
--- a/xlators/cluster/ec/src/ec-types.h
ca3909
+++ b/xlators/cluster/ec/src/ec-types.h
ca3909
@@ -150,6 +150,7 @@ struct _ec_fd {
ca3909
     loc_t loc;
ca3909
     uintptr_t open;
ca3909
     int32_t flags;
ca3909
+    uint64_t bad_version;
ca3909
     ec_fd_status_t fd_status[0];
ca3909
 };
ca3909
 
ca3909
@@ -180,6 +181,7 @@ struct _ec_inode {
ca3909
     uint64_t dirty[2];
ca3909
     struct list_head heal;
ca3909
     ec_stripe_list_t stripe_cache;
ca3909
+    uint64_t bad_version;
ca3909
 };
ca3909
 
ca3909
 typedef int32_t (*fop_heal_cbk_t)(call_frame_t *, void *, xlator_t *, int32_t,
ca3909
-- 
ca3909
1.8.3.1
ca3909