Merge lp:~vlad-lesin/percona-server/5.5-bug801989 into lp:percona-server/5.5

[Vlad Lesin]

The https://bugs.launchpad.net/percona-server/+bug/801989 bugfix.

The main idea of this fix is to return "true" on success and "false" otherwise from os_file_set_nocache. That allows to process O_DIRECT flag setting failures. Except that all calls of os_file_set_nocache are wrapped with code which abort execution on failure.

I haven't found the way to test this bug with MTR. The only idea is to use LD_PRELOAD and the library which just returns "false" on "fcntl" and "open" with O_DIRECT. But I'm not sure if such test should be MTR test. It's supposed MTR tests are platform-independed but we need at least to build library and use platform-specific environment for testing.

Jenkins tests: http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/489/

[Alexey Kopytov (akopytov)]

Vlad, some comments on the patch:

  - I don't think we should crash the server after failing to set
    O_DIRECT. This way you are essentially resurrecting
    http://bugs.mysql.com/bug.php?id=26662

    The warning is relatively harmless, the InnoDB will just do buffered
    I/O on file systems that do not support O_DIRECT such as tmpfs.

    It only becomes a problem in XtraDB in case of ALL_O_DIRECT, because
    in this case XtraDB does not flush the log files, even if O_DIRECT
    fails (the correctness of not doing fsync() when O_DIRECT succeeds
    is a separate issue).

    So what we should do is to disable ALL_O_DIRECT (i.e. O_DIRECT for
    log files) and switch to O_DIRECT (i.e. O_DIRECT on data files only)
    if setting O_DIRECT on a log file fails. That is, essentially use
    the fix suggested by Yasufumi, but perhaps a cleaner variant,
    i.e. in os_file_create_func() check the value returned by
    os_file_set_nocache() introduced by your patch, and if it failed, do
    srv_unix_file_flush_method = SRV_UNIX_O_DIRECT). This way we will at
    least do fsync() on log files when setting O_DIRECT failed.

  - the bug also affects 5.1. I have updated the bug accordingly

  - after fixing the above, please put description to revision comments
    in addition to MP. Note that it's ok to reference bug numbers by
    their number (e.g. "bug #...") rather than by the full URL. It's
    unambiguous, but shorter

  - to answer your concerns about the test case, I don't think we should
    have a regression test for this fix.

review: Needs Fixing

[Stewart Smith (stewart)]

Alexey Kopytov <email address hidden> writes:
> - after fixing the above, please put description to revision comments
> in addition to MP. Note that it's ok to reference bug numbers by
> their number (e.g. "bug #...") rather than by the full URL. It's
> unambiguous, but shorter

If the notation lp:1234 is used, then Launchpad will automatically link
to the correct bug number (lp and bzr use bug-tracker:number format)
--
Stewart Smith

[Alexey Kopytov (akopytov)]

On Mon, 10 Sep 2012 15:56:05 +1000, Stewart Smith wrote:
> Alexey Kopytov <email address hidden> writes:
>> - after fixing the above, please put description to revision
>> comments in addition to MP. Note that it's ok to reference bug
>> numbers by their number (e.g. "bug #...") rather than by the full
>> URL. It's unambiguous, but shorter
>
> If the notation lp:1234 is used, then Launchpad will automatically
> link to the correct bug number (lp and bzr use bug-tracker:number
> format)
>

Same goes for "bug #1234", "bug 1234" and "bug1234".

[Vlad Lesin (vlad-lesin)]

> So what we should do is to disable ALL_O_DIRECT (i.e. O_DIRECT for
> log files) and switch to O_DIRECT (i.e. O_DIRECT on data files only)
> if setting O_DIRECT on a log file fails. That is, essentially use
> the fix suggested by Yasufumi, but perhaps a cleaner variant,
> i.e. in os_file_create_func() check the value returned by
> os_file_set_nocache() introduced by your patch, and if it failed, do
> srv_unix_file_flush_method = SRV_UNIX_O_DIRECT). This way we will at
> least do fsync() on log files when setting O_DIRECT failed.
Done.
> - the bug also affects 5.1. I have updated the bug accordingly
It will be separate MP.

> - after fixing the above, please put description to revision comments
> in addition to MP. Note that it's ok to reference bug numbers by
> their number (e.g. "bug #...") rather than by the full URL. It's
> unambiguous, but shorter
Done.

[Alexey Kopytov (akopytov)]

On 12.09.12 1:30, Vladislav Lesin wrote:
>> - the bug also affects 5.1. I have updated the bug accordingly
> It will be separate MP.
>

Well, in that case this MP must be a merge revision from a 5.1-based
tree. This is the way we implement fixes in both branches: the revision
is first committed to the lowest version branch and then merged upwards.

[Alexey Kopytov (akopytov)]

Also the change around the "type != OS_LOG_FILE && srv_unix_file_flush_method == SRV_UNIX_O_DIRECT" case doesn't really change anything in behavior. Do we really need to touch the code there just to add a comment and an unnecessary typecast?

review: Needs Fixing

[Vlad Lesin (vlad-lesin)]

> Well, in that case this MP must be a merge revision from a 5.1-based
> tree. This is the way we implement fixes in both branches: the revision
> is first committed to the lowest version branch and then merged upwards.
The MP for 5.1 is done.

[Vlad Lesin (vlad-lesin)]

> Also the change around the "type != OS_LOG_FILE && srv_unix_file_flush_method
> == SRV_UNIX_O_DIRECT" case doesn't really change anything in behavior. Do we
> really need to touch the code there just to add a comment and an unnecessary
> typecast?
The comment and typecast are removed.

[Alexey Kopytov (akopytov)]

Vlad,

On 13.09.12 11:45, Vladislav Lesin wrote:
>> Well, in that case this MP must be a merge revision from a 5.1-based
>> tree. This is the way we implement fixes in both branches: the revision
>> is first committed to the lowest version branch and then merged upwards.
> The MP for 5.1 is done.
>

You misunderstood me. You should not commit a separate revision for 5.5,
you should merge your revision from the 5.1 branch. Since our trees are
currently not merged, you can use a GCA 5.1 clone to commit your 5.1
revision, then you can merge it to 5.5.

Unmerged revisions

297. By Vlad Lesin on 2012-09-13

The bug #801989 fix.

The main idea of this fix is to return "true" on success and "false" otherwise
from os_file_set_nocache(). That allows to process O_DIRECT flag setting failures.

If setting O_DIRECT failes on log files the srv_unix_file_flush_method is set
to SRV_UNIX_O_DIRECT value to do fsync() on log files. If O_DIRECT fails on
other files the error is ignored to do buffered I/O on filesystems that do not
support direct I/O(for example tmpfs).

=== modified file 'Percona-Server/storage/innobase/include/os0file.h'
--- Percona-Server/storage/innobase/include/os0file.h	2012-08-07 06:10:00 +0000
+++ Percona-Server/storage/innobase/include/os0file.h	2012-09-13 07:44:07 +0000
@@ -503,9 +503,10 @@
 				used by a backup program reading the file */
 	ibool*		success);/*!< out: TRUE if succeed, FALSE if error */
 /****************************************************************//**
-Tries to disable OS caching on an opened file descriptor. */
+Tries to disable OS caching on an opened file descriptor.
+@return TRUE if operation is success and FALSE otherwise */
 UNIV_INTERN
-void
+ibool
 os_file_set_nocache(
 /*================*/
 	int		fd,		/*!< in: file descriptor to alter */

=== modified file 'Percona-Server/storage/innobase/os/os0file.c'
--- Percona-Server/storage/innobase/os/os0file.c	2012-08-07 06:10:00 +0000
+++ Percona-Server/storage/innobase/os/os0file.c	2012-09-13 07:44:07 +0000
@@ -1321,9 +1321,10 @@
 }
 
 /****************************************************************//**
-Tries to disable OS caching on an opened file descriptor. */
+Tries to disable OS caching on an opened file descriptor.
+@return TRUE if operation is success and FALSE otherwise */
 UNIV_INTERN
-void
+ibool
 os_file_set_nocache(
 /*================*/
 	int		fd		/*!< in: file descriptor to alter */
@@ -1344,6 +1345,7 @@
 			"  InnoDB: Failed to set DIRECTIO_ON "
 			"on file %s: %s: %s, continuing anyway\n",
 			file_name, operation_name, strerror(errno_save));
+		return FALSE;
 	}
 #elif defined(O_DIRECT)
 	if (fcntl(fd, F_SETFL, O_DIRECT) == -1) {
@@ -1361,8 +1363,10 @@
 				"'Invalid argument' on Linux on tmpfs, "
 				"see MySQL Bug#26662\n");
 		}
+		return FALSE;
 	}
 #endif
+	return TRUE;
 }
 
 /****************************************************************//**
@@ -1593,7 +1597,10 @@
 
 	/* ALL_O_DIRECT: O_DIRECT also for transaction log file */
 	if (srv_unix_file_flush_method == SRV_UNIX_ALL_O_DIRECT) {
-		os_file_set_nocache(file, name, mode_str);
+		/* Do fsync() on log files when setting O_DIRECT fails.
+		See log_io_complete() */
+		if (!os_file_set_nocache(file, name, mode_str))
+			srv_unix_file_flush_method = SRV_UNIX_O_DIRECT;
 	}
 
 #ifdef USE_FILE_LOCK