From b924c8ca8a133fc9413c8ed1407e63f1658c7e79 Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@redhat.com>

Date: Tue, 12 May 2020 23:54:54 +0200

Subject: [PATCH 523/526] open-behind: rewrite of internal logic

There was a critical flaw in the previous implementation of open-behind.

When an open is done in the background, it's necessary to take a

reference on the fd_t object because once we "fake" the open answer,

the fd could be destroyed. However as long as there's a reference,

the release function won't be called. So, if the application closes

the file descriptor without having actually opened it, there will

always remain at least 1 reference, causing a leak.

To avoid this problem, the previous implementation didn't take a

reference on the fd_t, so there were races where the fd could be

destroyed while it was still in use.

To fix this, I've implemented a new xlator cbk that gets called from

fuse when the application closes a file descriptor.

The whole logic of handling background opens have been simplified and

it's more efficient now. Only if the fop needs to be delayed until an

open completes, a stub is created. Otherwise no memory allocations are

needed.

Correctly handling the close request while the open is still pending

has added a bit of complexity, but overall normal operation is simpler.

Upstream patch:

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

> Change-Id: I6376a5491368e0e1c283cc452849032636261592

> Fixes: #1225

> Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

BUG: 1830713

Change-Id: I6376a5491368e0e1c283cc452849032636261592

Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

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

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

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

---

 libglusterfs/src/fd.c                              |   26 +

 libglusterfs/src/glusterfs/fd.h                    |    3 +

 libglusterfs/src/glusterfs/xlator.h                |    4 +

 libglusterfs/src/libglusterfs.sym                  |    1 +

 tests/basic/open-behind/open-behind.t              |  183 +++

 tests/basic/open-behind/tester-fd.c                |   99 ++

 tests/basic/open-behind/tester.c                   |  444 +++++++

 tests/basic/open-behind/tester.h                   |  145 +++

 tests/bugs/glusterfs/bug-873962-spb.t              |    1 +

 xlators/mount/fuse/src/fuse-bridge.c               |    2 +

 .../open-behind/src/open-behind-messages.h         |    6 +-

 xlators/performance/open-behind/src/open-behind.c  | 1302 ++++++++------------

 12 files changed, 1393 insertions(+), 823 deletions(-)

 create mode 100644 tests/basic/open-behind/open-behind.t

 create mode 100644 tests/basic/open-behind/tester-fd.c

 create mode 100644 tests/basic/open-behind/tester.c

 create mode 100644 tests/basic/open-behind/tester.h

diff --git a/libglusterfs/src/fd.c b/libglusterfs/src/fd.c

index 314546a..e4ec401 100644

--- a/libglusterfs/src/fd.c

+++ b/libglusterfs/src/fd.c

@@ -501,6 +501,32 @@ out:

 }

 void

+fd_close(fd_t *fd)

+{

+    xlator_t *xl, *old_THIS;

+

+    old_THIS = THIS;

+

+    for (xl = fd->inode->table->xl->graph->first; xl != NULL; xl = xl->next) {

+        if (!xl->call_cleanup) {

+            THIS = xl;

+

+            if (IA_ISDIR(fd->inode->ia_type)) {

+                if (xl->cbks->fdclosedir != NULL) {

+                    xl->cbks->fdclosedir(xl, fd);

+                }

+            } else {

+                if (xl->cbks->fdclose != NULL) {

+                    xl->cbks->fdclose(xl, fd);

+                }

+            }

+        }

+    }

+

+    THIS = old_THIS;

+}

+

+void

 fd_unref(fd_t *fd)

 {

     int32_t refcount = 0;

diff --git a/libglusterfs/src/glusterfs/fd.h b/libglusterfs/src/glusterfs/fd.h

index cdbe289..4d157c4 100644

--- a/libglusterfs/src/glusterfs/fd.h

+++ b/libglusterfs/src/glusterfs/fd.h

@@ -107,6 +107,9 @@ fd_ref(fd_t *fd);

 void

 fd_unref(fd_t *fd);

+void

+fd_close(fd_t *fd);

+

 fd_t *

 fd_create(struct _inode *inode, pid_t pid);

diff --git a/libglusterfs/src/glusterfs/xlator.h b/libglusterfs/src/glusterfs/xlator.h

index 8650ccc..273039a 100644

--- a/libglusterfs/src/glusterfs/xlator.h

+++ b/libglusterfs/src/glusterfs/xlator.h

@@ -705,6 +705,8 @@ typedef size_t (*cbk_inodectx_size_t)(xlator_t *this, inode_t *inode);

 typedef size_t (*cbk_fdctx_size_t)(xlator_t *this, fd_t *fd);

+typedef void (*cbk_fdclose_t)(xlator_t *this, fd_t *fd);

+

 struct xlator_cbks {

     cbk_forget_t forget;

     cbk_release_t release;

@@ -715,6 +717,8 @@ struct xlator_cbks {

     cbk_ictxmerge_t ictxmerge;

     cbk_inodectx_size_t ictxsize;

     cbk_fdctx_size_t fdctxsize;

+    cbk_fdclose_t fdclose;

+    cbk_fdclose_t fdclosedir;

 };

 typedef int32_t (*dumpop_priv_t)(xlator_t *this);

diff --git a/libglusterfs/src/libglusterfs.sym b/libglusterfs/src/libglusterfs.sym

index bc770e2..0a0862e 100644

--- a/libglusterfs/src/libglusterfs.sym

+++ b/libglusterfs/src/libglusterfs.sym

@@ -456,6 +456,7 @@ event_unregister_close

 fd_anonymous

 fd_anonymous_with_flags

 fd_bind

+fd_close

 fd_create

 fd_create_uint64

 __fd_ctx_del

diff --git a/tests/basic/open-behind/open-behind.t b/tests/basic/open-behind/open-behind.t

new file mode 100644

index 0000000..5e865d6

--- /dev/null

+++ b/tests/basic/open-behind/open-behind.t

@@ -0,0 +1,183 @@

+#!/bin/bash

+

+WD="$(dirname "${0}")"

+

+. ${WD}/../../include.rc

+. ${WD}/../../volume.rc

+

+function assign() {

+    local _assign_var="${1}"

+    local _assign_value="${2}"

+

+    printf -v "${_assign_var}" "%s" "${_assign_value}"

+}

+

+function pipe_create() {

+    local _pipe_create_var="${1}"

+    local _pipe_create_name

+    local _pipe_create_fd

+

+    _pipe_create_name="$(mktemp -u)"

+    mkfifo "${_pipe_create_name}"

+    exec {_pipe_create_fd}<>"${_pipe_create_name}"

+    rm "${_pipe_create_name}"

+

+    assign "${_pipe_create_var}" "${_pipe_create_fd}"

+}

+

+function pipe_close() {

+    local _pipe_close_fd="${!1}"

+

+    exec {_pipe_close_fd}>&-

+}

+

+function tester_start() {

+    declare -ag tester

+    local tester_in

+    local tester_out

+

+    pipe_create tester_in

+    pipe_create tester_out

+

+    ${WD}/tester <&${tester_in} >&${tester_out} &

+

+    tester=("$!" "${tester_in}" "${tester_out}")

+}

+

+function tester_send() {

+    declare -ag tester

+    local tester_res

+    local tester_extra

+

+    echo "${*}" >&${tester[1]}

+

+    read -t 3 -u ${tester[2]} tester_res tester_extra

+    echo "${tester_res} ${tester_extra}"

+    if [[ "${tester_res}" == "OK" ]]; then

+        return 0

+    fi

+

+    return 1

+}

+

+function tester_stop() {

+    declare -ag tester

+    local tester_res

+

+    tester_send "quit"

+

+    tester_res=0

+    if ! wait ${tester[0]}; then

+        tester_res=$?

+    fi

+

+    unset tester

+

+    return ${tester_res}

+}

+

+function count_open() {

+    local file="$(realpath "${B0}/${V0}/${1}")"

+    local count="0"

+    local inode

+    local ref

+

+    inode="$(stat -c %i "${file}")"

+

+    for fd in /proc/${BRICK_PID}/fd/*; do

+        ref="$(readlink "${fd}")"

+        if [[ "${ref}" == "${B0}/${V0}/"* ]]; then

+            if [[ "$(stat -c %i "${ref}")" == "${inode}" ]]; then

+                count="$((${count} + 1))"

+            fi

+        fi

+    done

+

+    echo "${count}"

+}

+

+cleanup

+

+TEST build_tester ${WD}/tester.c ${WD}/tester-fd.c

+

+TEST glusterd

+TEST pidof glusterd

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

+TEST ${CLI} volume set ${V0} flush-behind off

+TEST ${CLI} volume set ${V0} write-behind off

+TEST ${CLI} volume set ${V0} quick-read off

+TEST ${CLI} volume set ${V0} stat-prefetch on

+TEST ${CLI} volume set ${V0} io-cache off

+TEST ${CLI} volume set ${V0} open-behind on

+TEST ${CLI} volume set ${V0} lazy-open off

+TEST ${CLI} volume set ${V0} read-after-open off

+TEST ${CLI} volume start ${V0}

+

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+BRICK_PID="$(get_brick_pid ${V0} ${H0} ${B0}/${V0})"

+

+TEST touch "${M0}/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+TEST tester_start

+

+TEST tester_send fd open 0 "${M0}/test"

+EXPECT_WITHIN 5 "1" count_open "/test"

+TEST tester_send fd close 0

+EXPECT_WITHIN 5 "0" count_open "/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+TEST ${CLI} volume set ${V0} lazy-open on

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+TEST tester_send fd open 0 "${M0}/test"

+sleep 2

+EXPECT "0" count_open "/test"

+TEST tester_send fd write 0 "test"

+EXPECT "1" count_open "/test"

+TEST tester_send fd close 0

+EXPECT_WITHIN 5 "0" count_open "/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+TEST tester_send fd open 0 "${M0}/test"

+EXPECT "0" count_open "/test"

+EXPECT "test" tester_send fd read 0 64

+# Even though read-after-open is disabled, use-anonymous-fd is also disabled,

+# so reads need to open the file first.

+EXPECT "1" count_open "/test"

+TEST tester_send fd close 0

+EXPECT "0" count_open "/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+TEST tester_send fd open 0 "${M0}/test"

+EXPECT "0" count_open "/test"

+TEST tester_send fd open 1 "${M0}/test"

+EXPECT "2" count_open "/test"

+TEST tester_send fd close 0

+EXPECT_WITHIN 5 "1" count_open "/test"

+TEST tester_send fd close 1

+EXPECT_WITHIN 5 "0" count_open "/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+TEST ${CLI} volume set ${V0} read-after-open on

+TEST ${GFS} --volfile-id=/${V0} --volfile-server=${H0} ${M0};

+

+TEST tester_send fd open 0 "${M0}/test"

+EXPECT "0" count_open "/test"

+EXPECT "test" tester_send fd read 0 64

+EXPECT "1" count_open "/test"

+TEST tester_send fd close 0

+EXPECT_WITHIN 5 "0" count_open "/test"

+

+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0

+

+TEST tester_stop

+

+cleanup

diff --git a/tests/basic/open-behind/tester-fd.c b/tests/basic/open-behind/tester-fd.c

new file mode 100644

index 0000000..00f02bc

--- /dev/null

+++ b/tests/basic/open-behind/tester-fd.c

@@ -0,0 +1,99 @@

+/*

+  Copyright (c) 2020 Red Hat, Inc. <http://www.redhat.com>

+  This file is part of GlusterFS.

+

+  This file is licensed to you under your choice of the GNU Lesser

+  General Public License, version 3 or any later version (LGPLv3 or

+  later), or the GNU General Public License, version 2 (GPLv2), in all

+  cases as published by the Free Software Foundation.

+*/

+

+#include "tester.h"

+

+#include <stdlib.h>

+#include <unistd.h>

+#include <sys/types.h>

+#include <sys/stat.h>

+#include <fcntl.h>

+#include <string.h>

+#include <ctype.h>

+#include <errno.h>

+

+static int32_t

+fd_open(context_t *ctx, command_t *cmd)

+{

+    obj_t *obj;

+    int32_t fd;

+

+    obj = cmd->args[0].obj.ref;

+

+    fd = open(cmd->args[1].str.data, O_RDWR);

+    if (fd < 0) {

+        return error(errno, "open() failed");

+    }

+

+    obj->type = OBJ_TYPE_FD;

+    obj->fd = fd;

+

+    out_ok("%d", fd);

+

+    return 0;

+}

+

+static int32_t

+fd_close(context_t *ctx, command_t *cmd)

+{

+    obj_t *obj;

+

+    obj = cmd->args[0].obj.ref;

+    obj->type = OBJ_TYPE_NONE;

+

+    if (close(obj->fd) != 0) {

+        return error(errno, "close() failed");

+    }

+

+    out_ok();

+

+    return 0;

+}

+

+static int32_t

+fd_write(context_t *ctx, command_t *cmd)

+{

+    ssize_t len, ret;

+

+    len = strlen(cmd->args[1].str.data);

+    ret = write(cmd->args[0].obj.ref->fd, cmd->args[1].str.data, len);

+    if (ret < 0) {

+        return error(errno, "write() failed");

+    }

+

+    out_ok("%zd", ret);

+

+    return 0;

+}

+

+static int32_t

+fd_read(context_t *ctx, command_t *cmd)

+{

+    char data[cmd->args[1].num.value + 1];

+    ssize_t ret;

+

+    ret = read(cmd->args[0].obj.ref->fd, data, cmd->args[1].num.value);

+    if (ret < 0) {

+        return error(errno, "read() failed");

+    }

+

+    data[ret] = 0;

+

+    out_ok("%zd %s", ret, data);

+

+    return 0;

+}

+

+command_t fd_commands[] = {

+    {"open", fd_open, CMD_ARGS(ARG_VAL(OBJ_TYPE_NONE), ARG_STR(1024))},

+    {"close", fd_close, CMD_ARGS(ARG_VAL(OBJ_TYPE_FD))},

+    {"write", fd_write, CMD_ARGS(ARG_VAL(OBJ_TYPE_FD), ARG_STR(1024))},

+    {"read", fd_read, CMD_ARGS(ARG_VAL(OBJ_TYPE_FD), ARG_NUM(0, 1024))},

+    CMD_END};

diff --git a/tests/basic/open-behind/tester.c b/tests/basic/open-behind/tester.c

new file mode 100644

index 0000000..b2da71c

--- /dev/null

+++ b/tests/basic/open-behind/tester.c

@@ -0,0 +1,444 @@

+/*

+  Copyright (c) 2020 Red Hat, Inc. <http://www.redhat.com>

+  This file is part of GlusterFS.

+

+  This file is licensed to you under your choice of the GNU Lesser

+  General Public License, version 3 or any later version (LGPLv3 or

+  later), or the GNU General Public License, version 2 (GPLv2), in all

+  cases as published by the Free Software Foundation.

+*/

+

+#include "tester.h"

+

+#include <stdlib.h>

+#include <unistd.h>

+#include <string.h>

+#include <ctype.h>

+#include <errno.h>

+

+static void *

+mem_alloc(size_t size)

+{

+    void *ptr;

+

+    ptr = malloc(size);

+    if (ptr == NULL) {

+        error(ENOMEM, "Failed to allocate memory (%zu bytes)", size);

+    }

+

+    return ptr;

+}

+

+static void

+mem_free(void *ptr)

+{

+    free(ptr);

+}

+

+static bool

+buffer_create(context_t *ctx, size_t size)

+{

+    ctx->buffer.base = mem_alloc(size);

+    if (ctx->buffer.base == NULL) {

+        return false;

+    }

+

+    ctx->buffer.size = size;

+    ctx->buffer.len = 0;

+    ctx->buffer.pos = 0;

+

+    return true;

+}

+

+static void

+buffer_destroy(context_t *ctx)

+{

+    mem_free(ctx->buffer.base);

+    ctx->buffer.size = 0;

+    ctx->buffer.len = 0;

+}

+

+static int32_t

+buffer_get(context_t *ctx)

+{

+    ssize_t len;

+

+    if (ctx->buffer.pos >= ctx->buffer.len) {

+        len = read(0, ctx->buffer.base, ctx->buffer.size);

+        if (len < 0) {

+            return error(errno, "read() failed");

+        }

+        if (len == 0) {

+            return 0;

+        }

+

+        ctx->buffer.len = len;

+        ctx->buffer.pos = 0;

+    }

+

+    return ctx->buffer.base[ctx->buffer.pos++];

+}

+

+static int32_t

+str_skip_spaces(context_t *ctx, int32_t current)

+{

+    while ((current > 0) && (current != '\n') && isspace(current)) {

+        current = buffer_get(ctx);

+    }

+

+    return current;

+}

+

+static int32_t

+str_token(context_t *ctx, char *buffer, uint32_t size, int32_t current)

+{

+    uint32_t len;

+

+    current = str_skip_spaces(ctx, current);

+

+    len = 0;

+    while ((size > 0) && (current > 0) && (current != '\n') &&

+           !isspace(current)) {

+        len++;

+        *buffer++ = current;

+        size--;

+        current = buffer_get(ctx);

+    }

+

+    if (len == 0) {

+        return error(ENODATA, "Expecting a token");

+    }

+

+    if (size == 0) {

+        return error(ENOBUFS, "Token too long");

+    }

+

+    *buffer = 0;

+

+    return current;

+}

+

+static int32_t

+str_number(context_t *ctx, uint64_t min, uint64_t max, uint64_t *value,

+           int32_t current)

+{

+    char text[32], *ptr;

+    uint64_t num;

+

+    current = str_token(ctx, text, sizeof(text), current);

+    if (current > 0) {

+        num = strtoul(text, &ptr, 0);

+        if ((*ptr != 0) || (num < min) || (num > max)) {

+            return error(ERANGE, "Invalid number");

+        }

+        *value = num;

+    }

+

+    return current;

+}

+

+static int32_t

+str_eol(context_t *ctx, int32_t current)

+{

+    current = str_skip_spaces(ctx, current);

+    if (current != '\n') {

+        return error(EINVAL, "Expecting end of command");

+    }

+

+    return current;

+}

+

+static void

+str_skip(context_t *ctx, int32_t current)

+{

+    while ((current > 0) && (current != '\n')) {

+        current = buffer_get(ctx);

+    }

+}

+

+static int32_t

+cmd_parse_obj(context_t *ctx, arg_t *arg, int32_t current)

+{

+    obj_t *obj;

+    uint64_t id;

+

+    current = str_number(ctx, 0, ctx->obj_count, &id, current);

+    if (current <= 0) {

+        return current;

+    }

+

+    obj = &ctx->objs[id];

+    if (obj->type != arg->obj.type) {

+        if (obj->type != OBJ_TYPE_NONE) {

+            return error(EBUSY, "Object is in use");

+        }

+        return error(ENOENT, "Object is not defined");

+    }

+

+    arg->obj.ref = obj;

+

+    return current;

+}

+

+static int32_t

+cmd_parse_num(context_t *ctx, arg_t *arg, int32_t current)

+{

+    return str_number(ctx, arg->num.min, arg->num.max, &arg->num.value,

+                      current);

+}

+

+static int32_t

+cmd_parse_str(context_t *ctx, arg_t *arg, int32_t current)

+{

+    return str_token(ctx, arg->str.data, arg->str.size, current);

+}

+

+static int32_t

+cmd_parse_args(context_t *ctx, command_t *cmd, int32_t current)

+{

+    arg_t *arg;

+

+    for (arg = cmd->args; arg->type != ARG_TYPE_NONE; arg++) {

+        switch (arg->type) {

+            case ARG_TYPE_OBJ:

+                current = cmd_parse_obj(ctx, arg, current);

+                break;

+            case ARG_TYPE_NUM:

+                current = cmd_parse_num(ctx, arg, current);

+                break;

+            case ARG_TYPE_STR:

+                current = cmd_parse_str(ctx, arg, current);

+                break;

+            default:

+                return error(EINVAL, "Unknown argument type");

+        }

+    }

+

+    if (current < 0) {

+        return current;

+    }

+

+    current = str_eol(ctx, current);

+    if (current <= 0) {

+        return error(EINVAL, "Syntax error");

+    }

+

+    return cmd->handler(ctx, cmd);

+}

+

+static int32_t

+cmd_parse(context_t *ctx, command_t *cmds)

+{

+    char text[32];

+    command_t *cmd;

+    int32_t current;

+

+    cmd = cmds;

+    do {

+        current = str_token(ctx, text, sizeof(text), buffer_get(ctx));

+        if (current <= 0) {

+            return current;

+        }

+

+        while (cmd->name != NULL) {

+            if (strcmp(cmd->name, text) == 0) {

+                if (cmd->handler != NULL) {

+                    return cmd_parse_args(ctx, cmd, current);

+                }

+                cmd = cmd->cmds;

+                break;

+            }

+            cmd++;

+        }

+    } while (cmd->name != NULL);

+

+    str_skip(ctx, current);

+

+    return error(ENOTSUP, "Unknown command");

+}

+

+static void

+cmd_fini(context_t *ctx, command_t *cmds)

+{

+    command_t *cmd;

+    arg_t *arg;

+

+    for (cmd = cmds; cmd->name != NULL; cmd++) {

+        if (cmd->handler == NULL) {

+            cmd_fini(ctx, cmd->cmds);

+        } else {

+            for (arg = cmd->args; arg->type != ARG_TYPE_NONE; arg++) {

+                switch (arg->type) {

+                    case ARG_TYPE_STR: