| Message ID | 20180213202701.15858-21-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 13.02.2018 um 21:27 hat Eric Blake geschrieben: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the vvfat driver accordingly. Note that we > can rely on the block driver having already clamped limits to our > block size, and simplify accordingly. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Fam Zheng <famz@redhat.com> > > --- > v5-v7: no change > v4: rebase to interface tweak > v3: no change > v2: rebase to earlier changes, simplify > --- > block/vvfat.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/block/vvfat.c b/block/vvfat.c > index 7e06ebacf61..4a17a49e128 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -3088,15 +3088,13 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > return ret; > } > > -static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file) > +static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs, > + bool want_zero, int64_t offset, > + int64_t bytes, int64_t *n, > + int64_t *map, > + BlockDriverState **file) > { > - *n = bs->total_sectors - sector_num; > - if (*n > nb_sectors) { > - *n = nb_sectors; > - } else if (*n < 0) { > - return 0; > - } > + *n = bytes; > return BDRV_BLOCK_DATA; > } Preexisting, but this is inconsistent with other protocol drivers as far as OFFSET_VALID is concerned (as I hinted in another mail, I like it better this way, but it's still inconsistent). Do we actually need any callback here or could the solution be to simply remove it like with nvme? Kevin
On 02/14/2018 07:12 AM, Kevin Wolf wrote: > Am 13.02.2018 um 21:27 hat Eric Blake geschrieben: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the vvfat driver accordingly. Note that we >> can rely on the block driver having already clamped limits to our >> block size, and simplify accordingly. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Fam Zheng <famz@redhat.com> >> { >> - *n = bs->total_sectors - sector_num; >> - if (*n > nb_sectors) { >> - *n = nb_sectors; >> - } else if (*n < 0) { >> - return 0; >> - } >> + *n = bytes; >> return BDRV_BLOCK_DATA; >> } > > Preexisting, but this is inconsistent with other protocol drivers as far > as OFFSET_VALID is concerned (as I hinted in another mail, I like it > better this way, but it's still inconsistent). > > Do we actually need any callback here or could the solution be to simply > remove it like with nvme? Does that mean io.c's defaults for protocol drivers is wrong? It defaults to setting OFFSET_VALID and *map on all protocol drivers without a callback (at least nvme, nbd); I didn't delete this callback because I noticed the difference in return value, and couldn't justify whether it was intentional. Also, vvfat is weird - it is somewhat of a format driver, rather than just a protocol; even though it sets .protocol_name. It may be possible for vvfat to actually set OFFSET_VALID to particular offsets within the host file that correspond to what the guest would read, where it is not a simple 1:1 mapping, precisely because it is imposing format on the host file. However, vvfat is one of those things that I try to avoid as much as possible, because it is just so weird.
Am 14.02.2018 um 15:50 hat Eric Blake geschrieben: > On 02/14/2018 07:12 AM, Kevin Wolf wrote: > > Am 13.02.2018 um 21:27 hat Eric Blake geschrieben: > > > We are gradually moving away from sector-based interfaces, towards > > > byte-based. Update the vvfat driver accordingly. Note that we > > > can rely on the block driver having already clamped limits to our > > > block size, and simplify accordingly. > > > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > Reviewed-by: Fam Zheng <famz@redhat.com> > > > > { > > > - *n = bs->total_sectors - sector_num; > > > - if (*n > nb_sectors) { > > > - *n = nb_sectors; > > > - } else if (*n < 0) { > > > - return 0; > > > - } > > > + *n = bytes; > > > return BDRV_BLOCK_DATA; > > > } > > > > Preexisting, but this is inconsistent with other protocol drivers as far > > as OFFSET_VALID is concerned (as I hinted in another mail, I like it > > better this way, but it's still inconsistent). > > > > Do we actually need any callback here or could the solution be to simply > > remove it like with nvme? > > Does that mean io.c's defaults for protocol drivers is wrong? It defaults > to setting OFFSET_VALID and *map on all protocol drivers without a callback > (at least nvme, nbd); I didn't delete this callback because I noticed the > difference in return value, and couldn't justify whether it was intentional. > Also, vvfat is weird - it is somewhat of a format driver, rather than just a > protocol; even though it sets .protocol_name. It may be possible for vvfat > to actually set OFFSET_VALID to particular offsets within the host file that > correspond to what the guest would read, where it is not a simple 1:1 > mapping, precisely because it is imposing format on the host file. However, > vvfat is one of those things that I try to avoid as much as possible, > because it is just so weird. As I said in my reply to the null block driver, OFFSET_VALID doesn't really make sense for protocol drivers anyway. Making use of it with vvfat isn't any more practical than directly accessing the undefined data of the null driver. I think the unwritten rule at the moment is that protocols should always set OFFSET_VALID and *file = bs (even though it doesn't make sense). So with the current interface, I'd consider deleting the callback a vvfat fix. I also think that we should possibly look into changing the interface so that protocols don't set OFFSET_VALID and *file, but then the default handling would change too, and deleting the callback in vvfat would still be right. As this is preexisting, I'm okay with just merging the series as it is, and then we can handle this while dealing with the more fundamental question what protocol drivers should return in general. Kevin
diff --git a/block/vvfat.c b/block/vvfat.c index 7e06ebacf61..4a17a49e128 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -3088,15 +3088,13 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes, return ret; } -static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file) +static int coroutine_fn vvfat_co_block_status(BlockDriverState *bs, + bool want_zero, int64_t offset, + int64_t bytes, int64_t *n, + int64_t *map, + BlockDriverState **file) { - *n = bs->total_sectors - sector_num; - if (*n > nb_sectors) { - *n = nb_sectors; - } else if (*n < 0) { - return 0; - } + *n = bytes; return BDRV_BLOCK_DATA; } @@ -3257,7 +3255,7 @@ static BlockDriver bdrv_vvfat = { .bdrv_co_preadv = vvfat_co_preadv, .bdrv_co_pwritev = vvfat_co_pwritev, - .bdrv_co_get_block_status = vvfat_co_get_block_status, + .bdrv_co_block_status = vvfat_co_block_status, }; static void bdrv_vvfat_init(void)