MINOR: muxes: adjust takeover with buf_wait interaction

Takeover operation defines an argument <release>. It's a boolean which
if set indicate that freed connection resources during the takeover does
not have to be reallocated on the new thread. Typically, it is set to
false when takever is performed to reuse a connection. However, when
used to be able to delete a connection from a different thread,
<release> should be set to true.

Previously, <release> was only set in conjunction with "del server"
handler. This operation was performed under thread isolation, which
guarantee that not thread-safe operation such as removal from buf_wait
list could be performed on takeover if <release> was true. In the
contrary case, takeover operation would fail.

Recently, "del server" handler has been adjusted to remove idle
connection cleanup with takeover. As such, <release> is never set to
true in remaining takeover usage.

However, takeover is also used to enforce strict-maxconn on a server.
This is performed to delete a connection from any thread, which is the
primary reason of <release> to true. But for the moment as takeover
implementers considers that thread isolation is active if <release> is
set, this is not yet applicable for strict-maxconn usage.

Thus, the purpose of this patch is to adjust takeover implementation.
Remove assumption between <release> and thread-isolation mode. It's not
possible to remove a connection from a buf_wait list, an error will be
return in any case.

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index 09c5f6156c0de80e7c1fb04c78b2fd40adca34f8..83a2a1446a67a8565e6bb92f6d1d689f8bfb319c 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -4435,18 +4435,21 @@ static int fcgi_takeover(struct connection *conn, int orig_tid, int release)
        struct task *new_task = NULL;
        struct tasklet *new_tasklet = NULL;
 
+       /* If the connection is attached to a buffer_wait (extremely rare),
+        * it will be woken up at any instant by its own thread and we can't
+        * undo it anyway, so let's give up on this one. It's not interesting
+        * at least for reuse anyway since it's not usable right now.
+        *
+        * TODO if takeover is used to free conn (release == 1), buf_wait may
+        * prevent this which is not desirable.
+        */
+       if (LIST_INLIST(&fcgi->buf_wait.list))
+               goto fail;
+
        /* Pre-allocate tasks so that we don't have to roll back after the xprt
         * has been migrated.
         */
        if (!release) {
-               /* If the connection is attached to a buffer_wait (extremely
-                * rare), it will be woken up at any instant by its own thread
-                * and we can't undo it anyway, so let's give up on this one.
-                * It's not interesting anyway since it's not usable right now.
-                */
-               if (LIST_INLIST(&fcgi->buf_wait.list))
-                       goto fail;
-
                new_task = task_new_here();
                new_tasklet = tasklet_new();
                if (!new_task || !new_tasklet)
@@ -4503,20 +4506,6 @@ static int fcgi_takeover(struct connection *conn, int orig_tid, int release)
                                            SUB_RETRY_RECV, &fcgi->wait_event);
        }
 
-       if (release) {
-               /* we're being called for a server deletion and are running
-                * under thread isolation. That's the only way we can
-                * unregister a possible subscription of the original
-                * connection from its owner thread's queue, as this involves
-                * manipulating thread-unsafe areas. Note that it is not
-                * possible to just call b_dequeue() here as it would update
-                * the current thread's bufq_map and not the original one.
-                */
-               BUG_ON(!thread_isolated());
-               if (LIST_INLIST(&fcgi->buf_wait.list))
-                       _b_dequeue(&fcgi->buf_wait, orig_tid);
-       }
-
        if (new_task)
                __task_free(new_task);
        return 0;

diff --git a/src/mux_h1.c b/src/mux_h1.c
index a12535a73e9352445a855239bc4a08a58f1dc808..457ae45f8dec29f2c4234fc15b968cd3e1d7be62 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -5596,18 +5596,21 @@ static int h1_takeover(struct connection *conn, int orig_tid, int release)
        struct task *new_task = NULL;
        struct tasklet *new_tasklet = NULL;
 
+       /* If the connection is attached to a buffer_wait (extremely rare),
+        * it will be woken up at any instant by its own thread and we can't
+        * undo it anyway, so let's give up on this one. It's not interesting
+        * at least for reuse anyway since it's not usable right now.
+        *
+        * TODO if takeover is used to free conn (release == 1), buf_wait may
+        * prevent this which is not desirable.
+        */
+       if (LIST_INLIST(&h1c->buf_wait.list))
+               goto fail;
+
        /* Pre-allocate tasks so that we don't have to roll back after the xprt
         * has been migrated.
         */
        if (!release) {
-               /* If the connection is attached to a buffer_wait (extremely
-                * rare), it will be woken up at any instant by its own thread
-                * and we can't undo it anyway, so let's give up on this one.
-                * It's not interesting anyway since it's not usable right now.
-                */
-               if (LIST_INLIST(&h1c->buf_wait.list))
-                       goto fail;
-
                new_task = task_new_here();
                new_tasklet = tasklet_new();
                if (!new_task || !new_tasklet)
@@ -5664,20 +5667,6 @@ static int h1_takeover(struct connection *conn, int orig_tid, int release)
                                           SUB_RETRY_RECV, &h1c->wait_event);
        }
 
-       if (release) {
-               /* we're being called for a server deletion and are running
-                * under thread isolation. That's the only way we can
-                * unregister a possible subscription of the original
-                * connection from its owner thread's queue, as this involves
-                * manipulating thread-unsafe areas. Note that it is not
-                * possible to just call b_dequeue() here as it would update
-                * the current thread's bufq_map and not the original one.
-                */
-               BUG_ON(!thread_isolated());
-               if (LIST_INLIST(&h1c->buf_wait.list))
-                       _b_dequeue(&h1c->buf_wait, orig_tid);
-       }
-
        if (new_task)
                __task_free(new_task);
        return 0;

diff --git a/src/mux_h2.c b/src/mux_h2.c
index e98ecd21ec2eb2d5aedff96af8a373c4fb82295a..876b199641d8897fb0ba6c52a81395de1bb850b5 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -8375,18 +8375,21 @@ static int h2_takeover(struct connection *conn, int orig_tid, int release)
        struct task *new_task = NULL;
        struct tasklet *new_tasklet = NULL;
 
+       /* If the connection is attached to a buffer_wait (extremely rare),
+        * it will be woken up at any instant by its own thread and we can't
+        * undo it anyway, so let's give up on this one. It's not interesting
+        * at least for reuse anyway since it's not usable right now.
+        *
+        * TODO if takeover is used to free conn (release == 1), buf_wait may
+        * prevent this which is not desirable.
+        */
+       if (LIST_INLIST(&h2c->buf_wait.list))
+               goto fail;
+
        /* Pre-allocate tasks so that we don't have to roll back after the xprt
         * has been migrated.
         */
        if (!release) {
-               /* If the connection is attached to a buffer_wait (extremely
-                * rare), it will be woken up at any instant by its own thread
-                * and we can't undo it anyway, so let's give up on this one.
-                * It's not interesting anyway since it's not usable right now.
-                */
-               if (LIST_INLIST(&h2c->buf_wait.list))
-                       goto fail;
-
                new_task = task_new_here();
                new_tasklet = tasklet_new();
                if (!new_task || !new_tasklet)
@@ -8443,20 +8446,6 @@ static int h2_takeover(struct connection *conn, int orig_tid, int release)
                                           SUB_RETRY_RECV, &h2c->wait_event);
        }
 
-       if (release) {
-               /* we're being called for a server deletion and are running
-                * under thread isolation. That's the only way we can
-                * unregister a possible subscription of the original
-                * connection from its owner thread's queue, as this involves
-                * manipulating thread-unsafe areas. Note that it is not
-                * possible to just call b_dequeue() here as it would update
-                * the current thread's bufq_map and not the original one.
-                */
-               BUG_ON(!thread_isolated());
-               if (LIST_INLIST(&h2c->buf_wait.list))
-                       _b_dequeue(&h2c->buf_wait, orig_tid);
-       }
-
        if (new_task)
                __task_free(new_task);
        return 0;

diff --git a/src/mux_spop.c b/src/mux_spop.c
index 37a9f35d21df042d6ce8d5f6f9d5d9dd1b249dbd..8fc2faeb2d4db225dccab22ab7eeab592e703fab 100644 (file)
--- a/src/mux_spop.c
+++ b/src/mux_spop.c
@@ -3633,17 +3633,21 @@ static int spop_takeover(struct connection *conn, int orig_tid, int release)
        struct task *new_task = NULL;
        struct tasklet *new_tasklet = NULL;
 
+       /* If the connection is attached to a buffer_wait (extremely rare),
+        * it will be woken up at any instant by its own thread and we can't
+        * undo it anyway, so let's give up on this one. It's not interesting
+        * at least for reuse anyway since it's not usable right now.
+        *
+        * TODO if takeover is used to free conn (release == 1), buf_wait may
+        * prevent this which is not desirable.
+        */
+       if (LIST_INLIST(&spop_conn->buf_wait.list))
+               goto fail;
+
        /* Pre-allocate tasks so that we don't have to roll back after the xprt
         * has been migrated.
         */
        if (!release) {
-               /* If the connection is attached to a buffer_wait (extremely
-                * rare), it will be woken up at any instant by its own thread
-                * and we can't undo it anyway, so let's give up on this one.
-                * It's not interesting anyway since it's not usable right now.
-                */
-               if (LIST_INLIST(&spop_conn->buf_wait.list))
-                       goto fail;
 
                new_task = task_new_here();
                new_tasklet = tasklet_new();
@@ -3701,20 +3705,6 @@ static int spop_takeover(struct connection *conn, int orig_tid, int release)
                                                 SUB_RETRY_RECV, &spop_conn->wait_event);
        }
 
-       if (release) {
-               /* we're being called for a server deletion and are running
-                * under thread isolation. That's the only way we can
-                * unregister a possible subscription of the original
-                * connection from its owner thread's queue, as this involves
-                * manipulating thread-unsafe areas. Note that it is not
-                * possible to just call b_dequeue() here as it would update
-                * the current thread's bufq_map and not the original one.
-                */
-               BUG_ON(!thread_isolated());
-               if (LIST_INLIST(&spop_conn->buf_wait.list))
-                       _b_dequeue(&spop_conn->buf_wait, orig_tid);
-       }
-
        if (new_task)
                __task_free(new_task);
        return 0;