TDB-184 : Broken fsync error handling by george-lorch · Pull Request #410 · percona/PerconaFT

george-lorch · 2018-04-11T16:56:01Z

Factored out i/o error reporting to be common and try to obtain actual
filename from descriptor.
Fixed ENOSPC reporting to use common i/o reporting.
Fixed fsync retry logic to retry EINTR, EIO, and ENOLCK based on InnoDB code
and upstream mysql issues 34823 and 90296. Retry will print message every 100
attempts in a loop.
Touched up 'slow fsync' tracking and reporting to use new i/o error reporting.
Fixed close retry logic to be same as fsync.

george-lorch · 2018-04-11T16:57:17Z

https://jenkins.percona.com/view/TokuDB/job/PerconaFT-param/122/
https://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/2147/
https://jenkins.percona.com/view/5.7/job/mysql-5.7-param/1766/
https://jenkins.percona.com/view/5.7/job/mysql-5.7-param/1771/

The second 5.7 jenkins is a matrix reload on 4 failures due to some odd git state weirdness, they failed to clone/checkout my branch.

laurynas-biveinis · 2018-04-11T16:58:21Z

portability/file.cc

 }

+
+void report_io_problem(int fd, const char *fmt, ...) __attribute__((format(printf, 2, 3)));


Cannot attach attribute to definition to save a declaration (in front?)

Ohh, good catch, I forgot attribute could be used like that.

laurynas-biveinis · 2018-04-11T16:58:46Z

portability/file.cc

+    va_list ap;
+    va_start(ap, fmt);
+
+    const int tstr_length = 26;


static constexpr

laurynas-biveinis · 2018-04-11T17:00:13Z

portability/file.cc

+            buf);
+#endif
+    fflush(stderr);
+}


laurynas-biveinis · 2018-04-11T17:01:08Z

portability/file.cc

+                report_io_problem(
+                    fd, "Failed write of [%" PRIu64 "] bytes.", (uint64_t)len);
+                int out_of_disk_space = 1;
+                assert(!out_of_disk_space);  // Give an error message that might


Huh. bool at least?

This was original FT code and I saw no reason to change it, funky as it is.

oh, and static constexpr yada yada if you decide to touch it

laurynas-biveinis · 2018-04-11T17:02:11Z

portability/file.cc

+            report_io_problem(fd,
+                              "Write of [%" PRIu64
+                              "] bytes interrupted.  Retrying.",
+                              (uint64_t)len);


C++ style cast

similar, slightly modified FT code, but yeah

laurynas-biveinis · 2018-04-11T17:02:20Z

portability/file.cc

+        case ENOSPC: {
+            if (toku_assert_on_write_enospc) {
+                report_io_problem(
+                    fd, "Failed write of [%" PRIu64 "] bytes.", (uint64_t)len);


C++ style cast

laurynas-biveinis · 2018-04-11T17:03:13Z

portability/file.cc

+                                             // be useful if this is the only one
+                                             // that survives.
+            } else {
+                toku_sync_fetch_and_add(&toku_write_enospc_total, 1);


Would it be too much work to convert to std::atomic?

probably not as these are pretty local to file.cc

laurynas-biveinis · 2018-04-11T17:04:49Z

portability/file.cc

+                // fallthrough
+                case EINTR:
+                // fallthrough
+                case EIO:


But the issue being fixed here is that EIO must fail at once, exactly the opposite of EINTR/ENOLCK

That is not what I came to understand about EIO, which could be incorrect. https://stackoverflow.com/questions/42434872/writing-programs-to-cope-with-i-o-errors-causing-lost-writes-on-linux

Otherwise, what is the point of this exercise at all where FT would generally assert on all errors that were not EINTR or ENOSPC.

That's what I was referring to with my comment on the TDB issue: "I didn't realise that was a release-build assert too - in which case the bug is not nearly as serious"

Actually, yeah, the discussion meanders around from retry indefinitely, to retry exactly once, to fail immediately as the write is lost forever. So yeah, now I think the original suggestion reported in 90296 is correct, abort immediately as a write is lost.

laurynas-biveinis · 2018-04-11T17:05:55Z

portability/file.cc

-    }
+
+        if (r == -1) {
+            int what_errno = get_error_errno();


Mostly duplicated code with the above

- Factored out i/o error reporting to be common and try to obtain actual filename from descriptor. - Factored out write, close and fsync error handling into a common function that handles special cases sch as ENOLCK, EINTR, ENOSPC, and EIO. See upstream MySQL issues 34823 and 90296 for details on ENOLCK and EIO. - Fixed write, close, and fsync retry logic to use new common handler. - Touched up 'slow fsync' tracking and reporting to use new i/o error reporting. - Converted metric trackers to std::atomic where appropriate. - Various bits of code reformatting and conversions to std::atomic and constexpr, #include style fixes.

george-lorch · 2018-04-11T21:16:05Z

OK, I reworked it a bit more and factored out the write error handing as well as it wasn't really doing what the function names might indicate. Here is a new set of jenkins runs. Please take another look and approach it as if it were a new review and not just the items commented.

https://jenkins.percona.com/view/TokuDB/job/PerconaFT-param/123/
https://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/2151/
https://jenkins.percona.com/view/5.7/job/mysql-5.7-param/1772/

laurynas-biveinis

Looks much better now!

laurynas-biveinis · 2018-04-12T08:11:40Z

portability/file.cc

+    va_list ap;
+    va_start(ap, fmt);
+
+    constexpr int tstr_length = 26;


static constexpr (but only if there are other comments to address)

george-lorch requested a review from laurynas-biveinis April 11, 2018 16:56

laurynas-biveinis suggested changes Apr 11, 2018

View reviewed changes

george-lorch force-pushed the TDB-184 branch from 4d7237c to 5287c28 Compare April 11, 2018 19:34

laurynas-biveinis approved these changes Apr 12, 2018

View reviewed changes

		}


		void report_io_problem(int fd, const char *fmt, ...) __attribute__((format(printf, 2, 3)));

Conversation

george-lorch commented Apr 11, 2018

Uh oh!

george-lorch commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurynas-biveinis Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

george-lorch commented Apr 11, 2018

Uh oh!

laurynas-biveinis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

laurynas-biveinis Apr 11, 2018 •

edited

Loading