PXB-3862 : Address PR #1780 review comments

https://perconadev.atlassian.net/browse/PXB-3862

- Rework probe_storage() in page terms and reorder its phases: every
  read now fetches whole 16KB pages at page-aligned offsets (the same
  shape as the copy loop's reads, and aligned as O_DIRECT requires),
  and the sequential bandwidth phase runs first, on pages nothing has
  touched yet, so cross-phase caching can only understate the round
  trip measured afterwards - understating it shrinks the resulting
  cost, the safe direction. One file descriptor serves both phases:
  pread carries no file position, and closing/reopening would evict
  nothing (the OS page cache is keyed by inode; the device's internal
  cache is beyond user space either way).

- Return std::optional<Probe_result> from probe_storage() instead of
  carrying a valid flag.

- Print sizes in the calibration and refusal log lines through
  xtrabackup::utils::human_readable(), consistent with the rest of
  the log.

- Also reject ERANGE overflow from strtoull when parsing
  --page-tracking-merge-gap.

- Document why the probe candidate is always an InnoDB tablespace
  (the datafiles iterator walks the InnoDB fil system only - files of
  other engines never appear in it - and the changed-page map is
  keyed by InnoDB space id), and why the UNIV_PAGE_SIZE divisibility
  assertion in rf_page_tracking_init holds structurally; its
  release-build failure mode degrades to merge_gap = 0, the old
  strictly-consecutive reads - a performance fallback, never a
  correctness risk.

- Pass the read-filter context to range_get_next_page() so it
  accumulates into the stat counters directly, removing the
  out-parameters (and, likewise, the block scope in probe_storage's
  sequential phase); reword the option help text around "merge" and
  reflow it.

- Update the probe-candidate comment now that PXB-3502 (PR #1774,
  largest-files-first ordering) is merged: the running-max scan does
  not depend on iteration order.

- Rename the internal read-filter field max_gap to merge_gap so code,
  comments and the option name all use one term.

- Name the calibration margin READ_REQUEST_COST_MARGIN = 1.5 and
  document at the constant why that value (bounded from both sides by
  the two instrumented machines), instead of an inline * 2 / 3.

- Have probe_storage() drop its own sample regions from the OS page
  cache (posix_fadvise DONTNEED, ~16MB, surgical) before measuring, so
  on buffered servers the probe times the device rather than pages a
  previous read left in RAM; a no-op under O_DIRECT.

Author: satya-bodapati

storage/innobase/xtrabackup/src/changed_page_tracking.cc

@@ -22,6 +22,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 #include "backup_mysql.h"
 #include "common.h"
 #include "components/mysqlbackup/backup_comp_constants.h"
+#include "read_filt.h"
 #include "srv0srv.h"
 #include "xb0xb.h"
 #include "xtrabackup.h"
@@ -273,16 +274,16 @@ range to its last: the caller then issues one pread covering the whole
 range (sliced into --read-buffer-size pieces when larger). A gap is
 the count of unchanged pages strictly BETWEEN two changed pages, not
 their id difference: between changed pages 1 and 4 the gap is 2
-(pages 2 and 3). Gaps <= max_gap are combined across, so nearby
+(pages 2 and 3). Gaps <= merge_gap are combined across, so nearby
 changed pages form one larger sequential read instead of one read
 request each; the gap pages become filler - read, then dropped by the
 incremental write filter's LSN check: extra read volume, never backup
-content. A gap > max_gap ends the range; runs of consecutive changed
+content. A gap > merge_gap ends the range; runs of consecutive changed
 pages (gap 0) always stay whole.
 
   changed pages [1,3,6,9]  (gaps 1,2,2):
-    max_gap=0:  4 reads: [1] [3] [6] [9]
-    max_gap=2:  1 read:  [1-9]   (filler 2,4,5,7,8 read, dropped)
+    merge_gap=0:  4 reads: [1] [3] [6] [9]
+    merge_gap=2:  1 read:  [1-9]   (filler 2,4,5,7,8 read, dropped)
 
 Counting: the loop inspects one neighbouring pair per iteration and
 refuses (breaks) BEFORE the counting code, so reaching that code means
@@ -291,16 +292,16 @@ right there. A refused gap becomes the space between two ranges and is
 counted as skipped pages by the caller (rf_page_tracking_get_next_batch)
 instead - each gap lands in exactly one of the two. Worked trace on the
 shared example in read_filt.h (changed pages 1,2,3,4,7,20,21,
-max_gap=4): (4,7) gap 2 -> filler_pages += 2, combined_gaps = 1;
+merge_gap=4): (4,7) gap 2 -> filler_pages += 2, combined_gaps = 1;
 (7,20) gap 12 -> park on 7, range [1-7] ends; the caller then counts
 20 - 8 = 12 skipped when the next call builds [20-21].
 
-max_gap comes from --page-tracking-merge-gap: by default ("auto") the
+merge_gap comes from --page-tracking-merge-gap: by default ("auto") the
 storage's measured read request cost converted to pages of this
 tablespace's physical page size (see xb_io_probe.h). */
-void range_get_next_page(xb_page_set *page_set, ulint max_gap,
-                         ulint *filler_pages, ulint *combined_gaps) {
+void range_get_next_page(xb_page_set *page_set, xb_read_filt_ctxt_t *ctxt) {
   ut_ad(page_set->current_page_it != page_set->pages.end());
+  const ulint merge_gap = ctxt->merge_gap;
 
   while (true) {
     auto current_page = *page_set->current_page_it;
@@ -311,7 +312,7 @@ void range_get_next_page(xb_page_set *page_set, ulint max_gap,
     }
     auto next_page = *page_set->current_page_it;
     ut_ad(next_page > current_page);
-    if (next_page > current_page + 1 + max_gap) {
+    if (next_page > current_page + 1 + merge_gap) {
       /* gap too large to combine across: park the iterator on the last
       page of the current range so the read batch ends there */
       --page_set->current_page_it;
@@ -320,8 +321,8 @@ void range_get_next_page(xb_page_set *page_set, ulint max_gap,
     if (next_page > current_page + 1) {
       /* past the refusal check, so this gap of >= 1 unchanged pages was
       just combined into the range: its pages are filler */
-      *filler_pages += next_page - current_page - 1;
-      ++*combined_gaps;
+      ctxt->stat_filler_pages += next_page - current_page - 1;
+      ctxt->stat_combined_gaps++;
     }
   }
 }

storage/innobase/xtrabackup/src/changed_page_tracking.h

@@ -26,6 +26,10 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 #include "common.h"
 #include "mysql.h"
 
+/* the read-filter context (read_filt.h includes this header, so only a
+declaration is possible here) */
+struct xb_read_filt_ctxt_t;
+
 namespace pagetracking {
 typedef std::set<page_no_t>::iterator page_iterator;
 
@@ -64,19 +68,18 @@ return true if installed */
 bool is_component_installed(MYSQL *connection);
 
 /** Move the current_page_it iterator to point to the last page id of the
-current block. Changed pages separated by gaps of at most max_gap
+current block. Changed pages separated by gaps of at most merge_gap
 unchanged pages belong to the same block, so that they are read with one
 sequential read.
 @param[in/out] page_set       page_set
-@param[in]     max_gap        largest gap that may be combined across,
-                              in pages of the tablespace's physical
-                              page size
-@param[in/out] filler_pages   incremented by the unchanged pages inside
-                              every gap combined into this block
-@param[in/out] combined_gaps  incremented by the number of gaps combined
-                              into this block */
-void range_get_next_page(xb_page_set *page_set, ulint max_gap,
-                         ulint *filler_pages, ulint *combined_gaps);
+@param[in/out] ctxt           read-filter context: ctxt->merge_gap bounds
+                              the gaps merged into this block (in pages
+                              of the tablespace's physical page size);
+                              ctxt->stat_filler_pages and
+                              ctxt->stat_combined_gaps accumulate the
+                              unchanged pages inside merged gaps and the
+                              number of gaps merged */
+void range_get_next_page(xb_page_set *page_set, xb_read_filt_ctxt_t *ctxt);
 
 /** Set the backupid
 @param[in] connection  MySQL connection handler

storage/innobase/xtrabackup/src/read_filt.cc

@@ -29,6 +29,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 #include "common.h"
 #include "dict0dict.h"
 #include "fil_cur.h"
+#include "utils.h"
 #include "xb0xb.h"
 #include "xb_io_probe.h"
 #include "xtrabackup.h"
@@ -46,7 +47,7 @@ static void common_init(
   ctxt->buffer_capacity = cursor->buf_size;
   ctxt->page_size = cursor->page_size;
   ctxt->space_id = cursor->space_id;
-  ctxt->max_gap = 0;
+  ctxt->merge_gap = 0;
   ctxt->stat_batches = 0;
   ctxt->stat_total_changed_pages = 0;
   ctxt->stat_groups = 0;
@@ -125,7 +126,7 @@ static void rf_page_tracking_init(xb_read_filt_ctxt_t *ctxt,
   common_init(ctxt, cursor);
   ctxt->filter_batch_end = 0;
 
-  /* Full-scan spaces never consult max_gap; spaces without changed pages
+  /* Full-scan spaces never consult merge_gap; spaces without changed pages
   are not read at all. */
   if (space_id == dict_sys_t::s_dict_space_id ||
       full_scan_tables.find(space_id) != full_scan_tables.end() ||
@@ -140,9 +141,19 @@ static void rf_page_tracking_init(xb_read_filt_ctxt_t *ctxt,
   proportionally smaller one. In auto mode (the default) the limit is
   the read request cost - measured single-threaded at backup start, or
   the conservative fallback - converted to pages of this tablespace's
-  physical page size: combine every gap cheaper than one saved read. */
+  physical page size: combine every gap cheaper than one saved read.
+
+  The divisibility below holds for every InnoDB tablespace this filter
+  can see: the page-tracking filter is only selected for spaces in the
+  server's changed-page map (InnoDB by construction), and every valid
+  physical page size - 1K-16K compressed, up to the 64K server page
+  size uncompressed - is a power of two not larger than UNIV_PAGE_SIZE.
+  Were it ever violated in a release build, the integer arithmetic
+  degrades toward merge_gap = 0, i.e. the old strictly-consecutive reads:
+  a performance fallback, never a correctness risk (the incremental
+  write filter still gates every page by its LSN). */
   ut_ad(UNIV_PAGE_SIZE % ctxt->page_size == 0);
-  ctxt->max_gap =
+  ctxt->merge_gap =
       static_cast<ulint>(opt_page_tracking_merge_gap_auto
                              ? xb_read_request_cost / ctxt->page_size
                              : uint64_t{opt_page_tracking_merge_gap} *
@@ -266,14 +277,9 @@ static void rf_page_tracking_get_next_batch(xb_fil_cur_t *cursor,
       }
 
       ctxt->offset = next_page_id * ctxt->page_size;
-      /* Find the end of the current page tracking block */
-      {
-        ulint filler = 0, combined = 0;
-        pagetracking::range_get_next_page(space, ctxt->max_gap, &filler,
-                                          &combined);
-        ctxt->stat_filler_pages += filler;
-        ctxt->stat_combined_gaps += combined;
-      }
+      /* Find the end of the current page tracking block; the walker
+      adds the pages it merges across into ctxt's stat counters */
+      pagetracking::range_get_next_page(space, ctxt);
       ut_ad(space->current_page_it != space->pages.end());
 
       ctxt->filter_batch_end = (*space->current_page_it) + 1;
@@ -335,7 +341,7 @@ static void rf_page_tracking_deinit(xb_fil_cur_t *cursor) {
   xb::info() << std::fixed << "pagetracking: " << cursor->rel_path << ": "
              << ctxt->stat_total_changed_pages << " changed pages in " << ranges
              << " ranges (avg gap " << std::setprecision(1) << avg_gap
-             << " pages); merge-gap=" << ctxt->max_gap
+             << " pages); merge-gap=" << ctxt->merge_gap
              << (opt_page_tracking_merge_gap_auto ? " (auto)" : "")
              << " combined them into " << ctxt->stat_groups
              << " reads: request reduction " << reduction
@@ -355,10 +361,11 @@ static void rf_page_tracking_deinit(xb_fil_cur_t *cursor) {
       avg_gap_bytes <= pagetracking::READ_REQUEST_COST_MAX_BYTES) {
     xb::info() << std::fixed << std::setprecision(1)
                << "pagetracking: " << cursor->rel_path << ": typical gap "
-               << avg_gap << " pages (" << avg_gap_bytes / 1024
-               << "KB) costs more than one read request ("
-               << xb_read_request_cost / 1024
-               << "KB); reads stay individual - if sequential read "
+               << avg_gap << " pages ("
+               << xtrabackup::utils::human_readable(avg_gap_bytes)
+               << ") costs more than one read request ("
+               << xtrabackup::utils::human_readable(xb_read_request_cost)
+               << "); reads stay individual - if sequential read "
                   "throughput is high, --page-tracking-merge-gap="
                << static_cast<uint64_t>(avg_gap + 1.0) << " may be faster";
   }

storage/innobase/xtrabackup/src/read_filt.h

@@ -39,20 +39,20 @@ struct xb_read_filt_ctxt_t {
   ulint filter_batch_end;             /*!< the ending page id of the
                                       current changed page block in
                                       the page tracking */
-  ulint max_gap;                      /*!< largest changed-page gap to
-                                      combine into one read, in pages of
-                                      page_size (--page-tracking-merge-gap
-                                      scaled for compressed tablespaces,
-                                      or the read request cost in
-                                      pages) */
+  ulint merge_gap;                    /*!< largest changed-page gap to
+                                    combine into one read, in pages of
+                                    page_size (--page-tracking-merge-gap
+                                    scaled for compressed tablespaces,
+                                    or the read request cost in
+                                    pages) */
   /* Statistics accumulated while the file is read, reported by the
   filter's deinit in one log line; they never influence any decision.
 
   Shared example used in the field comments below: changed pages
 
       1,2,3,4   7   20,21          (3 runs of consecutive pages)
 
-  with max_gap = 4. A gap counts the unchanged pages BETWEEN two
+  with merge_gap = 4. A gap counts the unchanged pages BETWEEN two
   changed pages (between 4 and 7 it is 2: pages 5,6 - not the id
   difference 3). The gap of 2 (pages 5,6) is combined across, so
   [1-7] becomes one read group; the gap of 12 (pages 8..19) is not,
@@ -79,7 +79,7 @@ struct xb_read_filt_ctxt_t {
                                   would have read: here 3, so combining
                                   saved one request */
   ulint stat_combined_gaps;       /*!< gaps of unchanged pages combined
-                                  across (gap <= max_gap), each joining
+                                  across (gap <= merge_gap), each joining
                                   two ranges into one read. Example: 1
                                   (the 2-page gap between 4 and 7) */
   ulint stat_filler_pages;        /*!< unchanged pages read only as
@@ -91,7 +91,7 @@ struct xb_read_filt_ctxt_t {
                                   amplification = (total_changed +
                                   filler) / total_changed */
   ulint stat_skipped_pages;       /*!< unchanged pages in refused gaps
-                                  (gap > max_gap): never read, seeked
+                                  (gap > merge_gap): never read, seeked
                                   past. Example: 12 (pages 8..19).
                                   avg gap = (filler + skipped) /
                                   (ranges - 1) describes how scattered