| Message ID | 20180213202701.15858-10-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 13.02.2018 um 21:26 hat Eric Blake geschrieben: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the null driver accordingly. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Fam Zheng <famz@redhat.com> > > --- > v6-v7: no change > v5: minor fix to type of 'ret' > v4: rebase to interface tweak > v3: no change > v2: rebase to mapping parameter > --- > block/null.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/block/null.c b/block/null.c > index 214d394fff4..806a8631e4d 100644 > --- a/block/null.c > +++ b/block/null.c > @@ -223,22 +223,23 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state, > return 0; > } > > -static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, int *pnum, > - BlockDriverState **file) > +static int coroutine_fn null_co_block_status(BlockDriverState *bs, > + bool want_zero, int64_t offset, > + int64_t bytes, int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > BDRVNullState *s = bs->opaque; > - off_t start = sector_num * BDRV_SECTOR_SIZE; > + int ret = BDRV_BLOCK_OFFSET_VALID; > > - *pnum = nb_sectors; > + *pnum = bytes; > + *map = offset; > *file = bs; > > if (s->read_zeroes) { > - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO; > - } else { > - return BDRV_BLOCK_OFFSET_VALID | start; > + ret |= BDRV_BLOCK_ZERO; > } > + return ret; > } Preexisting, but I think this return value is wrong. OFFSET_VALID without DATA is to documented to have the following semantics: * DATA ZERO OFFSET_VALID * f t t sectors preallocated, read as zero, returned file not * necessarily zero at offset * f f t sectors preallocated but read from backing_hd, * returned file contains garbage at offset I'm not sure what OFFSET_VALID is even supposed to mean for null. Or in fact, what it is supposed to mean for any protocol driver, because normally it just means I can use this offset for accessing bs->file. But protocol drivers don't have a bs->file, so it's interesting to see that they still all set this flag. OFFSET_VALID | DATA might be excusable because I can see that it's convenient that a protocol driver refers to itself as *file instead of returning NULL there and then the offset is valid (though it would be pointless to actually follow the file pointer), but OFFSET_VALID without DATA probably isn't. Kevin
On 02/14/2018 06:05 AM, Kevin Wolf wrote: > Am 13.02.2018 um 21:26 hat Eric Blake geschrieben: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the null driver accordingly. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Fam Zheng <famz@redhat.com> >> >> if (s->read_zeroes) { >> - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO; >> - } else { >> - return BDRV_BLOCK_OFFSET_VALID | start; >> + ret |= BDRV_BLOCK_ZERO; >> } >> + return ret; >> } > > Preexisting, but I think this return value is wrong. OFFSET_VALID > without DATA is to documented to have the following semantics: > > * DATA ZERO OFFSET_VALID > * f t t sectors preallocated, read as zero, returned file not > * necessarily zero at offset > * f f t sectors preallocated but read from backing_hd, > * returned file contains garbage at offset > > I'm not sure what OFFSET_VALID is even supposed to mean for null. Yeah, and I was even thinking about that a bit yesterday when figuring out what to do with nvme. It does highlight the fact that you get garbage when reading from the null driver (unless the zero option was enabled, then ZERO is set and you know you read zeros instead) - but there no pointer that is preallocated (whether it contains garbage or otherwise) that you can actually dereference to read what the guest would see. > > Or in fact, what it is supposed to mean for any protocol driver, because > normally it just means I can use this offset for accessing bs->file. But > protocol drivers don't have a bs->file, so it's interesting to see that > they still all set this flag. > > OFFSET_VALID | DATA might be excusable because I can see that it's > convenient that a protocol driver refers to itself as *file instead of > returning NULL there and then the offset is valid (though it would be > pointless to actually follow the file pointer), but OFFSET_VALID without > DATA probably isn't. Hmm, you're probably right. Maybe that means I should tweak the documentation to be more explicit: for a format driver, OFFSET_VALID can always be used (and *file will be set to the underlying protocol driver); but for a protocol driver, OFFSET_VALID only makes sense if *file is the BDS itself and there is an actual buffer to read (that is, the protocol driver must also be returning DATA and/or ZERO). Or maybe we can indeed state that protocol drivers always set *file to NULL (there is no further backing file to reference), and thus never need to return OFFSET_VALID (but I'm not sure whether that will accidentally propagate back up the call stack and negatively affect status queries of format drivers). Since it is pre-existing, should I respin to address the issue in a separate patch, or should that be a followup after this series?
Am 14.02.2018 um 15:44 hat Eric Blake geschrieben: > On 02/14/2018 06:05 AM, Kevin Wolf wrote: > > Am 13.02.2018 um 21:26 hat Eric Blake geschrieben: > > > We are gradually moving away from sector-based interfaces, towards > > > byte-based. Update the null driver accordingly. > > > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > Reviewed-by: Fam Zheng <famz@redhat.com> > > > > > > > if (s->read_zeroes) { > > > - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO; > > > - } else { > > > - return BDRV_BLOCK_OFFSET_VALID | start; > > > + ret |= BDRV_BLOCK_ZERO; > > > } > > > + return ret; > > > } > > > > Preexisting, but I think this return value is wrong. OFFSET_VALID > > without DATA is to documented to have the following semantics: > > > > * DATA ZERO OFFSET_VALID > > * f t t sectors preallocated, read as zero, returned file not > > * necessarily zero at offset > > * f f t sectors preallocated but read from backing_hd, > > * returned file contains garbage at offset > > > > I'm not sure what OFFSET_VALID is even supposed to mean for null. > > Yeah, and I was even thinking about that a bit yesterday when figuring out > what to do with nvme. It does highlight the fact that you get garbage when > reading from the null driver (unless the zero option was enabled, then ZERO > is set and you know you read zeros instead) - but there no pointer that is > preallocated (whether it contains garbage or otherwise) that you can > actually dereference to read what the guest would see. > > > > > Or in fact, what it is supposed to mean for any protocol driver, because > > normally it just means I can use this offset for accessing bs->file. But > > protocol drivers don't have a bs->file, so it's interesting to see that > > they still all set this flag. > > > > OFFSET_VALID | DATA might be excusable because I can see that it's > > convenient that a protocol driver refers to itself as *file instead of > > returning NULL there and then the offset is valid (though it would be > > pointless to actually follow the file pointer), but OFFSET_VALID without > > DATA probably isn't. > > Hmm, you're probably right. Maybe that means I should tweak the > documentation to be more explicit: for a format driver, OFFSET_VALID can > always be used (and *file will be set to the underlying protocol driver); > but for a protocol driver, OFFSET_VALID only makes sense if *file is the BDS > itself and there is an actual buffer to read (that is, the protocol driver > must also be returning DATA and/or ZERO). Or maybe we can indeed state that > protocol drivers always set *file to NULL (there is no further backing file > to reference), and thus never need to return OFFSET_VALID (but I'm not sure > whether that will accidentally propagate back up the call stack and > negatively affect status queries of format drivers). > > Since it is pre-existing, should I respin to address the issue in a separate > patch, or should that be a followup after this series? It's a more fundamental question that shouldn't hold up this series. I just wanted to raise it while I was looking at it. So yes, a followup is fine. Kevin
diff --git a/block/null.c b/block/null.c index 214d394fff4..806a8631e4d 100644 --- a/block/null.c +++ b/block/null.c @@ -223,22 +223,23 @@ static int null_reopen_prepare(BDRVReopenState *reopen_state, return 0; } -static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, int *pnum, - BlockDriverState **file) +static int coroutine_fn null_co_block_status(BlockDriverState *bs, + bool want_zero, int64_t offset, + int64_t bytes, int64_t *pnum, + int64_t *map, + BlockDriverState **file) { BDRVNullState *s = bs->opaque; - off_t start = sector_num * BDRV_SECTOR_SIZE; + int ret = BDRV_BLOCK_OFFSET_VALID; - *pnum = nb_sectors; + *pnum = bytes; + *map = offset; *file = bs; if (s->read_zeroes) { - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO; - } else { - return BDRV_BLOCK_OFFSET_VALID | start; + ret |= BDRV_BLOCK_ZERO; } + return ret; } static void null_refresh_filename(BlockDriverState *bs, QDict *opts) @@ -270,7 +271,7 @@ static BlockDriver bdrv_null_co = { .bdrv_co_flush_to_disk = null_co_flush, .bdrv_reopen_prepare = null_reopen_prepare, - .bdrv_co_get_block_status = null_co_get_block_status, + .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, }; @@ -290,7 +291,7 @@ static BlockDriver bdrv_null_aio = { .bdrv_aio_flush = null_aio_flush, .bdrv_reopen_prepare = null_reopen_prepare, - .bdrv_co_get_block_status = null_co_get_block_status, + .bdrv_co_block_status = null_co_block_status, .bdrv_refresh_filename = null_refresh_filename, };