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

[Tarantool-patches] [PATCH v2] 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>
---
branch gorcunov/gh-4651-partial-write-2
v2:
 - Use write_should_retry helper to not duplicate code
 - Return -1 on any other error, so that fio.write() return
   false

 src/lib/core/coio_file.c | 80 ++++++++++++++++++++++++++++++++++------
 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, 166 insertions(+), 11 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 62388344e..af5fb30ce 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -161,13 +161,49 @@ coio_file_close(int fd)
 	return coio_wait_done(req, &eio);
 }
 
+static inline int
+write_should_retry(void)
+{
+	/* Retry on O_NONBLOCK */
+	if (errno == EAGAIN)
+		return true;
+	/* Retry if been interrupted */
+	if (errno == EINTR)
+		return true;
+	/* Unexpected error */
+	return false;
+}
+
 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 (!write_should_retry()) {
+				pos = -1;
+				break;
+			}
+		} else {
+			left -= res;
+			pos += res;
+		}
+	}
+	return pos;
 }
 
 ssize_t
@@ -200,7 +236,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 +249,30 @@ 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 (!write_should_retry()) {
+				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 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 v2] fio/coio: Handle partial writes

[Cyrill Gorcunov]

On Wed, Dec 04, 2019 at 02:17:49PM +0300, Cyrill Gorcunov wrote:
...
> +static inline int
> +write_should_retry(void)
> +{
> +	/* Retry on O_NONBLOCK */
> +	if (errno == EAGAIN)
> +		return true;
> +	/* Retry if been interrupted */
> +	if (errno == EINTR)
> +		return true;
> +	/* Unexpected error */
> +	return false;
> +}

Guys, I just thought, previously if the file we're writting to
has been opened with o_nonblock and eagain happened we were
returning an error. With this patch we gonna retry write operation,
and this changes behaviour. Maybe I should consider EINTR only?

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

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/12/04 14:21]:
> +static inline int
> +write_should_retry(void)
> +{
> +	/* Retry on O_NONBLOCK */
> +	if (errno == EAGAIN)
> +		return true;

Usually you add EWOULDBLOCK here too.

> +	/* Retry if been interrupted */
> +	if (errno == EINTR)
> +		return true;

Not sure you should handle this. It's up to the client whether to
make or not a write interruptible, it is managed by fcntl. 
Otherwise you may block indefinitely inside a syscall.



-- 
Konstantin Osipov, Moscow, Russia

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

[Cyrill Gorcunov]

On Wed, Dec 04, 2019 at 02:34:47PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/12/04 14:21]:
> > +static inline int
> > +write_should_retry(void)
> > +{
> > +	/* Retry on O_NONBLOCK */
> > +	if (errno == EAGAIN)
> > +		return true;
> 
> Usually you add EWOULDBLOCK here too.

True. It somehow escaped from the drafts while I've been
merging the series of fixes. Thanks!

> > +	/* Retry if been interrupted */
> > +	if (errno == EINTR)
> > +		return true;
> 
> Not sure you should handle this. It's up to the client whether to
> make or not a write interruptible, it is managed by fcntl. 
> Otherwise you may block indefinitely inside a syscall.

OK, thanks, will drop.

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

[Cyrill Gorcunov]

Writting less bytes than requested is 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.

Fixes #4651

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
branch gorcunov/gh-4651-partial-write-3

v3:
 - Just count bytes written returning any error happened

 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..fb704f938 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) {
+			pos = -1;
+			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) {
+			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 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 v3] fio/coio: Handle partial writes

[Alexander Turenko]

I have three comments and all are up to you.

Please, rebase the patch on top of the current master (errinj.result was
changed).

WBR, Alexander Turenko.

On Wed, Dec 04, 2019 at 05:35:11PM +0300, Cyrill Gorcunov wrote:
> Writting less bytes than requested is 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.

I would expect that coio_*() functions will have the same behaviour as
its libc counterparts except that they'll yield a current fiber instead
of blocking a current thread.

So spin in a loop looks more as fio.c work for me, however I would not
say that I have a strict vision here.

If you'll decide to keep this logic in coio, at least left comments in
the header file for those functions.

> 
> Fixes #4651
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> branch gorcunov/gh-4651-partial-write-3

> +	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
> <...>
> +
> +		if (inj != NULL && inj->iparam > 0)
> +			chunk = (ssize_t)inj->iparam;
> +		else
> +			chunk = left;
> +

AFAIR, we have macros for error injections, which allow to avoid
producing any extra machine code for a release build. It seems they can
be used here. Can you elaborate, please?

Nit: We usually split a cast operator from its argument with a
whitespace, e.g. `(ssize_t) inj->iparam`.

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

(If you'll keep the loop in coio.) I would do it in coio_write() itself:
this way the logic in coio_pwrite() and coio_write() will be quite
similar and so easier to verify if I don't miss something.

Just wonder: is there any reason to use eio_custom() for write? pwrite
uses eio_write(), which support both write and pwrite (it checks whether
the offset >= 0.

I tracked it down to:

commit b9616280533a6bba1bf4739ce11a57461262560f
Author: Dmitry E. Oboukhov <unera@debian.org>
Date:   Mon Aug 11 20:03:30 2014 +0400

    fiber-io tests.

And it is the whole description.

Anyway, I just curious.

>  	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) {
> +			pos = -1;
> +			break;
> +		} else {
> +			left -= res;
> +			pos += res;
> +		}
> +	}
> +	return pos;
>  }

> 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

Now it should be rebased on top of master, this hunk was changed.

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

[Cyrill Gorcunov]

On Fri, Feb 21, 2020 at 01:10:26AM +0300, Alexander Turenko wrote:
> I have three comments and all are up to you.
> 
> Please, rebase the patch on top of the current master (errinj.result was
> changed).

OK, will do.

> On Wed, Dec 04, 2019 at 05:35:11PM +0300, Cyrill Gorcunov wrote:
> > Writting less bytes than requested is 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.
> 
> I would expect that coio_*() functions will have the same behaviour as
> its libc counterparts except that they'll yield a current fiber instead
> of blocking a current thread.

They do yeild current fiber until completion notification arrives.
coio_wait_done does exactly that.

> 
> So spin in a loop looks more as fio.c work for me, however I would not
> say that I have a strict vision here.
> 
> If you'll decide to keep this logic in coio, at least left comments in
> the header file for those functions.

I'm not sure I follow you here. Our all coio_ helpers do the same thing:
they yield a current fiber and wait for completion.

> > 
> > Fixes #4651
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> > branch gorcunov/gh-4651-partial-write-3
> 
> > +	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
> > <...>
> > +
> > +		if (inj != NULL && inj->iparam > 0)
> > +			chunk = (ssize_t)inj->iparam;
> > +		else
> > +			chunk = left;
> > +
> 
> AFAIR, we have macros for error injections, which allow to avoid
> producing any extra machine code for a release build. It seems they can
> be used here. Can you elaborate, please?

I think I didn't use them to follow current inj code used
in this file, for unification sake. But indeed maybe macors
will look better. Will take a look.

> 
> Nit: We usually split a cast operator from its argument with a
> whitespace, e.g. `(ssize_t) inj->iparam`.

No we don't, there are number of examples where we put cast
right before operand and I think it is a way more correct
because code reader should consider such cast as signle entry
together with operator itself. So I prefer this style.

> >  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;
> > +
> 
> (If you'll keep the loop in coio.) I would do it in coio_write() itself:
> this way the logic in coio_pwrite() and coio_write() will be quite
> similar and so easier to verify if I don't miss something.

I've no objection here, can move into coio_write.

> 
> Just wonder: is there any reason to use eio_custom() for write? pwrite
> uses eio_write(), which support both write and pwrite (it checks whether
> the offset >= 0.

Sounds reasonable. Letme recheck everything again and resend the patch.

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

[Alexander Turenko]

> > On Wed, Dec 04, 2019 at 05:35:11PM +0300, Cyrill Gorcunov wrote:
> > > Writting less bytes than requested is 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.
> > 
> > I would expect that coio_*() functions will have the same behaviour as
> > its libc counterparts except that they'll yield a current fiber instead
> > of blocking a current thread.
> 
> They do yeild current fiber until completion notification arrives.
> coio_wait_done does exactly that.

I meant handling of a partial write: whether a function should
block/yield or report amount of written bytes to a caller.

So, my points here are the following:

* coio_write() should reflect write() in handling partial writes;
* fio lua module should handle partial writes on its own.

There is one interesting thing I found:

We have src/lib/core/fio.[ch], which handles partial writes, but
src/lua/fio.[ch] (Lua module) does not use it. It looks unusual.

> 
> > > 
> > > Fixes #4651
> > > 
> > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > > ---
> > > branch gorcunov/gh-4651-partial-write-3
> > 
> > > +	struct errinj *inj = errinj(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_INT);
> > > <...>
> > > +
> > > +		if (inj != NULL && inj->iparam > 0)
> > > +			chunk = (ssize_t)inj->iparam;
> > > +		else
> > > +			chunk = left;
> > > +
> > 
> > AFAIR, we have macros for error injections, which allow to avoid
> > producing any extra machine code for a release build. It seems they can
> > be used here. Can you elaborate, please?
> 
> I think I didn't use them to follow current inj code used
> in this file, for unification sake. But indeed maybe macors
> will look better. Will take a look.
> 
> > 
> > Nit: We usually split a cast operator from its argument with a
> > whitespace, e.g. `(ssize_t) inj->iparam`.
> 
> No we don't, there are number of examples where we put cast
> right before operand and I think it is a way more correct
> because code reader should consider such cast as signle entry
> together with operator itself. So I prefer this style.

You're right.

1. `man 1 indent` shows -ncs (--no-space-after-casts) in the Linux style
   description.
2. It is also copy-pasted to [1] without -npsl
   (--dont-break-procedure-type).
3. Vladimir D. uses it in this way in vinyl code.
4. The type cast operation has the same priority as other unary
   operations.

Don't know why I was sure that a space should be here.

[1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure-guidelines---checklist

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

[Cyrill Gorcunov]

Writing less bytes than requested is perfectly fine. In turn out
that 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.

Note that we're not handling nonblocking writes here (which
could return EAGAIN) simply because we need an other api
which would accept timeouts.

Fixes #4651

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
v2 by Vlad:
 - drop void* arithmetics
 - test comment style fixups
 - fix typo in changelog

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

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

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index e2345567c..0f21380c5 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, (char *)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	= (char *)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 79073f529..261ab22a8 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -145,6 +145,7 @@ struct errinj {
 	_(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL, {.bparam = false})\
 	_(ERRINJ_VY_RUN_OPEN, ERRINJ_INT, {.iparam = -1})\
 	_(ERRINJ_AUTO_UPGRADE, 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..8a08755a2 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,75 @@ fh6:close()
 ---
 - true
 ...
+--
+-- gh-4651: 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..92ba3ff20 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,27 @@ fh6:seek(0)
 fh6:read()
 fh6:close()
 
+--
+-- gh-4651: 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 a066bc4bc..1166f2b30 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -48,6 +48,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.26.2