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

Proposed by Vlad Lesin on 2012-09-08
Status: Work in progress
Proposed branch: lp:~vlad-lesin/percona-server/5.5-bug801989
Merge into: lp:percona-server/5.5
Diff against target: 64 lines (+13/-5)
2 files modified
Percona-Server/storage/innobase/include/os0file.h (+3/-2)
Percona-Server/storage/innobase/os/os0file.c (+10/-3)
To merge this branch: bzr merge lp:~vlad-lesin/percona-server/5.5-bug801989
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) 2012-09-08 Needs Fixing on 2012-09-12
Review via email: mp+123403@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Alexey Kopytov (akopytov) wrote :

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) 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)
--
Stewart Smith

Alexey Kopytov (akopytov) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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) 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.

Vlad Lesin (vlad-lesin) wrote :

> 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) wrote :

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).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/storage/innobase/include/os0file.h'
2--- Percona-Server/storage/innobase/include/os0file.h 2012-08-07 06:10:00 +0000
3+++ Percona-Server/storage/innobase/include/os0file.h 2012-09-13 07:44:07 +0000
4@@ -503,9 +503,10 @@
5 used by a backup program reading the file */
6 ibool* success);/*!< out: TRUE if succeed, FALSE if error */
7 /****************************************************************//**
8-Tries to disable OS caching on an opened file descriptor. */
9+Tries to disable OS caching on an opened file descriptor.
10+@return TRUE if operation is success and FALSE otherwise */
11 UNIV_INTERN
12-void
13+ibool
14 os_file_set_nocache(
15 /*================*/
16 int fd, /*!< in: file descriptor to alter */
17
18=== modified file 'Percona-Server/storage/innobase/os/os0file.c'
19--- Percona-Server/storage/innobase/os/os0file.c 2012-08-07 06:10:00 +0000
20+++ Percona-Server/storage/innobase/os/os0file.c 2012-09-13 07:44:07 +0000
21@@ -1321,9 +1321,10 @@
22 }
23
24 /****************************************************************//**
25-Tries to disable OS caching on an opened file descriptor. */
26+Tries to disable OS caching on an opened file descriptor.
27+@return TRUE if operation is success and FALSE otherwise */
28 UNIV_INTERN
29-void
30+ibool
31 os_file_set_nocache(
32 /*================*/
33 int fd /*!< in: file descriptor to alter */
34@@ -1344,6 +1345,7 @@
35 " InnoDB: Failed to set DIRECTIO_ON "
36 "on file %s: %s: %s, continuing anyway\n",
37 file_name, operation_name, strerror(errno_save));
38+ return FALSE;
39 }
40 #elif defined(O_DIRECT)
41 if (fcntl(fd, F_SETFL, O_DIRECT) == -1) {
42@@ -1361,8 +1363,10 @@
43 "'Invalid argument' on Linux on tmpfs, "
44 "see MySQL Bug#26662\n");
45 }
46+ return FALSE;
47 }
48 #endif
49+ return TRUE;
50 }
51
52 /****************************************************************//**
53@@ -1593,7 +1597,10 @@
54
55 /* ALL_O_DIRECT: O_DIRECT also for transaction log file */
56 if (srv_unix_file_flush_method == SRV_UNIX_ALL_O_DIRECT) {
57- os_file_set_nocache(file, name, mode_str);
58+ /* Do fsync() on log files when setting O_DIRECT fails.
59+ See log_io_complete() */
60+ if (!os_file_set_nocache(file, name, mode_str))
61+ srv_unix_file_flush_method = SRV_UNIX_O_DIRECT;
62 }
63
64 #ifdef USE_FILE_LOCK

Subscribers

People subscribed via source and target branches