Percona XtraBackup moved to https://jira.percona.com/projects/PXB

Merge lp:~akopytov/percona-xtrabackup/bug1190779-2.0 into lp:percona-xtrabackup/2.0

  1. bug1190779-2.0
  2. Merge into 2.0
Proposed by Alexey Kopytov
Status: Merged
Approved by: Alexey Kopytov
Approved revision: no longer in the source branch.
Merged at revision: 573
Proposed branch: lp:~akopytov/percona-xtrabackup/bug1190779-2.0
Merge into: lp:percona-xtrabackup/2.0
Diff against target: 196 lines (+17/-57)
1 file modified
src/xtrabackup.cc (+17/-57)
To merge this branch: bzr merge lp:~akopytov/percona-xtrabackup/bug1190779-2.0
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
George Ormond Lorch III Pending
Review via email: mp+173932@code.launchpad.net

Description of the change

    Bug #1190779: lp:1055547 repeatable on XFS

    The problem was in an length-unaligned I/O request issued while
    manipulating xtrabackup_logfile with O_DIRECT enabled.

    We don't actually need O_DIRECT in those cases, so the fix was to
    disable O_DIRECT.. The patch also removes userspace buffer alignment
    code and implements other minor cleanups.

To post a comment you must log in.
Revision history for this message
Alexey Kopytov (akopytov) wrote :

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

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

I would prefer not to see "UNIV_PAGE_SIZE_MAX * 128" through out the code. I think the actual log_buf size should be defined such that there never be a discrepancy. Just my $.02. I realize that some of the calculations done are tied to this value and as such think those should have also been defined ("if (file_size < 2*1024*1024L)" and "while (file_size < 2*1024*1024L)")

Revision history for this message
Alexey Kopytov (akopytov) wrote :

I agree, having a constant for UNIV_PAGE_SIZE_MAX would be fine. In my case I just had to replace UNIV_PAGE_SIZE with UNIV_PAGE_SIZE_MAX.

It's hard to decide where to stop when cleaning up XtraBackup code :) I preferred to stop at this point.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/xtrabackup.cc'
2--- src/xtrabackup.cc 2013-07-10 12:29:03 +0000
3+++ src/xtrabackup.cc 2013-07-10 12:47:51 +0000
4@@ -6460,8 +6460,7 @@
5 ibool success;
6
7 ulint field;
8- byte* log_buf;
9- byte* log_buf_ = NULL;
10+ byte log_buf[UNIV_PAGE_SIZE_MAX * 128]; /* 2 MB */
11
12 IB_INT64 file_size;
13
14@@ -6515,11 +6514,6 @@
15 goto error;
16 }
17
18- log_buf_ = static_cast<byte *>
19- (ut_malloc(LOG_FILE_HDR_SIZE + UNIV_PAGE_SIZE_MAX));
20- log_buf = static_cast<byte *>
21- (ut_align(log_buf_, UNIV_PAGE_SIZE_MAX));
22-
23 success = xb_os_file_read(src_file, log_buf, 0,
24 LOG_FILE_HDR_SIZE);
25 if (!success) {
26@@ -6531,9 +6525,6 @@
27 msg(" xtrabackup: 'ib_logfile0' seems to be "
28 "'xtrabackup_logfile'. will retry.\n");
29
30- ut_free(log_buf_);
31- log_buf_ = NULL;
32-
33 os_file_close(src_file);
34 src_file = XB_FILE_UNDEFINED;
35
36@@ -6549,28 +6540,16 @@
37 msg(" xtrabackup: Fatal error: cannot find %s.\n",
38 src_path);
39
40- ut_free(log_buf_);
41- log_buf_ = NULL;
42-
43 os_file_close(src_file);
44 src_file = XB_FILE_UNDEFINED;
45
46 goto error;
47 }
48
49-#ifdef USE_POSIX_FADVISE
50- posix_fadvise(src_file, 0, 0, POSIX_FADV_SEQUENTIAL);
51- posix_fadvise(src_file, 0, 0, POSIX_FADV_DONTNEED);
52-#endif
53-
54- xb_file_set_nocache(src_file, src_path, "OPEN");
55-
56 file_size = os_file_get_size(src_file);
57
58
59 /* TODO: We should skip the following modifies, if it is not the first time. */
60- log_buf_ = static_cast<byte *>(ut_malloc(UNIV_PAGE_SIZE * 129));
61- log_buf = static_cast<byte *>(ut_align(log_buf_, UNIV_PAGE_SIZE));
62
63 /* read log file header */
64 success = xb_os_file_read(src_file, log_buf, 0, LOG_FILE_HDR_SIZE);
65@@ -6665,15 +6644,15 @@
66 goto error;
67 }
68
69- /* expand file size (9/8) and align to UNIV_PAGE_SIZE */
70+ /* expand file size (9/8) and align to UNIV_PAGE_SIZE_MAX */
71
72- if (file_size % UNIV_PAGE_SIZE) {
73- memset(log_buf, 0, UNIV_PAGE_SIZE);
74+ if (file_size % UNIV_PAGE_SIZE_MAX) {
75+ memset(log_buf, 0, UNIV_PAGE_SIZE_MAX);
76 success = xb_os_file_write(src_path, src_file, log_buf,
77 file_size,
78- UNIV_PAGE_SIZE
79+ UNIV_PAGE_SIZE_MAX
80 - (ulint) (file_size
81- % UNIV_PAGE_SIZE));
82+ % UNIV_PAGE_SIZE_MAX));
83 if (!success) {
84 goto error;
85 }
86@@ -6685,40 +6664,41 @@
87 {
88 ulint expand;
89
90- memset(log_buf, 0, UNIV_PAGE_SIZE * 128);
91- expand = (ulint) (file_size / UNIV_PAGE_SIZE / 8);
92+ memset(log_buf, 0, UNIV_PAGE_SIZE_MAX * 128);
93+ expand = (ulint) (file_size / UNIV_PAGE_SIZE_MAX / 8);
94
95 for (; expand > 128; expand -= 128) {
96 success = xb_os_file_write(src_path, src_file, log_buf,
97 file_size,
98- UNIV_PAGE_SIZE * 128);
99+ UNIV_PAGE_SIZE_MAX * 128);
100 if (!success) {
101 goto error;
102 }
103- file_size += UNIV_PAGE_SIZE * 128;
104+ file_size += UNIV_PAGE_SIZE_MAX * 128;
105 }
106
107 if (expand) {
108 success = xb_os_file_write(src_path, src_file, log_buf,
109 file_size,
110- expand * UNIV_PAGE_SIZE);
111+ expand * UNIV_PAGE_SIZE_MAX);
112 if (!success) {
113 goto error;
114 }
115- file_size += UNIV_PAGE_SIZE * expand;
116+ file_size += UNIV_PAGE_SIZE_MAX * expand;
117 }
118 }
119
120 /* make larger than 2MB */
121 if (file_size < 2*1024*1024L) {
122- memset(log_buf, 0, UNIV_PAGE_SIZE);
123+ memset(log_buf, 0, UNIV_PAGE_SIZE_MAX);
124 while (file_size < 2*1024*1024L) {
125 success = xb_os_file_write(src_path, src_file, log_buf,
126- file_size, UNIV_PAGE_SIZE);
127+ file_size,
128+ UNIV_PAGE_SIZE_MAX);
129 if (!success) {
130 goto error;
131 }
132- file_size += UNIV_PAGE_SIZE;
133+ file_size += UNIV_PAGE_SIZE_MAX;
134 }
135 file_size = os_file_get_size(src_file);
136 }
137@@ -6753,21 +6733,16 @@
138 }
139 xtrabackup_logfile_is_renamed = TRUE;
140
141- ut_free(log_buf_);
142-
143 return(FALSE);
144
145 skip_modify:
146 os_file_close(src_file);
147 src_file = XB_FILE_UNDEFINED;
148- ut_free(log_buf_);
149 return(FALSE);
150
151 error:
152 if (src_file != XB_FILE_UNDEFINED)
153 os_file_close(src_file);
154- if (log_buf_)
155- ut_free(log_buf_);
156 msg("xtrabackup: Error: xtrabackup_init_temp_log() failed.\n");
157 return(TRUE); /*ERROR*/
158 }
159@@ -7472,10 +7447,7 @@
160 char src_path[FN_REFLEN];
161 char dst_path[FN_REFLEN];
162 ibool success;
163-
164- byte* log_buf;
165- byte* log_buf_ = NULL;
166-
167+ byte log_buf[UNIV_PAGE_SIZE_MAX];
168
169 if (!xtrabackup_logfile_is_renamed)
170 return(FALSE);
171@@ -7516,16 +7488,6 @@
172 goto error;
173 }
174
175-#ifdef USE_POSIX_FADVISE
176- posix_fadvise(src_file, 0, 0, POSIX_FADV_DONTNEED);
177-#endif
178-
179- xb_file_set_nocache(src_file, src_path, "OPEN");
180-
181- log_buf_ = static_cast<byte *>(ut_malloc(LOG_FILE_HDR_SIZE +
182- UNIV_PAGE_SIZE_MAX));
183- log_buf = static_cast<byte *>(ut_align(log_buf_, UNIV_PAGE_SIZE_MAX));
184-
185 success = xb_os_file_read(src_file, log_buf, 0, LOG_FILE_HDR_SIZE);
186 if (!success) {
187 goto error;
188@@ -7546,8 +7508,6 @@
189 error:
190 if (src_file != XB_FILE_UNDEFINED)
191 os_file_close(src_file);
192- if (log_buf_)
193- ut_free(log_buf_);
194 msg("xtrabackup: Error: xtrabackup_close_temp_log() failed.\n");
195 return(TRUE); /*ERROR*/
196 }

Subscribers

People subscribed via source and target branches