[PATCH v2] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[PATCH v2] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[Cyrill Gorcunov]

eio library provides a portable version of sendfile syscall
which works more efficient than explicit copying fileby 4K
chunks.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
v2:
 - Use sendfile in a cycle to address files more than 2G size
 - Proper testing of large files remains opened: current CI engine
   is hardly capable of managing it. I tested 200M files with manual
   splitting (to make sure the offsets do really work) but for longterm
   we still might need to invent something
 - Another question which remains -- what to do with partially copied
   files, neither code before the patch or after do not clean up parts
   of copied data. Should not we clean it up on error path? If yes then
   I'll prepare another patch on top.

 src/lib/core/coio_file.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index c5b2db781..541e0f05a 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -571,7 +571,7 @@ static void
 coio_do_copyfile(eio_req *req)
 {
 	struct coio_file_task *eio = (struct coio_file_task *)req->data;
-
+	ssize_t pos, ret, left;
 	struct stat st;
 	if (stat(eio->copyfile.source, &st) < 0) {
 		goto error;
@@ -588,22 +588,18 @@ 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)
+	for (left = st.st_size, pos = 0; left > 0;) {
+		ret = eio_sendfile_sync(dest_fd, source_fd, pos, left);
+		if (ret < 0) {
+			say_syserror("sendfile, [%s -> %s]",
+				     fio_filename(source_fd),
+				     fio_filename(dest_fd));
 			goto error_copy;
+		}
+		pos += ret;
+		left -= ret;
 	}
+
 	req->result = 0;
 	close(source_fd);
 	close(dest_fd);
-- 
2.20.1

Re: [PATCH v2] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[Vladimir Davydov]

On Tue, Apr 16, 2019 at 12:14:10AM +0300, Cyrill Gorcunov wrote:
> eio library provides a portable version of sendfile syscall
> which works more efficient than explicit copying fileby 4K
> chunks.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> v2:
>  - Use sendfile in a cycle to address files more than 2G size
>  - Proper testing of large files remains opened: current CI engine
>    is hardly capable of managing it. I tested 200M files with manual
>    splitting (to make sure the offsets do really work) but for longterm
>    we still might need to invent something

We could use an error injection to reduce the size of data fed to
sendfile in one go.

>  - Another question which remains -- what to do with partially copied
>    files, neither code before the patch or after do not clean up parts
>    of copied data. Should not we clean it up on error path? If yes then
>    I'll prepare another patch on top.

Nobody complained => I don't think we need to do anything about it.

> 
>  src/lib/core/coio_file.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> index c5b2db781..541e0f05a 100644
> --- a/src/lib/core/coio_file.c
> +++ b/src/lib/core/coio_file.c
> @@ -571,7 +571,7 @@ static void
>  coio_do_copyfile(eio_req *req)
>  {
>  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> -
> +	ssize_t pos, ret, left;
>  	struct stat st;
>  	if (stat(eio->copyfile.source, &st) < 0) {
>  		goto error;
> @@ -588,22 +588,18 @@ 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)
> +	for (left = st.st_size, pos = 0; left > 0;) {

st.st_size has time off_t while left has type ssize_t. They differ on
32-bit machines AFAIR.

> +		ret = eio_sendfile_sync(dest_fd, source_fd, pos, left);
> +		if (ret < 0) {
> +			say_syserror("sendfile, [%s -> %s]",
> +				     fio_filename(source_fd),
> +				     fio_filename(dest_fd));
>  			goto error_copy;
> +		}
> +		pos += ret;
> +		left -= ret;
>  	}
> +
>  	req->result = 0;
>  	close(source_fd);
>  	close(dest_fd);

Re: [PATCH v2] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[Cyrill Gorcunov]

On Tue, Apr 16, 2019 at 08:35:10PM +0300, Vladimir Davydov wrote:
> >  - Proper testing of large files remains opened: current CI engine
> >    is hardly capable of managing it. I tested 200M files with manual
> >    splitting (to make sure the offsets do really work) but for longterm
> >    we still might need to invent something
> 
> We could use an error injection to reduce the size of data fed to
> sendfile in one go.

I don't remember if I already asked -- mind to point me where our
error injection examples are?

> >  - Another question which remains -- what to do with partially copied
> >    files, neither code before the patch or after do not clean up parts
> >    of copied data. Should not we clean it up on error path? If yes then
> >    I'll prepare another patch on top.
> 
> Nobody complained => I don't think we need to do anything about it.

ok

> > +	for (left = st.st_size, pos = 0; left > 0;) {
> 
> st.st_size has time off_t while left has type ssize_t. They differ on
> 32-bit machines AFAIR.

Well, not exactly. It depends on __USE_FILE_OFFSET64 definition
but true, better to be on a safe side and use off_t type instead.
Thanks, will update!

Re: [PATCH v2] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[Vladimir Davydov]

On Tue, Apr 16, 2019 at 08:51:33PM +0300, Cyrill Gorcunov wrote:
> On Tue, Apr 16, 2019 at 08:35:10PM +0300, Vladimir Davydov wrote:
> > >  - Proper testing of large files remains opened: current CI engine
> > >    is hardly capable of managing it. I tested 200M files with manual
> > >    splitting (to make sure the offsets do really work) but for longterm
> > >    we still might need to invent something
> > 
> > We could use an error injection to reduce the size of data fed to
> > sendfile in one go.
> 
> I don't remember if I already asked -- mind to point me where our
> error injection examples are?

Try grepping ERRINJ in tests and code, but I see you've already figured
out how to use them.