MIME-Version: 1.0
References: <20211029200421.65090-1-trondmy@kernel.org> <20211029200421.65090-2-trondmy@kernel.org>
 <20211029200421.65090-3-trondmy@kernel.org> <CALF+zOmG+nUVPX6P0TET_JibH-E2qOvbuz5Fm=7qQk_ycM7y8Q@mail.gmail.com>
In-Reply-To: <CALF+zOmG+nUVPX6P0TET_JibH-E2qOvbuz5Fm=7qQk_ycM7y8Q@mail.gmail.com>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Fri, 3 Dec 2021 11:36:05 -0500
Message-ID: <CALF+zO=o0DZxiyBYzkai+50u_3PsoB0_rNqyxhvmSgxT6S1dZg@mail.gmail.com>
Subject: Re: [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock()
To:     trondmy@kernel.org
Cc:     linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 3, 2021 at 11:10 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> >
> > Move the error handling into a single switch statement for clarity.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  net/sunrpc/xprtsock.c | 68 ++++++++++++++++++-------------------------
> >  1 file changed, 28 insertions(+), 40 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 291b63136c08..7fb302e202bc 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt,
> >  static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >  {
> >         struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> > -       int ret = -ENOTCONN;
> >
> >         if (!transport->inet) {
> >                 struct sock *sk = sock->sk;
> > @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >         }
> >
> >         if (!xprt_bound(xprt))
> > -               goto out;
> > +               return -ENOTCONN;
> >
> >         xs_set_memalloc(xprt);
> >
> > @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >
> >         /* Tell the socket layer to start connecting... */
> >         set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
> > -       ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> > -       switch (ret) {
> > -       case 0:
> > -               xs_set_srcport(transport, sock);
> > -               fallthrough;
> > -       case -EINPROGRESS:
> > -               /* SYN_SENT! */
> > -               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > -                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > -               break;
> > -       case -EADDRNOTAVAIL:
> > -               /* Source port number is unavailable. Try a new one! */
> > -               transport->srcport = 0;
> > -       }
> > -out:
> > -       return ret;
> > +       return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> >  }
> >
> >  /**
> > @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >                 container_of(work, struct sock_xprt, connect_worker.work);
> >         struct socket *sock = transport->sock;
> >         struct rpc_xprt *xprt = &transport->xprt;
> > -       int status = -EIO;
> > +       int status;
> >
> >         if (!sock) {
> >                 sock = xs_create_sock(xprt, transport,
> >                                 xs_addr(xprt)->sa_family, SOCK_STREAM,
> >                                 IPPROTO_TCP, true);
> >                 if (IS_ERR(sock)) {
> > -                       status = PTR_ERR(sock);
> > +                       xprt_wake_pending_tasks(xprt, PTR_ERR(sock));
> >                         goto out;
> >                 }
> >         }
> > @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >                         xprt, -status, xprt_connected(xprt),
> >                         sock->sk->sk_state);
> >         switch (status) {
> > -       default:
> > -               printk("%s: connect returned unhandled error %d\n",
> > -                       __func__, status);
> > -               fallthrough;
> > -       case -EADDRNOTAVAIL:
> > -               /* We're probably in TIME_WAIT. Get rid of existing socket,
> > -                * and retry
> > -                */
> > -               xs_tcp_force_close(xprt);
> > -               break;
> >         case 0:
> > +               xs_set_srcport(transport, sock);
> > +               fallthrough;
> >         case -EINPROGRESS:
> > +               /* SYN_SENT! */
> > +               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > +                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > +               fallthrough;
> >         case -EALREADY:
> > -               xprt_unlock_connect(xprt, transport);
> > -               return;
> > +               goto out_unlock;
> > +       case -EADDRNOTAVAIL:
> > +               /* Source port number is unavailable. Try a new one! */
> > +               transport->srcport = 0;
> > +               status = -EAGAIN;
> > +               break;
> >         case -EINVAL:
> >                 /* Happens, for instance, if the user specified a link
> >                  * local IPv6 address without a scope-id.
> > @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >         case -EHOSTUNREACH:
> >         case -EADDRINUSE:
> >         case -ENOBUFS:
> > -               /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > -                * We need to wake them first to ensure the correct error code.
> > -                */
> > -               xprt_wake_pending_tasks(xprt, status);
> > -               xs_tcp_force_close(xprt);
> > -               goto out;
> > +               break;
> > +       default:
> > +               printk("%s: connect returned unhandled error %d\n",
> > +                       __func__, status);
> > +               status = -EAGAIN;
> >         }
> > -       status = -EAGAIN;
> > +
> > +       /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > +        * We need to wake them first to ensure the correct error code.
> > +        */
> > +       xprt_wake_pending_tasks(xprt, status);
> > +       xs_tcp_force_close(xprt);
>
> Isn't this a problem to call xs_tcp_force_close() unconditionally at
> the bottom of xs_tcp_setup_socket()?
> Before this patch we only called this depending on certain returns
> from kernel_connect().
>
Nevermind - I see the fallthroughs and out_unlock

>
>
> >  out:
> >         xprt_clear_connecting(xprt);
> > +out_unlock:
> >         xprt_unlock_connect(xprt, transport);
> > -       xprt_wake_pending_tasks(xprt, status);
> >  }
> >
> >  /**
> > --
> > 2.31.1
> >