[PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Yao Liu]

Some I/O requests that have been sent succussfully but have not yet been
replied won't be resubmitted after reconnecting because of server restart,
so we add a list to track them.

Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
---
 drivers/block/nbd.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 053958a..ca69d6e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -113,6 +113,8 @@ struct nbd_device {
 	struct list_head list;
 	struct task_struct *task_recv;
 	struct task_struct *task_setup;
+	struct mutex outstanding_lock;
+	struct list_head outstanding_queue;
 };
 
 #define NBD_CMD_REQUEUED	1
@@ -125,6 +127,7 @@ struct nbd_cmd {
 	blk_status_t status;
 	unsigned long flags;
 	u32 cmd_cookie;
+	struct list_head outstanding_entry;
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -619,6 +622,24 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	return 0;
 }
 
+static struct nbd_cmd *nbd_get_cmd(struct nbd_device *nbd,
+					struct nbd_cmd *xcmd)
+{
+	struct nbd_cmd *cmd, *tmp;
+
+	mutex_lock(&nbd->outstanding_lock);
+	list_for_each_entry_safe(cmd, tmp, &nbd->outstanding_queue, outstanding_entry) {
+		if (cmd != xcmd)
+			continue;
+		list_del_init(&cmd->outstanding_entry);
+		mutex_unlock(&nbd->outstanding_lock);
+		return cmd;
+	}
+	mutex_unlock(&nbd->outstanding_lock);
+
+	return ERR_PTR(-ENOENT);
+}
+
 /* NULL returned = something went wrong, inform userspace */
 static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
 {
@@ -714,12 +735,30 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
 				req, bvec.bv_len);
 		}
 	}
+	cmd = nbd_get_cmd(nbd, cmd);
+	if (IS_ERR(cmd)) {
+		dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p which not in outstanding queue\n",
+			tag, req);
+		ret = -ENOENT;
+	}
 out:
 	trace_nbd_payload_received(req, handle);
 	mutex_unlock(&cmd->lock);
 	return ret ? ERR_PTR(ret) : cmd;
 }
 
+static void nbd_requeue_outstanding(struct nbd_device *nbd)
+{
+	struct nbd_cmd *cmd, *tmp;
+
+	mutex_lock(&nbd->outstanding_lock);
+	list_for_each_entry_safe(cmd, tmp, &nbd->outstanding_queue, outstanding_entry) {
+		nbd_requeue_cmd(cmd);
+		list_del_init(&cmd->outstanding_entry);
+	}
+	mutex_unlock(&nbd->outstanding_lock);
+}
+
 static void recv_work(struct work_struct *work)
 {
 	struct recv_thread_args *args = container_of(work,
@@ -742,6 +781,7 @@ static void recv_work(struct work_struct *work)
 
 		blk_mq_complete_request(blk_mq_rq_from_pdu(cmd));
 	}
+	nbd_requeue_outstanding(nbd);
 	atomic_dec(&config->recv_threads);
 	wake_up(&config->recv_wq);
 	nbd_config_put(nbd);
@@ -892,6 +932,10 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 		nbd_mark_nsock_dead(nbd, nsock, 1);
 		nbd_requeue_cmd(cmd);
 		ret = 0;
+	} else if (ret == 0) {
+		mutex_lock(&nbd->outstanding_lock);
+		list_add_tail(&cmd->outstanding_entry, &nbd->outstanding_queue);
+		mutex_unlock(&nbd->outstanding_lock);
 	}
 out:
 	mutex_unlock(&nsock->tx_lock);
@@ -1615,6 +1659,8 @@ static int nbd_dev_add(int index)
 	refcount_set(&nbd->config_refs, 0);
 	refcount_set(&nbd->refs, 1);
 	INIT_LIST_HEAD(&nbd->list);
+	mutex_init(&nbd->outstanding_lock);
+	INIT_LIST_HEAD(&nbd->outstanding_queue);
 	disk->major = NBD_MAJOR;
 	disk->first_minor = index << part_shift;
 	disk->fops = &nbd_fops;
-- 
1.8.3.1

[PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Yao Liu]

Some nbd client implementations have a userland's daemon, so we should
inform client daemon to clean up and exit.

Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
---
 drivers/block/nbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ca69d6e..22e86f4 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -246,7 +246,7 @@ static int nbd_disconnected(struct nbd_config *config)
 static void nbd_mark_nsock_dead(struct nbd_device *nbd, struct nbd_sock *nsock,
 				int notify)
 {
-	if (!nsock->dead && notify && !nbd_disconnected(nbd->config)) {
+	if (!nsock->dead && notify) {
 		struct link_dead_args *args;
 		args = kmalloc(sizeof(struct link_dead_args), GFP_NOIO);
 		if (args) {
@@ -1891,7 +1891,6 @@ static void nbd_disconnect_and_put(struct nbd_device *nbd)
 {
 	mutex_lock(&nbd->config_lock);
 	nbd_disconnect(nbd);
-	nbd_clear_sock(nbd);
 	mutex_unlock(&nbd->config_lock);
 	if (test_and_clear_bit(NBD_HAS_CONFIG_REF,
 			       &nbd->config->runtime_flags))
-- 
1.8.3.1

[PATCH 3/3] nbd: mark sock as dead even if it's the last one

[Yao Liu]

When sock dead, nbd_read_stat should return a ERR_PTR and then we should
mark sock as dead and wait for a reconnection if the dead sock is the last
one, because nbd_xmit_timeout won't resubmit while num_connections <= 1.

Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
---
 drivers/block/nbd.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 22e86f4..a557a83 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -716,15 +716,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
 			if (result <= 0) {
 				dev_err(disk_to_dev(nbd->disk), "Receive data failed (result %d)\n",
 					result);
-				/*
-				 * If we've disconnected or we only have 1
-				 * connection then we need to make sure we
-				 * complete this request, otherwise error out
-				 * and let the timeout stuff handle resubmitting
-				 * this request onto another connection.
-				 */
-				if (nbd_disconnected(config) ||
-				    config->num_connections <= 1) {
+				if (nbd_disconnected(config)) {
 					cmd->status = BLK_STS_IOERR;
 					goto out;
 				}
-- 
1.8.3.1

Re: [PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Josef Bacik]

On Fri, May 24, 2019 at 05:43:54PM +0800, Yao Liu wrote:
> Some I/O requests that have been sent succussfully but have not yet been
> replied won't be resubmitted after reconnecting because of server restart,
> so we add a list to track them.
> 
> Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>

Nack, this is what the timeout stuff is supposed to handle.  The commands will
timeout and we'll resubmit them if we have alive sockets.  Thanks,

Josef

Re: [PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Josef Bacik]

On Fri, May 24, 2019 at 05:43:55PM +0800, Yao Liu wrote:
> Some nbd client implementations have a userland's daemon, so we should
> inform client daemon to clean up and exit.
> 
> Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>

Except the nbd_disconnected() check is for the case that the client told us
specifically to disconnect, so we don't want to send the notification to
re-connect because we've already been told we want to tear everything down.
Nack to this as well.  Thanks,

Josef

Re: [PATCH 3/3] nbd: mark sock as dead even if it's the last one

[Josef Bacik]

On Fri, May 24, 2019 at 05:43:56PM +0800, Yao Liu wrote:
> When sock dead, nbd_read_stat should return a ERR_PTR and then we should
> mark sock as dead and wait for a reconnection if the dead sock is the last
> one, because nbd_xmit_timeout won't resubmit while num_connections <= 1.

num_connections is the total number of connections that the device was set up
with, not how many are left.  Now since we have the dead_conn_timeout timeout
stuff now which didn't exist when I originally wrote this code I'd be ok with
doing that, but not the way you have it now.  It would be something more like

	if (nbd_disconnected(config) ||
	    (config->num_connections <= 1 &&
	     !config->dead_conn_timeout)

instead.  Thanks,

Josef

Re: Re: [PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Yao Liu]

On Fri, May 24, 2019 at 09:07:42AM -0400, Josef Bacik wrote:
> On Fri, May 24, 2019 at 05:43:54PM +0800, Yao Liu wrote:
> > Some I/O requests that have been sent succussfully but have not yet been
> > replied won't be resubmitted after reconnecting because of server restart,
> > so we add a list to track them.
> > 
> > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> 
> Nack, this is what the timeout stuff is supposed to handle.  The commands will
> timeout and we'll resubmit them if we have alive sockets.  Thanks,
> 
> Josef
> 

On the one hand, if num_connections == 1 and the only sock has dead,
then we do nbd_genl_reconfigure to reconnect within dead_conn_timeout,
nbd_xmit_timeout will not resubmit commands that have been sent
succussfully but have not yet been replied. The log is as follows:
 
[270551.108746] block nbd0: Receive control failed (result -104)
[270551.108747] block nbd0: Send control failed (result -32)
[270551.108750] block nbd0: Request send failed, requeueing
[270551.116207] block nbd0: Attempted send on invalid socket
[270556.119584] block nbd0: reconnected socket
[270581.161751] block nbd0: Connection timed out
[270581.165038] block nbd0: shutting down sockets
[270581.165041] print_req_error: I/O error, dev nbd0, sector 5123224 flags 8801
[270581.165149] print_req_error: I/O error, dev nbd0, sector 5123232 flags 8801
[270581.165580] block nbd0: Connection timed out
[270581.165587] print_req_error: I/O error, dev nbd0, sector 844680 flags 8801
[270581.166184] print_req_error: I/O error, dev nbd0, sector 5123240 flags 8801
[270581.166554] block nbd0: Connection timed out
[270581.166576] print_req_error: I/O error, dev nbd0, sector 844688 flags 8801
[270581.167124] print_req_error: I/O error, dev nbd0, sector 5123248 flags 8801
[270581.167590] block nbd0: Connection timed out
[270581.167597] print_req_error: I/O error, dev nbd0, sector 844696 flags 8801
[270581.168021] print_req_error: I/O error, dev nbd0, sector 5123256 flags 8801
[270581.168487] block nbd0: Connection timed out
[270581.168493] print_req_error: I/O error, dev nbd0, sector 844704 flags 8801
[270581.170183] print_req_error: I/O error, dev nbd0, sector 5123264 flags 8801
[270581.170540] block nbd0: Connection timed out
[270581.173333] block nbd0: Connection timed out
[270581.173728] block nbd0: Connection timed out
[270581.174135] block nbd0: Connection timed out
 
On the other hand, if we wait nbd_xmit_timeout to handle resubmission,
the I/O requests will have a big delay. For example, if timeout time is 30s,
and from sock dead to nbd_genl_reconfigure returned OK we only spend
2s, the I/O requests will still be handled by nbd_xmit_timeout after 30s.

Re: Re: [PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Yao Liu]

On Fri, May 24, 2019 at 09:08:58AM -0400, Josef Bacik wrote:
> On Fri, May 24, 2019 at 05:43:55PM +0800, Yao Liu wrote:
> > Some nbd client implementations have a userland's daemon, so we should
> > inform client daemon to clean up and exit.
> > 
> > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> 
> Except the nbd_disconnected() check is for the case that the client told us
> specifically to disconnect, so we don't want to send the notification to
> re-connect because we've already been told we want to tear everything down.
> Nack to this as well.  Thanks,
> 
> Josef
> 

But in userland, client daemon process and process which send disconnect
command are not same process, so they are not clear to each other, so
client daemon expect driver inform it to exit.
In addition, client daemon will get nbd status with nbd_genl_status interface
after it get notified and it should not re-connect if status connected == 0

Re: Re: [PATCH 3/3] nbd: mark sock as dead even if it's the last one

[Yao Liu]

On Fri, May 24, 2019 at 09:17:15AM -0400, Josef Bacik wrote:
> On Fri, May 24, 2019 at 05:43:56PM +0800, Yao Liu wrote:
> > When sock dead, nbd_read_stat should return a ERR_PTR and then we should
> > mark sock as dead and wait for a reconnection if the dead sock is the last
> > one, because nbd_xmit_timeout won't resubmit while num_connections <= 1.
> 
> num_connections is the total number of connections that the device was set up
> with, not how many are left.  Now since we have the dead_conn_timeout timeout
> stuff now which didn't exist when I originally wrote this code I'd be ok with
> doing that, but not the way you have it now.  It would be something more like
> 
> 	if (nbd_disconnected(config) ||
> 	    (config->num_connections <= 1 &&
> 	     !config->dead_conn_timeout)
> 
> instead.  Thanks,
> 
> Josef
> 

Your solution is better indeed.

Re: [PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Mike Christie]

On 05/27/2019 01:23 PM, Yao Liu wrote:
> On Fri, May 24, 2019 at 09:08:58AM -0400, Josef Bacik wrote:
>> On Fri, May 24, 2019 at 05:43:55PM +0800, Yao Liu wrote:
>>> Some nbd client implementations have a userland's daemon, so we should
>>> inform client daemon to clean up and exit.
>>>
>>> Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
>>
>> Except the nbd_disconnected() check is for the case that the client told us
>> specifically to disconnect, so we don't want to send the notification to
>> re-connect because we've already been told we want to tear everything down.
>> Nack to this as well.  Thanks,
>>
>> Josef
>>
> 
> But in userland, client daemon process and process which send disconnect
> command are not same process, so they are not clear to each other, so
> client daemon expect driver inform it to exit.
> In addition, client daemon will get nbd status with nbd_genl_status interface
> after it get notified and it should not re-connect if status connected == 0
> 

When using the netlink interface you get the NBD_CMD_LINK_DEAD first
then the configs_refs goes to zero right?

nbd_disconnect_and_put -> sock_shutdown -> nbd_mark_nsock_dead

then later we do the final nbd_config_put?

Maybe it would be best to add a new netlink event to signal what has
happened, because the above nl and stat algorithm seems like a pain. The
NBD_CMD_LINK_DEAD will be sent, then userspace has to possibly poll the
status to check if this was caused due to nbd_genl_disconnect instead of
a downed link due to something like a command timeout, because the
refcount may not be down when userspace gets the NL event.

Or, I guess the admin/tool process could just send a msg to the daemon
process to tell it to do the netlink disconnect request.

Re: [PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Josef Bacik]

On Tue, May 28, 2019 at 02:23:23AM +0800, Yao Liu wrote:
> On Fri, May 24, 2019 at 09:08:58AM -0400, Josef Bacik wrote:
> > On Fri, May 24, 2019 at 05:43:55PM +0800, Yao Liu wrote:
> > > Some nbd client implementations have a userland's daemon, so we should
> > > inform client daemon to clean up and exit.
> > > 
> > > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> > 
> > Except the nbd_disconnected() check is for the case that the client told us
> > specifically to disconnect, so we don't want to send the notification to
> > re-connect because we've already been told we want to tear everything down.
> > Nack to this as well.  Thanks,
> > 
> > Josef
> > 
> 
> But in userland, client daemon process and process which send disconnect
> command are not same process, so they are not clear to each other, so
> client daemon expect driver inform it to exit.
> In addition, client daemon will get nbd status with nbd_genl_status interface
> after it get notified and it should not re-connect if status connected == 0

Right this is the point.  The daemon is dumb, if it gets a disconnected message
it'll try to reconnect, so we don't want to send a disconnected message if we
were specifically told to disconnect.  We don't want to rely on userspace to try
and manage this weird state machine.  If you want userland to know its time to
disconnect then have your implementation handle being told to disconnect and
handle all the things.  Thanks,

Josef

Re: [PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Josef Bacik]

On Tue, May 28, 2019 at 02:07:43AM +0800, Yao Liu wrote:
> On Fri, May 24, 2019 at 09:07:42AM -0400, Josef Bacik wrote:
> > On Fri, May 24, 2019 at 05:43:54PM +0800, Yao Liu wrote:
> > > Some I/O requests that have been sent succussfully but have not yet been
> > > replied won't be resubmitted after reconnecting because of server restart,
> > > so we add a list to track them.
> > > 
> > > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> > 
> > Nack, this is what the timeout stuff is supposed to handle.  The commands will
> > timeout and we'll resubmit them if we have alive sockets.  Thanks,
> > 
> > Josef
> > 
> 
> On the one hand, if num_connections == 1 and the only sock has dead,
> then we do nbd_genl_reconfigure to reconnect within dead_conn_timeout,
> nbd_xmit_timeout will not resubmit commands that have been sent
> succussfully but have not yet been replied. The log is as follows:
>  
> [270551.108746] block nbd0: Receive control failed (result -104)
> [270551.108747] block nbd0: Send control failed (result -32)
> [270551.108750] block nbd0: Request send failed, requeueing
> [270551.116207] block nbd0: Attempted send on invalid socket
> [270556.119584] block nbd0: reconnected socket
> [270581.161751] block nbd0: Connection timed out
> [270581.165038] block nbd0: shutting down sockets
> [270581.165041] print_req_error: I/O error, dev nbd0, sector 5123224 flags 8801
> [270581.165149] print_req_error: I/O error, dev nbd0, sector 5123232 flags 8801
> [270581.165580] block nbd0: Connection timed out
> [270581.165587] print_req_error: I/O error, dev nbd0, sector 844680 flags 8801
> [270581.166184] print_req_error: I/O error, dev nbd0, sector 5123240 flags 8801
> [270581.166554] block nbd0: Connection timed out
> [270581.166576] print_req_error: I/O error, dev nbd0, sector 844688 flags 8801
> [270581.167124] print_req_error: I/O error, dev nbd0, sector 5123248 flags 8801
> [270581.167590] block nbd0: Connection timed out
> [270581.167597] print_req_error: I/O error, dev nbd0, sector 844696 flags 8801
> [270581.168021] print_req_error: I/O error, dev nbd0, sector 5123256 flags 8801
> [270581.168487] block nbd0: Connection timed out
> [270581.168493] print_req_error: I/O error, dev nbd0, sector 844704 flags 8801
> [270581.170183] print_req_error: I/O error, dev nbd0, sector 5123264 flags 8801
> [270581.170540] block nbd0: Connection timed out
> [270581.173333] block nbd0: Connection timed out
> [270581.173728] block nbd0: Connection timed out
> [270581.174135] block nbd0: Connection timed out
>  
> On the other hand, if we wait nbd_xmit_timeout to handle resubmission,
> the I/O requests will have a big delay. For example, if timeout time is 30s,
> and from sock dead to nbd_genl_reconfigure returned OK we only spend
> 2s, the I/O requests will still be handled by nbd_xmit_timeout after 30s.

We have to wait for the full timeout anyway to know that the socket went down,
so it'll be re-submitted right away and then we'll wait on the new connection.

Now we could definitely have requests that were submitted well after the first
thing that failed, so their timeout would be longer than simply retrying them,
but we have no idea of knowing which ones timed out and which ones didn't.  This
way lies pain, because we have to matchup tags with handles.  This is why we
rely on the generic timeout infrastructure, so everything is handled correctly
without ending up with duplicate submissions/replies.  Thanks,

Josef

Re: [PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Yao Liu]

On Tue, May 28, 2019 at 12:57:59PM -0400, Josef Bacik wrote:
> On Tue, May 28, 2019 at 02:07:43AM +0800, Yao Liu wrote:
> > On Fri, May 24, 2019 at 09:07:42AM -0400, Josef Bacik wrote:
> > > On Fri, May 24, 2019 at 05:43:54PM +0800, Yao Liu wrote:
> > > > Some I/O requests that have been sent succussfully but have not yet been
> > > > replied won't be resubmitted after reconnecting because of server restart,
> > > > so we add a list to track them.
> > > > 
> > > > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> > > 
> > > Nack, this is what the timeout stuff is supposed to handle.  The commands will
> > > timeout and we'll resubmit them if we have alive sockets.  Thanks,
> > > 
> > > Josef
> > > 
> > 
> > On the one hand, if num_connections == 1 and the only sock has dead,
> > then we do nbd_genl_reconfigure to reconnect within dead_conn_timeout,
> > nbd_xmit_timeout will not resubmit commands that have been sent
> > succussfully but have not yet been replied. The log is as follows:
> >  
> > [270551.108746] block nbd0: Receive control failed (result -104)
> > [270551.108747] block nbd0: Send control failed (result -32)
> > [270551.108750] block nbd0: Request send failed, requeueing
> > [270551.116207] block nbd0: Attempted send on invalid socket
> > [270556.119584] block nbd0: reconnected socket
> > [270581.161751] block nbd0: Connection timed out
> > [270581.165038] block nbd0: shutting down sockets
> > [270581.165041] print_req_error: I/O error, dev nbd0, sector 5123224 flags 8801
> > [270581.165149] print_req_error: I/O error, dev nbd0, sector 5123232 flags 8801
> > [270581.165580] block nbd0: Connection timed out
> > [270581.165587] print_req_error: I/O error, dev nbd0, sector 844680 flags 8801
> > [270581.166184] print_req_error: I/O error, dev nbd0, sector 5123240 flags 8801
> > [270581.166554] block nbd0: Connection timed out
> > [270581.166576] print_req_error: I/O error, dev nbd0, sector 844688 flags 8801
> > [270581.167124] print_req_error: I/O error, dev nbd0, sector 5123248 flags 8801
> > [270581.167590] block nbd0: Connection timed out
> > [270581.167597] print_req_error: I/O error, dev nbd0, sector 844696 flags 8801
> > [270581.168021] print_req_error: I/O error, dev nbd0, sector 5123256 flags 8801
> > [270581.168487] block nbd0: Connection timed out
> > [270581.168493] print_req_error: I/O error, dev nbd0, sector 844704 flags 8801
> > [270581.170183] print_req_error: I/O error, dev nbd0, sector 5123264 flags 8801
> > [270581.170540] block nbd0: Connection timed out
> > [270581.173333] block nbd0: Connection timed out
> > [270581.173728] block nbd0: Connection timed out
> > [270581.174135] block nbd0: Connection timed out
> >  
> > On the other hand, if we wait nbd_xmit_timeout to handle resubmission,
> > the I/O requests will have a big delay. For example, if timeout time is 30s,
> > and from sock dead to nbd_genl_reconfigure returned OK we only spend
> > 2s, the I/O requests will still be handled by nbd_xmit_timeout after 30s.
> 
> We have to wait for the full timeout anyway to know that the socket went down,
> so it'll be re-submitted right away and then we'll wait on the new connection.
> 
> Now we could definitely have requests that were submitted well after the first
> thing that failed, so their timeout would be longer than simply retrying them,
> but we have no idea of knowing which ones timed out and which ones didn't.  This
> way lies pain, because we have to matchup tags with handles.  This is why we
> rely on the generic timeout infrastructure, so everything is handled correctly
> without ending up with duplicate submissions/replies.  Thanks,
> 
> Josef
> 

But as I mentioned before, if num_connections == 1, nbd_xmit_timeout won't re-submit
commands and I/O error will occur. Should we change the condition
		if (config->num_connections > 1)
to
		if (config->num_connections >= 1)
?

Re: [PATCH 2/3] nbd: notify userland even if nbd has already disconnected

[Yao Liu]

On Tue, May 28, 2019 at 11:36:21AM -0500, Mike Christie wrote:
> On 05/27/2019 01:23 PM, Yao Liu wrote:
> > On Fri, May 24, 2019 at 09:08:58AM -0400, Josef Bacik wrote:
> >> On Fri, May 24, 2019 at 05:43:55PM +0800, Yao Liu wrote:
> >>> Some nbd client implementations have a userland's daemon, so we should
> >>> inform client daemon to clean up and exit.
> >>>
> >>> Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> >>
> >> Except the nbd_disconnected() check is for the case that the client told us
> >> specifically to disconnect, so we don't want to send the notification to
> >> re-connect because we've already been told we want to tear everything down.
> >> Nack to this as well.  Thanks,
> >>
> >> Josef
> >>
> > 
> > But in userland, client daemon process and process which send disconnect
> > command are not same process, so they are not clear to each other, so
> > client daemon expect driver inform it to exit.
> > In addition, client daemon will get nbd status with nbd_genl_status interface
> > after it get notified and it should not re-connect if status connected == 0
> > 
> 
> When using the netlink interface you get the NBD_CMD_LINK_DEAD first
> then the configs_refs goes to zero right?
> 
> nbd_disconnect_and_put -> sock_shutdown -> nbd_mark_nsock_dead
> 
> then later we do the final nbd_config_put?
> 
> Maybe it would be best to add a new netlink event to signal what has
> happened, because the above nl and stat algorithm seems like a pain. The
> NBD_CMD_LINK_DEAD will be sent, then userspace has to possibly poll the
> status to check if this was caused due to nbd_genl_disconnect instead of
> a downed link due to something like a command timeout, because the
> refcount may not be down when userspace gets the NL event.
> 
> Or, I guess the admin/tool process could just send a msg to the daemon
> process to tell it to do the netlink disconnect request.
> 

Adding a new netlink event sames good.

Re: [PATCH 1/3] nbd: fix connection timed out error after reconnecting to server

[Josef Bacik]

On Wed, May 29, 2019 at 03:04:46AM +0800, Yao Liu wrote:
> On Tue, May 28, 2019 at 12:57:59PM -0400, Josef Bacik wrote:
> > On Tue, May 28, 2019 at 02:07:43AM +0800, Yao Liu wrote:
> > > On Fri, May 24, 2019 at 09:07:42AM -0400, Josef Bacik wrote:
> > > > On Fri, May 24, 2019 at 05:43:54PM +0800, Yao Liu wrote:
> > > > > Some I/O requests that have been sent succussfully but have not yet been
> > > > > replied won't be resubmitted after reconnecting because of server restart,
> > > > > so we add a list to track them.
> > > > > 
> > > > > Signed-off-by: Yao Liu <yotta.liu@ucloud.cn>
> > > > 
> > > > Nack, this is what the timeout stuff is supposed to handle.  The commands will
> > > > timeout and we'll resubmit them if we have alive sockets.  Thanks,
> > > > 
> > > > Josef
> > > > 
> > > 
> > > On the one hand, if num_connections == 1 and the only sock has dead,
> > > then we do nbd_genl_reconfigure to reconnect within dead_conn_timeout,
> > > nbd_xmit_timeout will not resubmit commands that have been sent
> > > succussfully but have not yet been replied. The log is as follows:
> > >  
> > > [270551.108746] block nbd0: Receive control failed (result -104)
> > > [270551.108747] block nbd0: Send control failed (result -32)
> > > [270551.108750] block nbd0: Request send failed, requeueing
> > > [270551.116207] block nbd0: Attempted send on invalid socket
> > > [270556.119584] block nbd0: reconnected socket
> > > [270581.161751] block nbd0: Connection timed out
> > > [270581.165038] block nbd0: shutting down sockets
> > > [270581.165041] print_req_error: I/O error, dev nbd0, sector 5123224 flags 8801
> > > [270581.165149] print_req_error: I/O error, dev nbd0, sector 5123232 flags 8801
> > > [270581.165580] block nbd0: Connection timed out
> > > [270581.165587] print_req_error: I/O error, dev nbd0, sector 844680 flags 8801
> > > [270581.166184] print_req_error: I/O error, dev nbd0, sector 5123240 flags 8801
> > > [270581.166554] block nbd0: Connection timed out
> > > [270581.166576] print_req_error: I/O error, dev nbd0, sector 844688 flags 8801
> > > [270581.167124] print_req_error: I/O error, dev nbd0, sector 5123248 flags 8801
> > > [270581.167590] block nbd0: Connection timed out
> > > [270581.167597] print_req_error: I/O error, dev nbd0, sector 844696 flags 8801
> > > [270581.168021] print_req_error: I/O error, dev nbd0, sector 5123256 flags 8801
> > > [270581.168487] block nbd0: Connection timed out
> > > [270581.168493] print_req_error: I/O error, dev nbd0, sector 844704 flags 8801
> > > [270581.170183] print_req_error: I/O error, dev nbd0, sector 5123264 flags 8801
> > > [270581.170540] block nbd0: Connection timed out
> > > [270581.173333] block nbd0: Connection timed out
> > > [270581.173728] block nbd0: Connection timed out
> > > [270581.174135] block nbd0: Connection timed out
> > >  
> > > On the other hand, if we wait nbd_xmit_timeout to handle resubmission,
> > > the I/O requests will have a big delay. For example, if timeout time is 30s,
> > > and from sock dead to nbd_genl_reconfigure returned OK we only spend
> > > 2s, the I/O requests will still be handled by nbd_xmit_timeout after 30s.
> > 
> > We have to wait for the full timeout anyway to know that the socket went down,
> > so it'll be re-submitted right away and then we'll wait on the new connection.
> > 
> > Now we could definitely have requests that were submitted well after the first
> > thing that failed, so their timeout would be longer than simply retrying them,
> > but we have no idea of knowing which ones timed out and which ones didn't.  This
> > way lies pain, because we have to matchup tags with handles.  This is why we
> > rely on the generic timeout infrastructure, so everything is handled correctly
> > without ending up with duplicate submissions/replies.  Thanks,
> > 
> > Josef
> > 
> 
> But as I mentioned before, if num_connections == 1, nbd_xmit_timeout won't re-submit
> commands and I/O error will occur. Should we change the condition
> 		if (config->num_connections > 1)
> to
> 		if (config->num_connections >= 1)
> ?

Only if you don't have the patch 3 in place though right?  If you fix patch 3 to
allow requeuing if you have a dead connection timer set then you can requeue and
everything is a-ok.  Thanks,

Josef