Merge lp:~gl-az/percona-xtrabackup/2.0-io-block-size into lp:percona-xtrabackup/2.0

[George Ormond Lorch III]

This implements the io-block-size option for XtraBackup and innobackupex (just for passthrough) which specifies number of pages to read/write per cycle.
Changes made:
- Increased the default from 64 pages to 256 pages (4M).
- Changed the ds_buffer size when chained behind compression to (UNIV_PAGE_SIZE * (xtrabackup_iosize + 1)), the one extra page is for any additional formatting overhead that would normally be seen in compression and xbstream.
- Corrected the retry logic in fil_cur.cc so that an individual page will be re-read up to a pre-determined number of times and for each validation failure, only that page will be re-read. Previous logic would re-read the entire block and continue counting retry failures even across failed, then successfully read pages.
- Created very simple test case that sets io-block-size to a larger value.
- Corrupt page reads are tested as part of bug766033.sh which creates a corrupted dataset and looks for the error message.
- Not tested as part of any test is a page that has been read and failed validation, then re-read and passed.

[George Ormond Lorch III (gl-az)]

Jenkins: http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/427

Other notes:
- In recent testing on two systems, 256 pages seems to be a sweet spot with slowly increasing benefit up to about 512 pages. At 256 pages, a consistent 20-25% reduction in backup time is observed with only a very minor hit to concurrent load throughput.

[Stewart Smith (stewart)]

George Ormond Lorch III <email address hidden> writes:
> - Not tested as part of any test is a page that has been read and
> failed validation, then re-read and passed.

This would be a really awesome test to have however... I'm not sure if
Alexey has any ideas of how we might do this with debug sync and the
like... but I think it could be quite worthwhile to have a test.

--
Stewart Smith

[George Ormond Lorch III (gl-az)]

Completely agree, I even looked into it a bit. I'm pretty sure you're
right though, it would need some sync in order to make it deterministic.
I certainly see this situation 'passing' in my local brute force tests
but it would be _really_ nice to have more of this sort of test in the
suite.

On 5/1/2013 10:12 AM, Stewart Smith wrote:
> George Ormond Lorch III <email address hidden> writes:
>> - Not tested as part of any test is a page that has been read and
>> failed validation, then re-read and passed.
> This would be a really awesome test to have however... I'm not sure if
> Alexey has any ideas of how we might do this with debug sync and the
> like... but I think it could be quite worthwhile to have a test.
>

--
George O. Lorch III
Software Engineer, Percona
[phone] x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

[Alexey Kopytov (akopytov)]

We also have bug #1093385 and https://bugs.launchpad.net/percona-xtrabackup/+bug/1095249/comments/1. Basically, in XB 2.0+ we are discarding read-ahead on each read. So it would be interesting to test the effect of this change with that bug fixed. I sense that the effect will be much lower (if at all measurable).

George, would you be willing to fix that bug and rerun tests with different block sizes? The fix would be to use FADV_DONTNEED to only discard already consumed data from the cache, rather than the entire file.

Regarding the test case, we used to have sporadic page validation failures in Jenkins before bug #1081882 was fixed. So it is covered in a way. But I agree, creating a dedicated test that would trigger a partially written page reliably doesn't look feasible.

review: Needs Information

[George Ormond Lorch III (gl-az)]

Certainly can do that Alexey. Will post back when I have some numbers...
On 5/1/2013 11:49 PM, Alexey Kopytov wrote:
> Review: Needs Information
>
> We also have bug #1093385 and https://bugs.launchpad.net/percona-xtrabackup/+bug/1095249/comments/1. Basically, in XB 2.0+ we are discarding read-ahead on each read. So it would be interesting to test the effect of this change with that bug fixed. I sense that the effect will be much lower (if at all measurable).
>
> George, would you be willing to fix that bug and rerun tests with different block sizes? The fix would be to use FADV_DONTNEED to only discard already consumed data from the cache, rather than the entire file.
>
> Regarding the test case, we used to have sporadic page validation failures in Jenkins before bug #1081882 was fixed. So it is covered in a way. But I agree, creating a dedicated test that would trigger a partially written page reliably doesn't look feasible.

--
George O. Lorch III
Software Engineer, Percona
[phone] x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

Unmerged revisions

552. By George Ormond Lorch III on 2013-04-30

This implements the io-block-size option for XtraBackup and innobackupex (just for passthrough) which specifies number of pages to read/write per cycle.
Changes made:
- Increased the default from 64 pages to 256 pages (4M).
- Corrected the retry logic in xtrabackup_copy_datafile so that an individual page will be re-read up to a pre-determined number of times and for each validation failure, only that page will be re-read. Previous logic would re-read the entire block and continue counting retry failures even across failed and then successfully read pages.
- Created very simple test case that sets io-block-size to a larger value.
- Corrupt page reads are tested as part of bug766033.sh which creates a corrupted dataset and looks for the error message.
- Not tested as part of any test is a page that has been read and failed validation, then re-read and passed.

=== modified file 'innobackupex'
--- innobackupex	2013-04-28 18:34:11 +0000
+++ innobackupex	2013-05-01 16:01:24 +0000
@@ -81,6 +81,7 @@
 my $option_tables_file = '';
 my $option_throttle = '';
 my $option_sleep = '';
+my $option_io_block_size = '';
 my $option_compress = 999;
 my $option_compress_threads = 1;
 my $option_uncompress = '';
@@ -1158,6 +1159,11 @@
     if ($option_throttle) {
         $options = $options . " --throttle=$option_throttle";
     }
+
+    if ($option_io_block_size) {
+        $options = $options . " --io-buffer-size=$option_io_block_size";
+    }
+
     if ($option_log_copy_interval) {
         $options = $options . " --log-copy-interval=$option_log_copy_interval";
     }
@@ -1998,7 +2004,8 @@
     }
 
     # read command line options
-    $rcode = GetOptions('compress' => \$option_compress,
+    $rcode = GetOptions('io-block-size=s' => \$option_io_block_size,
+                        'compress' => \$option_compress,
 	    		'compress-threads=i' => \$option_compress_threads,
                         'help' => \$option_help,
                         'version' => \$option_version,
@@ -3142,6 +3149,10 @@
 
 This option specifies the log sequence number (LSN) to use for the incremental backup.  The option accepts a string argument. It is used with the --incremental option. It is used instead of specifying --incremental-basedir. For databases created by MySQL and Percona Server 5.0-series versions, specify the LSN as two 32-bit integers in high:low format. For databases created in 5.1 and later, specify the LSN as a single 64-bit integer.
 
+=item --io-block-size
+
+This option specifies the number of InnoDB pages that will be read per read+write I/O cycle. It is passed directly to xtrabackup's --io-block-size option. See xtrabackup documentation for details.
+
 =item --move-back
 
 Move all the files in a previously made backup from the backup directory to the actual datadir location. Use with caution, as it removes backup files.

=== modified file 'src/xtrabackup.cc'
--- src/xtrabackup.cc	2013-04-29 05:45:52 +0000
+++ src/xtrabackup.cc	2013-05-01 16:01:24 +0000
@@ -1413,6 +1413,8 @@
 xb_stream_fmt_t xtrabackup_stream_fmt;
 my_bool xtrabackup_stream = FALSE;
 
+ulonglong xtrabackup_iosize = 64;
+
 static const char *xtrabackup_compress_alg = NULL;
 ibool xtrabackup_compress = FALSE;
 uint xtrabackup_compress_threads;
@@ -1645,6 +1647,7 @@
   OPT_XTRA_CREATE_IB_LOGFILE,
   OPT_XTRA_PARALLEL,
   OPT_XTRA_STREAM,
+  OPT_XTRA_IOSIZE,
   OPT_XTRA_COMPRESS,
   OPT_XTRA_COMPRESS_THREADS,
   OPT_INNODB,
@@ -1817,6 +1820,11 @@
    (G_PTR*) &xtrabackup_stream_str, (G_PTR*) &xtrabackup_stream_str, 0, GET_STR,
    REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
 
+  {"io-block-size", OPT_XTRA_IOSIZE,
+   "Size of working buffer(s) in pages. The default value is 64.",
+   (G_PTR*) &xtrabackup_iosize, (G_PTR*) &xtrabackup_iosize,
+   0, GET_ULL, REQUIRED_ARG, 64, 1, ULONGLONG_MAX, 0, 0, 0},
+
   {"compress", OPT_XTRA_COMPRESS, "Compress individual backup files using the "
    "specified compression algorithm. Currently the only supported algorithm "
    "is 'quicklz'. It is also the default algorithm, i.e. the one used when "
@@ -4032,7 +4040,7 @@
 }
 
 /* TODO: We may tune the behavior (e.g. by fil_aio)*/
-#define COPY_CHUNK 64
+#define COPY_CHUNK xtrabackup_iosize
 
 static
 my_bool
@@ -4204,6 +4212,8 @@
 		ulint chunk;
 		ulint chunk_offset;
 		ulint retry_count = 10;
+		const ulint max_retries = 10;
+		const ulint retry_wait = 100000;
 
 		if (file_size - offset > (IB_INT64) (COPY_CHUNK * page_size)) {
 			chunk = COPY_CHUNK * page_size;
@@ -4211,7 +4221,6 @@
 			chunk = (ulint)(file_size - offset);
 		}
 
-read_retry:
 		xtrabackup_io_throttling();
 
 		success = xb_os_file_read(src_file, page, offset, chunk);
@@ -4225,15 +4234,21 @@
 
 		/* check corruption and retry */
 		for (chunk_offset = 0; chunk_offset < chunk; chunk_offset += page_size) {
-			if (xb_buf_page_is_corrupted(page + chunk_offset,
+			retry_count = 0;
+			while (xb_buf_page_is_corrupted(page + chunk_offset,
 						     info.zip_size))
 			{
+				ulint page_no = ((offset + (IB_INT64)chunk_offset) >> page_size_shift);
+				IB_INT64 page_offset = offset + chunk_offset;
+				ulint retry_read_size = page_size;
+				if (page_size > chunk - chunk_offset) {
+					retry_read_size = chunk - chunk_offset;
+				}
+
 				if (
 				    trx_sys_sys_space(node->space->id)
-				    && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
-				    >= (IB_INT64) FSP_EXTENT_SIZE
-				    && ((offset + (IB_INT64)chunk_offset) >> page_size_shift)
-				    < (IB_INT64) FSP_EXTENT_SIZE * 3) {
+				    && page_no >= (IB_INT64) FSP_EXTENT_SIZE
+				    && page_no < (IB_INT64) FSP_EXTENT_SIZE * 3) {
 					/* double write buffer may have old data in the end
 					   or it may contain the other format page like COMPRESSED.
  					   So, we can pass the check of double write buffer.*/
@@ -4241,33 +4256,37 @@
 					msg("[%02u] xtrabackup: "
 					    "Page %lu seems double write "
 					    "buffer. passing the check.\n",
-					    thread_n,
-					    (ulint)((offset +
-						     (IB_INT64)chunk_offset) >>
-						    page_size_shift));
-				} else {
-					retry_count--;
-					if (retry_count == 0) {
-						msg("[%02u] xtrabackup: "
-						    "Error: 10 retries "
-						    "resulted in fail. File "
-						    "%s seems to be "
-						    "corrupted.\n",
-						    thread_n, node_path);
-						goto error;
-					}
+					    thread_n, page_no);
+					break;
+				}
+				
+				msg("[%02u] xtrabackup: "
+				    "Database page corruption detected at "
+				    "page %lu. retrying %lu of %lu...\n",
+				    thread_n, page_no, retry_count + 1,
+				    max_retries);
+
+				os_thread_sleep(retry_wait);
+
+				xtrabackup_io_throttling();
+
+				success = xb_os_file_read(src_file,
+							  page + chunk_offset,
+							  page_offset,
+							  retry_read_size);
+
+				if (!success) {
+					goto error;
+				}
+
+				retry_count++;
+				if (retry_count >= max_retries) {
 					msg("[%02u] xtrabackup: "
-					    "Database page corruption "
-					    "detected at page %lu. "
-					    "retrying...\n",
-					    thread_n,
-					    (ulint)((offset +
-						     (IB_INT64)chunk_offset)
-						    >> page_size_shift));
-
-					os_thread_sleep(100000);
-
-					goto read_retry;
+					    "Error: %ld retries resulted in "
+					    "failure. File %s seems to be "
+					    "corrupted.\n",
+					    thread_n, retry_count, node_path);
+					goto error;
 				}
 			}
 		}

=== added file 'test/t/xb_io_block_size.sh'
--- test/t/xb_io_block_size.sh	1970-01-01 00:00:00 +0000
+++ test/t/xb_io_block_size.sh	2013-05-01 16:01:24 +0000
@@ -0,0 +1,35 @@
+. inc/common.sh
+
+start_server --innodb_file_per_table
+
+load_dbase_schema sakila
+load_dbase_data sakila
+
+# Take backup
+vlog "Creating the backup directory: $topdir/backup"
+mkdir -p $topdir/backup
+innobackupex $topdir/backup --io-block-size=512
+backup_dir=`grep "innobackupex: Backup created in directory" $OUTFILE | awk -F\' '{ print $2}'`
+
+#echo "Backup dir in $backup_dir"
+
+stop_server
+# Remove datadir
+rm -r $mysql_datadir
+# Restore sakila
+vlog "Applying log"
+vlog "###########"
+vlog "# PREPARE #"
+vlog "###########"
+innobackupex --apply-log $backup_dir
+vlog "Restoring MySQL datadir"
+mkdir -p $mysql_datadir
+vlog "###########"
+vlog "# RESTORE #"
+vlog "###########"
+innobackupex --copy-back $backup_dir
+
+start_server
+# Check sakila
+run_cmd ${MYSQL} ${MYSQL_ARGS} -e "SELECT count(*) from actor" sakila
+