On Wed, Jun 08, 2011 at 06:50:25PM +0400, Dmitry Konishchev wrote:
> This patch optimizes 'qemu-img convert' operation for volumes which are
> almost fully unallocated. Here are the results of simple tests:
The optimization is to check allocation metadata instead of
unconditionally reading and then checking for all zeroes?
> diff --git a/qemu-img.c b/qemu-img.c
> index 4f162d1..9d905ed 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -38,6 +38,8 @@ typedef struct img_cmd_t {
> int (*handler)(int argc, char **argv);
> } img_cmd_t;
>
> +static const int SECTOR_SIZE = 512;
Why introduce a new constant instead of using BDRV_SECTOR_SIZE?
> +
> /* Default to cache=writeback as data integrity is not important for
> qemu-tcg. */
> #define BDRV_O_FLAGS BDRV_O_CACHE_WB
>
> @@ -531,7 +533,7 @@ static int is_not_zero(const uint8_t *sector, int len)
> }
>
> /*
> - * Returns true iff the first sector pointed to by 'buf' contains at least
> + * Returns true if the first sector pointed to by 'buf' contains at least
"iff" is not a typo. It means "if and only if".
> @@ -912,55 +944,109 @@ static int img_convert(int argc, char **argv)
> are present in both the output's and input's base images
> (no
> need to copy them). */
> if (out_baseimg) {
> - if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> - n, &n1)) {
> - sector_num += n1;
> + if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
> n, &cur_n)) {
> + sector_num += cur_n;
> continue;
> }
> - /* The next 'n1' sectors are allocated in the input
> image. Copy
> + /* The next 'cur_n' sectors are allocated in the input
> image. Copy
> only those as they may be followed by unallocated
> sectors. */
> - n = n1;
> + n = cur_n;
> }
> - } else {
> - n1 = n;
> }
>
> - ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
> - if (ret < 0) {
> - error_report("error while reading");
> - goto out;
> - }
> - /* NOTE: at the same time we convert, we do not write zero
> - sectors to have a chance to compress the image. Ideally, we
> - should add a specific call to have the info to go faster */
> - buf1 = buf;
> - while (n > 0) {
> - /* If the output image is being created as a copy on write
> image,
> - copy all sectors even the ones containing only NUL bytes,
> - because they may differ from the sectors in the base
> image.
> -
> - If the output is to a host device, we also write out
> - sectors that are entirely 0, since whatever data was
> - already there is garbage, not 0s. */
> - if (!has_zero_init || out_baseimg ||
> - is_allocated_sectors(buf1, n, &n1)) {
> - ret = bdrv_write(out_bs, sector_num, buf1, n1);
> - if (ret < 0) {
> - error_report("error while writing");
> - goto out;
> + /* If the output image is being created as a copy on write image,
> + copy all sectors even the ones containing only zero bytes,
> + because they may differ from the sectors in the base image.
> +
> + If the output is to a host device, we also write out
> + sectors that are entirely 0, since whatever data was
> + already there is garbage, not 0s. */
> + if (!has_zero_init || out_baseimg) {
> + ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n);
> + if (ret < 0) {
> + error_report("error while reading");
> + goto out;
> + }
> +
> + ret = bdrv_write(out_bs, sector_num, buf, n);
> + if (ret < 0) {
> + error_report("error while writing");
> + goto out;
> + }
> +
> + sector_num += n;
> + } else {
> + /* Look for the sectors in the image and if they are not
> + allocated - sequentially in all its backing images.
> +
> + Write only non-zero bytes to the output image. */
I think the recursive is_allocated() needs its own function. This
function is already long/complex enough :).
Stefan
