Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-14115

MS: Fix the aio read/write error caused by wrong useage of lpNumberOfBytesWritten

Details

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

      Attachments

        Activity

          Ascending order - Click to sort in descending order
          wlad Vladislav Vaintroub added a comment - - edited

          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()
          wlad Vladislav Vaintroub added a comment - - edited 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()
          wlad Vladislav Vaintroub added a comment - - edited

          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

          wlad Vladislav Vaintroub added a comment - - edited 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
          junsu Jun Su added a comment -

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

          junsu Jun Su added a comment - Thanks. this is actual a bug we noticed in our stress.

          People

            wlad Vladislav Vaintroub
            svoj Sergey Vojtovich
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Git Integration

                Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.