Merge lp:~akopytov/percona-xtrabackup/bug1095249-2.1 into lp:percona-xtrabackup/2.1

Proposed by Alexey Kopytov on 2013-01-03
Status: Merged
Approved by: Laurynas Biveinis on 2013-01-06
Approved revision: no longer in the source branch.
Merged at revision: 473
Proposed branch: lp:~akopytov/percona-xtrabackup/bug1095249-2.1
Merge into: lp:percona-xtrabackup/2.1
Diff against target: 395 lines (+272/-6)
8 files modified
src/Makefile (+2/-1)
src/datasink.c (+4/-0)
src/datasink.h (+2/-1)
src/ds_buffer.c (+186/-0)
src/ds_buffer.h (+31/-0)
src/ds_compress.c (+0/-2)
src/xtrabackup.c (+15/-2)
test/t/ib_compress_basic.sh (+32/-0)
To merge this branch: bzr merge lp:~akopytov/percona-xtrabackup/bug1095249-2.1
Reviewer Review Type Date Requested Status
Laurynas Biveinis Approve on 2013-01-06
George Ormond Lorch III g2 2013-01-03 Approve on 2013-01-03
Review via email: mp+141770@code.launchpad.net
To post a comment you must log in.
George Ormond Lorch III (gl-az) wrote :

Looks good except for tw things:
1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
2 - there is no call to ds_buffer_set_size to set the buffering size to 1M from the default 64K...does it make sense to make this a program option?

This brings up two issue in/with the 2.1 encryption work, small writes from compress->encrypt (encrypting extremely small buffers) and inefficient i/o. Also IIRC the encryption work implemented similar test case for basic encryption as I found it was missing. Should probably echo these comments on that MP and fix topology creation and destruction accordingly. This could be our first case of using multiple instances of a single data sink if we choose to go that route:
ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local

review: Approve (g2)
Alexey Kopytov (akopytov) wrote :

Hi George,

On Thu, 03 Jan 2013 17:47:22 -0000, George Ormond Lorch III wrote:
> Review: Approve g2
>
> Looks good except for tw things:
> 1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
> 2 - there is no call to ds_buffer_set_size to set the buffering size to 1M from the default 64K...does it make sense to make this a program option?
>

Good catch. Removed redundant includes and added a ds_buffer_set_size()
call.

Regarding making it a program option. We have
https://blueprints.launchpad.net/percona-xtrabackup/+spec/specify-io-block-size
which can make both read and write buffers configurable. So far they are
both hardcoded to 1 MB.

> This brings up two issue in/with the 2.1 encryption work, small writes from compress->encrypt (encrypting extremely small buffers) and inefficient i/o. Also IIRC the encryption work implemented similar test case for basic encryption as I found it was missing. Should probably echo these comments on that MP and fix topology creation and destruction accordingly. This could be our first case of using multiple instances of a single data sink if we choose to go that route:
> ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local
>

Yes, I see encryption also writes metadata in a separate write call, so
it will still be affected by the same problem even when compression will
use buffering (or no compression is used).

Thanks!

Alexey Kopytov (akopytov) wrote :

No idea why Launchpad decided to change the status to Merged. Reverting.

George Ormond Lorch III (gl-az) wrote :

> Hi George,
>
> On Thu, 03 Jan 2013 17:47:22 -0000, George Ormond Lorch III wrote:
> > Review: Approve g2
> >
> > Looks good except for tw things:
> > 1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really
> necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
> > 2 - there is no call to ds_buffer_set_size to set the buffering size to 1M
> from the default 64K...does it make sense to make this a program option?
> >
>
> Good catch. Removed redundant includes and added a ds_buffer_set_size()
> call.
>
> Regarding making it a program option. We have
> https://blueprints.launchpad.net/percona-xtrabackup/+spec/specify-io-block-
> size
> which can make both read and write buffers configurable. So far they are
> both hardcoded to 1 MB.
>
> > This brings up two issue in/with the 2.1 encryption work, small writes from
> compress->encrypt (encrypting extremely small buffers) and inefficient i/o.
> Also IIRC the encryption work implemented similar test case for basic
> encryption as I found it was missing. Should probably echo these comments on
> that MP and fix topology creation and destruction accordingly. This could be
> our first case of using multiple instances of a single data sink if we choose
> to go that route:
> > ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local
> >
>
> Yes, I see encryption also writes metadata in a separate write call, so
> it will still be affected by the same problem even when compression will
> use buffering (or no compression is used).
>
> Thanks!

Also FYI in case you really haven't had a chance to review much of the encryption work yet, I went ahead and exposed both compress and encrypt block sizes as program options which allow tuning of the individual compress and encrypt worker thread block sizes. In my testing these can actually have a pretty decent impact on overall backup speed when set properly (and the tuning ratios are not entirely intuitive). This is what lead me to propose the PLMCE 2013 tutorial on tuning XB options for best results. With this new i/o buffer, it may make the current results much more interesting. Once this change is merged into 2.1, I will rebase the encryption branch and also look into implementing the i/o block size program options ahead of it which should be simple enough. It would be nice to have all of these in the trunk by the time for PLMCE 2013 and included into my talk.

Alexey Kopytov (akopytov) wrote :

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/135/ (though compression tests are not currently executed by Jenkins anyway).

review: Approve
467. By Alexey Kopytov on 2013-01-07

2.1 version of the fix for

Bug #1095249: Built-in compression does inefficient I/O

(a more clean implementation, since 2.1 provides native datasink chaining)

The problem was the xtrabackup --compress did unbuffered writes to the
destination file or stream in very small chunks.

Fixed by:

- backporting datasink_buffer from the compact backups branch
- inserting it between datasink_compress and the destination datasink
- using a 1M buffer for output similar to the uncompressed backup

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Makefile'
2--- src/Makefile 2012-05-25 11:38:15 +0000
3+++ src/Makefile 2013-01-07 06:21:20 +0000
4@@ -28,6 +28,7 @@
5 ds_local.o \
6 ds_compress.o \
7 ds_tmpfile.o \
8+ ds_buffer.o \
9 datasink.o \
10 write_filt.o \
11 fil_cur.o \
12@@ -185,7 +186,7 @@
13 xbstream.o xbstream_read.o: %.o: %.c
14 $(CC) $(CFLAGS) $(INC) $(DEFS) -c $< -o $@
15
16-xbstream: $(XBSTREAMOBJS) $(MYSQLOBJS) ds_local.o datasink.o
17+xbstream: $(XBSTREAMOBJS) $(MYSQLOBJS) ds_local.o ds_buffer.o datasink.o
18 $(CC) $(CFLAGS) $^ $(INC) $(MYSQLOBJS) $(LIBS) -o $@
19
20 xtrabackup.o: xtrabackup.c xb_regex.h write_filt.h fil_cur.h xtrabackup.h
21
22=== modified file 'src/datasink.c'
23--- src/datasink.c 2012-02-16 18:02:07 +0000
24+++ src/datasink.c 2013-01-07 06:21:20 +0000
25@@ -25,6 +25,7 @@
26 #include "ds_stream.h"
27 #include "ds_local.h"
28 #include "ds_tmpfile.h"
29+#include "ds_buffer.h"
30
31 /************************************************************************
32 Create a datasink of the specified type */
33@@ -47,6 +48,9 @@
34 case DS_TYPE_TMPFILE:
35 ds = &datasink_tmpfile;
36 break;
37+ case DS_TYPE_BUFFER:
38+ ds = &datasink_buffer;
39+ break;
40 default:
41 msg("Unknown datasink type: %d\n", type);
42 return NULL;
43
44=== modified file 'src/datasink.h'
45--- src/datasink.h 2012-02-16 18:02:07 +0000
46+++ src/datasink.h 2013-01-07 06:21:20 +0000
47@@ -53,7 +53,8 @@
48 DS_TYPE_LOCAL,
49 DS_TYPE_STREAM,
50 DS_TYPE_COMPRESS,
51- DS_TYPE_TMPFILE
52+ DS_TYPE_TMPFILE,
53+ DS_TYPE_BUFFER
54 } ds_type_t;
55
56 /************************************************************************
57
58=== added file 'src/ds_buffer.c'
59--- src/ds_buffer.c 1970-01-01 00:00:00 +0000
60+++ src/ds_buffer.c 2013-01-07 06:21:20 +0000
61@@ -0,0 +1,186 @@
62+/******************************************************
63+Copyright (c) 2012 Percona Ireland Ltd.
64+
65+buffer datasink for XtraBackup.
66+
67+This program is free software; you can redistribute it and/or modify
68+it under the terms of the GNU General Public License as published by
69+the Free Software Foundation; version 2 of the License.
70+
71+This program is distributed in the hope that it will be useful,
72+but WITHOUT ANY WARRANTY; without even the implied warranty of
73+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
74+GNU General Public License for more details.
75+
76+You should have received a copy of the GNU General Public License
77+along with this program; if not, write to the Free Software
78+Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
79+
80+*******************************************************/
81+
82+/* Does buffered output to a destination datasink set with ds_set_pipe().
83+Writes to the destination datasink are guaranteed to not be smaller than a
84+specified buffer size (DS_DEFAULT_BUFFER_SIZE by default), with the only
85+exception for the last write for a file. */
86+
87+#include <my_base.h>
88+#include "common.h"
89+#include "datasink.h"
90+
91+#define DS_DEFAULT_BUFFER_SIZE (64 * 1024)
92+
93+typedef struct {
94+ ds_file_t *dst_file;
95+ char *buf;
96+ size_t pos;
97+ size_t size;
98+} ds_buffer_file_t;
99+
100+typedef struct {
101+ size_t buffer_size;
102+} ds_buffer_ctxt_t;
103+
104+static ds_ctxt_t *buffer_init(const char *root);
105+static ds_file_t *buffer_open(ds_ctxt_t *ctxt, const char *path,
106+ MY_STAT *mystat);
107+static int buffer_write(ds_file_t *file, const void *buf, size_t len);
108+static int buffer_close(ds_file_t *file);
109+static void buffer_deinit(ds_ctxt_t *ctxt);
110+
111+datasink_t datasink_buffer = {
112+ &buffer_init,
113+ &buffer_open,
114+ &buffer_write,
115+ &buffer_close,
116+ &buffer_deinit
117+};
118+
119+/* Change the default buffer size */
120+void ds_buffer_set_size(ds_ctxt_t *ctxt, size_t size)
121+{
122+ ds_buffer_ctxt_t *buffer_ctxt = (ds_buffer_ctxt_t *) ctxt->ptr;
123+
124+ buffer_ctxt->buffer_size = size;
125+}
126+
127+static ds_ctxt_t *
128+buffer_init(const char *root)
129+{
130+ ds_ctxt_t *ctxt;
131+ ds_buffer_ctxt_t *buffer_ctxt;
132+
133+ ctxt = my_malloc(sizeof(ds_ctxt_t) + sizeof(ds_buffer_ctxt_t),
134+ MYF(MY_FAE));
135+ buffer_ctxt = (ds_buffer_ctxt_t *) (ctxt + 1);
136+ buffer_ctxt->buffer_size = DS_DEFAULT_BUFFER_SIZE;
137+
138+ ctxt->ptr = buffer_ctxt;
139+ ctxt->root = my_strdup(root, MYF(MY_FAE));
140+
141+ return ctxt;
142+}
143+
144+static ds_file_t *
145+buffer_open(ds_ctxt_t *ctxt, const char *path, MY_STAT *mystat)
146+{
147+ ds_buffer_ctxt_t *buffer_ctxt;
148+ ds_ctxt_t *pipe_ctxt;
149+ ds_file_t *dst_file;
150+ ds_file_t *file;
151+ ds_buffer_file_t *buffer_file;
152+
153+ pipe_ctxt = ctxt->pipe_ctxt;
154+ xb_a(pipe_ctxt != NULL);
155+
156+ dst_file = ds_open(pipe_ctxt, path, mystat);
157+ if (dst_file == NULL) {
158+ exit(EXIT_FAILURE);
159+ }
160+
161+ buffer_ctxt = (ds_buffer_ctxt_t *) ctxt->ptr;
162+
163+ file = (ds_file_t *) my_malloc(sizeof(ds_file_t) +
164+ sizeof(ds_buffer_file_t) +
165+ buffer_ctxt->buffer_size,
166+ MYF(MY_FAE));
167+
168+ buffer_file = (ds_buffer_file_t *) (file + 1);
169+ buffer_file->dst_file = dst_file;
170+ buffer_file->buf = (char *) (buffer_file + 1);
171+ buffer_file->size = buffer_ctxt->buffer_size;
172+ buffer_file->pos = 0;
173+
174+ file->path = dst_file->path;
175+ file->ptr = buffer_file;
176+
177+ return file;
178+}
179+
180+static int
181+buffer_write(ds_file_t *file, const void *buf, size_t len)
182+{
183+ ds_buffer_file_t *buffer_file;
184+
185+ buffer_file = (ds_buffer_file_t *) file->ptr;
186+
187+ while (len > 0) {
188+ if (buffer_file->pos + len > buffer_file->size) {
189+ if (buffer_file->pos > 0) {
190+ size_t bytes;
191+
192+ bytes = buffer_file->size - buffer_file->pos;
193+ memcpy(buffer_file->buf + buffer_file->pos, buf,
194+ bytes);
195+
196+ if (ds_write(buffer_file->dst_file,
197+ buffer_file->buf,
198+ buffer_file->size)) {
199+ return 1;
200+ }
201+
202+ buffer_file->pos = 0;
203+
204+ buf = (const char *) buf + bytes;
205+ len -= bytes;
206+ } else {
207+ /* We don't have any buffered bytes, just write
208+ the entire source buffer */
209+ if (ds_write(buffer_file->dst_file, buf, len)) {
210+ return 1;
211+ }
212+ break;
213+ }
214+ } else {
215+ memcpy(buffer_file->buf + buffer_file->pos, buf, len);
216+ buffer_file->pos += len;
217+ break;
218+ }
219+ }
220+
221+ return 0;
222+}
223+
224+static int
225+buffer_close(ds_file_t *file)
226+{
227+ ds_buffer_file_t *buffer_file;
228+
229+ buffer_file = (ds_buffer_file_t *) file->ptr;
230+ if (buffer_file->pos > 0) {
231+ ds_write(buffer_file->dst_file, buffer_file->buf,
232+ buffer_file->pos);
233+ }
234+
235+ ds_close(buffer_file->dst_file);
236+
237+ MY_FREE(file);
238+
239+ return 0;
240+}
241+
242+static void
243+buffer_deinit(ds_ctxt_t *ctxt)
244+{
245+ MY_FREE(ctxt->root);
246+ MY_FREE(ctxt);
247+}
248
249=== added file 'src/ds_buffer.h'
250--- src/ds_buffer.h 1970-01-01 00:00:00 +0000
251+++ src/ds_buffer.h 2013-01-07 06:21:20 +0000
252@@ -0,0 +1,31 @@
253+/******************************************************
254+Copyright (c) 2012 Percona Ireland Ltd.
255+
256+buffer datasink for XtraBackup.
257+
258+This program is free software; you can redistribute it and/or modify
259+it under the terms of the GNU General Public License as published by
260+the Free Software Foundation; version 2 of the License.
261+
262+This program is distributed in the hope that it will be useful,
263+but WITHOUT ANY WARRANTY; without even the implied warranty of
264+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
265+GNU General Public License for more details.
266+
267+You should have received a copy of the GNU General Public License
268+along with this program; if not, write to the Free Software
269+Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
270+
271+*******************************************************/
272+
273+#ifndef DS_BUFFER_H
274+#define DS_BUFFER_H
275+
276+#include "datasink.h"
277+
278+extern datasink_t datasink_buffer;
279+
280+/* Change the default buffer size */
281+void ds_buffer_set_size(ds_ctxt_t *ctxt, size_t size);
282+
283+#endif
284
285=== modified file 'src/ds_compress.c'
286--- src/ds_compress.c 2012-06-14 14:48:55 +0000
287+++ src/ds_compress.c 2013-01-07 06:21:20 +0000
288@@ -25,8 +25,6 @@
289 #include <zlib.h>
290 #include "common.h"
291 #include "datasink.h"
292-#include "ds_stream.h"
293-#include "ds_local.h"
294
295 #define COMPRESS_CHUNK_SIZE (64 * 1024UL)
296 #define MY_QLZ_COMPRESS_OVERHEAD 400
297
298=== modified file 'src/xtrabackup.c'
299--- src/xtrabackup.c 2012-11-21 13:51:49 +0000
300+++ src/xtrabackup.c 2013-01-07 06:21:20 +0000
301@@ -87,6 +87,7 @@
302 #include "fil_cur.h"
303 #include "write_filt.h"
304 #include "xtrabackup.h"
305+#include "ds_buffer.h"
306
307 my_bool innodb_inited= 0;
308
309@@ -261,6 +262,7 @@
310 static ds_ctxt_t *ds_compress = NULL;
311 static ds_ctxt_t *ds_tmpfile = NULL;
312 static ds_ctxt_t *ds_stream = NULL;
313+static ds_ctxt_t *ds_buffer = NULL;
314
315 /* ======== Datafiles iterator ======== */
316 typedef struct {
317@@ -2124,21 +2126,28 @@
318 if (xtrabackup_compress) {
319 ds_compress = ds_create(xtrabackup_target_dir,
320 DS_TYPE_COMPRESS);
321+ /* Use a 1 MB buffer for compressed output stream */
322+ ds_buffer = ds_create(xtrabackup_target_dir,
323+ DS_TYPE_BUFFER);
324+ ds_buffer_set_size(ds_buffer, 1024 * 1024);
325+
326 ds_data = ds_compress;
327
328 if (xtrabackup_stream) {
329 /* Streaming compressed backup */
330- ds_set_pipe(ds_compress, ds_stream);
331+ ds_set_pipe(ds_buffer, ds_stream);
332 /* Bypass compression for meta files */
333 ds_meta = ds_stream;
334 } else {
335 /* Local compressed backup */
336 ds_local = ds_create(xtrabackup_target_dir,
337 DS_TYPE_LOCAL);
338- ds_set_pipe(ds_compress, ds_local);
339+ ds_set_pipe(ds_buffer, ds_local);
340 /* Bypass compression for meta files */
341 ds_meta = ds_local;
342 }
343+
344+ ds_set_pipe(ds_compress, ds_buffer);
345 } else {
346 /* Streaming uncompressed backup */
347 ds_data = ds_stream;
348@@ -2177,6 +2186,10 @@
349 ds_destroy(ds_compress);
350 ds_compress = NULL;
351 }
352+ if (ds_buffer != NULL) {
353+ ds_destroy(ds_buffer);
354+ ds_buffer = NULL;
355+ }
356 if (ds_stream != NULL) {
357 ds_destroy(ds_stream);
358 ds_stream = NULL;
359
360=== added file 'test/t/ib_compress_basic.sh'
361--- test/t/ib_compress_basic.sh 1970-01-01 00:00:00 +0000
362+++ test/t/ib_compress_basic.sh 2013-01-07 06:21:20 +0000
363@@ -0,0 +1,32 @@
364+########################################################################
365+# Basic test for local compressed backups
366+########################################################################
367+
368+. inc/common.sh
369+
370+if ! which qpress > /dev/null 2>&1 ; then
371+ echo "Requires qpress to be installed" > $SKIPPED_REASON
372+ exit $SKIPPED_EXIT_CODE
373+fi
374+
375+start_server --innodb_file_per_table
376+load_sakila
377+
378+innobackupex --compress --no-timestamp $topdir/backup
379+
380+stop_server
381+rm -rf ${MYSQLD_DATADIR}/*
382+
383+cd $topdir/backup
384+
385+for i in *.qp; do qpress -d $i ./; done; \
386+for i in sakila/*.qp; do qpress -d $i sakila/; done
387+
388+innobackupex --apply-log $topdir/backup
389+
390+innobackupex --copy-back $topdir/backup
391+
392+start_server
393+
394+# TODO: proper validation
395+run_cmd ${MYSQL} ${MYSQL_ARGS} -e "SELECT count(*) from actor" sakila

Subscribers

People subscribed via source and target branches

to all changes: