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

[PATCH] lib/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 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

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

[Vladimir Davydov]

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);

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

[Cyrill Gorcunov]

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 :)

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

[Vladimir Davydov]

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.

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

[Cyrill Gorcunov]

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.

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

[Vladimir Davydov]

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.

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

[Cyrill Gorcunov]

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