From 06c59167d972b7034f3ee3d93dc2629bf53043df Mon Sep 17 00:00:00 2001

From: Raghavendra Talur <rtalur@redhat.com>

Date: Wed, 2 Nov 2016 19:51:26 +0530

Subject: [PATCH 150/157] gfapi: async fops should unref in callbacks

    Backport of http://review.gluster.org/#/c/15768

If fd is unref'd at the end of async call then the unref in cbks would

lead to double unref and possible crash. Removing duplicate unrefs.

Added unref only in failure cases.

A simple test case has been added to test async write case. Need to

extend the same for other async APIs too.

Details:

All glfd based calls in libgfapi, except for glfs_open and glfs_close,

behave in the same way. At the start of the operation, they take a ref

on glfd and fd. At the end of the operation, they unref it. Async calls

are a little different as they unref in the cbk function. A successfull

open call does not unref either the glfd or fd, thereby functioning as a

reference for a OPEN file object. glfs_close makes a syncop_flush call

sandwiched between a fd ref and unref(this can be removed, more on this

below), followed by a call to glfs_mark_glfd_for_deletion which unrefs

glfd and also calls glfs_fd_destroy as a release function thereby doing

a unref on fd too.

Functionally, there is no problem with how everything works when as

described above. However, it is a little non-intuitive that we need to

perform a fd_unref as a consequence of a implicit fd_ref that happens

within glfs_resolve_fd. As we perform a GF_REF_GET(glfd) at the start of

every operation, it would be worthwhile to remove the fd_ref that

glfs_resovle_fd takes and do away with explicit fd_unref()s at the end

of every operation. This is the same reason why we don't need the fd_ref

in glfs_close. This is however not in the scope of this patch.

Change-Id: I86b1d3b2ad846b16ea527d541dc82b5e90b0ba85

BUG: 1391093

Reviewed-on: http://review.gluster.org/15768

Smoke: Gluster Build System <jenkins@build.gluster.org>

NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>

Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>

Reviewed-by: Xavier Hernandez <xhernandez@datalab.es>

Reviewed-by: soumya k <skoduri@redhat.com>

Reviewed-by: Prasanna Kumar Kalever <pkalever@redhat.com>

CentOS-regression: Gluster Build System <jenkins@build.gluster.org>

Reviewed-by: Shyamsundar Ranganathan <srangana@redhat.com>

Signed-off-by: Raghavendra Talur <rtalur@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/89229

---

 api/src/glfs-fops.c                        |   30 ++---

 tests/basic/gfapi/gfapi-async-calls-test.c |  177 ++++++++++++++++++++++++++++

 tests/basic/gfapi/gfapi-async-calls-test.t |   26 ++++

 3 files changed, 215 insertions(+), 18 deletions(-)

 create mode 100644 tests/basic/gfapi/gfapi-async-calls-test.c

 create mode 100644 tests/basic/gfapi/gfapi-async-calls-test.t

diff --git a/api/src/glfs-fops.c b/api/src/glfs-fops.c

index bbbf61d..41d4795 100644

--- a/api/src/glfs-fops.c

+++ b/api/src/glfs-fops.c

@@ -934,10 +934,9 @@ pub_glfs_preadv_async (struct glfs_fd *glfd, const struct iovec *iovec,

 			   offset, flags, NULL);

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 if (gio) {

@@ -1252,10 +1251,9 @@ pub_glfs_pwritev_async (struct glfs_fd *glfd, const struct iovec *iovec,

         ret = 0;

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 GF_FREE (gio);

@@ -1439,10 +1437,9 @@ glfs_fsync_async_common (struct glfs_fd *glfd, glfs_io_cbk fn, void *data,

                            subvol->fops->fsync, fd, dataonly, NULL);

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 GF_FREE (gio);

@@ -1693,10 +1690,9 @@ pub_glfs_ftruncate_async (struct glfs_fd *glfd, off_t offset, glfs_io_cbk fn,

         ret = 0;

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 GF_FREE (gio);

@@ -2552,10 +2548,9 @@ pub_glfs_discard_async (struct glfs_fd *glfd, off_t offset, size_t len,

         ret = 0;

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 GF_FREE (gio);

@@ -2640,10 +2635,9 @@ pub_glfs_zerofill_async (struct glfs_fd *glfd, off_t offset, off_t len,

                            subvol->fops->zerofill, fd, offset, len, NULL);

         ret = 0;

 out:

-        if (fd)

-                fd_unref (fd);

-

         if (ret) {

+                if (fd)

+                        fd_unref (fd);

                 if (glfd)

                         GF_REF_PUT (glfd);

                 GF_FREE (gio);

diff --git a/tests/basic/gfapi/gfapi-async-calls-test.c b/tests/basic/gfapi/gfapi-async-calls-test.c

new file mode 100644

index 0000000..277067b

--- /dev/null

+++ b/tests/basic/gfapi/gfapi-async-calls-test.c

@@ -0,0 +1,177 @@

+#include <fcntl.h>

+#include <unistd.h>

+#include <time.h>

+#include <limits.h>

+#include <string.h>

+#include <stdio.h>

+#include <stdlib.h>

+#include <errno.h>

+#include <glusterfs/api/glfs.h>

+#include <glusterfs/api/glfs-handles.h>

+

+#define LOG_ERR(msg) do { \

+        fprintf (stderr, "%s : Error (%s)\n", msg, strerror (errno)); \

+        } while (0)

+

+int cbk_complete = 0;

+int cbk_ret_val = -1;

+

+int

+fill_iov (struct iovec *iov, char fillchar, int count)

+{

+        int ret = -1;

+

+        iov->iov_base = calloc (count + 1, sizeof(fillchar));

+        if (iov->iov_base == NULL) {

+                return ret;

+        } else {

+                iov->iov_len = count;

+                ret = 0;

+        }

+        memset (iov->iov_base, fillchar, count);

+        memset (iov->iov_base + count, '\0', 1);

+

+        return ret;

+}

+

+glfs_t *

+init_glfs (const char *hostname, const char *volname,

+           const char *logfile)

+{

+        int     ret     = -1;

+        glfs_t *fs      = NULL;

+

+        fs = glfs_new (volname);

+        if (!fs) {

+                LOG_ERR ("glfs_new failed");

+                return NULL;

+        }

+

+        ret = glfs_set_volfile_server (fs, "tcp", hostname, 24007);

+        if (ret < 0) {

+                LOG_ERR ("glfs_set_volfile_server failed");

+                goto out;

+        }

+

+        ret = glfs_set_logging (fs, logfile, 7);

+        if (ret < 0) {

+                LOG_ERR ("glfs_set_logging failed");

+                goto out;

+        }

+

+        ret = glfs_init (fs);

+        if (ret < 0) {

+                LOG_ERR ("glfs_init failed");

+                goto out;

+        }

+

+        ret = 0;

+out:

+        if (ret) {

+                glfs_fini (fs);

+                fs = NULL;

+        }

+

+        return fs;

+}

+

+void

+write_async_cbk (glfs_fd_t *fd, ssize_t ret, void *cookie)

+{

+

+        if (ret < 0) {

+                LOG_ERR ("glfs_write failed");

+        }

+        cbk_ret_val = ret;

+        cbk_complete = 1;

+}

+

+int

+write_async (glfs_t *fs, glfs_fd_t *glfd, int char_count)

+{

+        ssize_t         ret          = -1;

+        int             flags        = O_RDWR;

+        const char     *buff         = "This is from my prog\n";

+        struct iovec    iov          = {0};

+        void           *write_cookie = NULL;

+        void           *read_cookie  = NULL;

+

+

+

+        ret = fill_iov (&iov, 'a', char_count);

+        if (ret) {

+                LOG_ERR ("failed to create iov");

+                goto out;

+        }

+

+        write_cookie = strdup ("write_cookie");

+        ret = glfs_pwritev_async (glfd, &iov, 1, 0, flags, write_async_cbk,

+                                  &write_cookie);

+out:

+        if (ret < 0) {

+                LOG_ERR ("glfs_pwritev async failed");

+        }

+        return ret;

+

+}

+

+int

+main (int argc, char *argv[])

+{

+        int         ret      = 0;

+        char       *hostname = NULL;

+        char       *volname  = NULL;

+        char       *logfile  = NULL;

+        glfs_t     *fs       = NULL;

+        const char *filename = "glfs_test.txt";

+        int         flags    = (O_RDWR|O_CREAT);

+        glfs_fd_t  *glfd     = NULL;

+        int         count    = 200;

+

+        if (argc != 4) {

+                fprintf (stderr, "Invalid argument\n");

+                exit(1);

+        }

+

+        hostname = argv[1];

+        volname = argv[2];

+        logfile = argv[3];

+

+        fs = init_glfs (hostname, volname, logfile);

+        if (fs == NULL) {

+                LOG_ERR ("init_glfs failed");

+                return -1;

+        }

+

+        glfd = glfs_creat (fs, filename, flags, 0644);

+        if (glfd == NULL) {

+                LOG_ERR ("glfs_creat failed");

+                exit(1);

+        }

+

+        ret = write_async (fs, glfd, count);

+        if (ret) {

+                LOG_ERR ("glfs_test_function failed");

+                exit(1);

+        }

+

+        while (cbk_complete != 1) {

+                sleep(1);

+        }

+

+        ret = glfs_close (glfd);

+        if (ret < 0) {

+                LOG_ERR ("glfs close  failed");

+        }

+

+        /*

+         * skipping fini

+         */

+

+        if (cbk_ret_val == count)

+                return 0;

+        else

+                return -1;

+}

+

+

diff --git a/tests/basic/gfapi/gfapi-async-calls-test.t b/tests/basic/gfapi/gfapi-async-calls-test.t

new file mode 100644

index 0000000..9e5cd7c

--- /dev/null

+++ b/tests/basic/gfapi/gfapi-async-calls-test.t

@@ -0,0 +1,26 @@

+#!/bin/bash

+

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

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

+

+cleanup;

+EXIT_EARLY=1;

+

+TEST glusterd

+

+TEST $CLI volume create $V0 ${H0}:$B0/brick1;

+EXPECT 'Created' volinfo_field $V0 'Status';

+

+TEST $CLI volume start $V0;

+EXPECT 'Started' volinfo_field $V0 'Status';

+

+logdir=`gluster --print-logdir`

+

+TEST build_tester $(dirname $0)/gfapi-async-calls-test.c -lgfapi

+

+

+TEST ./$(dirname $0)/gfapi-async-calls-test ${H0} $V0 $logdir/gfapi-async-calls-test.log

+

+cleanup_tester $(dirname $0)/gfapi-async-calls-test

+

+cleanup;

-- 

1.7.1