| Message ID | 1465694320-13500-1-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sat, 06/11 19:18, Eric Blake wrote: > Commit e253f4b8 converted mirroring from sector-based bdrv_aio_* > to byte-based blk_aio_*, but failed to account for the subtle > difference in signatures (the former takes a semi-redundant length, > the latter takes a flags parameter). Since all of our flags are > currently smaller in size than BDRV_SECTOR_SIZE, it has no ill > effects until we either perform sub-sector mirroring, or we start > asserting that no unexpected flags are set. I found it while > testing new asserts when qemu-iotests 132 started warning about an > unknown flag 0x200000. > > Add an assert to help us catch any other improper flags. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/block-backend.c | 2 ++ > block/mirror.c | 6 ++---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 34500e6..a5dc6e3 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -945,6 +945,8 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes, > acb->bh = NULL; > acb->has_returned = false; > > + assert(flags <= 0x1f); > + Maybe define BDRV_REQ_FLAGS_MAX in include/block/block.h so it's easier to keep this assertion valid when we introduce more flags in the future? Otherwise the patch looks good. Fam > co = qemu_coroutine_create(co_entry); > qemu_coroutine_enter(co, acb); > > diff --git a/block/mirror.c b/block/mirror.c > index fbbc496..41848b2 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -157,8 +157,7 @@ static void mirror_read_complete(void *opaque, int ret) > return; > } > blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, &op->qiov, > - op->nb_sectors * BDRV_SECTOR_SIZE, > - mirror_write_complete, op); > + 0, mirror_write_complete, op); > } > > static inline void mirror_clip_sectors(MirrorBlockJob *s, > @@ -275,8 +274,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num, > s->sectors_in_flight += nb_sectors; > trace_mirror_one_iteration(s, sector_num, nb_sectors); > > - blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, > - nb_sectors * BDRV_SECTOR_SIZE, > + blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, 0, > mirror_read_complete, op); > return ret; > } > -- > 2.5.5 > >
On 06/11/2016 08:15 PM, Fam Zheng wrote: > On Sat, 06/11 19:18, Eric Blake wrote: >> Commit e253f4b8 converted mirroring from sector-based bdrv_aio_* >> to byte-based blk_aio_*, but failed to account for the subtle >> difference in signatures (the former takes a semi-redundant length, >> the latter takes a flags parameter). Since all of our flags are >> currently smaller in size than BDRV_SECTOR_SIZE, it has no ill >> effects until we either perform sub-sector mirroring, or we start >> asserting that no unexpected flags are set. I found it while >> testing new asserts when qemu-iotests 132 started warning about an >> unknown flag 0x200000. >> >> Add an assert to help us catch any other improper flags. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> block/block-backend.c | 2 ++ >> block/mirror.c | 6 ++---- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 34500e6..a5dc6e3 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -945,6 +945,8 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes, >> acb->bh = NULL; >> acb->has_returned = false; >> >> + assert(flags <= 0x1f); >> + > > Maybe define BDRV_REQ_FLAGS_MAX in include/block/block.h so it's easier to keep > this assertion valid when we introduce more flags in the future? Oh, good idea. And I also don't know if blk_aio_prwv() is really the best place (it happened to be nicer for getting a good gdb backtrace of the culprit caller prior to entering coroutines, since coroutines tend to kill the call trace - but is too narrow in that it doesn't catch non-aio entry into the block layer) - so I should probably make the actual asserts live in better places like bdrv_driver_p*. I'll split the patch between bug fix and assertion additions, v2 coming up later today. > > Otherwise the patch looks good. >
diff --git a/block/block-backend.c b/block/block-backend.c index 34500e6..a5dc6e3 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -945,6 +945,8 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes, acb->bh = NULL; acb->has_returned = false; + assert(flags <= 0x1f); + co = qemu_coroutine_create(co_entry); qemu_coroutine_enter(co, acb); diff --git a/block/mirror.c b/block/mirror.c index fbbc496..41848b2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -157,8 +157,7 @@ static void mirror_read_complete(void *opaque, int ret) return; } blk_aio_pwritev(s->target, op->sector_num * BDRV_SECTOR_SIZE, &op->qiov, - op->nb_sectors * BDRV_SECTOR_SIZE, - mirror_write_complete, op); + 0, mirror_write_complete, op); } static inline void mirror_clip_sectors(MirrorBlockJob *s, @@ -275,8 +274,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num, s->sectors_in_flight += nb_sectors; trace_mirror_one_iteration(s, sector_num, nb_sectors); - blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, - nb_sectors * BDRV_SECTOR_SIZE, + blk_aio_preadv(source, sector_num * BDRV_SECTOR_SIZE, &op->qiov, 0, mirror_read_complete, op); return ret; }
Commit e253f4b8 converted mirroring from sector-based bdrv_aio_* to byte-based blk_aio_*, but failed to account for the subtle difference in signatures (the former takes a semi-redundant length, the latter takes a flags parameter). Since all of our flags are currently smaller in size than BDRV_SECTOR_SIZE, it has no ill effects until we either perform sub-sector mirroring, or we start asserting that no unexpected flags are set. I found it while testing new asserts when qemu-iotests 132 started warning about an unknown flag 0x200000. Add an assert to help us catch any other improper flags. Signed-off-by: Eric Blake <eblake@redhat.com> --- block/block-backend.c | 2 ++ block/mirror.c | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-)