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 <h...@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