Merge lp://qastaging/~vlad-lesin/percona-server/5.6-logical-readahead into lp://qastaging/percona-server/5.6

Proposed by Vlad Lesin
Status: Rejected
Rejected by: Laurynas Biveinis
Proposed branch: lp://qastaging/~vlad-lesin/percona-server/5.6-logical-readahead
Merge into: lp://qastaging/percona-server/5.6
Diff against target: 2278 lines (+1520/-71)
40 files modified
client/client_priv.h (+4/-1)
client/mysqldump.c (+38/-0)
mysql-test/include/have_native_aio.inc (+6/-0)
mysql-test/suite/innodb/r/innodb_logical_read_ahead.result (+47/-0)
mysql-test/suite/innodb/r/innodb_logical_read_ahead_correctness.result (+88/-0)
mysql-test/suite/innodb/r/innodb_merge_read.result (+30/-0)
mysql-test/suite/innodb/t/innodb_logical_read_ahead-master.opt (+2/-0)
mysql-test/suite/innodb/t/innodb_logical_read_ahead.test (+55/-0)
mysql-test/suite/innodb/t/innodb_logical_read_ahead_correctness-master.opt (+2/-0)
mysql-test/suite/innodb/t/innodb_logical_read_ahead_correctness.test (+107/-0)
mysql-test/suite/innodb/t/innodb_merge_read-master.opt (+1/-0)
mysql-test/suite/innodb/t/innodb_merge_read.test (+42/-0)
mysql-test/suite/sys_vars/r/innodb_lra_n_node_recs_before_sleep_basic.result (+28/-0)
mysql-test/suite/sys_vars/r/innodb_lra_size_basic.result (+28/-0)
mysql-test/suite/sys_vars/r/innodb_lra_sleep_basic.result (+30/-0)
mysql-test/suite/sys_vars/r/innodb_lra_test_basic.result (+8/-0)
mysql-test/suite/sys_vars/t/innodb_lra_n_node_recs_before_sleep_basic.test (+14/-0)
mysql-test/suite/sys_vars/t/innodb_lra_size_basic-master.opt (+1/-0)
mysql-test/suite/sys_vars/t/innodb_lra_size_basic.test (+14/-0)
mysql-test/suite/sys_vars/t/innodb_lra_sleep_basic.test (+14/-0)
mysql-test/suite/sys_vars/t/innodb_lra_test_basic-master.opt (+1/-0)
mysql-test/suite/sys_vars/t/innodb_lra_test_basic.test (+8/-0)
storage/innobase/btr/btr0cur.cc (+1/-0)
storage/innobase/btr/btr0pcur.cc (+26/-18)
storage/innobase/buf/buf0rea.cc (+33/-10)
storage/innobase/fil/fil0fil.cc (+8/-3)
storage/innobase/handler/ha_innodb.cc (+61/-0)
storage/innobase/include/btr0pcur.h (+3/-2)
storage/innobase/include/btr0pcur.ic (+51/-17)
storage/innobase/include/buf0rea.h (+37/-0)
storage/innobase/include/fil0fil.h (+5/-2)
storage/innobase/include/os0file.h (+24/-6)
storage/innobase/include/os0file.ic (+6/-2)
storage/innobase/include/srv0srv.h (+42/-0)
storage/innobase/include/trx0trx.h (+96/-1)
storage/innobase/os/os0file.cc (+99/-9)
storage/innobase/row/row0purge.cc (+9/-0)
storage/innobase/row/row0sel.cc (+319/-0)
storage/innobase/srv/srv0srv.cc (+9/-0)
storage/innobase/trx/trx0trx.cc (+123/-0)
To merge this branch: bzr merge lp://qastaging/~vlad-lesin/percona-server/5.6-logical-readahead
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Needs Resubmitting
Review via email: mp+216857@code.qastaging.launchpad.net

Description of the change

Porting logical readahead feature from Facebook branch. The original patches are here:

https://github.com/facebook/mysql-5.6/commit/f9d1a5332eb2c82c028638d3b93b5a3592a69ffa
https://github.com/facebook/mysql-5.6/commit/f8e361952612d00979f7cf744f487e48b15cb5a6
https://github.com/facebook/mysql-5.6/commit/f69a4ea522bce24e4cdcc7696d5fad29587cf87a

The main difference is multiple io's commit is enabled only for logical read-ahead in this branch in comparison with the original implementation where it is enabled by default for all operations. See explanation in commit comments.

Jenkins testing:
http://jenkins.percona.com/view/PS 5.6/job/percona-server-5.6-param/589

To post a comment you must log in.
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

This is not a full review, but addressing this bit can happen in parallel with the rest of the review: the MP needs a blueprint, or even several blueprints, corresponding to each commit (the commits IMHO are well-split). The blueprints must be self-contained.

review: Needs Information
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

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: Needs Fixing
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

Work on this MP must continue on github.

review: Needs Resubmitting

Unmerged revisions

578. By Vlad Lesin

Add mysqldump support for logical read ahead

Summary:
Adds options to mysqldump:
 --lra-size=X
 --lra-sleep=X
 --lra-n-node-recs-before-sleep=X

These just inject SET statements to set these session variables.

The original implementation is here:
https://github.com/facebook/mysql-5.6/commit/f69a4ea522bce24e4cdcc7696d5fad29587cf87a

577. By Vlad Lesin

When the session variable innodb_lra_size is set to N, we issue async
read requests for the next M logical pages where the total size of the M
pages on disk is N megabytes. The max allowed value of innodb_lra_size
is is 16384 which corresponds to prefetching 16GB of data. We may choose
to use smaller values in production.

The original implementation can be found here:
https://github.com/facebook/mysql-5.6/commit/f8e361952612d00979f7cf744f487e48b15cb5a6

This implementation does not contain code for flashcahe.

576. By Vlad Lesin

Merge aio page read requests

Summary:
Tries to submit multiple aio page read requests together to improve read
performance.

The original code and description can be found here:
https://github.com/facebook/mysql-5.6/commit/f9d1a5332eb2c82c028638d3b93b5a3592a69ffa

The difference between this and the original implementation is that fil_io()
macros invokes _fil_io() function with enabled io's buffering by default in
the original implementation, it can cause the errors connected with waiting
io finishing just after fil_io() invocation.

For example log_archive_do() waits io's finishing on log_sys->archive_lock
mutex, but the mutex is not being unlocked as io's were buffered and
uncommited and io_handler_thread() does not process io's completion in
fil_aio_wait(). Potentially there can be the same errors so io's buffering
is disabled by default and will be enabled only for logical readahead code.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches