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

[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

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.

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

[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