[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] cow: stop using mmap
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] cow: stop using mmap |
Date: |
Mon, 10 May 2010 11:41:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100330 Fedora/3.0.4-1.fc12 Thunderbird/3.0.4 |
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