[Tarantool-patches] [PATCH] fio/coio: Handle partial writes

[Tarantool-patches] [PATCH] fio/coio: Handle partial writes

[Cyrill Gorcunov]

Writting less bytes than requested is perfectly fine
from OS point of view. In turn our fio.write/pwrite
api simply returns 'true' even if only a part of
a buffer has been written.

We can't change the api without breaking backward
compatibility (otherwise we could simply return number
of bytes written, instead of current true/false sign).

For this sake both coio_write and coio_pwrite tries
to deliver complete data in a cycle.

Fixes #4651

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 https://github.com/tarantool/tarantool/compare/gorcunov/gh-4651-partial-write
 https://github.com/tarantool/tarantool/issues/4651

 src/lib/core/coio_file.c | 63 ++++++++++++++++++++++++++++------
 src/lib/core/errinj.h    |  1 +
 test/app/fio.result      | 73 ++++++++++++++++++++++++++++++++++++++++
 test/app/fio.test.lua    | 21 ++++++++++++
 test/box/errinj.result   |  2 ++
 5 files changed, 149 insertions(+), 11 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 62388344e..90903c8df 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -164,10 +164,31 @@ coio_file_close(int fd)
 ssize_t
 coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
 {
-	INIT_COEIO_FILE(eio);
-	eio_req *req = eio_write(fd, (void *) buf, count, offset,
-				 0, coio_complete, &eio);
-	return coio_wait_done(req, &eio);
+	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
+	ssize_t left = count, pos = 0, res, chunk;
+	eio_req *req;
+
+	while (left > 0) {
+		INIT_COEIO_FILE(eio);
+
+		if (inj != NULL && inj->iparam > 0)
+			chunk = (ssize_t)inj->iparam;
+		else
+			chunk = left;
+
+		req = eio_write(fd, (void *)buf + pos, chunk,
+				offset + pos, EIO_PRI_DEFAULT,
+				coio_complete, &eio);
+		res = coio_wait_done(req, &eio);
+		if (res < 0) {
+			if (errno != EINTR)
+				break;
+		} else {
+			left -= res;
+			pos += res;
+		}
+	}
+	return pos;
 }
 
 ssize_t
@@ -200,7 +221,12 @@ coio_preadn(int fd, void *buf, size_t count, off_t offset)
 static void
 coio_do_write(eio_req *req)
 {
+	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
 	struct coio_file_task *eio = (struct coio_file_task *)req->data;
+
+	if (inj != NULL && inj->iparam > 0)
+		eio->write.count = (ssize_t)inj->iparam;
+
 	req->result = write(eio->write.fd, eio->write.buf, eio->write.count);
 	eio->errorno = errno;
 }
@@ -208,13 +234,28 @@ coio_do_write(eio_req *req)
 ssize_t
 coio_write(int fd, const void *buf, size_t count)
 {
-	INIT_COEIO_FILE(eio);
-	eio.write.buf = buf;
-	eio.write.count = count;
-	eio.write.fd = fd;
-	eio_req *req = eio_custom(coio_do_write, 0,
-				  coio_complete, &eio);
-	return coio_wait_done(req, &eio);
+	ssize_t left = count, pos = 0, res;
+	eio_req *req;
+
+	while (left > 0) {
+		INIT_COEIO_FILE(eio);
+
+		eio.write.buf	= buf + pos;
+		eio.write.count	= left;
+		eio.write.fd	= fd;
+
+		req = eio_custom(coio_do_write, EIO_PRI_DEFAULT,
+				 coio_complete, &eio);
+		res = coio_wait_done(req, &eio);
+		if (res < 0) {
+			if (errno != EINTR)
+				break;
+		} else {
+			left -= res;
+			pos += res;
+		}
+	}
+	return pos;
 }
 
 static void
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 3072a00ea..f90554d06 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -134,6 +134,7 @@ struct errinj {
 	_(ERRINJ_SQL_NAME_NORMALIZATION, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_SWIM_FD_ONLY, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT, {.iparam = -1}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/app/fio.result b/test/app/fio.result
index f83c43f44..6eafa24e6 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -776,6 +776,12 @@ file5 = fio.pathjoin(tree, 'file.5')
 file6 = fio.pathjoin(tree, 'file.6')
 ---
 ...
+file7 = fio.pathjoin(tree, 'file.7')
+---
+...
+file8 = fio.pathjoin(tree, 'file.8')
+---
+...
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
 ---
 ...
@@ -865,6 +871,73 @@ fh6:close()
 ---
 - true
 ...
+-- test partial write/pwrite via one byte transfer
+fh7 = fio.open(file7, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+---
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', 1)
+---
+- ok
+...
+fh7:write("one byte transfer, write")
+---
+- true
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', -1)
+---
+- ok
+...
+fh7:seek(0)
+---
+- 0
+...
+fh7:read()
+---
+- one byte transfer, write
+...
+fh7:close()
+---
+- true
+...
+fh8 = fio.open(file8, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+---
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', 1)
+---
+- ok
+...
+fh8:pwrite("one byte transfer, ", 0)
+---
+- true
+...
+fh8:seek(0)
+---
+- 0
+...
+fh8:read()
+---
+- 'one byte transfer, '
+...
+fh8:pwrite("pwrite", 19)
+---
+- true
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', -1)
+---
+- ok
+...
+fh8:seek(0)
+---
+- 0
+...
+fh8:read()
+---
+- one byte transfer, pwrite
+...
+fh8:close()
+---
+- true
+...
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 ---
 ...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 56c957d8a..3ed06bcda 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -251,6 +251,8 @@ file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
 file5 = fio.pathjoin(tree, 'file.5')
 file6 = fio.pathjoin(tree, 'file.6')
+file7 = fio.pathjoin(tree, 'file.7')
+file8 = fio.pathjoin(tree, 'file.8')
 
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
 fh1:write("gogo")
@@ -280,6 +282,25 @@ fh6:seek(0)
 fh6:read()
 fh6:close()
 
+-- test partial write/pwrite via one byte transfer
+fh7 = fio.open(file7, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', 1)
+fh7:write("one byte transfer, write")
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', -1)
+fh7:seek(0)
+fh7:read()
+fh7:close()
+fh8 = fio.open(file8, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', 1)
+fh8:pwrite("one byte transfer, ", 0)
+fh8:seek(0)
+fh8:read()
+fh8:pwrite("pwrite", 19)
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', -1)
+fh8:seek(0)
+fh8:read()
+fh8:close()
+
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 res
 err:match("failed to copy") ~= nil
diff --git a/test/box/errinj.result b/test/box/errinj.result
index a148346e8..9b831769f 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -25,6 +25,8 @@ errinj.info()
     state: 0
   ERRINJ_WAL_WRITE:
     state: false
+  ERRINJ_COIO_WRITE_CHUNK:
+    state: -1
   ERRINJ_HTTPC_EXECUTE:
     state: false
   ERRINJ_VYRUN_DATA_READ:
-- 
2.20.1

Re: [Tarantool-patches] [PATCH] fio/coio: Handle partial writes

[Cyrill Gorcunov]

On Tue, Nov 26, 2019 at 09:05:03PM +0300, Cyrill Gorcunov wrote:
> Writting less bytes than requested is perfectly fine
> from OS point of view. In turn our fio.write/pwrite
> api simply returns 'true' even if only a part of
> a buffer has been written.
> 
> We can't change the api without breaking backward
> compatibility (otherwise we could simply return number
> of bytes written, instead of current true/false sign).
> 
> For this sake both coio_write and coio_pwrite tries
> to deliver complete data in a cycle.

Drop it please, there is a small typo in it. I'll rework
and send v2.

[Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Cyrill Gorcunov]

Writting less bytes than requested is perfectly fine.
In turn our fio.write/pwrite api simply returns 'true'
even if only some part of a buffer has been written.

Thus make coio_write and coio_pwrite to write the
whole data in a cycle. Note in most situations there
will be only one pass, partial writes are really the
rare cases.

Fixes #4651

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/4651
branch gorcunov/gh-4651-partial-write

 src/lib/core/coio_file.c | 62 ++++++++++++++++++++++++++++------
 src/lib/core/errinj.h    |  1 +
 test/app/fio.result      | 73 ++++++++++++++++++++++++++++++++++++++++
 test/app/fio.test.lua    | 21 ++++++++++++
 test/box/errinj.result   |  1 +
 5 files changed, 147 insertions(+), 11 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index e2345567c..e290214bc 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -164,10 +164,30 @@ coio_file_close(int fd)
 ssize_t
 coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
 {
-	INIT_COEIO_FILE(eio);
-	eio_req *req = eio_write(fd, (void *) buf, count, offset,
-				 0, coio_complete, &eio);
-	return coio_wait_done(req, &eio);
+	ssize_t left = count, pos = 0, res, chunk;
+	eio_req *req;
+
+	while (left > 0) {
+		INIT_COEIO_FILE(eio);
+		chunk = left;
+
+		ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
+			chunk = 1;
+		});
+
+		req = eio_write(fd, (void *)buf + pos, chunk,
+				offset + pos, EIO_PRI_DEFAULT,
+				coio_complete, &eio);
+		res = coio_wait_done(req, &eio);
+		if (res < 0) {
+			pos = -1;
+			break;
+		} else {
+			left -= res;
+			pos += res;
+		}
+	}
+	return pos;
 }
 
 ssize_t
@@ -201,6 +221,11 @@ static void
 coio_do_write(eio_req *req)
 {
 	struct coio_file_task *eio = (struct coio_file_task *)req->data;
+
+	ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
+		eio->write.count = 1;
+	});
+
 	req->result = write(eio->write.fd, eio->write.buf, eio->write.count);
 	eio->errorno = errno;
 }
@@ -208,13 +233,28 @@ coio_do_write(eio_req *req)
 ssize_t
 coio_write(int fd, const void *buf, size_t count)
 {
-	INIT_COEIO_FILE(eio);
-	eio.write.buf = buf;
-	eio.write.count = count;
-	eio.write.fd = fd;
-	eio_req *req = eio_custom(coio_do_write, 0,
-				  coio_complete, &eio);
-	return coio_wait_done(req, &eio);
+	ssize_t left = count, pos = 0, res;
+	eio_req *req;
+
+	while (left > 0) {
+		INIT_COEIO_FILE(eio);
+
+		eio.write.buf	= buf + pos;
+		eio.write.count	= left;
+		eio.write.fd	= fd;
+
+		req = eio_custom(coio_do_write, EIO_PRI_DEFAULT,
+				 coio_complete, &eio);
+		res = coio_wait_done(req, &eio);
+		if (res < 0) {
+			pos = -1;
+			break;
+		} else {
+			left -= res;
+			pos += res;
+		}
+	}
+	return pos;
 }
 
 static void
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 7577ed11a..e95611c1e 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -140,6 +140,7 @@ struct errinj {
 	_(ERRINJ_RELAY_FASTER_THAN_TX, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
 	_(ERRINJ_TXN_COMMIT_ASYNC, ERRINJ_BOOL, {.bparam = false})\
+	_(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_BOOL, {.bparam = false}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/app/fio.result b/test/app/fio.result
index 783fa4fab..73fbd29e5 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -776,6 +776,12 @@ file5 = fio.pathjoin(tree, 'file.5')
 file6 = fio.pathjoin(tree, 'file.6')
 ---
 ...
+file7 = fio.pathjoin(tree, 'file.7')
+---
+...
+file8 = fio.pathjoin(tree, 'file.8')
+---
+...
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
 ---
 ...
@@ -865,6 +871,73 @@ fh6:close()
 ---
 - true
 ...
+-- test partial write/pwrite via one byte transfer
+fh7 = fio.open(file7, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+---
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', true)
+---
+- ok
+...
+fh7:write("one byte transfer, write")
+---
+- true
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', false)
+---
+- ok
+...
+fh7:seek(0)
+---
+- 0
+...
+fh7:read()
+---
+- one byte transfer, write
+...
+fh7:close()
+---
+- true
+...
+fh8 = fio.open(file8, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+---
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', true)
+---
+- ok
+...
+fh8:pwrite("one byte transfer, ", 0)
+---
+- true
+...
+fh8:seek(0)
+---
+- 0
+...
+fh8:read()
+---
+- 'one byte transfer, '
+...
+fh8:pwrite("pwrite", 19)
+---
+- true
+...
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', false)
+---
+- ok
+...
+fh8:seek(0)
+---
+- 0
+...
+fh8:read()
+---
+- one byte transfer, pwrite
+...
+fh8:close()
+---
+- true
+...
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 ---
 ...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 6825b882f..733e0f25f 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -251,6 +251,8 @@ file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
 file5 = fio.pathjoin(tree, 'file.5')
 file6 = fio.pathjoin(tree, 'file.6')
+file7 = fio.pathjoin(tree, 'file.7')
+file8 = fio.pathjoin(tree, 'file.8')
 
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0777', 8))
 fh1:write("gogo")
@@ -280,6 +282,25 @@ fh6:seek(0)
 fh6:read()
 fh6:close()
 
+-- test partial write/pwrite via one byte transfer
+fh7 = fio.open(file7, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', true)
+fh7:write("one byte transfer, write")
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', false)
+fh7:seek(0)
+fh7:read()
+fh7:close()
+fh8 = fio.open(file8, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, tonumber('0644', 8))
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', true)
+fh8:pwrite("one byte transfer, ", 0)
+fh8:seek(0)
+fh8:read()
+fh8:pwrite("pwrite", 19)
+errinj.set('ERRINJ_COIO_WRITE_CHUNK', false)
+fh8:seek(0)
+fh8:read()
+fh8:close()
+
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 res
 err:match("failed to copy") ~= nil
diff --git a/test/box/errinj.result b/test/box/errinj.result
index de877b708..6b4892500 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -47,6 +47,7 @@ evals
   - ERRINJ_BUILD_INDEX_DELAY: false
   - ERRINJ_CHECK_FORMAT_DELAY: false
   - ERRINJ_COIO_SENDFILE_CHUNK: -1
+  - ERRINJ_COIO_WRITE_CHUNK: false
   - ERRINJ_DYN_MODULE_COUNT: 0
   - ERRINJ_FIBER_MADVISE: false
   - ERRINJ_FIBER_MPROTECT: -1
-- 
2.20.1

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Alexander Turenko]

> issue https://github.com/tarantool/tarantool/issues/4651
> branch gorcunov/gh-4651-partial-write

> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> index e2345567c..e290214bc 100644
> --- a/src/lib/core/coio_file.c
> +++ b/src/lib/core/coio_file.c
> @@ -164,10 +164,30 @@ coio_file_close(int fd)
>  ssize_t
>  coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
>  {

In Febrary ([1]) we start discussing whether the loop should be in fio
or in coio. coio_p?write() returns amount of written bytes, so it seems
logical to keep it performing one write and move the loop to
src/lua/fio.c.

To be honest, I don't sure here. If you have a reason to keep the logic
here, please, explain it.

I'll CC Vlad, maybe he has more strong vision here.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014403.html

> @@ -201,6 +221,11 @@ static void
>  coio_do_write(eio_req *req)
>  {
>  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> +
> +	ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
> +		eio->write.count = 1;
> +	});

Why not set it right in coio_write() to don't spread the logic?

> diff --git a/test/app/fio.result b/test/app/fio.result
> index 783fa4fab..73fbd29e5 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -776,6 +776,12 @@ file5 = fio.pathjoin(tree, 'file.5')
>  file6 = fio.pathjoin(tree, 'file.6')
>  ---
>  ...
> +file7 = fio.pathjoin(tree, 'file.7')
> +---
> +...
> +file8 = fio.pathjoin(tree, 'file.8')
> +---
> +...

Nit: SOP now requires to add a test file with gh-... name when fixing a
bug.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Cyrill Gorcunov]

On Wed, May 06, 2020 at 08:08:18PM +0300, Alexander Turenko wrote:
> > issue https://github.com/tarantool/tarantool/issues/4651
> > branch gorcunov/gh-4651-partial-write
> 
> > diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> > index e2345567c..e290214bc 100644
> > --- a/src/lib/core/coio_file.c
> > +++ b/src/lib/core/coio_file.c
> > @@ -164,10 +164,30 @@ coio_file_close(int fd)
> >  ssize_t
> >  coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
> >  {
> 
> In Febrary ([1]) we start discussing whether the loop should be in fio
> or in coio. coio_p?write() returns amount of written bytes, so it seems
> logical to keep it performing one write and move the loop to
> src/lua/fio.c.
> 
> To be honest, I don't sure here. If you have a reason to keep the logic
> here, please, explain it.

Because lua interface should not dive into low-level coio_ engine, moreover
implementing it at this level we'are allowed to reuse coio_ helpers in
future. In turn if we implement this on fio level we can't reuse it anywhere
outside.

> I'll CC Vlad, maybe he has more strong vision here.
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014403.html
> 
> > @@ -201,6 +221,11 @@ static void
> >  coio_do_write(eio_req *req)
> >  {
> >  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> > +
> > +	ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
> > +		eio->write.count = 1;
> > +	});
> 
> Why not set it right in coio_write() to don't spread the logic?

Because the error should happen as close to native write call
as possible to simulate real behaviour.

So I rebased the patch on latest master and pushed it back.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Alexander Turenko]

Since I have no other objections (but still not sure about a level where
it should be fixed), let's consider the patch LGTM for me, if Vlad will
agree with the proposed way to fix it.

WBR, Alexander Turenko.

On Fri, May 15, 2020 at 12:00:44PM +0300, Cyrill Gorcunov wrote:
> On Wed, May 06, 2020 at 08:08:18PM +0300, Alexander Turenko wrote:
> > > issue https://github.com/tarantool/tarantool/issues/4651
> > > branch gorcunov/gh-4651-partial-write
> > 
> > > diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> > > index e2345567c..e290214bc 100644
> > > --- a/src/lib/core/coio_file.c
> > > +++ b/src/lib/core/coio_file.c
> > > @@ -164,10 +164,30 @@ coio_file_close(int fd)
> > >  ssize_t
> > >  coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
> > >  {
> > 
> > In Febrary ([1]) we start discussing whether the loop should be in fio
> > or in coio. coio_p?write() returns amount of written bytes, so it seems
> > logical to keep it performing one write and move the loop to
> > src/lua/fio.c.
> > 
> > To be honest, I don't sure here. If you have a reason to keep the logic
> > here, please, explain it.
> 
> Because lua interface should not dive into low-level coio_ engine, moreover
> implementing it at this level we'are allowed to reuse coio_ helpers in
> future. In turn if we implement this on fio level we can't reuse it anywhere
> outside.
> 
> > I'll CC Vlad, maybe he has more strong vision here.
> > 
> > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014403.html
> > 
> > > @@ -201,6 +221,11 @@ static void
> > >  coio_do_write(eio_req *req)
> > >  {
> > >  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> > > +
> > > +	ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
> > > +		eio->write.count = 1;
> > > +	});
> > 
> > Why not set it right in coio_write() to don't spread the logic?
> 
> Because the error should happen as close to native write call
> as possible to simulate real behaviour.
> 
> So I rebased the patch on latest master and pushed it back.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Vladislav Shpilevoy]

On 15/05/2020 11:00, Cyrill Gorcunov wrote:
> On Wed, May 06, 2020 at 08:08:18PM +0300, Alexander Turenko wrote:
>>> issue https://github.com/tarantool/tarantool/issues/4651
>>> branch gorcunov/gh-4651-partial-write
>>
>>> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
>>> index e2345567c..e290214bc 100644
>>> --- a/src/lib/core/coio_file.c
>>> +++ b/src/lib/core/coio_file.c
>>> @@ -164,10 +164,30 @@ coio_file_close(int fd)
>>>  ssize_t
>>>  coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
>>>  {
>>
>> In Febrary ([1]) we start discussing whether the loop should be in fio
>> or in coio. coio_p?write() returns amount of written bytes, so it seems
>> logical to keep it performing one write and move the loop to
>> src/lua/fio.c.
>>
>> To be honest, I don't sure here. If you have a reason to keep the logic
>> here, please, explain it.
> 
> Because lua interface should not dive into low-level coio_ engine, moreover
> implementing it at this level we'are allowed to reuse coio_ helpers in
> future.

Talking of lua diving into low-level things - our Lua C API is supposed to
work with low-level things, to wrap them, and provide easier to use Lua API.
So for flexibility purposes there are no any obstacles to move IO-retry
cycles into lua/ and box/lua where needed. To keep the low-level API low-level.

When you move retries into the lowest possible API, it becomes impossible
to let partial writes handling to other code. If such would appear in future.

Probably the best compromise would be to keep the old calls as is in coio,
but add new methods like coio_write_all(), coio_read_all(), etc. Which would
retry, and would be used by Lua C API.

I am ok with any option, including the current patch. I just don't agree, that
it is the only true way.

> In turn if we implement this on fio level we can't reuse it anywhere
> outside.

On the other hand, when partial writes don't exist, we can't get them
when need.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 4 comments below.

On 21/04/2020 23:39, Cyrill Gorcunov wrote:
> Writting less bytes than requested is perfectly fine.

1. 'Writting' -> 'Writing'.

> In turn our fio.write/pwrite api simply returns 'true'
> even if only some part of a buffer has been written.
> 
> Thus make coio_write and coio_pwrite to write the
> whole data in a cycle. Note in most situations there
> will be only one pass, partial writes are really the
> rare cases.
> 
> Fixes #4651
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> issue https://github.com/tarantool/tarantool/issues/4651
> branch gorcunov/gh-4651-partial-write
> 
>  src/lib/core/coio_file.c | 62 ++++++++++++++++++++++++++++------
>  src/lib/core/errinj.h    |  1 +
>  test/app/fio.result      | 73 ++++++++++++++++++++++++++++++++++++++++
>  test/app/fio.test.lua    | 21 ++++++++++++
>  test/box/errinj.result   |  1 +
>  5 files changed, 147 insertions(+), 11 deletions(-)
> 
> diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> index e2345567c..e290214bc 100644
> --- a/src/lib/core/coio_file.c
> +++ b/src/lib/core/coio_file.c
> @@ -164,10 +164,30 @@ coio_file_close(int fd)
>  ssize_t
>  coio_pwrite(int fd, const void *buf, size_t count, off_t offset)
>  {
> -	INIT_COEIO_FILE(eio);
> -	eio_req *req = eio_write(fd, (void *) buf, count, offset,
> -				 0, coio_complete, &eio);
> -	return coio_wait_done(req, &eio);
> +	ssize_t left = count, pos = 0, res, chunk;
> +	eio_req *req;
> +
> +	while (left > 0) {
> +		INIT_COEIO_FILE(eio);
> +		chunk = left;
> +
> +		ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
> +			chunk = 1;
> +		});
> +
> +		req = eio_write(fd, (void *)buf + pos, chunk,

2. As it was fairly noticed by Timur in one of my
patches for lsregion - lets better avoid 'void' pointer
arithmetic. And use uint8_t * or char *.

> +				offset + pos, EIO_PRI_DEFAULT,
> +				coio_complete, &eio);
> +		res = coio_wait_done(req, &eio);
> +		if (res < 0) {
> +			pos = -1;

3. What if eio_write() returns EAGAIN/EINTR/WOULDBLOCK? Can it
happen at all? Can it happen, if the descriptor is not
blocking? Can it happen if the underlying FS is a network
file system, like sshfs via FUSE?

Coio socket library handles these errors and retries them.

> +			break;
> +		} else {
> +			left -= res;
> +			pos += res;
> +		}
> +	}
> +	return pos;
>  }
> diff --git a/test/app/fio.result b/test/app/fio.result
> index 783fa4fab..73fbd29e5 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -865,6 +871,73 @@ fh6:close()
>  ---
>  - true
>  ...
> +-- test partial write/pwrite via one byte transfer

4. Please

- Add the issue reference, like this:

	--
	-- gh-4651: ...
	--

- Start sentences from a capital letter, and finish with dot.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Cyrill Gorcunov]

On Wed, Jun 10, 2020 at 12:55:04AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 4 comments below.
> 
> On 21/04/2020 23:39, Cyrill Gorcunov wrote:
> > Writting less bytes than requested is perfectly fine.
> 
> 1. 'Writting' -> 'Writing'.

Yup, thanks!

> > +
> > +	while (left > 0) {
> > +		INIT_COEIO_FILE(eio);
> > +		chunk = left;
> > +
> > +		ERROR_INJECT(ERRINJ_COIO_WRITE_CHUNK, {
> > +			chunk = 1;
> > +		});
> > +
> > +		req = eio_write(fd, (void *)buf + pos, chunk,
> 
> 2. As it was fairly noticed by Timur in one of my
> patches for lsregion - lets better avoid 'void' pointer
> arithmetic. And use uint8_t * or char *.

OK (though the compilers we work with support this kind of arithmetics
pretty well, moreover we already depend heavilty on gcc/llvm extensions,
but no problem, will update).

> > +				offset + pos, EIO_PRI_DEFAULT,
> > +				coio_complete, &eio);
> > +		res = coio_wait_done(req, &eio);
> > +		if (res < 0) {
> > +			pos = -1;
> 
> 3. What if eio_write() returns EAGAIN/EINTR/WOULDBLOCK? Can it
> happen at all? Can it happen, if the descriptor is not
> blocking? Can it happen if the underlying FS is a network
> file system, like sshfs via FUSE?
> 
> Coio socket library handles these errors and retries them.

This patch doesn't address retries. Strictly speaking we should
(for example coio_preadn does). But I think it is separate issue.
I'll file a bug.

> 
> 4. Please
> 
> - Add the issue reference, like this:
> 
> 	--
> 	-- gh-4651: ...
> 	--
> 
> - Start sentences from a capital letter, and finish with dot.

Sure, thanks!

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Vladislav Shpilevoy]

Thanks for the fixes! Seems like you didn't push them.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Cyrill Gorcunov]

On Thu, Jun 11, 2020 at 09:36:08PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes! Seems like you didn't push them.

Hmm... I pushed into gorcunov/gh-4651-partial-write-2 branch.
https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017606.html

If you mean to merge into master branch -- I don't have such rights ;)

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Vladislav Shpilevoy]

On 11/06/2020 21:43, Cyrill Gorcunov wrote:
> On Thu, Jun 11, 2020 at 09:36:08PM +0200, Vladislav Shpilevoy wrote:
>> Thanks for the fixes! Seems like you didn't push them.
> 
> Hmm... I pushed into gorcunov/gh-4651-partial-write-2 branch.
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017606.html
> 
> If you mean to merge into master branch -- I don't have such rights ;)

Aha, so you changed the branch, but didn't say it anywhere. I was
looking at the branch you specified in the cover letter. Just a note -
better do a force push on the same branch. Having all these v1, v2, v15,
does not simplify the review. Even when you mention branch change in
one of the emails, still during the active review process with lots of
emails this one could be lost, and people would review the old branch
accidentally.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Cyrill Gorcunov]

On Thu, Jun 11, 2020 at 09:56:31PM +0200, Vladislav Shpilevoy wrote:
> > If you mean to merge into master branch -- I don't have such rights ;)
> 
> Aha, so you changed the branch, but didn't say it anywhere. I was
> looking at the branch you specified in the cover letter. Just a note -
> better do a force push on the same branch. Having all these v1, v2, v15,
> does not simplify the review. Even when you mention branch change in
> one of the emails, still during the active review process with lots of
> emails this one could be lost, and people would review the old branch
> accidentally.

Sorry for confusing you. I did it on a purpose -- I never do a force push
until everything is settle down, mostly for history reason and to keep
old changes and being able to make a diff between branches. Also I
sent a new version as a separate email.

Still I'll mark for myself that you prefer a force push instead and
will try to follow this rule next time. Thanks for review! And sorry
again for confusion.

Re: [Tarantool-patches] [PATCH] fio/coio: handle partial writes

[Vladislav Shpilevoy]

LGTM.

But the branch name is gorcunov/gh-4651-partial-write-2.