* [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode @ 2019-04-10 19:36 Cyrill Gorcunov 2019-04-12 13:43 ` Vladimir Davydov 0 siblings, 1 reply; 7+ messages in thread From: Cyrill Gorcunov @ 2019-04-10 19:36 UTC (permalink / raw) To: tml; +Cc: Vladimir Davydov, Cyrill Gorcunov eio library provides a portable version of sendfile syscall which works a way more efficient than explicit copying file by 4K chunks. Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- Test app/fio.test.lua which uses this helper passed. src/lib/core/coio_file.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c index c5b2db781..27806d7d5 100644 --- a/src/lib/core/coio_file.c +++ b/src/lib/core/coio_file.c @@ -588,22 +588,14 @@ coio_do_copyfile(eio_req *req) goto error_dest; } - enum { COPY_FILE_BUF_SIZE = 4096 }; - - char buf[COPY_FILE_BUF_SIZE]; - - while (true) { - ssize_t nread = fio_read(source_fd, buf, sizeof(buf)); - if (nread < 0) - goto error_copy; - - if (nread == 0) - break; /* eof */ - - ssize_t nwritten = fio_writen(dest_fd, buf, nread); - if (nwritten < 0) - goto error_copy; + ssize_t nwritten = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size); + if (nwritten != st.st_size) { + say_syserror("sendfile, [%s -> %s]", + fio_filename(source_fd), + fio_filename(dest_fd)); + goto error_copy; } + req->result = 0; close(source_fd); close(dest_fd); -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-10 19:36 [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode Cyrill Gorcunov @ 2019-04-12 13:43 ` Vladimir Davydov 2019-04-12 14:05 ` Cyrill Gorcunov 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Davydov @ 2019-04-12 13:43 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Wed, Apr 10, 2019 at 10:36:24PM +0300, Cyrill Gorcunov wrote: > eio library provides a portable version of sendfile syscall > which works a way more efficient than explicit copying file > by 4K chunks. > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> > --- > > Test app/fio.test.lua which uses this helper passed. > > src/lib/core/coio_file.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c > index c5b2db781..27806d7d5 100644 > --- a/src/lib/core/coio_file.c > +++ b/src/lib/core/coio_file.c > @@ -588,22 +588,14 @@ coio_do_copyfile(eio_req *req) > goto error_dest; > } > > - enum { COPY_FILE_BUF_SIZE = 4096 }; > - > - char buf[COPY_FILE_BUF_SIZE]; > - > - while (true) { > - ssize_t nread = fio_read(source_fd, buf, sizeof(buf)); > - if (nread < 0) > - goto error_copy; > - > - if (nread == 0) > - break; /* eof */ > - > - ssize_t nwritten = fio_writen(dest_fd, buf, nread); > - if (nwritten < 0) > - goto error_copy; > + ssize_t nwritten = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size); > + if (nwritten != st.st_size) { > + say_syserror("sendfile, [%s -> %s]", > + fio_filename(source_fd), > + fio_filename(dest_fd)); Other coio/fio methods don't log errors so I don't think this one should, either. An extract from the sendfile manual page: } sendfile() will transfer at most 0x7ffff000 (2,147,479,552) bytes, } returning the number of bytes actually transferred. (This is true on } both 32-bit and 64-bit systems.) So I don't think using sendfile is equivalent to read+write... > + goto error_copy; > } > + > req->result = 0; > close(source_fd); > close(dest_fd); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-12 13:43 ` Vladimir Davydov @ 2019-04-12 14:05 ` Cyrill Gorcunov 2019-04-12 14:19 ` Vladimir Davydov 0 siblings, 1 reply; 7+ messages in thread From: Cyrill Gorcunov @ 2019-04-12 14:05 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tml On Fri, Apr 12, 2019 at 04:43:23PM +0300, Vladimir Davydov wrote: > > + ssize_t nwritten = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size); > > + if (nwritten != st.st_size) { > > + say_syserror("sendfile, [%s -> %s]", > > + fio_filename(source_fd), > > + fio_filename(dest_fd)); > > Other coio/fio methods don't log errors so I don't think this one > should, either. But fio_ methods which we used before the patch do print an error, so we should continue printings. > An extract from the sendfile manual page: > > } sendfile() will transfer at most 0x7ffff000 (2,147,479,552) bytes, > } returning the number of bytes actually transferred. (This is true on > } both 32-bit and 64-bit systems.) > > So I don't think using sendfile is equivalent to read+write... Actaully man page is a bit wrong, at most it may handle up to 0x7fffffff bytes. And here is a question do we really ever going to transfer such big files? Moreover using read/write with 4K buffer for big files is pure and utter crap, the only thing which saves situation a bit is a page cache, but still such small buffer requires a number of context switches :) But I agree that using sendfile here actually may trim file. I'll take a look at evening but I think we should rather use sendfile with offsets in a cycle. Seriouly, using 4K buffer is the worst possible algo :) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-12 14:05 ` Cyrill Gorcunov @ 2019-04-12 14:19 ` Vladimir Davydov 2019-04-12 14:42 ` Cyrill Gorcunov 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Davydov @ 2019-04-12 14:19 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Fri, Apr 12, 2019 at 05:05:58PM +0300, Cyrill Gorcunov wrote: > On Fri, Apr 12, 2019 at 04:43:23PM +0300, Vladimir Davydov wrote: > > > + ssize_t nwritten = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size); > > > + if (nwritten != st.st_size) { > > > + say_syserror("sendfile, [%s -> %s]", > > > + fio_filename(source_fd), > > > + fio_filename(dest_fd)); > > > > Other coio/fio methods don't log errors so I don't think this one > > should, either. > > But fio_ methods which we used before the patch do print an error, > so we should continue printings. Ah, okay, I see. > > > An extract from the sendfile manual page: > > > > } sendfile() will transfer at most 0x7ffff000 (2,147,479,552) bytes, > > } returning the number of bytes actually transferred. (This is true on > > } both 32-bit and 64-bit systems.) > > > > So I don't think using sendfile is equivalent to read+write... > > Actaully man page is a bit wrong, at most it may handle up to 0x7fffffff > bytes. And here is a question do we really ever going to transfer such big > files? Moreover using read/write with 4K buffer for big files is pure and > utter crap, the only thing which saves situation a bit is a page cache, > but still such small buffer requires a number of context switches :) It's not a very hot function, to begin with. I doubt it's used that often or for copying big files. Besides, it is handled in a coio thread (i.e. not in tx) so we can afford it to be a bit slow. > > But I agree that using sendfile here actually may trim file. I'll take > a look at evening but I think we should rather use sendfile with offsets > in a cycle. Seriouly, using 4K buffer is the worst possible algo :) Yeah, it would be great to make use of sendfile, but we must make sure we won't occasionally break anything. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-12 14:19 ` Vladimir Davydov @ 2019-04-12 14:42 ` Cyrill Gorcunov 2019-04-12 14:53 ` Vladimir Davydov 0 siblings, 1 reply; 7+ messages in thread From: Cyrill Gorcunov @ 2019-04-12 14:42 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tml On Fri, Apr 12, 2019 at 05:19:16PM +0300, Vladimir Davydov wrote: ... > > But I agree that using sendfile here actually may trim file. I'll take > > a look at evening but I think we should rather use sendfile with offsets > > in a cycle. Seriouly, using 4K buffer is the worst possible algo :) > > Yeah, it would be great to make use of sendfile, but we must make sure > we won't occasionally break anything. I will have to update tests. The problem only is that we've to test big files here, have no clue if our CI engine will allow us to generate/copy such sizes. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-12 14:42 ` Cyrill Gorcunov @ 2019-04-12 14:53 ` Vladimir Davydov 2019-04-12 14:59 ` Cyrill Gorcunov 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Davydov @ 2019-04-12 14:53 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tml On Fri, Apr 12, 2019 at 05:42:12PM +0300, Cyrill Gorcunov wrote: > On Fri, Apr 12, 2019 at 05:19:16PM +0300, Vladimir Davydov wrote: > ... > > > But I agree that using sendfile here actually may trim file. I'll take > > > a look at evening but I think we should rather use sendfile with offsets > > > in a cycle. Seriouly, using 4K buffer is the worst possible algo :) > > > > Yeah, it would be great to make use of sendfile, but we must make sure > > we won't occasionally break anything. > > I will have to update tests. The problem only is that we've to test > big files here, have no clue if our CI engine will allow us to > generate/copy such sizes. True, I don't think it's worth testing it. I just wanted to say that we need to make sure it works - check it manually and I will too. Alternatively, we could add an error injection, reducing the sendfile copy size, but I think it's not really necessary. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode 2019-04-12 14:53 ` Vladimir Davydov @ 2019-04-12 14:59 ` Cyrill Gorcunov 0 siblings, 0 replies; 7+ messages in thread From: Cyrill Gorcunov @ 2019-04-12 14:59 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tml On Fri, Apr 12, 2019 at 05:53:47PM +0300, Vladimir Davydov wrote: > > True, I don't think it's worth testing it. > > I just wanted to say that we need to make sure it works - check it > manually and I will too. Current tests are passed even without "cycled sendfile" simply because current tests are not operating with huge files (i ran them manually before sending the patch). I'll recheck everything once again before sending v2. Thanks for review! > Alternatively, we could add an error injection, reducing the sendfile > copy size, but I think it's not really necessary. OK ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-12 14:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-10 19:36 [PATCH] lib/core/coio_file: Use eio_sendfile_sync instead of a chunk mode Cyrill Gorcunov 2019-04-12 13:43 ` Vladimir Davydov 2019-04-12 14:05 ` Cyrill Gorcunov 2019-04-12 14:19 ` Vladimir Davydov 2019-04-12 14:42 ` Cyrill Gorcunov 2019-04-12 14:53 ` Vladimir Davydov 2019-04-12 14:59 ` Cyrill Gorcunov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox