Patchwork [2/3] cow: stop using mmap

login
register
mail settings
Submitter Christoph Hellwig
Date May 7, 2010, 3:17 p.m.
Message ID <20100507151719.GB15249@lst.de>
Download mbox | patch
Permalink /patch/51952/
State New
Headers show

Comments

Christoph Hellwig - May 7, 2010, 3:17 p.m.
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 <hch@lst.de>
Kevin Wolf - May 10, 2010, 9:41 a.m.
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 <hch@lst.de>

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
Christoph Hellwig - May 10, 2010, 10:46 a.m.
On Mon, May 10, 2010 at 11:41:15AM +0200, Kevin Wolf wrote:
> 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.

Yes, this was missing a quilt refresh after fixing that and running
QA..

Patch

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)) {
+       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;
 }
 
 static int cow_read(BlockDriverState *bs, int64_t sector_num,
@@ -158,7 +185,7 @@  static int cow_read(BlockDriverState *bs
     int ret, n;
 
     while (nb_sectors > 0) {
-        if (is_changed(s->cow_bitmap, sector_num, nb_sectors, &n)) {
+        if (cow_is_allocated(bs, sector_num, nb_sectors, &n)) {
             ret = pread(s->fd, buf, n * 512,
                         s->cow_sectors_offset + sector_num * 512);
             if (ret != n * 512)
@@ -184,21 +211,19 @@  static int cow_write(BlockDriverState *b
                      const uint8_t *buf, int nb_sectors)
 {
     BDRVCowState *s = bs->opaque;
-    int ret, i;
+    int ret;
 
     ret = pwrite(s->fd, buf, nb_sectors * 512,
                  s->cow_sectors_offset + sector_num * 512);
     if (ret != nb_sectors * 512)
         return -1;
-    for (i = 0; i < nb_sectors; i++)
-        cow_set_bit(s->cow_bitmap, sector_num + i);
-    return 0;
+
+    return cow_update_bitmap(bs, sector_num, nb_sectors);
 }
 
 static void cow_close(BlockDriverState *bs)
 {
     BDRVCowState *s = bs->opaque;
-    munmap((void *)s->cow_bitmap_addr, s->cow_bitmap_size);
     close(s->fd);
 }
 
@@ -308,4 +333,3 @@  static void bdrv_cow_init(void)
 }
 
 block_init(bdrv_cow_init);
-#endif
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h	2010-05-06 21:49:22.167011390 +0200
+++ qemu/qemu-common.h	2010-05-07 16:58:54.894013975 +0200
@@ -163,9 +163,6 @@  void qemu_free(void *ptr);
 char *qemu_strdup(const char *str);
 char *qemu_strndup(const char *str, size_t size);
 
-void *get_mmap_addr(unsigned long size);
-
-
 void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);
 
Index: qemu/qemu-malloc.c
===================================================================
--- qemu.orig/qemu-malloc.c	2010-05-06 21:49:22.174010343 +0200
+++ qemu/qemu-malloc.c	2010-05-07 16:58:54.900253673 +0200
@@ -32,11 +32,6 @@  static void *oom_check(void *ptr)
     return ptr;
 }
 
-void *get_mmap_addr(unsigned long size)
-{
-    return NULL;
-}
-
 void qemu_free(void *ptr)
 {
     free(ptr);