Message ID | 20100507151719.GB15249@lst.de |
---|---|
State | New |
Headers | show |
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
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..
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);
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>