[Maria-developers] Fusion-io atomic writes patch comments
Hi -
We ported your patch to Percona Server, meanwhile we collected the
following comments which perhaps are useful for you. These are
addressed in our XtraDB port, so you will pick them up, but since you
patched InnoDB as well, you might consider addressing them there as
well. I tried to order them roughly in the order of importance.
The comments below refer to rev 3720 of 5.5 tree (and one comment to
MariaDB 10 trunk).
- Where are the testcases? sys_vars test doesn't count :)
- fil_extend_space_to_desired_
size() fails to consider that it might
be extending a single node on a multi-node tablespace. It will
calculate the desired size as if the last node was the only one,
overextending the tablespace by the size of all the previous nodes.
Fix is replacing
offset_high = size_after_extend * page_size / (4ULL*1024*1024*1024);
offset_low = size_after_extend * page_size % (4ULL*1024*1024*1024);
with
offset_high = (size_after_extend - file_start_page_no)
* page_size / (4ULL * 1024 * 1024 * 1024);
offset_low = (size_after_extend - file_start_page_no)
* page_size % (4ULL * 1024 * 1024 * 1024);
- In the same function in InnoDB 5.6 / MariaDB 10.0 it is necessary to set
node->being_extended = FALSE; before jumping to complete_io.
- In the same function, complete_io: label may skip over the
fil_node_complete_io() call, which is required for os_aio()
tablespace-extending call only.
- os_file_set_atomic_writes() should return not an int, but bool or
ibool instead, also its return value currently is raw ioctl() result,
which is used as a bool at callers anyway and reversed from the InnoDB
convention of true/non-zero meaning success. While at that, it's also
nice to add __attribute__((warn_unused_result)) to it.
- If ioctl() failed, then it's customary for the failed I/O call
handling to call one of the os_file_handle_error... functions, here
os_file_handle_error_no_exit() looks like a good fit.
- If atomic writes are unsupported, then I'm not sure why set errno /
LastError. No syscall has failed, the atomic writes unsupported-ness
is communicated by the os_file_set_atomic_writes() return value.
- os_file_set_atomic_writes() declaration should not depend on
__linux__ etc, thus we pushed the #ifdef to function body.
- InnoDB uses UNIV_LINUX instead of __linux__, the build sys checks
for the presence of sys/ioctl.h and defines corresponding define, thus
we replaced
#ifdef __linux__
#include
-----Original Message----- From: Maria-developers [mailto:maria-developers- bounces+wlad=montyprogram.com@lists.launchpad.net] On Behalf Of Laurynas Biveinis Sent: Freitag, 24. Mai 2013 13:10 To: maria-developers@lists.launchpad.net Subject: [Maria-developers] Fusion-io atomic writes patch comments
Hi -
We ported your patch to Percona Server, meanwhile we collected the following comments which perhaps are useful for you. These are addressed in our XtraDB port, so you will pick them up, but since you patched InnoDB as well, you might consider addressing them there as well. I tried to order them roughly in the order of importance.
The comments below refer to rev 3720 of 5.5 tree (and one comment to MariaDB 10 trunk).
Thanks Laurynas. There are no testcases for atomic-writedness, it is not easily unit-testable (and requires upper-class hardware in Buildbot instead of working class VM:) We ran sysbench manually, and some other tests, and that was it. I'm looking forward to see what you'll come up with. Thanks for constructive hints. (Some of the comments seem non-issues, if you ask me, __linux__ vs UNIV_LINUX, or setting errno, last error , etc) . That Windows snippet was actually needed. Thanks again, Wlad
Regards, -- Laurynas
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (2)
-
Laurynas Biveinis
-
Vladislav Vaintroub