| Message ID | 1461750767-23273-7-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 27, 2016 at 11:52:36AM +0200, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- > 1 file changed, 29 insertions(+), 17 deletions(-) > > diff --git a/block/bochs.c b/block/bochs.c > index af8b7ab..d148454 100644 > --- a/block/bochs.c > +++ b/block/bochs.c > @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags, > int ret; > > bs->read_only = 1; // no write support yet > + bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */ Can this be the default in block.c? Drivers that have other alignment characteristics can set it explicitly but most drivers will want BDRV_SECTOR_SIZE so it's nice to make it common code. > > ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs)); > if (ret < 0) { > @@ -221,39 +222,50 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num) > return bitmap_offset + (512 * (s->bitmap_blocks + extent_offset)); > } > > -static int bochs_read(BlockDriverState *bs, int64_t sector_num, > - uint8_t *buf, int nb_sectors) > +static int coroutine_fn > +bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > + QEMUIOVector *qiov, int flags) > { > + BDRVBochsState *s = bs->opaque; > + uint64_t sector_num = offset >> BDRV_SECTOR_BITS; > + int nb_sectors = bytes >> BDRV_SECTOR_BITS; > + uint64_t bytes_done = 0; > + QEMUIOVector local_qiov; > int ret; > > + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); > + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); > + > + qemu_iovec_init(&local_qiov, qiov->niov); > + qemu_co_mutex_lock(&s->lock); > + > while (nb_sectors > 0) { > int64_t block_offset = seek_to_sector(bs, sector_num); > if (block_offset < 0) { > return block_offset; s->lock must be unlocked. > - } else if (block_offset > 0) { > - ret = bdrv_pread(bs->file->bs, block_offset, buf, 512); > + } > + > + qemu_iovec_reset(&local_qiov); > + qemu_iovec_concat(&local_qiov, qiov, bytes_done, 512); > + > + if (block_offset > 0) { > + ret = bdrv_co_preadv(bs->file->bs, block_offset, 512, > + &local_qiov, 0); > if (ret < 0) { > return ret; Same here.
Am 27.04.2016 um 16:06 hat Stefan Hajnoczi geschrieben: > On Wed, Apr 27, 2016 at 11:52:36AM +0200, Kevin Wolf wrote: > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- > > 1 file changed, 29 insertions(+), 17 deletions(-) > > > > diff --git a/block/bochs.c b/block/bochs.c > > index af8b7ab..d148454 100644 > > --- a/block/bochs.c > > +++ b/block/bochs.c > > @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags, > > int ret; > > > > bs->read_only = 1; // no write support yet > > + bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */ > > Can this be the default in block.c? Drivers that have other alignment > characteristics can set it explicitly but most drivers will want > BDRV_SECTOR_SIZE so it's nice to make it common code. I absolutely don't expect BDRV_SECTOR_SIZE to be what most drivers will want. It's basically a sign that a driver is either a protocol that imposes restrictions (like O_DIRECT on raw-posix) or it is broken and we don't care enough about the emulation overhead to fix it (like this one). The expected value for most block drivers is 1. > > while (nb_sectors > 0) { > > int64_t block_offset = seek_to_sector(bs, sector_num); > > if (block_offset < 0) { > > return block_offset; > > s->lock must be unlocked. I can't believe I messed this up in so many places. Thanks for catching this, I'll have to go through all patches and check the locking. Kevin
On 04/27/2016 03:52 AM, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- > 1 file changed, 29 insertions(+), 17 deletions(-) > > static void bochs_close(BlockDriverState *bs) > @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = { > .instance_size = sizeof(BDRVBochsState), > .bdrv_probe = bochs_probe, > .bdrv_open = bochs_open, > - .bdrv_read = bochs_co_read, > + .bdrv_co_preadv = bochs_co_preadv, > .bdrv_close = bochs_close, > }; Alignment is funky here. I'd rather just get rid of all the extra spaces, if that's easier than having half but not all of the = aligned.
Am 27.04.2016 um 17:51 hat Eric Blake geschrieben: > On 04/27/2016 03:52 AM, Kevin Wolf wrote: > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- > > 1 file changed, 29 insertions(+), 17 deletions(-) > > > > > static void bochs_close(BlockDriverState *bs) > > @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = { > > .instance_size = sizeof(BDRVBochsState), > > .bdrv_probe = bochs_probe, > > .bdrv_open = bochs_open, > > - .bdrv_read = bochs_co_read, > > + .bdrv_co_preadv = bochs_co_preadv, > > .bdrv_close = bochs_close, > > }; > > Alignment is funky here. I'd rather just get rid of all the extra > spaces, if that's easier than having half but not all of the = aligned. Alignment is funky by definition when there are tabs involved and new code follows the coding style and uses spaces instead. With a tab stop of 4, this is aligned correctly. The other option would be to just convert the whole struct to spaces, but that's not really related to this patch and I think we're avoiding pure style cleanup patches to keep things like 'git blame' useful. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 27.04.2016 um 17:51 hat Eric Blake geschrieben: >> On 04/27/2016 03:52 AM, Kevin Wolf wrote: >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > --- >> > block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- >> > 1 file changed, 29 insertions(+), 17 deletions(-) >> > >> >> > static void bochs_close(BlockDriverState *bs) >> > @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = { >> > .instance_size = sizeof(BDRVBochsState), >> > .bdrv_probe = bochs_probe, >> > .bdrv_open = bochs_open, >> > - .bdrv_read = bochs_co_read, >> > + .bdrv_co_preadv = bochs_co_preadv, >> > .bdrv_close = bochs_close, >> > }; >> >> Alignment is funky here. I'd rather just get rid of all the extra >> spaces, if that's easier than having half but not all of the = aligned. > > Alignment is funky by definition when there are tabs involved and new > code follows the coding style and uses spaces instead. With a tab stop > of 4, this is aligned correctly. Four? It comes out correctly with tab-width 8 for me. You can have any tab stops you want as long as it's multiples of eight. > The other option would be to just convert the whole struct to spaces, > but that's not really related to this patch and I think we're avoiding > pure style cleanup patches to keep things like 'git blame' useful. We normally untabify the lines we touch anyway, and leave the rest alone.
diff --git a/block/bochs.c b/block/bochs.c index af8b7ab..d148454 100644 --- a/block/bochs.c +++ b/block/bochs.c @@ -104,6 +104,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags, int ret; bs->read_only = 1; // no write support yet + bs->request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O supported */ ret = bdrv_pread(bs->file->bs, 0, &bochs, sizeof(bochs)); if (ret < 0) { @@ -221,39 +222,50 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num) return bitmap_offset + (512 * (s->bitmap_blocks + extent_offset)); } -static int bochs_read(BlockDriverState *bs, int64_t sector_num, - uint8_t *buf, int nb_sectors) +static int coroutine_fn +bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + QEMUIOVector *qiov, int flags) { + BDRVBochsState *s = bs->opaque; + uint64_t sector_num = offset >> BDRV_SECTOR_BITS; + int nb_sectors = bytes >> BDRV_SECTOR_BITS; + uint64_t bytes_done = 0; + QEMUIOVector local_qiov; int ret; + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); + + qemu_iovec_init(&local_qiov, qiov->niov); + qemu_co_mutex_lock(&s->lock); + while (nb_sectors > 0) { int64_t block_offset = seek_to_sector(bs, sector_num); if (block_offset < 0) { return block_offset; - } else if (block_offset > 0) { - ret = bdrv_pread(bs->file->bs, block_offset, buf, 512); + } + + qemu_iovec_reset(&local_qiov); + qemu_iovec_concat(&local_qiov, qiov, bytes_done, 512); + + if (block_offset > 0) { + ret = bdrv_co_preadv(bs->file->bs, block_offset, 512, + &local_qiov, 0); if (ret < 0) { return ret; } } else { - memset(buf, 0, 512); + qemu_iovec_memset(&local_qiov, 0, 0, 512); } nb_sectors--; sector_num++; - buf += 512; + bytes_done += 512; } - return 0; -} -static coroutine_fn int bochs_co_read(BlockDriverState *bs, int64_t sector_num, - uint8_t *buf, int nb_sectors) -{ - int ret; - BDRVBochsState *s = bs->opaque; - qemu_co_mutex_lock(&s->lock); - ret = bochs_read(bs, sector_num, buf, nb_sectors); qemu_co_mutex_unlock(&s->lock); - return ret; + qemu_iovec_destroy(&local_qiov); + + return 0; } static void bochs_close(BlockDriverState *bs) @@ -267,7 +279,7 @@ static BlockDriver bdrv_bochs = { .instance_size = sizeof(BDRVBochsState), .bdrv_probe = bochs_probe, .bdrv_open = bochs_open, - .bdrv_read = bochs_co_read, + .bdrv_co_preadv = bochs_co_preadv, .bdrv_close = bochs_close, };
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/bochs.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-)