Comments. Take bitmap mutex lock when changing bitmap.changed.

storage/maria/ha_maria.cc:
  comment
storage/maria/ma_checkpoint.c:
  comment
storage/maria/ma_extra.c:
  use bitmap mutex when changing bitmap.changed, sounds safer
storage/maria/ma_pagecache.c:
  comment
storage/maria/ma_recovery.c:
  comments

5 files changed, 23 insertions, 7 deletions

diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc
index 34c7bd1455e..efcfca5bd62 100644
--- a/storage/maria/ha_maria.cc
+++ b/storage/maria/ha_maria.cc
@@ -2381,7 +2381,8 @@ int ha_maria::external_lock(THD *thd, int lock_type)
         Note that we can come here without having an exclusive lock on the
         table, for example in this case:
         external_lock(F_(WR|RD)LCK); thr_lock() which fails due to lock
-        abortion; external_lock(F_UNLCK).
+        abortion; external_lock(F_UNLCK). Fortunately, the re-enabling happens
+        only if we were the thread which disabled logging.
       */
       if (_ma_reenable_logging_for_table(file, TRUE))
         DBUG_RETURN(1);
diff --git a/storage/maria/ma_checkpoint.c b/storage/maria/ma_checkpoint.c
index a78b31edce7..6d1a6332c54 100644
--- a/storage/maria/ma_checkpoint.c
+++ b/storage/maria/ma_checkpoint.c
@@ -653,6 +653,14 @@ pthread_handler_t ma_checkpoint_background(void *arg)
             We use FLUSH_KEEP_LAZY: if a file is already in flush, it's
             smarter to move to the next file than wait for this one to be
             completely flushed, which may take long.
+            StaleFilePointersInFlush: notice how below we use "dfile" which
+            is an OS file descriptor plus some function and MARIA_SHARE
+            pointers; this data dates from a previous checkpoint; since then,
+            the table may have been closed (so MARIA_SHARE* became stale), and
+            the file descriptor reassigned to another table which does not
+            have the same CRC-read-set callbacks: it is thus important that
+            flush_pagecache_blocks_with_filter() does not use the pointers,
+            only the OS file descriptor.
           */
           int res=
             flush_pagecache_blocks_with_filter(maria_pagecache,
diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c
index c311a035e9b..92e0b0d4e15 100644
--- a/storage/maria/ma_extra.c
+++ b/storage/maria/ma_extra.c
@@ -592,7 +592,11 @@ int _ma_flush_table_files(MARIA_HA *info, uint flush_data_or_index,
           error= 1;
       }
       else
-        info->s->bitmap.changed= 0;
+      {
+        pthread_mutex_lock(&share->bitmap.bitmap_lock);
+        share->bitmap.changed= 0;
+        pthread_mutex_unlock(&share->bitmap.bitmap_lock);
+      }
       if (flush_pagecache_blocks(share->pagecache, &info->dfile,
                                  flush_type_for_data))
         error= 1;
diff --git a/storage/maria/ma_pagecache.c b/storage/maria/ma_pagecache.c
index 0a5cea8a541..3887d6af7bc 100644
--- a/storage/maria/ma_pagecache.c
+++ b/storage/maria/ma_pagecache.c
@@ -4227,11 +4227,11 @@ static int flush_cached_blocks(PAGECACHE *pagecache,
        @todo IO If page is contiguous with next page to flush, group flushes
        in one single my_pwrite().
     */
-    /*
+    /**
       It is important to use block->hash_link->file below and not 'file', as
-      the first one is right and the second may have different content (and
-      this matters for callbacks, bitmap pages and data pages have different
-      ones).
+      the first one is right and the second may have different out-of-date
+      content (see StaleFilePointersInFlush in ma_checkpoint.c).
+      @todo change argument of functions to be File.
     */
     error= pagecache_fwrite(pagecache, &block->hash_link->file,
                             block->buffer,
diff --git a/storage/maria/ma_recovery.c b/storage/maria/ma_recovery.c
index 8470f509140..08aba4fdcd6 100644
--- a/storage/maria/ma_recovery.c
+++ b/storage/maria/ma_recovery.c
@@ -3326,7 +3326,10 @@ void _ma_tmp_disable_logging_for_table(MARIA_HA *info,
 /**
    Re-enables logging for a table which had it temporarily disabled.
 
-   Only the thread which disabled logging is allowed to reenable it.
+   Only the thread which disabled logging is allowed to reenable it. Indeed,
+   re-enabling logging affects all open instances, one must have exclusive
+   access to the table to do that. In practice, the one which disables has
+   such access.
 
    @param  info            table
    @param  flush_pages     if function needs to flush pages first