Merge lp:~vlad-lesin/percona-server/5.5-bug801989 into lp:percona-server/5.5
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alexey Kopytov (community) | 2012-09-08 | Needs Fixing on 2012-09-12 | |
Review via email:
|
Description of the change
The https:/
The main idea of this fix is to return "true" on success and "false" otherwise from os_file_
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://
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)
>
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_
> os_file_
> srv_unix_
> 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_
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_
> == 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).
Vlad, some comments on the patch:
- I don't think we should crash the server after failing to set bugs.mysql. com/bug. php?id= 26662
O_DIRECT. This way you are essentially resurrecting
http://
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 create_ func() check the value returned by file_set_ nocache( ) introduced by your patch, and if it failed, do unix_file_ flush_method = SRV_UNIX_O_DIRECT). This way we will at
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_
os_
srv_
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.