[MDEV-14115] MS: Fix the aio read/write error caused by wrong useage of lpNumberOfBytesWritten Created: 2017-10-24  Updated: 2018-10-03  Resolved: 2017-10-28

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Affects Version/s: None
Fix Version/s: 10.2.10, 10.3.3

Type: Bug Priority: Major
Reporter: Sergey Vojtovich Assignee: Vladislav Vaintroub
Resolution: Fixed Votes: 0
Labels: contribution, foundation

Sprint: 10.3.3-1

 Description    

From 9fdb2929e289210bdc801aa714d72aff27ae20de Mon Sep 17 00:00:00 2001
 
From: Suna Liu <sunl@microsoft.com>
 
Date: Sat, 30 Sep 2017 02:11:16 +0000
 
Subject: [PATCH] Merged PR 65296: Fix the aio read/write error caused by wrong
 
 useage of lpNumberOfBytesWritten
 
 
 
Fix the aio read/write error caused by wrong useage of lpNumberOfBytesWritten
 
 
 
Related work items: #81926
 
---
 
 storage/innobase/os/os0file.cc | 11 ++++++++---
 
 1 file changed, 8 insertions(+), 3 deletions(-)
 
 
 
diff --git a/storage/innobase/os/os0file.cc b/storage/innobase/os/os0file.cc
 
index 8113a34998a..126462f3c91 100644
 
--- a/storage/innobase/os/os0file.cc
 
+++ b/storage/innobase/os/os0file.cc
 
@@ -7505,7 +7505,7 @@ try_again:
 
 #ifdef WIN_ASYNC_IO
 
                        ret = ReadFile(
 
                                file.m_file, slot->ptr, slot->len,
 
-                               &slot->n_bytes, &slot->control);
 
+                               NULL, &slot->control);
 
 #elif defined(LINUX_NATIVE_AIO)
 
                        if (!array->linux_dispatch(slot)) {
 
                                goto err_exit;
 
@@ -7523,7 +7523,7 @@ try_again:
 
 #ifdef WIN_ASYNC_IO
 
                        ret = WriteFile(
 
                                file.m_file, slot->ptr, slot->len,
 
-                               &slot->n_bytes, &slot->control);
 
+                               NULL, &slot->control);
 
 #elif defined(LINUX_NATIVE_AIO)
 
                        if (!array->linux_dispatch(slot)) {
 
                                goto err_exit;
 
@@ -7540,7 +7540,9 @@ try_again:
 
 
 
 #ifdef WIN_ASYNC_IO
 
        if (srv_use_native_aio) {
 
-               if ((ret && slot->len == slot->n_bytes)
 
+               if ((ret
 
+                     && GetOverlappedResult(file.m_file, &slot->control, &slot->n_bytes, TRUE)
 
+                     && slot->len == slot->n_bytes)
 
                     || (!ret && GetLastError() == ERROR_IO_PENDING)) {
 
                        /* aio was queued successfully! */
 
 
 
@@ -7591,6 +7593,9 @@ err_exit:
 
                << ", bytes=" << n
 
                << ", read_only=" << read_only
 
                << " - slot_len=" << slot->len << ", slot_nbytes=" << slot->n_bytes
 
+#ifdef WIN_ASYNC_IO
 
+               << ", slot_control_InternalHigh=" << slot->control.InternalHigh
 
+#endif
 
                << " - ret=" << ret << ", last_err=" << GetLastError();
 
 
 
        array->release_with_mutex(slot);



 Comments   
Comment by Vladislav Vaintroub [ 2017-10-24 ]

It would be nice to get an extended comment about what is done and why the patch was necessary.

  • MSDN tells me that it is better not to rely on WriteFile() or ReadFile() lpBytesWrittes or lpBytesRead, if asynchronous IO is used. It says it is better to use GetOverlappedResult(). Does this MSDN remark also apply to asynchronous IO that completes immediately, like in the patch?. Under which circumstances the lpBytesRead/Written can be wrong?
  • Why are you using GetOverlappedResult with last parameter (bWait) set to TRUE? when ReadFile/WriteFile return TRUE, and this means that overlapped IO is completed already, without delay. Can bWait be FALSE instead, as we are not waiting to queued IO here?
  • This patch is against which codebase? We have a several asynchronous ReadFile and WriteFile in our codebase in our codebase's os0file.cc, and so far none of those uses GetOverlappedResult() for async IO that completed immediately. Should this logic applied everywhere then? The hunk under err_exit in the patch does not correspond to our os0file.cc , we just have a call to os_file_handle_error()
Comment by Vladislav Vaintroub [ 2017-10-28 ]

Ok, I think I got the idea . I think the it could be much easier solved

i.e this 

  if ((ret && slot->len == slot->n_bytes)
 
	|| (!ret && GetLastError() == ERROR_IO_PENDING)) {

can just be changed to this

 if (ret || (GetLastError() == ERROR_IO_PENDING))

since the code in this function only means to submit io, and the checks for full/partial writes are done later, after GetQueuedCompletionStatus().

Thanks for pointing out the potential bug.I changed our ReadFile()/WriteFile() calls inside Innodb to not use output length parameters, on this, and a more places here https://github.com/MariaDB/server/commit/97df230aed24dc9ba1990c955c083c2d53d1f723

Comment by Jun Su [ 2017-11-13 ]

Thanks. this is actual a bug we noticed in our stress.

Generated at Thu Feb 08 08:11:02 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.