[PATCH 0/7] cifs: smbd: Improve reliability on transport reconnect

[PATCH 0/7] cifs: smbd: Improve reliability on transport reconnect

[longli]

From: Long Li <longli@microsoft.com>

Long Li (7):
  cifs: Don't display RDMA transport on reconnect
  cifs: smbd: Invalidate and deregister memory registration on re-send
  cifs: smbd: Return -EINVAL when the number of iovs exceeds
    SMBDIRECT_MAX_SGE
  cifs: smbd: Add messages on RDMA session destroy and reconnection
  cifs: smbd: Return -ECONNABORTED when trasnport is not in connected
    state
  cifs: smbd: Only queue work for error recovery on memory registration
  cifs: smbd: Return -EAGAIN when transport is reconnecting

 fs/cifs/cifs_debug.c |  5 +++++
 fs/cifs/file.c       | 19 +++++++++++++++++--
 fs/cifs/smbdirect.c  | 36 +++++++++++++++++++++---------------
 fs/cifs/transport.c  |  7 +++++--
 4 files changed, 48 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH 1/7] cifs: Don't display RDMA transport on reconnect

[longli]

From: Long Li <longli@microsoft.com>

On reconnect, the transport data structure is NULL and its information is not
available.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/cifs_debug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 0b4eee3bed66..efb2928ff6c8 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -256,6 +256,11 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 		if (!server->rdma)
 			goto skip_rdma;
 
+		if (!server->smbd_conn) {
+			seq_printf(m, "\nSMBDirect transport not available");
+			goto skip_rdma;
+		}
+
 		seq_printf(m, "\nSMBDirect (in hex) protocol version: %x "
 			"transport status: %x",
 			server->smbd_conn->protocol,
-- 
2.17.1

[PATCH 2/7] cifs: smbd: Invalidate and deregister memory registration on re-send

[longli]

From: Long Li <longli@microsoft.com>

On re-send, there might be a reconnect and all prevoius memory registrations
need to be invalidated and deregistered.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/file.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 4b95700c507c..3c4e01e56798 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2747,9 +2747,17 @@ cifs_resend_wdata(struct cifs_writedata *wdata, struct list_head *wdata_list,
 		if (!rc) {
 			if (wdata->cfile->invalidHandle)
 				rc = -EAGAIN;
-			else
+			else {
+#ifdef CONFIG_CIFS_SMB_DIRECT
+				if (wdata->mr) {
+					wdata->mr->need_invalidate = true;
+					smbd_deregister_mr(wdata->mr);
+					wdata->mr = NULL;
+				}
+#endif
 				rc = server->ops->async_writev(wdata,
 					cifs_uncached_writedata_release);
+			}
 		}
 
 		/* If the write was successfully sent, we are done */
@@ -3472,7 +3480,14 @@ static int cifs_resend_rdata(struct cifs_readdata *rdata,
 		if (!rc) {
 			if (rdata->cfile->invalidHandle)
 				rc = -EAGAIN;
-			else
+			else {
+#ifdef CONFIG_CIFS_SMB_DIRECT
+				if (rdata->mr) {
+					rdata->mr->need_invalidate = true;
+					smbd_deregister_mr(rdata->mr);
+					rdata->mr = NULL;
+				}
+#endif
 				rc = server->ops->async_readv(rdata);
 		}
 
-- 
2.17.1

[PATCH 3/7] cifs: smbd: Return -EINVAL when the number of iovs exceeds SMBDIRECT_MAX_SGE

[longli]

From: Long Li <longli@microsoft.com>

While it's not friendly to fail user processes that issue more iovs
than we support, at least we should return the correct error code so the
user process gets a chance to retry with smaller number of iovs.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index cd07e5301d42..d41a9345f90d 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1069,7 +1069,7 @@ static int smbd_post_send_data(
 
 	if (n_vec > SMBDIRECT_MAX_SGE) {
 		cifs_dbg(VFS, "Can't fit data to SGL, n_vec=%d\n", n_vec);
-		return -ENOMEM;
+		return -EINVAL;
 	}
 
 	sg_init_table(sgl, n_vec);
-- 
2.17.1

[PATCH 4/7] cifs: smbd: Add messages on RDMA session destroy and reconnection

[longli]

From: Long Li <longli@microsoft.com>

Log these activities to help production support.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index d41a9345f90d..227ef51c0712 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1476,6 +1476,7 @@ void smbd_destroy(struct TCP_Server_Info *server)
 	info->transport_status = SMBD_DESTROYED;
 
 	destroy_workqueue(info->workqueue);
+	log_rdma_event(INFO,  "rdma session destroyed\n");
 	kfree(info);
 }
 
@@ -1505,8 +1506,9 @@ int smbd_reconnect(struct TCP_Server_Info *server)
 	log_rdma_event(INFO, "creating rdma session\n");
 	server->smbd_conn = smbd_get_connection(
 		server, (struct sockaddr *) &server->dstaddr);
-	log_rdma_event(INFO, "created rdma session info=%p\n",
-		server->smbd_conn);
+
+	if (server->smbd_conn)
+		cifs_dbg(VFS, "RDMA transport re-established\n");
 
 	return server->smbd_conn ? 0 : -ENOENT;
 }
-- 
2.17.1

[PATCH 5/7] cifs: smbd: Return -ECONNABORTED when trasnport is not in connected state

[longli]

From: Long Li <longli@microsoft.com>

The transport should return this error so the upper layer will reconnect.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index 227ef51c0712..cf001f10d555 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1970,7 +1970,7 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
 
 	if (info->transport_status != SMBD_CONNECTED) {
 		log_read(ERR, "disconnected\n");
-		return 0;
+		return -ECONNABORTED;
 	}
 
 	goto again;
-- 
2.17.1

[PATCH 6/7] cifs: smbd: Only queue work for error recovery on memory registration

[longli]

From: Long Li <longli@microsoft.com>

It's not necessary to queue invalidated memory registration to work queue, as
all we need to do is to unmap the SG and make it usable again. This can save
CPU cycles in normal data paths as memory registration errors are rare and
normally only happens during reconnection.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index cf001f10d555..c00629a41d81 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2269,12 +2269,7 @@ static void smbd_mr_recovery_work(struct work_struct *work)
 	int rc;
 
 	list_for_each_entry(smbdirect_mr, &info->mr_list, list) {
-		if (smbdirect_mr->state == MR_INVALIDATED)
-			ib_dma_unmap_sg(
-				info->id->device, smbdirect_mr->sgl,
-				smbdirect_mr->sgl_count,
-				smbdirect_mr->dir);
-		else if (smbdirect_mr->state == MR_ERROR) {
+		if (smbdirect_mr->state == MR_ERROR) {
 
 			/* recover this MR entry */
 			rc = ib_dereg_mr(smbdirect_mr->mr);
@@ -2602,11 +2597,20 @@ int smbd_deregister_mr(struct smbd_mr *smbdirect_mr)
 		 */
 		smbdirect_mr->state = MR_INVALIDATED;
 
-	/*
-	 * Schedule the work to do MR recovery for future I/Os
-	 * MR recovery is slow and we don't want it to block the current I/O
-	 */
-	queue_work(info->workqueue, &info->mr_recovery_work);
+	if (smbdirect_mr->state == MR_INVALIDATED) {
+		ib_dma_unmap_sg(
+			info->id->device, smbdirect_mr->sgl,
+			smbdirect_mr->sgl_count,
+			smbdirect_mr->dir);
+		smbdirect_mr->state = MR_READY;
+		if (atomic_inc_return(&info->mr_ready_count) == 1)
+			wake_up_interruptible(&info->wait_mr);
+	} else
+		/*
+		 * Schedule the work to do MR recovery for future I/Os
+		 * MR recovery is slow and we don't want it to block the
+		 * current I/O */
+		queue_work(info->workqueue, &info->mr_recovery_work);
 
 done:
 	if (atomic_dec_and_test(&info->mr_used_count))
-- 
2.17.1

[PATCH 7/7] cifs: smbd: Return -EAGAIN when transport is reconnecting

[longli]

From: Long Li <longli@microsoft.com>

During reconnecting, the transport may have already been destroyed and is in
the process being reconnected. In this case, return -EAGAIN to not fail and
to retry this I/O.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/transport.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index afe66f9cb15e..66fde7bfec4f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -318,8 +318,11 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
 	int val = 1;
 	__be32 rfc1002_marker;
 
-	if (cifs_rdma_enabled(server) && server->smbd_conn) {
-		rc = smbd_send(server, num_rqst, rqst);
+	if (cifs_rdma_enabled(server)) {
+		/* return -EAGAIN when connecting or reconnecting */
+		rc = -EAGAIN;
+		if (server->smbd_conn)
+			rc = smbd_send(server, num_rqst, rqst);
 		goto smbd_done;
 	}
 
-- 
2.17.1

Re: [PATCH 6/7] cifs: smbd: Only queue work for error recovery on memory registration

[Steve French]

I cleaned up minor cosmetic nit spotted by checkpatch

$ scripts/checkpatch.pl
0001-cifs-smbd-Only-queue-work-for-error-recovery-on-memo.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#7:
It's not necessary to queue invalidated memory registration to work queue, as

WARNING: Block comments use a trailing */ on a separate line
#58: FILE: fs/cifs/smbdirect.c:2614:
+ * current I/O */

total: 0 errors, 2 warnings, 38 lines checked

On Wed, Oct 16, 2019 at 4:11 PM longli--- via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> From: Long Li <longli@microsoft.com>
>
> It's not necessary to queue invalidated memory registration to work queue, as
> all we need to do is to unmap the SG and make it usable again. This can save
> CPU cycles in normal data paths as memory registration errors are rare and
> normally only happens during reconnection.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smbdirect.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index cf001f10d555..c00629a41d81 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2269,12 +2269,7 @@ static void smbd_mr_recovery_work(struct work_struct *work)
>         int rc;
>
>         list_for_each_entry(smbdirect_mr, &info->mr_list, list) {
> -               if (smbdirect_mr->state == MR_INVALIDATED)
> -                       ib_dma_unmap_sg(
> -                               info->id->device, smbdirect_mr->sgl,
> -                               smbdirect_mr->sgl_count,
> -                               smbdirect_mr->dir);
> -               else if (smbdirect_mr->state == MR_ERROR) {
> +               if (smbdirect_mr->state == MR_ERROR) {
>
>                         /* recover this MR entry */
>                         rc = ib_dereg_mr(smbdirect_mr->mr);
> @@ -2602,11 +2597,20 @@ int smbd_deregister_mr(struct smbd_mr *smbdirect_mr)
>                  */
>                 smbdirect_mr->state = MR_INVALIDATED;
>
> -       /*
> -        * Schedule the work to do MR recovery for future I/Os
> -        * MR recovery is slow and we don't want it to block the current I/O
> -        */
> -       queue_work(info->workqueue, &info->mr_recovery_work);
> +       if (smbdirect_mr->state == MR_INVALIDATED) {
> +               ib_dma_unmap_sg(
> +                       info->id->device, smbdirect_mr->sgl,
> +                       smbdirect_mr->sgl_count,
> +                       smbdirect_mr->dir);
> +               smbdirect_mr->state = MR_READY;
> +               if (atomic_inc_return(&info->mr_ready_count) == 1)
> +                       wake_up_interruptible(&info->wait_mr);
> +       } else
> +               /*
> +                * Schedule the work to do MR recovery for future I/Os
> +                * MR recovery is slow and we don't want it to block the
> +                * current I/O */
> +               queue_work(info->workqueue, &info->mr_recovery_work);
>
>  done:
>         if (atomic_dec_and_test(&info->mr_used_count))
> --
> 2.17.1
>
>


-- 
Thanks,

Steve

Re: [PATCH 7/7] cifs: smbd: Return -EAGAIN when transport is reconnecting

[Steve French]

Tentatively merged the 7 patch series into cifs-2.6.git for-next
pending more reviews and xfstests regression testing runs

On Wed, Oct 16, 2019 at 4:11 PM longli--- via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> From: Long Li <longli@microsoft.com>
>
> During reconnecting, the transport may have already been destroyed and is in
> the process being reconnected. In this case, return -EAGAIN to not fail and
> to retry this I/O.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/transport.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index afe66f9cb15e..66fde7bfec4f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -318,8 +318,11 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
>         int val = 1;
>         __be32 rfc1002_marker;
>
> -       if (cifs_rdma_enabled(server) && server->smbd_conn) {
> -               rc = smbd_send(server, num_rqst, rqst);
> +       if (cifs_rdma_enabled(server)) {
> +               /* return -EAGAIN when connecting or reconnecting */
> +               rc = -EAGAIN;
> +               if (server->smbd_conn)
> +                       rc = smbd_send(server, num_rqst, rqst);
>                 goto smbd_done;
>         }
>
> --
> 2.17.1
>
>


-- 
Thanks,

Steve