On 10/02/2012 07:20 AM, Jeff Layton wrote: > On Mon, 1 Oct 2012 20:46:01 -0500 > Steve French <smfrench@xxxxxxxxx> wrote: > >> 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. >> 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. > > 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 > >> On Mon, Oct 1, 2012 at 8:40 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >>> On Mon, 01 Oct 2012 13:10:45 +0530 >>> Suresh Jayaraman <sjayaraman@xxxxxxxx> wrote: >>> >>>> 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@xxxxxxxx> >>>> --- >>>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c >>>> index d9b639b..a33db4c 100644 >>>> --- a/fs/cifs/transport.c >>>> +++ b/fs/cifs/transport.c >>>> @@ -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); >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> 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 <jlayton@xxxxxxxxxx> >> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
