cifs: retry kernel_sendmsg only in case of -EAGAIN

RFC PATCH In review Suresh Jayaraman 3 participants 7 messages Oct 1, 2012 – Oct 2, 2012 6 messages not in the archive

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Suresh Jayaraman

author

2012-10-01 07:40 UTC #0 lore raw

RFC PATCH 1/1 cifs: retry kernel_sendmsg only in case of -EAGAIN

In smb_sendv(), we seem to retry if kernel_sendmsg() returned either -ENOSPC or -EAGAIN. In either case after multiple attempts, we set the error to -EAGAIN before breaking out of the loop.

First, it is not clear to me when kernel_sendmsg() can return -ENOSPC, and what it would mean and why should we retry. It makes me wonder whether this check is part of some old code. Also, there seem to be no need to set the error back to -EAGAIN before we break out the loop. Fix this by making cifs retry only if kernel_sendmsg() returns -EAGAIN.

If the above discussion make sense, here is a patch to fix this.
---

Retry kernel_sendmsg() only in case of -EAGAIN and remove redundant error assignment.

Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>

---

fs/cifs/transport.c +1 -2

index d9b639b..a33db4c 100644
@@ -154,7 +154,7 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 	while (total_len) {
 		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
 				    n_vec - first_vec, total_len);
-		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+		if (rc == -EAGAIN) {
 			i++;
 			/*
 			 * If blocking send we try 3 times, since each can block

@@ -177,7 +177,6 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 			if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
 				cERROR(1, "sends on sock %p stuck for 15 seconds",
 				    ssocket);
-				rc = -EAGAIN;
 				break;
 			}
 			msleep(1 << i);

Jeff Layton

Acked-by

2012-10-02 01:40 UTC #2 lore raw

In reply to a message not in the archive

Quoted 41 lines · On Mon, 01 Oct 2012 13:10:45 +0530 Suresh Jayaraman…

In smb_sendv(), we seem to retry if kernel_sendmsg() returned either -ENOSPC or -EAGAIN. In either case after multiple attempts, we set the error to -EAGAIN before breaking out of the loop.
First, it is not clear to me when kernel_sendmsg() can return -ENOSPC, and what it would mean and why should we retry. It makes me wonder whether this check is part of some old code. Also, there seem to be no need to set the error back to -EAGAIN before we break out the loop. Fix this by making cifs retry only if kernel_sendmsg() returns -EAGAIN.
If the above discussion make sense, here is a patch to fix this.
---
Retry kernel_sendmsg() only in case of -EAGAIN and remove redundant error assignment.
Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>
---
fs/cifs/transport.c +1 -3
index d9b639b..a33db4c 100644
@@ -154,7 +154,7 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 	while (total_len) {
 		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
 				    n_vec - first_vec, total_len);
-		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+		if (rc == -EAGAIN) {
 			i++;
 			/*
 			 * If blocking send we try 3 times, since each can block
@@ -177,7 +177,6 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 			if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
 				cERROR(1, "sends on sock %p stuck for 15 seconds",
 				    ssocket);
-				rc = -EAGAIN;
 				break;
 			}
 			msleep(1 << i);
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

That ENOSPC has been there a long time, and I think you're correct that it's of questionable value.

Acked-by Jeff Layton <[email protected]>

Steve French

2012-10-02 01:46 UTC #4 lore raw

In reply to a message not in the archive

How can we be certain that ENOSPC is no longer returned? When it was returned then the obvious thing was to block briefly and retry.

Does it do any harm?

Quoted 52 lines · Jeff Layton

Quoted 41 lines · On Mon, 01 Oct 2012 13:10:45 +0530 Suresh Jayaraman…
In smb_sendv(), we seem to retry if kernel_sendmsg() returned either -ENOSPC or -EAGAIN. In either case after multiple attempts, we set the error to -EAGAIN before breaking out of the loop.
First, it is not clear to me when kernel_sendmsg() can return -ENOSPC, and what it would mean and why should we retry. It makes me wonder whether this check is part of some old code. Also, there seem to be no need to set the error back to -EAGAIN before we break out the loop. Fix this by making cifs retry only if kernel_sendmsg() returns -EAGAIN.
If the above discussion make sense, here is a patch to fix this.
---
Retry kernel_sendmsg() only in case of -EAGAIN and remove redundant error assignment.
Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>
---
fs/cifs/transport.c +1 -3
index d9b639b..a33db4c 100644
@@ -154,7 +154,7 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
      while (total_len) {
              rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
                                  n_vec - first_vec, total_len);
-             if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+             if (rc == -EAGAIN) {
                      i++;
                      /*
                       * If blocking send we try 3 times, since each can block
@@ -177,7 +177,6 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
                      if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
                              cERROR(1, "sends on sock %p stuck for 15 seconds",
                                  ssocket);
-                             rc = -EAGAIN;
                              break;
                      }
                      msleep(1 << i);
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
That ENOSPC has been there a long time, and I think you're correct that it's of questionable value.
Acked-by Jeff Layton <[email protected]>

Signature · 3 lines

Thanks,

Steve

Jeff Layton

2012-10-02 01:50 UTC #6 lore raw

In reply to a message not in the archive

Quoted 5 lines · On Mon, 1 Oct 2012 20:46:01 -0500 Steve French…

How can we be certain that ENOSPC is no longer returned? When it was returned then the obvious thing was to block briefly and retry.
Does it do any harm?

Can you explain the situation that caused it to return -ENOSPC in the first place? Why that and not -EAGAIN? Or -ENOBUFS?

Quoted 56 lines · Jeff Layton

Quoted 52 lines
Quoted 41 lines · On Mon, 01 Oct 2012 13:10:45 +0530 Suresh Jayaraman…
In smb_sendv(), we seem to retry if kernel_sendmsg() returned either -ENOSPC or -EAGAIN. In either case after multiple attempts, we set the error to -EAGAIN before breaking out of the loop.
First, it is not clear to me when kernel_sendmsg() can return -ENOSPC, and what it would mean and why should we retry. It makes me wonder whether this check is part of some old code. Also, there seem to be no need to set the error back to -EAGAIN before we break out the loop. Fix this by making cifs retry only if kernel_sendmsg() returns -EAGAIN.
If the above discussion make sense, here is a patch to fix this.
---
Retry kernel_sendmsg() only in case of -EAGAIN and remove redundant error assignment.
Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>
---
fs/cifs/transport.c +1 -3
index d9b639b..a33db4c 100644
@@ -154,7 +154,7 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
      while (total_len) {
              rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
                                  n_vec - first_vec, total_len);
-             if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+             if (rc == -EAGAIN) {
                      i++;
                      /*
                       * If blocking send we try 3 times, since each can block
@@ -177,7 +177,6 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
                      if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
                              cERROR(1, "sends on sock %p stuck for 15 seconds",
                                  ssocket);
-                             rc = -EAGAIN;
                              break;
                      }
                      msleep(1 << i);
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
That ENOSPC has been there a long time, and I think you're correct that it's of questionable value.
Acked-by Jeff Layton <[email protected]>

Signature · 1 line

Jeff Layton <[email protected]>

Suresh Jayaraman

author

2012-10-02 10:22 UTC #8 lore raw

In reply to a message not in the archive

Quoted 4 lines · Jeff Layton

Quoted 1 line · On Mon, 1 Oct 2012 20:46:01 -0500 Steve French…
How can we be certain that ENOSPC is no longer returned? When it was

I had traced kernel_sendmsg() and the functions it is calling, but I couldn't find any of them returning -ENOSPC. Also, I couldn't find any other callers of kernel_sendmsg() checking for -ENOSPC.

Quoted 3 lines

Quoted 3 lines
returned then the obvious thing was to block briefly and retry.
Does it do any harm?

When I tried to analyze what might cause the error reported here
http://thread.gmane.org/gmane.linux.kernel/1367198/focus=1367498

I bumped into this code. I had a few questions that I couldn't answer like

a) When does kernel_sendmsg return -ENOSPC?
b) Even if we assume it returns -ENOSPC, does it really make sense to retry?
c) Does the error condition that lead to -ENOSPC rectify by the time we retry?
d) Why do we need to reset -ENOSPC to -EAGAIN before returning the error?

If we can answer these questions reasonably, we may not need this patch.

Quoted 3 lines

Can you explain the situation that caused it to return -ENOSPC in the first place? Why that and not -EAGAIN? Or -ENOBUFS?

I too thought -ENOBUFS might represent the condition better.

Thanks
Suresh

Quoted 59 lines

Quoted 57 lines · Jeff Layton
Quoted 52 lines
Quoted 41 lines · On Mon, 01 Oct 2012 13:10:45 +0530 Suresh Jayaraman…
In smb_sendv(), we seem to retry if kernel_sendmsg() returned either -ENOSPC or -EAGAIN. In either case after multiple attempts, we set the error to -EAGAIN before breaking out of the loop.
First, it is not clear to me when kernel_sendmsg() can return -ENOSPC, and what it would mean and why should we retry. It makes me wonder whether this check is part of some old code. Also, there seem to be no need to set the error back to -EAGAIN before we break out the loop. Fix this by making cifs retry only if kernel_sendmsg() returns -EAGAIN.
If the above discussion make sense, here is a patch to fix this.
---
Retry kernel_sendmsg() only in case of -EAGAIN and remove redundant error assignment.
Signed-off-by Suresh Jayaraman <sjayaraman-IBi9RG/[email protected]>
---
fs/cifs/transport.c +1 -3
index d9b639b..a33db4c 100644
@@ -154,7 +154,7 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
      while (total_len) {
              rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
                                  n_vec - first_vec, total_len);
-             if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+             if (rc == -EAGAIN) {
                      i++;
                      /*
                       * If blocking send we try 3 times, since each can block
@@ -177,7 +177,6 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
                      if ((i >= 14) || (!server->noblocksnd && (i > 2))) {
                              cERROR(1, "sends on sock %p stuck for 15 seconds",
                                  ssocket);
-                             rc = -EAGAIN;
                              break;
                      }
                      msleep(1 << i);
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
That ENOSPC has been there a long time, and I think you're correct that it's of questionable value.
Acked-by Jeff Layton <[email protected]>

Jeff Layton

2012-10-02 11:12 UTC #10 lore raw

In reply to a message not in the archive

Quoted 25 lines · On Tue, 02 Oct 2012 15:52:34 +0530 Suresh Jayaraman…

Quoted 4 lines · Jeff Layton
Quoted 1 line · On Mon, 1 Oct 2012 20:46:01 -0500 Steve French…
How can we be certain that ENOSPC is no longer returned? When it was
I had traced kernel_sendmsg() and the functions it is calling, but I couldn't find any of them returning -ENOSPC. Also, I couldn't find any other callers of kernel_sendmsg() checking for -ENOSPC.
Quoted 3 lines
Quoted 3 lines
returned then the obvious thing was to block briefly and retry.
Does it do any harm?
When I tried to analyze what might cause the error reported here
http://thread.gmane.org/gmane.linux.kernel/1367198/focus=1367498
I bumped into this code. I had a few questions that I couldn't answer like
a) When does kernel_sendmsg return -ENOSPC?
b) Even if we assume it returns -ENOSPC, does it really make sense to retry?
c) Does the error condition that lead to -ENOSPC rectify by the time we retry?
d) Why do we need to reset -ENOSPC to -EAGAIN before returning the error?

Quoted 8 lines

If we can answer these questions reasonably, we may not need this patch.
Quoted 3 lines
Can you explain the situation that caused it to return -ENOSPC in the first place? Why that and not -EAGAIN? Or -ENOBUFS?
I too thought -ENOBUFS might represent the condition better.

To lend further weight to the argument too. The sendmsg(2) manpage doesn't list -ENOSPC as an error return.

kernel_sendmsg() is basically the kernelspace wrapper around the same set of functions that the sendmsg() system call uses. If it's not a valid error return for sendmsg() then it almost certainly isn't one for kernel_sendmsg() either.

The harm in these situations is further obfuscation of some code that is already far too complicated, and the potential for papering over real bugs. If kernel_sendmsg returns ENOSPC, then that's likely a bug and is something we should deal with accordingly instead of blindly retrying.

If you're concerned about it however, one possibility might be to add a WARN_ON_ONCE that fires on an ENOSPC error for a few releases.

Signature · 1 line

Jeff Layton <[email protected]>

Steve French

2012-10-02 16:08 UTC #12 lore raw

In reply to a message not in the archive

Quoted 45 lines · Jeff Layton

Quoted 25 lines · On Tue, 02 Oct 2012 15:52:34 +0530 Suresh Jayaraman…
Quoted 4 lines · Jeff Layton
Quoted 1 line · On Mon, 1 Oct 2012 20:46:01 -0500 Steve French…
How can we be certain that ENOSPC is no longer returned? When it was
I had traced kernel_sendmsg() and the functions it is calling, but I couldn't find any of them returning -ENOSPC. Also, I couldn't find any other callers of kernel_sendmsg() checking for -ENOSPC.
Quoted 3 lines
Quoted 3 lines
returned then the obvious thing was to block briefly and retry.
Does it do any harm?
When I tried to analyze what might cause the error reported here
http://thread.gmane.org/gmane.linux.kernel/1367198/focus=1367498
I bumped into this code. I had a few questions that I couldn't answer like
a) When does kernel_sendmsg return -ENOSPC?
b) Even if we assume it returns -ENOSPC, does it really make sense to retry?
c) Does the error condition that lead to -ENOSPC rectify by the time we retry?
d) Why do we need to reset -ENOSPC to -EAGAIN before returning the error?
Quoted 8 lines
If we can answer these questions reasonably, we may not need this patch.
Quoted 3 lines
Can you explain the situation that caused it to return -ENOSPC in the first place? Why that and not -EAGAIN? Or -ENOBUFS?
I too thought -ENOBUFS might represent the condition better.
To lend further weight to the argument too. The sendmsg(2) manpage doesn't list -ENOSPC as an error return.
kernel_sendmsg() is basically the kernelspace wrapper around the same set of functions that the sendmsg() system call uses. If it's not a valid error return for sendmsg() then it almost certainly isn't one for kernel_sendmsg() either.

Quoted 8 lines

The harm in these situations is further obfuscation of some code that is already far too complicated, and the potential for papering over real bugs. If kernel_sendmsg returns ENOSPC, then that's likely a bug and is something we should deal with accordingly instead of blindly retrying.
If you're concerned about it however, one possibility might be to add a WARN_ON_ONCE that fires on an ENOSPC error for a few releases.

Seems reasonable, since if we did get an ENOSPC from a lower level driver (filtered through either sendmsg interface) we would expect the same strategy to deal with as EAGAIN - wait and retry.

Signature · 3 lines

Thanks,

Steve