You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Ames <gr...@raleigh.ibm.com> on 2000/07/14 22:01:08 UTC
[PATCH] ap_sendfile on Linux gets EINVAL
If TCP_NODELAY is set, ap_sendfile fails on Linux w/EINVAL. The setsockopt() for TCP_CORK
gets the failure.
A quick look at the kernel source (ain't open source nice?) reveals that TCP_CORK and
TCP_NODELAY are mutually exclusive. Makes sense to me - TCP_DELAY says send partial TCP
segments immediately; TCP_CORK says hold on to partial segments until the uncork comes
along.
This patch tests to see if TCP_NODELAY is set before setting TCP_CORK, then restores
TCP_NODELAY to its original value before exiting ap_sendfile. For now, it's using
getsockopt() to determine what's up with TCP_NODELAY. A possible future optimization
would be to maintain a flag in the ap_socket_t to say what the long term state of
TCP_NODELAY should be.
Another minor related change - only cork/uncork if headers are present.
Greg
Index: lib/apr/network_io/unix/sendrecv.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/lib/apr/network_io/unix/sendrecv.c,v
retrieving revision 1.32
diff -u -d -b -r1.32 sendrecv.c
--- sendrecv.c 2000/07/07 03:00:44 1.32
+++ sendrecv.c 2000/07/14 19:22:01
@@ -212,29 +212,75 @@
*/
#if defined(__linux__) && defined(HAVE_WRITEV)
+
+/* TCP_CORK keeps us from sending partial frames when we shouldn't
+ * however, it is mutually exclusive w/TCP_NODELAY
+ */
+
+static int os_cork(ap_socket_t *sock)
+{
+ /* Linux only for now */
+
+ int nodelay_off = 0, corkflag = 1, rv, delayflag;
+ socklen_t delaylen;
+
+ /* XXX it would be cheaper to use an ap_socket_t flag here */
+ rv = getsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
+ (void *) &delayflag, &delaylen);
+ if (rv == 0) {
+
+ if (delayflag != 0) {
+ /* turn off nodelay temporarily to allow cork */
+ rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
+ (const void *) &nodelay_off, sizeof(nodelay_off));
+ if (rv < 0) {
+ return rv;
+ }
+ }
+ }
+ rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
+ (const void *) &corkflag, sizeof(corkflag));
+ return rv == 0 ? delayflag : rv;
+}
+
+static int os_uncork(ap_socket_t *sock, int delayflag)
+{
+ /* Uncork to send queued frames - Linux only for now */
+
+ int corkflag = 0, rv;
+ rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
+ (const void *) &corkflag, sizeof(corkflag));
+ if (rv == 0) {
+
+ /* restore TCP_NODELAY to its original setting */
+ rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
+ (const void *) &delayflag, sizeof(delayflag));
+ }
+ return rv;
+}
+
ap_status_t ap_sendfile(ap_socket_t *sock, ap_file_t *file,
ap_hdtr_t *hdtr, ap_off_t *offset, ap_size_t *len,
ap_int32_t flags)
{
off_t off = *offset;
- int corkflag = 1;
- int rv, nbytes = 0, total_hdrbytes, i;
+ int rv, nbytes = 0, total_hdrbytes, i, delayflag;
ap_status_t arv;
/* Ignore flags for now. */
flags = 0;
- /* TCP_CORK keeps us from sending partial frames when we shouldn't */
- rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
- (const void *) &corkflag, sizeof(corkflag));
- if (rv == -1) {
- *len = 0;
+ if (hdtr->numheaders > 0) {
+ ap_int32_t hdrbytes;
+
+ /* cork before writing headers */
+ rv = os_cork(sock);
+ if (rv < 0) {
return errno;
}
+ delayflag = rv;
/* Now write the headers */
- if (hdtr->numheaders > 0) {
- ap_int32_t hdrbytes;
arv = ap_sendv(sock, hdtr->headers, hdtr->numheaders, &hdrbytes);
if (arv != APR_SUCCESS) {
*len = 0;
@@ -311,10 +357,10 @@
}
}
- /* Uncork to send queued frames */
- corkflag = 0;
- rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
- (const void *) &corkflag, sizeof(corkflag));
+ if (hdtr->numheaders > 0) {
+ /* if we corked, uncork & restore TCP_NODELAY setting */
+ rv = os_uncork(sock, delayflag);
+ }
(*len) = nbytes;
return rv < 0 ? errno : APR_SUCCESS;
Re: [PATCH] ap_sendfile on Linux gets EINVAL
Posted by Greg Ames <gr...@raleigh.ibm.com>.
Jeff Trawick wrote:
>
> To clarify: I'm presenting the idea that we shouldn't fail
> ap_sendfile() if enabling TCP_CORK fails. If the socket is fubar, the
> sendfile() will fail anyway. If the socket is not fubar, then we must
> have run into some other mutually exclusive socket setting which we
> don't know how to back out (or even if we should back it out). I'm
> not hung up on the behavior either way, but I think it is worth
> thinking about briefly.
>
Yep - did think about it. I'm a little cautious at the moment because of the recent
problem w/setsockopt. I'll add a comment that says these can be yanked when we're less
paranoid.
Greg
Re: [PATCH] ap_sendfile on Linux gets EINVAL
Posted by Jeff Trawick <tr...@ibm.net>.
> Date: Mon, 17 Jul 2000 14:47:47 -0400
> From: Greg Ames <gr...@raleigh.ibm.com>
>
> Jeff Trawick wrote:
> >
> > > + }
> > > + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
> > > + (const void *) &corkflag, sizeof(corkflag));
> >
> > If this fails, why do we give a !@#$ beyond avoiding the uncork call?
>
> This is a "should not occur" - ap_sendfile bails if os_cork returns
> a negative value.
To clarify: I'm presenting the idea that we shouldn't fail
ap_sendfile() if enabling TCP_CORK fails. If the socket is fubar, the
sendfile() will fail anyway. If the socket is not fubar, then we must
have run into some other mutually exclusive socket setting which we
don't know how to back out (or even if we should back it out). I'm
not hung up on the behavior either way, but I think it is worth
thinking about briefly.
> > > + /* restore TCP_NODELAY to its original setting */
> >
> > What if you didn't have to change TCP_NODELAY to begin with (i.e.,
> > original setting was off)? Don't turn it off again.
> >
> > > + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> > > + (const void *) &delayflag, sizeof(delayflag));
>
> It works either way. I'm optimizing for the normal case in production, where TCP_NODELAY
> is set. It happens to be broken at the moment, but it's a simple
> fix. Patch forthcoming.
Oh, no... don't tell me that you're being [pause] !!!!!!!!APACHE-CENTRIC!!!!!!!!!
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] ap_sendfile on Linux gets EINVAL
Posted by Greg Ames <gr...@raleigh.ibm.com>.
Jeff Trawick wrote:
>
> > Sender: greg@raleigh.ibm.com
>
> That header doesn't look right :)
>
You're right, it doesn't. Looks like this has been happening since I switched to the
Linux Netscape mailer, and "greg" is my Linux user account...hmmm
> > +static int os_cork(ap_socket_t *sock)
> > +{
> > + /* Linux only for now */
> > +
> > + int nodelay_off = 0, corkflag = 1, rv, delayflag;
> > + socklen_t delaylen;
> > +
> > + /* XXX it would be cheaper to use an ap_socket_t flag here */
>
> You must set delaylen to sizeof delayflag before this call. You've
> been getting lucky so far with the accidental values in autodata (>= 4
> will work fine in practice).
>
> > + rv = getsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> > + (void *) &delayflag, &delaylen);
Doh! My bad - I'll fix it.
> > + }
> > + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
> > + (const void *) &corkflag, sizeof(corkflag));
>
> If this fails, why do we give a !@#$ beyond avoiding the uncork call?
This is a "should not occur" - ap_sendfile bails if os_cork returns a negative value.
>
> > + /* restore TCP_NODELAY to its original setting */
>
> What if you didn't have to change TCP_NODELAY to begin with (i.e.,
> original setting was off)? Don't turn it off again.
>
> > + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> > + (const void *) &delayflag, sizeof(delayflag));
It works either way. I'm optimizing for the normal case in production, where TCP_NODELAY
is set. It happens to be broken at the moment, but it's a simple fix. Patch forthcoming.
Thanks for the review & welcome back.
Greg
Re: [PATCH] ap_sendfile on Linux gets EINVAL
Posted by Jeff Trawick <tr...@ibm.net>.
> Sender: greg@raleigh.ibm.com
That header doesn't look right :)
> Date: Fri, 14 Jul 2000 16:01:08 -0400
> From: Greg Ames <gr...@raleigh.ibm.com>
>
> Index: lib/apr/network_io/unix/sendrecv.c
> ===================================================================
> RCS file: /cvs/apache/apache-2.0/src/lib/apr/network_io/unix/sendrecv.c,v
> retrieving revision 1.32
> diff -u -d -b -r1.32 sendrecv.c
> --- sendrecv.c 2000/07/07 03:00:44 1.32
> +++ sendrecv.c 2000/07/14 19:22:01
> @@ -212,29 +212,75 @@
> */
>
> #if defined(__linux__) && defined(HAVE_WRITEV)
> +
> +/* TCP_CORK keeps us from sending partial frames when we shouldn't
> + * however, it is mutually exclusive w/TCP_NODELAY
> + */
> +
> +static int os_cork(ap_socket_t *sock)
> +{
> + /* Linux only for now */
> +
> + int nodelay_off = 0, corkflag = 1, rv, delayflag;
> + socklen_t delaylen;
> +
> + /* XXX it would be cheaper to use an ap_socket_t flag here */
You must set delaylen to sizeof delayflag before this call. You've
been getting lucky so far with the accidental values in autodata (>= 4
will work fine in practice).
> + rv = getsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> + (void *) &delayflag, &delaylen);
> + if (rv == 0) {
> +
> + if (delayflag != 0) {
> + /* turn off nodelay temporarily to allow cork */
> + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> + (const void *) &nodelay_off, sizeof(nodelay_off));
> + if (rv < 0) {
> + return rv;
> + }
> + }
> + }
> + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
> + (const void *) &corkflag, sizeof(corkflag));
If this fails, why do we give a !@#$ beyond avoiding the uncork call?
> + return rv == 0 ? delayflag : rv;
> +}
> +
> +static int os_uncork(ap_socket_t *sock, int delayflag)
> +{
> + /* Uncork to send queued frames - Linux only for now */
> +
> + int corkflag = 0, rv;
> + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_CORK,
> + (const void *) &corkflag, sizeof(corkflag));
> + if (rv == 0) {
> +
> + /* restore TCP_NODELAY to its original setting */
What if you didn't have to change TCP_NODELAY to begin with (i.e.,
original setting was off)? Don't turn it off again.
> + rv = setsockopt(sock->socketdes, SOL_TCP, TCP_NODELAY,
> + (const void *) &delayflag, sizeof(delayflag));
> + }
> + return rv;
> +}
> +
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...