[PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Suresh Jayaraman]

kernel_sendmsg() is less likely to return -ENOSPC and it might be
a bug to do so. However, in the past there might have been cases
where a -ENOSPC was returned from a low level driver.

Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
is no longer returned. This -ENOSPC specific handling will be removed
once we are sure it is no longer returned.


Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
---

 fs/cifs/transport.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d9b639b..c613fca 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -155,6 +155,12 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
 				    n_vec - first_vec, total_len);
 		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+			/*
+			 * Catch if a low level driver returns -ENOSPC. This
+			 * WARN_ON will be removed by 3.10 if no one reports
+			 * seeing this.
+			 */
+			WARN_ON_ONCE(rc == -ENOSPC);
 			i++;
 			/*
 			 * If blocking send we try 3 times, since each can block

Re: [PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Jeff Layton]

On Wed, 03 Oct 2012 21:27:12 +0530
Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:

> kernel_sendmsg() is less likely to return -ENOSPC and it might be
> a bug to do so. However, in the past there might have been cases
> where a -ENOSPC was returned from a low level driver.
> 
> Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
> is no longer returned. This -ENOSPC specific handling will be removed
> once we are sure it is no longer returned.
> 
> 
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
> ---
> 
>  fs/cifs/transport.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index d9b639b..c613fca 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -155,6 +155,12 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
>  		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
>  				    n_vec - first_vec, total_len);
>  		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
> +			/*
> +			 * Catch if a low level driver returns -ENOSPC. This
> +			 * WARN_ON will be removed by 3.10 if no one reports
> +			 * seeing this.
> +			 */
> +			WARN_ON_ONCE(rc == -ENOSPC);
>  			i++;
>  			/*
>  			 * If blocking send we try 3 times, since each can block
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

LGTM

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Steve French]

merged into cifs-2.6.git

On Wed, Oct 3, 2012 at 11:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 03 Oct 2012 21:27:12 +0530
> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
>
>> kernel_sendmsg() is less likely to return -ENOSPC and it might be
>> a bug to do so. However, in the past there might have been cases
>> where a -ENOSPC was returned from a low level driver.
>>
>> Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
>> is no longer returned. This -ENOSPC specific handling will be removed
>> once we are sure it is no longer returned.
>>
>>
>> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
>> ---
>>
>>  fs/cifs/transport.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index d9b639b..c613fca 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -155,6 +155,12 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
>>               rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
>>                                   n_vec - first_vec, total_len);
>>               if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
>> +                     /*
>> +                      * Catch if a low level driver returns -ENOSPC. This
>> +                      * WARN_ON will be removed by 3.10 if no one reports
>> +                      * seeing this.
>> +                      */
>> +                     WARN_ON_ONCE(rc == -ENOSPC);
>>                       i++;
>>                       /*
>>                        * If blocking send we try 3 times, since each can block
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> LGTM
>
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>



-- 
Thanks,

Steve

[PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Suresh Jayaraman]

kernel_sendmsg() is less likely to return -ENOSPC and it might be
a bug to do so. However, in the past there might have been cases
where a -ENOSPC was returned from a low level driver.

Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
is no longer returned. This -ENOSPC specific handling will be removed
once we are sure it is no longer returned.

Also, avoid setting -ENOSPC to -EAGAIN while at it.

Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
---
 fs/cifs/transport.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index d9b639b..afe000f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -155,6 +155,10 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
 		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
 				    n_vec - first_vec, total_len);
 		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
+			/*
+			 * Catch if a low level driver returns -ENOSPC.
+			 */
+			WARN_ON_ONCE(rc == -ENOSPC);
 			i++;
 			/*
 			 * If blocking send we try 3 times, since each can block
@@ -177,7 +181,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);

Re: [PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Jeff Layton]

On Wed, 03 Oct 2012 12:45:36 +0530
Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:

> kernel_sendmsg() is less likely to return -ENOSPC and it might be
> a bug to do so. However, in the past there might have been cases
> where a -ENOSPC was returned from a low level driver.
> 
> Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
> is no longer returned. This -ENOSPC specific handling will be removed
> once we are sure it is no longer returned.
> 
> Also, avoid setting -ENOSPC to -EAGAIN while at it.
> 
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
> ---
>  fs/cifs/transport.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index d9b639b..afe000f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -155,6 +155,10 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
>  		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
>  				    n_vec - first_vec, total_len);
>  		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
> +			/*
> +			 * Catch if a low level driver returns -ENOSPC.
> +			 */
> +			WARN_ON_ONCE(rc == -ENOSPC);
>  			i++;
>  			/*
>  			 * If blocking send we try 3 times, since each can block
> @@ -177,7 +181,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;

Do you really want to remove this reset? This will change the behavior
if the lower levels do indeed return -ENOSPC. That will now bubble back
up to the upper levels and it will likely abort the call instead of
retrying.

I think it would be best that we add the WARN_ON_ONCE but not change
anything else. If no one reports seeing that warning fire for 2 or 3
releases then we can safely remove the -ENOSPC handling. Maybe shoot
for that in 3.10 or so?

You may want to add a comment to remind us to revisit that for the 3.10
cycle.

>  				break;
>  			}
>  			msleep(1 << i);

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: WARN_ON_ONCE if kernel_sendmsg() returns -ENOSPC

[Suresh Jayaraman]

On 10/03/2012 03:34 PM, Jeff Layton wrote:
> On Wed, 03 Oct 2012 12:45:36 +0530
> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
> 
>> kernel_sendmsg() is less likely to return -ENOSPC and it might be
>> a bug to do so. However, in the past there might have been cases
>> where a -ENOSPC was returned from a low level driver.
>>
>> Add a WARN_ON_ONCE() to ensure that it is safe to assume that -ENOSPC
>> is no longer returned. This -ENOSPC specific handling will be removed
>> once we are sure it is no longer returned.
>>
>> Also, avoid setting -ENOSPC to -EAGAIN while at it.
>>
>> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org>
>> ---
>>  fs/cifs/transport.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index d9b639b..afe000f 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -155,6 +155,10 @@ smb_sendv(struct TCP_Server_Info *server, struct kvec *iov, int n_vec)
>>  		rc = kernel_sendmsg(ssocket, &smb_msg, &iov[first_vec],
>>  				    n_vec - first_vec, total_len);
>>  		if ((rc == -ENOSPC) || (rc == -EAGAIN)) {
>> +			/*
>> +			 * Catch if a low level driver returns -ENOSPC.
>> +			 */
>> +			WARN_ON_ONCE(rc == -ENOSPC);
>>  			i++;
>>  			/*
>>  			 * If blocking send we try 3 times, since each can block
>> @@ -177,7 +181,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;
> 
> Do you really want to remove this reset? This will change the behavior
> if the lower levels do indeed return -ENOSPC. That will now bubble back
> up to the upper levels and it will likely abort the call instead of
> retrying.

Yes, you are correct. I'll remove it.

> 
> I think it would be best that we add the WARN_ON_ONCE but not change
> anything else. If no one reports seeing that warning fire for 2 or 3
> releases then we can safely remove the -ENOSPC handling. Maybe shoot
> for that in 3.10 or so?
> 
> You may want to add a comment to remind us to revisit that for the 3.10
> cycle.
> 

Sure, I'll add that when I respin.


Thanks
Suresh