mgmt/gluster: Use fsync instead of O_SYNC

Glusterd uses O_SYNC to write to temp file then performs renames
to the actual file and performs fsync on parent directory. Until
this rename happens syncing writes to the file can be deferred.
In this patch O_SYNC open of temp file is removed and fsync of the
fd before rename is done.

Change-Id: Ie7da161b0daec845c7dcfab4154cc45c2f49d825
BUG: 908277
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
Reviewed-on: http://review.gluster.org/7370
Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com>
Reviewed-by: Raghavendra Bhat <raghavendra@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Anand Avati <avati@redhat.com>

6 files changed, 40 insertions, 45 deletions

diff --git a/libglusterfs/src/store.c b/libglusterfs/src/store.c
index 5beafaf3551..55a2ab459e2 100644
--- a/libglusterfs/src/store.c
+++ b/libglusterfs/src/store.c
@@ -59,20 +59,19 @@ gf_store_handle_create_on_absence (gf_store_handle_t **shandle,
 int32_t
 gf_store_mkstemp (gf_store_handle_t *shandle)
 {
-        int     fd = -1;
         char    tmppath[PATH_MAX] = {0,};
 
         GF_VALIDATE_OR_GOTO ("store", shandle, out);
         GF_VALIDATE_OR_GOTO ("store", shandle->path, out);
 
         snprintf (tmppath, sizeof (tmppath), "%s.tmp", shandle->path);
-        fd = open (tmppath, O_RDWR | O_CREAT | O_TRUNC | O_SYNC, 0600);
-        if (fd <= 0) {
+        shandle->tmp_fd = open (tmppath, O_RDWR | O_CREAT | O_TRUNC, 0600);
+        if (shandle->tmp_fd < 0) {
                 gf_log ("", GF_LOG_ERROR, "Failed to open %s, error: %s",
                         tmppath, strerror (errno));
         }
 out:
-        return fd;
+        return shandle->tmp_fd;
 }
 
 int
@@ -130,6 +129,12 @@ gf_store_rename_tmppath (gf_store_handle_t *shandle)
         GF_VALIDATE_OR_GOTO ("store", shandle, out);
         GF_VALIDATE_OR_GOTO ("store", shandle->path, out);
 
+        ret = fsync (shandle->tmp_fd);
+        if (ret) {
+                gf_log (THIS->name, GF_LOG_ERROR, "Failed to fsync %s, "
+                        "error: %s", shandle->path, strerror (errno));
+                goto out;
+        }
         snprintf (tmppath, sizeof (tmppath), "%s.tmp", shandle->path);
         ret = rename (tmppath, shandle->path);
         if (ret) {
@@ -140,6 +145,10 @@ gf_store_rename_tmppath (gf_store_handle_t *shandle)
 
         ret = gf_store_sync_direntry (tmppath);
 out:
+        if (shandle && shandle->tmp_fd >= 0) {
+                close (shandle->tmp_fd);
+                shandle->tmp_fd = -1;
+        }
         return ret;
 }
 
@@ -161,6 +170,10 @@ gf_store_unlink_tmppath (gf_store_handle_t *shandle)
                 ret = 0;
         }
 out:
+        if (shandle && shandle->tmp_fd >= 0) {
+                close (shandle->tmp_fd);
+                shandle->tmp_fd = -1;
+        }
         return ret;
 }
 
@@ -390,6 +403,7 @@ gf_store_handle_new (char *path, gf_store_handle_t **handle)
         shandle->path = spath;
         shandle->locked = F_ULOCK;
         *handle = shandle;
+        shandle->tmp_fd = -1;
 
         ret = 0;
 out:
diff --git a/libglusterfs/src/store.h b/libglusterfs/src/store.h
index 337103ff73e..beb17ac4a2f 100644
--- a/libglusterfs/src/store.h
+++ b/libglusterfs/src/store.h
@@ -20,6 +20,7 @@
 struct gf_store_handle_ {
         char    *path;
         int     fd;
+        int     tmp_fd;
         FILE    *read;
         int     locked;   /* state of lockf() */
 };
diff --git a/xlators/mgmt/glusterd/src/glusterd-quota.c b/xlators/mgmt/glusterd/src/glusterd-quota.c
index 7f798ad26de..3ed0196c15a 100644
--- a/xlators/mgmt/glusterd/src/glusterd-quota.c
+++ b/xlators/mgmt/glusterd/src/glusterd-quota.c
@@ -746,10 +746,6 @@ out:
                 close (conf_fd);
         }
 
-        if (fd != -1) {
-                close (fd);
-        }
-
         if (ret && (fd > 0)) {
                 gf_store_unlink_tmppath (volinfo->quota_conf_shandle);
         } else if (!ret) {
diff --git a/xlators/mgmt/glusterd/src/glusterd-store.c b/xlators/mgmt/glusterd/src/glusterd-store.c
index 26de774e3f5..eda176d1b6b 100644
--- a/xlators/mgmt/glusterd/src/glusterd-store.c
+++ b/xlators/mgmt/glusterd/src/glusterd-store.c
@@ -330,8 +330,6 @@ glusterd_store_perform_brick_store (glusterd_brickinfo_t *brickinfo)
 out:
         if (ret && (fd > 0))
                 gf_store_unlink_tmppath (brickinfo->shandle);
-        if (fd > 0)
-                close (fd);
         gf_log (THIS->name, GF_LOG_DEBUG, "Returning %d", ret);
         return ret;
 }
@@ -1094,8 +1092,6 @@ glusterd_store_perform_rbstate_store (glusterd_volinfo_t *volinfo)
 out:
         if (ret && (fd > 0))
                 gf_store_unlink_tmppath (volinfo->rb_shandle);
-        if (fd > 0)
-                close (fd);
         gf_log (THIS->name, GF_LOG_DEBUG, "Returning %d", ret);
         return ret;
 }
@@ -1175,8 +1171,6 @@ glusterd_store_perform_node_state_store (glusterd_volinfo_t *volinfo)
 out:
         if (ret && (fd > 0))
                 gf_store_unlink_tmppath (volinfo->node_state_shandle);
-        if (fd > 0)
-                close (fd);
         gf_log (THIS->name, GF_LOG_DEBUG, "Returning %d", ret);
         return ret;
 }
@@ -1205,8 +1199,6 @@ glusterd_store_perform_volume_store (glusterd_volinfo_t *volinfo)
 out:
         if (ret && (fd > 0))
                 gf_store_unlink_tmppath (volinfo->shandle);
-        if (fd > 0)
-                close (fd);
         gf_log (THIS->name, GF_LOG_DEBUG, "Returning %d", ret);
         return ret;
 }
@@ -1733,7 +1725,6 @@ out:
                         gf_store_unlink_tmppath (handle);
 
                 if (handle->fd > 0) {
-                        close (handle->fd);
                         handle->fd = 0;
                 }
         }
@@ -2705,9 +2696,9 @@ glusterd_store_options (xlator_t *this, dict_t *opts)
         if (ret)
                 goto out;
 out:
+        if ((ret < 0) && (fd > 0))
+                gf_store_unlink_tmppath (shandle);
         gf_store_handle_destroy (shandle);
-        if (fd >=0 )
-                close (fd);
         return ret;
 }
 
@@ -3455,9 +3446,6 @@ out:
                 ret = -1;
         }
 
-        if (fd > 0)
-                close (fd);
-
         gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
         return ret;
 }
@@ -3681,8 +3669,6 @@ glusterd_store_perform_peer_store (glusterd_peerinfo_t *peerinfo)
 out:
         if (ret && (fd > 0))
                 gf_store_unlink_tmppath (peerinfo->shandle);
-        if (fd > 0)
-                close (fd);
         gf_log ("", GF_LOG_DEBUG, "Returning %d", ret);
         return ret;
 }
@@ -4114,28 +4100,28 @@ out:
 int
 glusterd_store_save_quota_version_and_cksum (glusterd_volinfo_t *volinfo)
 {
-        int                 ret                  = -1;
-        char                cksum_path[PATH_MAX] = {0,};
-        char                path[PATH_MAX]       = {0,};
-        xlator_t           *this                 = NULL;
-        glusterd_conf_t    *conf                 = NULL;
-        char                buf[256]             = {0,};
-        int                 fd                   = -1;
+        gf_store_handle_t               *shandle = NULL;
+        glusterd_conf_t                 *conf = NULL;
+        xlator_t                        *this = NULL;
+        char                            path[PATH_MAX] = {0};
+        char                            cksum_path[PATH_MAX] = {0,};
+        char                            buf[256] = {0};
+        int                             fd = -1;
+        int32_t                         ret = -1;
 
         this = THIS;
-        GF_ASSERT (this);
         conf = this->private;
-        GF_ASSERT (conf);
 
         GLUSTERD_GET_VOLUME_DIR (path, volinfo, conf);
         snprintf (cksum_path, sizeof (cksum_path), "%s/%s", path,
                   GLUSTERD_VOL_QUOTA_CKSUM_FILE);
 
-        fd = open (cksum_path, O_RDWR | O_APPEND | O_CREAT| O_TRUNC, 0600);
+        ret = gf_store_handle_new (cksum_path, &shandle);
+        if (ret)
+                goto out;
 
-        if (-1 == fd) {
-                gf_log (this->name, GF_LOG_ERROR, "Unable to open %s,"
-                        "Reason: %s", cksum_path, strerror (errno));
+        fd = gf_store_mkstemp (shandle);
+        if (fd <= 0) {
                 ret = -1;
                 goto out;
         }
@@ -4155,10 +4141,13 @@ glusterd_store_save_quota_version_and_cksum (glusterd_volinfo_t *volinfo)
                 goto out;
         }
 
-        ret = 0;
+        ret = gf_store_rename_tmppath (shandle);
+        if (ret)
+                goto out;
 
 out:
-        if (fd != -1)
-                close (fd);
+        if ((ret < 0) && (fd > 0))
+                gf_store_unlink_tmppath (shandle);
+        gf_store_handle_destroy (shandle);
         return ret;
 }
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 7fd7eaeec5c..eb5cb33bb0e 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -3698,9 +3698,6 @@ glusterd_import_quota_conf (dict_t *peer_data, int vol_idx,
         ret = 0;
 
 out:
-        if (fd != -1)
-                close (fd);
-
         if (!ret) {
                 ret = glusterd_compute_cksum (new_volinfo, _gf_true);
                 if (ret)
diff --git a/xlators/nfs/server/src/mount3.c b/xlators/nfs/server/src/mount3.c
index 47ff3845e9d..e4cfcebefc5 100644
--- a/xlators/nfs/server/src/mount3.c
+++ b/xlators/nfs/server/src/mount3.c
@@ -356,7 +356,6 @@ __mount_rewrite_rmtab(struct mount3_state *ms, gf_store_handle_t *sh)
 
         gf_log (GF_MNT, GF_LOG_DEBUG, "Updated rmtab with %d entries", idx);
 
-        close (fd);
         if (gf_store_rename_tmppath (sh))
                 gf_log (GF_MNT, GF_LOG_ERROR, "Failed to overwrite rwtab %s",
                         sh->path);
@@ -365,7 +364,6 @@ __mount_rewrite_rmtab(struct mount3_state *ms, gf_store_handle_t *sh)
 
 fail:
         gf_log (GF_MNT, GF_LOG_ERROR, "Failed to update %s", sh->path);
-        close (fd);
         gf_store_unlink_tmppath (sh);
 }