Re: [PATCHv2] Use sendfile to copy data between file descriptors
2014-11-26 17:43 GMT+01:00 Denys Vlasenko vda.li...@googlemail.com: On Wed, Nov 26, 2014 at 4:06 PM, Denys Vlasenko vda.li...@googlemail.com wrote: Looks like it will always allocate the copy buffer, and always attempt the final read, even if sendfile worked perfectly. Can this be avoided? Please review attached patch. I can't compile it: $ make CC libbb/copyfd.o libbb/copyfd.c: In function ‘bb_full_fd_action’: libbb/copyfd.c:66:11: error: assignment to expression with array type buffer = mmap(NULL, CONFIG_FEATURE_COPYBUF_KB * 1024, ^ libbb/copyfd.c:70:16: error: lvalue required as left operand of assignment buffer_size = CONFIG_FEATURE_COPYBUF_KB * 1024; ^ libbb/copyfd.c:73:12: error: assignment to expression with array type buffer = alloca(4 * 1024); ^ libbb/copyfd.c:74:17: error: lvalue required as left operand of assignment buffer_size = 4 * 1024; ^ Bart ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
Thanks for catching it. So you were using 4k copy buffer. This explains why copies were slow for you... I pushed a tweaked patch to git, please let me know if there are still problems. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
2014-11-27 13:22 GMT+01:00 Denys Vlasenko vda.li...@googlemail.com: Thanks for catching it. So you were using 4k copy buffer. This explains why copies were slow for you... Yeah, I just ran make defconfig. I forgot it's configurable - regular coreutils copy tools use 4k buffers as well. I pushed a tweaked patch to git, please let me know if there are still problems. Are you sure you pushed it? It doesn't seem to be there. Bart ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On Thu, Nov 27, 2014 at 1:32 PM, Bartosz Gołaszewski bartekg...@gmail.com wrote: I pushed a tweaked patch to git, please let me know if there are still problems. Are you sure you pushed it? It doesn't seem to be there. Sorry, I didn't notice that git push ended with ! [rejected] master - master (non-fast-forward) thingy... Now it's there. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On Mon, Oct 20, 2014 at 2:56 PM, Bartosz Golaszewski
bartekg...@gmail.com wrote:
+#if ENABLE_FEATURE_USE_SENDFILE
+#include sys/sendfile.h
+#endif
+
+/*
+ * Returns 1 if all bytes have been copied, 0 otherwise.
+ */
+static int check_status(int *status, off_t *size, off_t *total, ssize_t rd)
+{
+ *total += rd;
+ if (*status 0) { /* if we aren't copying till EOF... */
+ *size -= rd;
+ if (!(*size)) {
+ /* 'size' bytes copied - all done */
+ *status = 0;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
/* Used by NOFORK applets (e.g. cat) - must not use xmalloc.
* size 0 means ignore write errors, used by tar --to-command
* size = 0 means copy till EOF
@@ -18,6 +40,7 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd,
off_t size)
int status = -1;
off_t total = 0;
bool continue_on_write_error = 0;
+ ssize_t rd;
#if CONFIG_FEATURE_COPYBUF_KB = 4
char buffer[CONFIG_FEATURE_COPYBUF_KB * 1024];
enum { buffer_size = sizeof(buffer) };
@@ -56,10 +79,29 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd,
off_t size)
status = 1; /* copy until eof */
}
- while (1) {
- ssize_t rd;
+#if ENABLE_FEATURE_USE_SENDFILE
+ {
+ ssize_t sz = size (size buffer_size)
+ ? size : MAXINT(ssize_t) - 0x;
- rd = safe_read(src_fd, buffer, size buffer_size ?
buffer_size : size);
+ while (1) {
+ rd = sendfile(dst_fd, src_fd, NULL, sz);
+ if (rd = 0) {
+ /* Might be EOF, might be an error,
+* to make sure fall back to the read-write
+* loop.
+*/
+ break;
+ } else if (check_status(status, size, total, rd)) {
+ break;
+ }
+ }
+ }
Looks like it will always allocate the copy buffer,
and always attempt the final read, even if sendfile worked perfectly.
Can this be avoided?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On Wed, Nov 26, 2014 at 4:06 PM, Denys Vlasenko
vda.li...@googlemail.com wrote:
Looks like it will always allocate the copy buffer,
and always attempt the final read, even if sendfile worked perfectly.
Can this be avoided?
Please review attached patch.
diff -d -urpN busybox.8/Config.in busybox.9/Config.in
--- busybox.8/Config.in 2014-08-15 13:04:28.0 +0200
+++ busybox.9/Config.in 2014-11-26 16:32:30.791687946 +0100
@@ -264,6 +264,13 @@ config PAM
Use PAM in some busybox applets (currently login and httpd) instead
of direct access to password database.
+config FEATURE_USE_SENDFILE
+bool Use sendfile system call
+default y
+help
+ When enabled, busybox will use the kernel sendfile() function
+ instead of read/write loops where applicable.
+
config LONG_OPTS
bool Support for --long-options
default y
diff -d -urpN busybox.8/libbb/copyfd.c busybox.9/libbb/copyfd.c
--- busybox.8/libbb/copyfd.c 2014-08-15 13:04:28.0 +0200
+++ busybox.9/libbb/copyfd.c 2014-11-26 17:38:22.814181931 +0100
@@ -8,6 +8,9 @@
*/
#include libbb.h
+#if ENABLE_FEATURE_USE_SENDFILE
+# include sys/sendfile.h
+#endif
/* Used by NOFORK applets (e.g. cat) - must not use xmalloc.
* size 0 means ignore write errors, used by tar --to-command
@@ -18,12 +21,13 @@ static off_t bb_full_fd_action(int src_f
int status = -1;
off_t total = 0;
bool continue_on_write_error = 0;
+ ssize_t sendfile_sz;
#if CONFIG_FEATURE_COPYBUF_KB = 4
char buffer[CONFIG_FEATURE_COPYBUF_KB * 1024];
enum { buffer_size = sizeof(buffer) };
#else
- char *buffer;
- int buffer_size;
+ char *buffer = buffer; /* for compiler */
+ int buffer_size = 0;
#endif
if (size 0) {
@@ -31,46 +35,58 @@ static off_t bb_full_fd_action(int src_f
continue_on_write_error = 1;
}
-#if CONFIG_FEATURE_COPYBUF_KB 4
- if (size 0 size = 4 * 1024)
- goto use_small_buf;
- /* We want page-aligned buffer, just in case kernel is clever
- * and can do page-aligned io more efficiently */
- buffer = mmap(NULL, CONFIG_FEATURE_COPYBUF_KB * 1024,
- PROT_READ | PROT_WRITE,
- MAP_PRIVATE | MAP_ANON,
- /* ignored: */ -1, 0);
- buffer_size = CONFIG_FEATURE_COPYBUF_KB * 1024;
- if (buffer == MAP_FAILED) {
- use_small_buf:
- buffer = alloca(4 * 1024);
- buffer_size = 4 * 1024;
- }
-#endif
-
if (src_fd 0)
goto out;
+ sendfile_sz = !ENABLE_FEATURE_USE_SENDFILE
+ ? 0
+ : MAXINT(ssize_t) - 0x;
if (!size) {
- size = buffer_size;
+ size = MAXINT(ssize_t) - 0x;
status = 1; /* copy until eof */
}
while (1) {
ssize_t rd;
- rd = safe_read(src_fd, buffer, size buffer_size ? buffer_size : size);
-
- if (!rd) { /* eof - all done */
- status = 0;
- break;
+ if (sendfile_sz) {
+#if ENABLE_FEATURE_USE_SENDFILE
+ rd = sendfile(dst_fd, src_fd, NULL,
+size sendfile_sz ? sendfile_sz : size);
+ if (rd = 0)
+goto read_ok;
+#endif
+ sendfile_sz = 0; /* do not try sendfile anymore */
}
+ if (CONFIG_FEATURE_COPYBUF_KB 4 buffer_size == 0) {
+ if (size 0 size = 4 * 1024)
+goto use_small_buf;
+ /* We want page-aligned buffer, just in case kernel is clever
+ * and can do page-aligned io more efficiently */
+ buffer = mmap(NULL, CONFIG_FEATURE_COPYBUF_KB * 1024,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANON,
+ /* ignored: */ -1, 0);
+ buffer_size = CONFIG_FEATURE_COPYBUF_KB * 1024;
+ if (buffer == MAP_FAILED) {
+ use_small_buf:
+buffer = alloca(4 * 1024);
+buffer_size = 4 * 1024;
+ }
+ }
+ rd = safe_read(src_fd, buffer,
+ size buffer_size ? buffer_size : size);
if (rd 0) {
bb_perror_msg(bb_msg_read_error);
break;
}
+ read_ok:
+ if (!rd) { /* eof - all done */
+ status = 0;
+ break;
+ }
/* dst_fd == -1 is a fake, else... */
- if (dst_fd = 0) {
+ if (dst_fd = 0 !sendfile_sz) {
ssize_t wr = full_write(dst_fd, buffer, rd);
if (wr rd) {
if (!continue_on_write_error) {
@@ -93,7 +109,7 @@ static off_t bb_full_fd_action(int src_f
out:
#if CONFIG_FEATURE_COPYBUF_KB 4
- if (buffer_size != 4 * 1024)
+ if (buffer_size 4 * 1024)
munmap(buffer, buffer_size);
#endif
return status ? -1 : total;
diff -d -urpN busybox.8/networking/Config.src busybox.9/networking/Config.src
--- busybox.8/networking/Config.src 2014-08-15 13:04:28.0 +0200
+++ busybox.9/networking/Config.src 2014-11-26 16:32:30.792687946 +0100
@@ -181,14 +181,6 @@ config FEATURE_HTTPD_RANGES
Range: bytes=NNN-[MMM] header. Allows for resuming interrupted
downloads, seeking in multimedia players etc.
-config FEATURE_HTTPD_USE_SENDFILE
- bool Use sendfile system call
- default y
- depends on HTTPD
- help
- When enabled, httpd will use the kernel sendfile() function
- instead of read/write loop.
-
config FEATURE_HTTPD_SETUID
bool Enable -u user option
default y
diff -d -urpN busybox.8/networking/httpd.c busybox.9/networking/httpd.c
--- busybox.8/networking/httpd.c 2014-08-15 13:04:28.0
Re: [PATCHv2] Use sendfile to copy data between file descriptors
Well, this is fortuitous. From the kernel mailing list: From: Pieter Smith pie...@boesman.nl Subject: [PATCH 2/2] fs: Support compiling out sendfile Date: Mon, 20 Oct 2014 23:48:37 +0200 Many embedded systems will not need this syscall, and omitting it saves space. Add a new EXPERT config option CONFIG_SENDFILE_SYSCALL (default y) to support compiling it out. There's a pleasing symmetry that while BusyBox gains the ability to configure in sendfile the kernel gains the ability to configure it out. Ron ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On Tue, Oct 21, 2014, at 08:46, Mike Frysinger wrote: On 20 Oct 2014 21:00, Lauri Kasanen wrote: +config FEATURE_USE_SENDFILE +bool Use sendfile system call +default y +help + When enabled, busybox will use the kernel sendfile() function + instead of read/write loops where applicable. Please include the kernel version requirement here. `man 2 sendfile`: In Linux kernels before 2.6.33, out_fd must refer to a socket. Since Linux 2.6.33 it can be any file. If it is a regular file, then sendfile() changes the file offset appropriately. -mike Yes, I know? The point was to tell the user configuring busybox that. - Lauri PS: Sorry, forgot the list the first time. -- http://www.fastmail.fm - Or how I learned to stop worrying and love email again ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On 21 Oct 2014 11:01, Lauri Kasanen wrote: On Tue, Oct 21, 2014, at 08:46, Mike Frysinger wrote: On 20 Oct 2014 21:00, Lauri Kasanen wrote: +config FEATURE_USE_SENDFILE +bool Use sendfile system call +default y +help + When enabled, busybox will use the kernel sendfile() function + instead of read/write loops where applicable. Please include the kernel version requirement here. `man 2 sendfile`: In Linux kernels before 2.6.33, out_fd must refer to a socket. Since Linux 2.6.33 it can be any file. If it is a regular file, then sendfile() changes the file offset appropriately. Yes, I know? The point was to tell the user configuring busybox that. chill dog. the fact you knew it doesn't mean the submitter did. nowhere did i say you were ignorant. -mike signature.asc Description: Digital signature ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCHv2] Use sendfile to copy data between file descriptors
Busybox already uses sendfile in httpd. This patch proposes to use it
globally to copy data between file descriptors.
It speeds up the copying on slow systems a lot - below are the times needed
to copy a 450Mb file with and without this option enabled on a BeagleBone
Black:
sendfile:
user0m0.000s
sys 0m8.170s
read/write:
user0m0.470s
sys 0m16.300s
It doesn't add a lot of bloat either:
function old new delta
bb_full_fd_action233 311 +78
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 78/0) Total: 78 bytes
textdata bss dec hex filename
81283941239552 826514 c9c92 busybox_old
81291741239552 826592 c9ce0 busybox_unstripped
This patch also moves USE_SENDFILE feature from httpd subconfiguration
to global settings.
Signed-off-by: Bartosz Golaszewski bartekg...@gmail.com
---
Config.in | 7 ++
libbb/copyfd.c| 59 ---
networking/Config.src | 8 ---
networking/httpd.c| 6 +++---
4 files changed, 57 insertions(+), 23 deletions(-)
diff --git a/Config.in b/Config.in
index b83beb5..3edaf8f 100644
--- a/Config.in
+++ b/Config.in
@@ -264,6 +264,13 @@ config PAM
Use PAM in some busybox applets (currently login and httpd) instead
of direct access to password database.
+config FEATURE_USE_SENDFILE
+bool Use sendfile system call
+default y
+help
+ When enabled, busybox will use the kernel sendfile() function
+ instead of read/write loops where applicable.
+
config LONG_OPTS
bool Support for --long-options
default y
diff --git a/libbb/copyfd.c b/libbb/copyfd.c
index eda2747..e10fe82 100644
--- a/libbb/copyfd.c
+++ b/libbb/copyfd.c
@@ -9,6 +9,28 @@
#include libbb.h
+#if ENABLE_FEATURE_USE_SENDFILE
+#include sys/sendfile.h
+#endif
+
+/*
+ * Returns 1 if all bytes have been copied, 0 otherwise.
+ */
+static int check_status(int *status, off_t *size, off_t *total, ssize_t rd)
+{
+ *total += rd;
+ if (*status 0) { /* if we aren't copying till EOF... */
+ *size -= rd;
+ if (!(*size)) {
+ /* 'size' bytes copied - all done */
+ *status = 0;
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
/* Used by NOFORK applets (e.g. cat) - must not use xmalloc.
* size 0 means ignore write errors, used by tar --to-command
* size = 0 means copy till EOF
@@ -18,6 +40,7 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd, off_t
size)
int status = -1;
off_t total = 0;
bool continue_on_write_error = 0;
+ ssize_t rd;
#if CONFIG_FEATURE_COPYBUF_KB = 4
char buffer[CONFIG_FEATURE_COPYBUF_KB * 1024];
enum { buffer_size = sizeof(buffer) };
@@ -56,10 +79,29 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd,
off_t size)
status = 1; /* copy until eof */
}
- while (1) {
- ssize_t rd;
+#if ENABLE_FEATURE_USE_SENDFILE
+ {
+ ssize_t sz = size (size buffer_size)
+ ? size : MAXINT(ssize_t) - 0x;
- rd = safe_read(src_fd, buffer, size buffer_size ? buffer_size
: size);
+ while (1) {
+ rd = sendfile(dst_fd, src_fd, NULL, sz);
+ if (rd = 0) {
+ /* Might be EOF, might be an error,
+* to make sure fall back to the read-write
+* loop.
+*/
+ break;
+ } else if (check_status(status, size, total, rd)) {
+ break;
+ }
+ }
+ }
+#endif
+
+ while (1) {
+ rd = safe_read(src_fd, buffer,
+ size buffer_size ? buffer_size : size);
if (!rd) { /* eof - all done */
status = 0;
@@ -80,15 +122,8 @@ static off_t bb_full_fd_action(int src_fd, int dst_fd,
off_t size)
dst_fd = -1;
}
}
- total += rd;
- if (status 0) { /* if we aren't copying till EOF... */
- size -= rd;
- if (!size) {
- /* 'size' bytes copied - all done */
- status = 0;
- break;
- }
- }
+ if (check_status(status, size, total, rd))
+ break;
}
out:
diff --git a/networking/Config.src b/networking/Config.src
Re: [PATCHv2] Use sendfile to copy data between file descriptors
+config FEATURE_USE_SENDFILE
+bool Use sendfile system call
+default y
+help
+ When enabled, busybox will use the kernel sendfile() function
+ instead of read/write loops where applicable.
Please include the kernel version requirement here.
+#if ENABLE_FEATURE_USE_SENDFILE
+ {
+ ssize_t sz = size (size buffer_size)
+ ? size : MAXINT(ssize_t) - 0x;
- rd = safe_read(src_fd, buffer, size buffer_size ?
buffer_size : size);
+ while (1) {
And here. Comment should be enough, as the call will probably error out
on older kernels.
+ rd = sendfile(dst_fd, src_fd, NULL, sz);
+ if (rd = 0) {
+ /* Might be EOF, might be an error,
+* to make sure fall back to the
- Lauri
--
http://www.fastmail.fm - A no graphics, no pop-ups email service
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCHv2] Use sendfile to copy data between file descriptors
On 20 Oct 2014 21:00, Lauri Kasanen wrote: +config FEATURE_USE_SENDFILE +bool Use sendfile system call +default y +help + When enabled, busybox will use the kernel sendfile() function + instead of read/write loops where applicable. Please include the kernel version requirement here. `man 2 sendfile`: In Linux kernels before 2.6.33, out_fd must refer to a socket. Since Linux 2.6.33 it can be any file. If it is a regular file, then sendfile() changes the file offset appropriately. -mike signature.asc Description: Digital signature ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
