Prefer mmap to sendfile unless a DRAINS_TO_FD flag is set. Allows add_file to work...

The sendfile() implementation for evbuffer_add_file is potentially more
efficient, but it has a problem: you can only use it to send bytes over
a socket using sendfile().  If you are writing bytes via SSL_send() or
via a filter, or if you need to be able to inspect your buffer, it
doesn't work.

As an easy fix, this patch disables the sendfile-based implementation of
evbuffer_add_file on an evbuffer unless the user sets a new
EVBUFFER_FLAG_DRAINS_TO_FD flag on that evbuffer, indicating that the
evbuffer will not be inspected, but only written out via
evbuffer_write(), evbuffer_write_atmost(), or drained with stuff like
evbuffer_drain() or evbuffer_add_buffer().  This flag is off by
default, except for evbuffers used for output on bufferevent_socket.

In the future, it could be interesting to make a best-effort file
segment implementation that tries to send via sendfile, but mmaps on
demand.  That's too much complexity for a stable release series, though.

diff --git a/buffer.c b/buffer.c
index 146bbdf43d84d7e3acbc8bb2be65b36ede0f874a..e404f2b4f1addf955a67d79a3061b316270781e2 100644 (file)
--- a/buffer.c
+++ b/buffer.c
@@ -327,6 +327,24 @@ evbuffer_new(void)
        return (buffer);
 }
 
+int
+evbuffer_set_flags(struct evbuffer *buf, ev_uint64_t flags)
+{
+       EVBUFFER_LOCK(buf);
+       buf->flags |= (ev_uint32_t)flags;
+       EVBUFFER_UNLOCK(buf);
+       return 0;
+}
+
+int
+evbuffer_clear_flags(struct evbuffer *buf, ev_uint64_t flags)
+{
+       EVBUFFER_LOCK(buf);
+       buf->flags &= ~(ev_uint32_t)flags;
+       EVBUFFER_UNLOCK(buf);
+       return 0;
+}
+
 void
 _evbuffer_incref(struct evbuffer *buf)
 {
@@ -2680,11 +2698,20 @@ evbuffer_add_file(struct evbuffer *outbuf, int fd,
 #if defined(USE_SENDFILE) || defined(_EVENT_HAVE_MMAP)
        struct evbuffer_chain *chain;
        struct evbuffer_chain_fd *info;
+#endif
+#if defined(USE_SENDFILE)
+       int sendfile_okay = 1;
 #endif
        int ok = 1;
 
 #if defined(USE_SENDFILE)
        if (use_sendfile) {
+               EVBUFFER_LOCK(outbuf);
+               sendfile_okay = outbuf->flags & EVBUFFER_FLAG_DRAINS_TO_FD;
+               EVBUFFER_UNLOCK(outbuf);
+       }
+
+       if (use_sendfile && sendfile_okay) {
                chain = evbuffer_chain_new(sizeof(struct evbuffer_chain_fd));
                if (chain == NULL) {
                        event_warn("%s: out of memory", __func__);

diff --git a/bufferevent_sock.c b/bufferevent_sock.c
index 089aedb4d8af5877ca5026201080e16f6401fc3c..1dc0f33006b8de6216e08972ce122fd7f272ab03 100644 (file)
--- a/bufferevent_sock.c
+++ b/bufferevent_sock.c
@@ -328,6 +328,7 @@ bufferevent_socket_new(struct event_base *base, evutil_socket_t fd,
                return NULL;
        }
        bufev = &bufev_p->bev;
+       evbuffer_set_flags(bufev->output, EVBUFFER_FLAG_DRAINS_TO_FD);
 
        event_assign(&bufev->ev_read, bufev->ev_base, fd,
            EV_READ|EV_PERSIST, bufferevent_readcb, bufev);

diff --git a/evbuffer-internal.h b/evbuffer-internal.h
index 5879f874d7bbe986f0a2e3fbfe422ef1288ddc53..89139602c7219d9c1e2ff056dbda1ee93b35e45d 100644 (file)
--- a/evbuffer-internal.h
+++ b/evbuffer-internal.h
@@ -129,6 +129,8 @@ struct evbuffer {
        /** True iff this buffer is set up for overlapped IO. */
        unsigned is_overlapped : 1;
 #endif
+       /** Zero or more EVBUFFER_FLAG_* bits */
+       ev_uint32_t flags;
 
        /** Used to implement deferred callbacks. */
        struct deferred_cb_queue *cb_queue;

diff --git a/include/event2/buffer.h b/include/event2/buffer.h
index 19f6f90ab04607550ae8e456e7cc91869940bd26..c53cb9c9e0794157f2585358ba04674b3d7b30db 100644 (file)
--- a/include/event2/buffer.h
+++ b/include/event2/buffer.h
@@ -141,7 +141,6 @@ struct evbuffer_iovec {
        occurred
  */
 struct evbuffer *evbuffer_new(void);
-
 /**
   Deallocate storage for an evbuffer.
 
@@ -175,6 +174,41 @@ void evbuffer_lock(struct evbuffer *buf);
 */
 void evbuffer_unlock(struct evbuffer *buf);
 
+
+/** If this flag is set, then we will not use evbuffer_peek(),
+ * evbuffer_remove(), evbuffer_remove_buffer(), and so on to read bytes
+ * from this buffer: we'll only take bytes out of this buffer by
+ * writing them to the network (as with evbuffer_write_atmost), by
+ * removing them without observing them (as with evbuffer_drain),
+ * or by copying them all out at once (as with evbuffer_add_buffer).
+ *
+ * Using this option allows the implementation to use sendfile-based
+ * operations for evbuffer_add_file(); see that function for more
+ * information.
+ *
+ * This flag is on by default for bufferevents that can take advantage
+ * of it; you should never actually need to set it on a bufferevent's
+ * output buffer.
+ */
+#define EVBUFFER_FLAG_DRAINS_TO_FD 1
+
+/** Change the flags that are set for an evbuffer by adding more.
+ *
+ * @param buffer the evbuffer that the callback is watching.
+ * @param cb the callback whose status we want to change.
+ * @param flags One or more EVBUFFER_FLAG_* options
+ * @return 0 on success, -1 on failure.
+ */
+int evbuffer_set_flags(struct evbuffer *buf, ev_uint64_t flags);
+/** Change the flags that are set for an evbuffer by removing some.
+ *
+ * @param buffer the evbuffer that the callback is watching.
+ * @param cb the callback whose status we want to change.
+ * @param flags One or more EVBUFFER_FLAG_* options
+ * @return 0 on success, -1 on failure.
+ */
+int evbuffer_clear_flags(struct evbuffer *buf, ev_uint64_t flags);
+
 /**
   Returns the total number of bytes stored in the evbuffer
 
@@ -408,8 +442,9 @@ int evbuffer_add_reference(struct evbuffer *outbuf,
   Copy data from a file into the evbuffer for writing to a socket.
 
   This function avoids unnecessary data copies between userland and
-  kernel.  Where available, it uses sendfile or splice; failing those,
-  it tries to use mmap.
+  kernel.  If sendfile is available and the EVBUFFER_FLAG_DRAINS_TO_FD
+  flag is set, it uses those functions.  Otherwise, it tries to use
+  mmap (or CreateFileMapping on Windows).
 
   The function owns the resulting file descriptor and will close it
   when finished transferring data.

diff --git a/test/regress_buffer.c b/test/regress_buffer.c
index 4deaef57a98cc9dbd7d2e18d437be2fdda6ac909..e6b8dfcb9ec0ae5c17595bad8b9bb0ab41298657 100644 (file)
--- a/test/regress_buffer.c
+++ b/test/regress_buffer.c
@@ -618,6 +618,9 @@ test_evbuffer_add_file(void *ptr)
                TT_DIE(("Didn't recognize the implementation"));
        }
 
+       /* Say that it drains to a fd so that we can use sendfile. */
+       evbuffer_set_flags(src, EVBUFFER_FLAG_DRAINS_TO_FD);
+
 #if defined(_EVENT_HAVE_SENDFILE) && defined(__sun__) && defined(__svr4__)
        /* We need to use a pair of AF_INET sockets, since Solaris
           doesn't support sendfile() over AF_UNIX. */