[PATCH 1/3] nbd: allow multiple disconnects to be sent

[PATCH 1/3] nbd: allow multiple disconnects to be sent

[josef]

From: Josef Bacik <jbacik@fb.com>

There's no reason to limit ourselves to one disconnect message per
socket.  Sometimes networks do strange things, might as well let
sysadmins hit the panic button as much as they want.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 87a0a29..f91e7ac 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -991,9 +991,8 @@ static int nbd_disconnect(struct nbd_device *nbd)
 	struct nbd_config *config = nbd->config;
 
 	dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
-	if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
-			      &config->runtime_flags))
-		send_disconnects(nbd);
+	set_bit(NBD_DISCONNECT_REQUESTED, &config->runtime_flags);
+	send_disconnects(nbd);
 	return 0;
 }
 
-- 
2.7.4

[PATCH 2/3] nbd: take tx_lock before disconnecting

[josef]

From: Josef Bacik <jbacik@fb.com>

We need to take the tx_lock so we don't interleave our disconnect
request between real data going down the wire.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f91e7ac..6aefe9f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -978,11 +978,15 @@ static void send_disconnects(struct nbd_device *nbd)
 	int i, ret;
 
 	for (i = 0; i < config->num_connections; i++) {
+		struct nbd_sock *nsock = config->socks[i];
+
 		iov_iter_kvec(&from, WRITE | ITER_KVEC, &iov, 1, sizeof(request));
+		mutex_lock(&nsock->tx_lock);
 		ret = sock_xmit(nbd, i, 1, &from, 0, NULL);
 		if (ret <= 0)
 			dev_err(disk_to_dev(nbd->disk),
 				"Send disconnect failed %d\n", ret);
+		mutex_unlock(&nsock->tx_lock);
 	}
 }
 
-- 
2.7.4

[PATCH 3/3] nbd: only set sndtimeo if we have a timeout set

[josef]

From: Josef Bacik <jbacik@fb.com>

A user reported that he was getting immediate disconnects with my
sndtimeo patch applied.  This is because by default the OSS nbd client
doesn't set a timeout, so we end up setting the sndtimeo to 0, which of
course means we have send errors a lot.  Instead only set our sndtimeo
if the user specified a timeout, otherwise we'll just wait forever like
we did previously.

Fixes: dc88e34d69d8 ("nbd: set sk->sk_sndtimeo for our sockets")
Reported-by: Adam Borowski <kilobyte@angband.pl>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/block/nbd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6aefe9f..64b19b1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -908,7 +908,8 @@ static int nbd_reconnect_socket(struct nbd_device *nbd, unsigned long arg)
 			continue;
 		}
 		sk_set_memalloc(sock->sk);
-		sock->sk->sk_sndtimeo = nbd->tag_set.timeout;
+		if (nbd->tag_set.timeout)
+			sock->sk->sk_sndtimeo = nbd->tag_set.timeout;
 		atomic_inc(&config->recv_threads);
 		refcount_inc(&nbd->config_refs);
 		old = nsock->sock;
@@ -1077,7 +1078,9 @@ static int nbd_start_device(struct nbd_device *nbd)
 			return -ENOMEM;
 		}
 		sk_set_memalloc(config->socks[i]->sock->sk);
-		config->socks[i]->sock->sk->sk_sndtimeo = nbd->tag_set.timeout;
+		if (nbd->tag_set.timeout)
+			config->socks[i]->sock->sk->sk_sndtimeo =
+				nbd->tag_set.timeout;
 		atomic_inc(&config->recv_threads);
 		refcount_inc(&nbd->config_refs);
 		INIT_WORK(&args->work, recv_work);
-- 
2.7.4

Re: [PATCH 1/3] nbd: allow multiple disconnects to be sent

[Jens Axboe]

On 07/21/2017 08:48 AM, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> There's no reason to limit ourselves to one disconnect message per
> socket.  Sometimes networks do strange things, might as well let
> sysadmins hit the panic button as much as they want.

Applied 1-3 for 4.13, thanks Josef.

-- 
Jens Axboe

Re: [Nbd] [PATCH 1/3] nbd: allow multiple disconnects to be sent

[Wouter Verhelst]

On Fri, Jul 21, 2017 at 10:48:13AM -0400, josef@toxicpanda.com wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> There's no reason to limit ourselves to one disconnect message per
> socket.  Sometimes networks do strange things, might as well let
> sysadmins hit the panic button as much as they want.

The protocol spec is pretty clear that any requests sent after the
disconnect request was sent out are not guaranteed to be processed
anymore.

Doesn't this allow more requests to be sent out? Or is the
NBD_DISCONNECT_REQUESTED flag enough to make that impossible?

-- 
Could you people please use IRC like normal people?!?

  -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008
     Hacklab

Re: [Nbd] [PATCH 1/3] nbd: allow multiple disconnects to be sent

[Wouter Verhelst]

On Mon, Jul 24, 2017 at 07:24:30PM -0400, Josef Bacik wrote:
> On Mon, Jul 24, 2017 at 10:08:21PM +0200, Wouter Verhelst wrote:
> > On Fri, Jul 21, 2017 at 10:48:13AM -0400, josef@toxicpanda.com wrote:
> > > From: Josef Bacik <jbacik@fb.com>
> > > 
> > > There's no reason to limit ourselves to one disconnect message per
> > > socket.  Sometimes networks do strange things, might as well let
> > > sysadmins hit the panic button as much as they want.
> > 
> > The protocol spec is pretty clear that any requests sent after the
> > disconnect request was sent out are not guaranteed to be processed
> > anymore.
> > 
> > Doesn't this allow more requests to be sent out? Or is the
> > NBD_DISCONNECT_REQUESTED flag enough to make that impossible?
> > 
> 
> This just allows users to call the disconnect ioctl/netlink thing multiple times
> and have it send the DISCONNECT command if they want.

Right.

> We've had problems with our in-hosue nbd server missing messages,

That's pretty bad...

> and it's just a pain to have to unstuck it because the server messed up.
> It's just for the rare case the server is being weird, not because we
> expect/guarantee that subsequent disconnect commands will be processed.
> Thanks,

Okay, makes sense. Just thought I'd ask :-)

-- 
Could you people please use IRC like normal people?!?

  -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008
     Hacklab