Re: [Qemu-devel] [PATCH 2/3] cow: stop using mmap

[Kevin Wolf]

Am 07.05.2010 17:17, schrieb Christoph Hellwig:
> We don't have an equivalent to mmap in the qemu block API, so read and
> write the bitmap directly.  At least in the dumb implementation added
> in this patch this is a lot less efficient, but it means cow can also
> work on windows, and over nbd or curl.  And it fixes qemu-iotests testcase
> 012 which did not work properly due to issues with read-only mmap access.
> 
> In addition we can also get rid of the now unused get_mmap_addr function.
> 
> Signed-off-by: Christoph Hellwig <address@hidden>

Unfortunately it's so slow that I had to stop qemu-iotests because it
wouldn't complete in finite time.

More importantly, without changes this patch doesn't even compile.

> Index: qemu/block/cow.c
> ===================================================================
> --- qemu.orig/block/cow.c     2010-05-07 16:58:13.614003848 +0200
> +++ qemu/block/cow.c  2010-05-07 17:07:35.326034649 +0200
> @@ -21,11 +21,9 @@
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
>   */
> -#ifndef _WIN32
>  #include "qemu-common.h"
>  #include "block_int.h"
>  #include "module.h"
> -#include <sys/mman.h>
>  
>  /**************************************************************/
>  /* COW block driver using file system holes */
> @@ -45,9 +43,6 @@ struct cow_header_v2 {
>  
>  typedef struct BDRVCowState {
>      int fd;
> -    uint8_t *cow_bitmap; /* if non NULL, COW mappings are used first */
> -    uint8_t *cow_bitmap_addr; /* mmap address of cow_bitmap */
> -    int cow_bitmap_size;
>      int64_t cow_sectors_offset;
>  } BDRVCowState;
>  
> @@ -68,6 +63,7 @@ static int cow_open(BlockDriverState *bs
>      BDRVCowState *s = bs->opaque;
>      int fd;
>      struct cow_header_v2 cow_header;
> +    int bitmap_size;
>      int64_t size;
>  
>      fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
> @@ -94,61 +90,92 @@ static int cow_open(BlockDriverState *bs
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file),
>              cow_header.backing_file);
>  
> -    /* mmap the bitmap */
> -    s->cow_bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
> -    s->cow_bitmap_addr = (void *)mmap(get_mmap_addr(s->cow_bitmap_size),
> -                                      s->cow_bitmap_size,
> -                                      PROT_READ | PROT_WRITE,
> -                                      MAP_SHARED, s->fd, 0);
> -    if (s->cow_bitmap_addr == MAP_FAILED)
> -        goto fail;
> -    s->cow_bitmap = s->cow_bitmap_addr + sizeof(cow_header);
> -    s->cow_sectors_offset = (s->cow_bitmap_size + 511) & ~511;
> +    bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
> +    s->cow_sectors_offset = (bitmap_size + 511) & ~511;
>      return 0;
>   fail:
>      close(fd);
>      return -1;
>  }
>  
> -static inline void cow_set_bit(uint8_t *bitmap, int64_t bitnum)
> +/*
> + * XXX(hch): right now these functions are extremly ineffcient.
> + * We should just read the whole bitmap we'll need in one go instead.
> + */
> +static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
>  {
> -    bitmap[bitnum / 8] |= (1 << (bitnum%8));
> +    BDRVCowState *s = bs->opaque;
> +    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
> +    uint8_t bitmap;
> +
> +    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
> +         sizeof(bitmap)) {

Whitespace error (line contains a tab). Repeated twice below.

> +       return -errno;
> +    }
> +
> +    bitmap |= (1 << (bitnum % 8));
> +
> +    if (pwrite(s->fd, &bitmap, sizeof(bitmap), offset) !=
> +         sizeof(bitmap)) {
> +       return -errno;
> +    }
> +    return 0;
>  }
>  
> -static inline int is_bit_set(const uint8_t *bitmap, int64_t bitnum)
> +static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
>  {
> -    return !!(bitmap[bitnum / 8] & (1 << (bitnum%8)));
> -}
> +    BDRVCowState *s = bs->opaque;
> +    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
> +    uint8_t bitmap;
> +
> +    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
> +         sizeof(bitmap)) {
> +       return -errno;
> +    }
>  
> +    return !!(bitmap & (1 << (bitnum % 8)));
> +}
>  
>  /* Return true if first block has been changed (ie. current version is
>   * in COW file).  Set the number of continuous blocks for which that
>   * is true. */
> -static inline int is_changed(uint8_t *bitmap,
> -                             int64_t sector_num, int nb_sectors,
> -                             int *num_same)
> +static int cow_is_allocated(BlockDriverState *bs, int64_t sector_num,
> +        int nb_sectors, int *num_same)
>  {
>      int changed;
>  
> -    if (!bitmap || nb_sectors == 0) {
> +    if (nb_sectors == 0) {
>       *num_same = nb_sectors;
>       return 0;
>      }
>  
> -    changed = is_bit_set(bitmap, sector_num);
> +    changed = is_bit_set(bs, sector_num);
> +    if (changed < 0) {
> +        return 0; /* XXX: how to return I/O errors? */
> +    }
> +
>      for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
> -     if (is_bit_set(bitmap, sector_num + *num_same) != changed)
> +     if (is_bit_set(bs, sector_num + *num_same) != changed)
>           break;
>      }
>  
>      return changed;
>  }
>  
> -static int cow_is_allocated(BlockDriverState *bs, int64_t sector_num,
> -                            int nb_sectors, int *pnum)
> +static void cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
> +        int nb_sectors)
>  {
> -    BDRVCowState *s = bs->opaque;
> -    return is_changed(s->cow_bitmap, sector_num, nb_sectors, pnum);
> +    int error = 0;
> +    int i;
> +
> +    for (i = 0; i < nb_sectors; i++) {
> +        error = cow_set_bit(bs, sector_num + i);
> +        if (error) {
> +            break;
> +        }
> +    }
> +
> +    return errror;

For one, there is a typo in the variable name, and also this is a void
function which shouldn't return anything. The caller still seems to use
the return value. Should probably stay an int function.

Kevin