[v8,20/21] vvfat: Switch to .bdrv_co_block_status()

Message ID 20180213202701.15858-21-eblake@redhat.com
State New
Headers show
Series
  • add byte-based block_status driver callbacks
Related show

Commit Message

Eric Blake Feb. 13, 2018, 8:27 p.m.
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(-)

Comments

Kevin Wolf Feb. 14, 2018, 1:12 p.m. | #1
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
Eric Blake Feb. 14, 2018, 2:50 p.m. | #2
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.
Kevin Wolf Feb. 14, 2018, 3 p.m. | #3
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

Patch

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)