Re: [Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Fam Zheng <famz@redhat.com>]

On Mon, 01/25 14:04, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is referring to. Its value should be ignored unless
> > BDRV_BLOCK_OFFSET_VALID in ret is set.
> > 
> > Until block drivers fill in the right value, let's clear it explicitly
> > right before calling .bdrv_get_block_status.
> > 
> > The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest
> > case 102 passing, and will be fixed once all drivers return the right file
> > pointer.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/io.c                | 38 ++++++++++++++++++++++++++------------
> >  block/iscsi.c             |  6 ++++--
> >  block/mirror.c            |  3 ++-
> >  block/parallels.c         |  2 +-
> >  block/qcow.c              |  2 +-
> >  block/qcow2.c             |  2 +-
> >  block/qed.c               |  3 ++-
> >  block/raw-posix.c         |  3 ++-
> >  block/raw_bsd.c           |  3 ++-
> >  block/sheepdog.c          |  2 +-
> >  block/vdi.c               |  2 +-
> >  block/vmdk.c              |  2 +-
> >  block/vpc.c               |  2 +-
> >  block/vvfat.c             |  2 +-
> >  include/block/block.h     | 11 +++++++----
> >  include/block/block_int.h |  3 ++-
> >  qemu-img.c                | 13 +++++++++----
> >  17 files changed, 64 insertions(+), 35 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 5bb353a..0836991 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> >  int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> >  {
> >      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
> > +    BlockDriverState *file;
> >      int n;
> >  
> >      target_sectors = bdrv_nb_sectors(bs);
> > @@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> >          if (nb_sectors <= 0) {
> >              return 0;
> >          }
> > -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> > +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
> >          if (ret < 0) {
> >              error_report("error getting block status at sector %" PRId64 ": %s",
> >                           sector_num, strerror(-ret));
> > @@ -1466,6 +1467,7 @@ int bdrv_flush_all(void)
> >  typedef struct BdrvCoGetBlockStatusData {
> >      BlockDriverState *bs;
> >      BlockDriverState *base;
> > +    BlockDriverState **file;
> >      int64_t sector_num;
> >      int nb_sectors;
> >      int *pnum;
> > @@ -1490,7 +1492,8 @@ typedef struct BdrvCoGetBlockStatusData {
> >   */
> >  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >                                                       int64_t sector_num,
> > -                                                     int nb_sectors, int *pnum)
> > +                                                     int nb_sectors, int *pnum,
> > +                                                     BlockDriverState **file)
> 
> This function has a comment explaining all parameters. Except **file
> now.

Will add comment for @file too.

> 
> My impression at the moment is that it generally returns bs->file for
> format drivers and bs itself for protocol drivers if the sector is
> allocated in this layer.

Yes.

> 
> >  {
> >      int64_t total_sectors;
> >      int64_t n;
> > @@ -1520,16 +1523,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >          return ret;
> >      }
> >  
> > -    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> > +    *file = NULL;
> > +    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
> > +                                            file);
> >      if (ret < 0) {
> >          *pnum = 0;
> > +        *file = NULL;
> 
> You specified that file is to be ignored unless BDRV_BLOCK_OFFSET_VALID
> is set. Why reset it here then? It's not reset in other error cases.

No particular reason, and it can be safely removed.

> 
> >          return ret;
> >      }
> >  
> >      if (ret & BDRV_BLOCK_RAW) {
> >          assert(ret & BDRV_BLOCK_OFFSET_VALID);
> >          return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> > -                                     *pnum, pnum);
> > +                                     *pnum, pnum, file);
> >      }
> >  
> >      if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
> 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 2ea5a4a..b8d29e1 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
> >  }
> >  
> >  static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> > -	int64_t sector_num, int nb_sectors, int* n)
> > +	int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
> >  {
> >      BDRVVVFATState* s = bs->opaque;
> >      *n = s->sector_count - sector_num;
> 
> This still returns NULL at the end of the series. Shouldn't it return bs
> like other protocol drivers do?

Yes, we need another patch for vvfat.

Thanks!

Fam