Review of commit 576. I think it will be easier if it will have its own MP (probably the other two commits too).
Code
- s/ibool/bool/g (in all three commits if applies)
- I'd add a defensive asserts at os_aio_free that for all arrays,
count[x] == 0. This would catch any unsubmitted buffered read
request, and any request buffered on the non-read array.
- s/ut_malloc+memset(0)/calloc in os_aio_array_create
- Why does buf_read_recv_pages call
os_aio_linux_dispatch_read_array_submit? It does not appear to
submit any buffered requests.
- fil_extend_space_to_desired_size calling os_aio with
should_buffer == TRUE is a (benign) typo?
- The abstraction level for buffered request submitting seems to
be off. I'd rename os_aio_linux_dispatch_read_array_submit to
os_aio_submit_buffered_requests and push #ifdef LINUX_NATIVE_AIO
down to it.
- buf_read_page_low header comment @return tag: edit to "1 if
read request is issued or buffered" to clarify that the function
returns the same for both buffered and immediatelly issued read
requests.
- s/read/ready in the buf_read_page_low should_buffer
comment. (https://github.com/facebook/mysql-5.6/commit/b1b4c7977136d57170f8bf500aaedba740b1c333)
- Make sure the patch does not break the build with performance
schema configured out
- os_aio_linux_dispatch_read_array_submit would need a /*===*/
comment, os_aio_func should_buffer arg declaration is misaligned
</pedantic>
Testcase
- --disable_warnings/DROP TABLE IF EXISTS/--enable_warnings idiom
is obsolete and should be removed. (in all three commits if
applies)
- innodb_merge_read.test needs --source
include/have_innodb_16k.inc, as the number of readahead requests
is likely to differ for other page sizes.
- innodb_merge_read-master.opt is redundant, as
--innodb-use-native-aio=1 is the default. The source
include/have_native_aio.inc check in the testcase itself is
enough.
- newline at the end of have_native_aio.inc
- I'd extend the innodb_merge_read testcase to check that linear
read ahead read buffering works for compressed tablespaces too
(there is code if (zip_size) then read(... should_buffer) else
read(... should_buffer)). That would cause move of the testcase
to the innodb_zip suite as well.
- (wishlist) Consider submitting https://github.com/webscalesql/webscalesql-5.6/commit/32e49b4d66eaa392d9f06198596db4b16e8b8d04
for Percona Server so that we exercise AIO with MTR --mem.
Review of commit 576. I think it will be easier if it will have its own MP (probably the other two commits too).
Code
- s/ibool/bool/g (in all three commits if applies) memset( 0)/calloc in os_aio_array_create aio_linux_ dispatch_ read_array_ submit? It does not appear to space_to_ desired_ size calling os_aio with linux_dispatch_ read_array_ submit to aio_submit_ buffered_ requests and push #ifdef LINUX_NATIVE_AIO /github. com/facebook/ mysql-5. 6/commit/ b1b4c7977136d57 170f8bf500aaedb a740b1c333) linux_dispatch_ read_array_ submit would need a /*===*/
- I'd add a defensive asserts at os_aio_free that for all arrays,
count[x] == 0. This would catch any unsubmitted buffered read
request, and any request buffered on the non-read array.
- s/ut_malloc+
- Why does buf_read_recv_pages call
os_
submit any buffered requests.
- fil_extend_
should_buffer == TRUE is a (benign) typo?
- The abstraction level for buffered request submitting seems to
be off. I'd rename os_aio_
os_
down to it.
- buf_read_page_low header comment @return tag: edit to "1 if
read request is issued or buffered" to clarify that the function
returns the same for both buffered and immediatelly issued read
requests.
- s/read/ready in the buf_read_page_low should_buffer
comment. (https:/
- Make sure the patch does not break the build with performance
schema configured out
- os_aio_
comment, os_aio_func should_buffer arg declaration is misaligned
</pedantic>
Testcase
- --disable_ warnings/ DROP TABLE IF EXISTS/ --enable_ warnings idiom merge_read. test needs --source have_innodb_ 16k.inc, as the number of readahead requests merge_read- master. opt is redundant, as -innodb- use-native- aio=1 is the default. The source have_native_ aio.inc check in the testcase itself is /github. com/webscalesql /webscalesql- 5.6/commit/ 32e49b4d66eaa39 2d9f06198596db4 b16e8b8d04
is obsolete and should be removed. (in all three commits if
applies)
- innodb_
include/
is likely to differ for other page sizes.
- innodb_
-
include/
enough.
- newline at the end of have_native_aio.inc
- I'd extend the innodb_merge_read testcase to check that linear
read ahead read buffering works for compressed tablespaces too
(there is code if (zip_size) then read(... should_buffer) else
read(... should_buffer)). That would cause move of the testcase
to the innodb_zip suite as well.
- (wishlist) Consider submitting
https:/
for Percona Server so that we exercise AIO with MTR --mem.