Code review comment for lp:~akopytov/percona-xtrabackup/bug1095249-2.1

> 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.