Merge lp:~stewart/percona-server/5.5.27-plus-bugfixes into lp:percona-server/5.5

Proposed by Stewart Smith on 2012-08-10
Status: Work in progress
Proposed branch: lp:~stewart/percona-server/5.5.27-plus-bugfixes
Merge into: lp:percona-server/5.5
Prerequisite: lp:~stewart/percona-server/5.5.27
Diff against target: 55 lines (+15/-6)
1 file modified
Percona-Server/storage/innobase/buf/buf0lru.c (+15/-6)
To merge this branch: bzr merge lp:~stewart/percona-server/5.5.27-plus-bugfixes
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Needs Information on 2012-08-15
Laurynas Biveinis (community) 2012-08-10 Needs Information on 2012-08-10
Review via email: mp+119085@code.launchpad.net

Description of the change

Include critical bugfixes based on Alexey's feedback to initial 5.5.27 branch. This merge req has that branch as a prerequisite.

http://jenkins.percona.com/job/percona-server-5.5-param/461/

To post a comment you must log in.

Does this apply to 5.1? (If it does, should be fixed there first and merged up...)

Why did you add the "impossible case" if (!block_mutex) ... ?

review: Needs Information
Stewart Smith (stewart) wrote :

Laurynas Biveinis <email address hidden> writes:

> Review: Needs Information
>
> Does this apply to 5.1? (If it does, should be fixed there first and
> merged up...)

Haven't checked to be honest :)

>
> Why did you add the "impossible case" if (!block_mutex) ... ?

It was there in original code that was missed during merge.... it is a
bit odd though.

--
Stewart Smith

> > Does this apply to 5.1? (If it does, should be fixed there first and
> > merged up...)
>
> Haven't checked to be honest :)

OK if the fixes do not apply for 5.1. If they do, then 5.1 has to be fixed and upmerged.

Stewart Smith (stewart) wrote :

> > > Does this apply to 5.1? (If it does, should be fixed there first and
> > > merged up...)
> >
> > Haven't checked to be honest :)
>
> OK if the fixes do not apply for 5.1. If they do, then 5.1 has to be fixed and
> upmerged.

FYI they don't seem to apply to 5.1 from code inspection.

Alexey Kopytov (akopytov) wrote :

I don't the "It may be impossible case" code in the original fix. The upstream code is:

---
  } else {

   block_mutex = buf_page_get_mutex(bpage);
   mutex_enter(block_mutex);

   if (bpage->buf_fix_count > 0) {

    mutex_exit(block_mutex);

    /* We cannot remove this page during
    this scan yet; maybe the system is
    currently reading it in, or flushing
    the modifications to the file */

    all_freed = FALSE;

    goto next_page;
   }
  }

---

Which appears to be rather different than what is now in Percona Server. Why?

review: Needs Information

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Percona-Server/storage/innobase/buf/buf0lru.c'
--- Percona-Server/storage/innobase/buf/buf0lru.c 2012-08-10 06:51:31 +0000
+++ Percona-Server/storage/innobase/buf/buf0lru.c 2012-08-10 06:51:33 +0000
@@ -472,7 +472,7 @@
472 mutex_t* block_mutex;472 mutex_t* block_mutex;
473 ibool processed = FALSE;473 ibool processed = FALSE;
474474
475 ut_ad(buf_pool_mutex_own(buf_pool));475 ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
476 ut_ad(buf_flush_list_mutex_own(buf_pool));476 ut_ad(buf_flush_list_mutex_own(buf_pool));
477477
478 block_mutex = buf_page_get_mutex(bpage);478 block_mutex = buf_page_get_mutex(bpage);
@@ -595,11 +595,11 @@
595 ibool all_freed;595 ibool all_freed;
596596
597 do {597 do {
598 buf_pool_mutex_enter(buf_pool);598 mutex_enter(&buf_pool->LRU_list_mutex);
599599
600 all_freed = buf_flush_or_remove_pages(buf_pool, id);600 all_freed = buf_flush_or_remove_pages(buf_pool, id);
601601
602 buf_pool_mutex_exit(buf_pool);602 mutex_exit(&buf_pool->LRU_list_mutex);
603603
604 ut_ad(buf_flush_validate(buf_pool));604 ut_ad(buf_flush_validate(buf_pool));
605605
@@ -659,8 +659,16 @@
659 goto next_page;659 goto next_page;
660 } else {660 } else {
661661
662 block_mutex = buf_page_get_mutex(bpage);662 block_mutex = buf_page_get_mutex_enter(bpage);
663 mutex_enter(block_mutex);663
664 if (!block_mutex) {
665 /* It may be impossible case...
666 Something wrong, so will be scan_again */
667
668 all_freed = FALSE;
669 goto next_page;
670 }
671
664672
665 if (bpage->buf_fix_count > 0) {673 if (bpage->buf_fix_count > 0) {
666674
@@ -694,7 +702,8 @@
694 ulint page_no;702 ulint page_no;
695 ulint zip_size;703 ulint zip_size;
696704
697 buf_pool_mutex_exit(buf_pool);705 mutex_exit(&buf_pool->LRU_list_mutex);
706 rw_lock_x_unlock(&buf_pool->page_hash_latch);
698707
699 zip_size = buf_page_get_zip_size(bpage);708 zip_size = buf_page_get_zip_size(bpage);
700 page_no = buf_page_get_page_no(bpage);709 page_no = buf_page_get_page_no(bpage);

Subscribers

People subscribed via source and target branches