| Message ID | 1380723636-18456-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 02, 2013 at 04:20:36PM +0200, Peter Lieven wrote: > if a raw device like an iscsi target or host device is used > the current implementation makes a second call out to get > the block status of bs->file. however, the raw driver already > has called bdrv_get_block_status on bs->file. > > v4: use a flag to detect the raw driver instead of the strncmp > hack. > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block.c | 4 ++-- > block/raw_bsd.c | 6 +++++- > include/block/block.h | 3 +++ > 3 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 93e113a..7fa2e43 100644 > --- a/block.c > +++ b/block.c > @@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > > if (bs->file && > (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > - (ret & BDRV_BLOCK_OFFSET_VALID)) { > + (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) { > ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, > *pnum, pnum); > if (ret2 >= 0) { > @@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > } > } > > - return ret; > + return ret & ~BDRV_BLOCK_RAW; > } > > /* Coroutine wrapper for bdrv_get_block_status() */ > diff --git a/block/raw_bsd.c b/block/raw_bsd.c > index d4ace60..a9e0209 100644 > --- a/block/raw_bsd.c > +++ b/block/raw_bsd.c > @@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, > int64_t sector_num, > int nb_sectors, int *pnum) > { > - return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); > + int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); > + if (ret < 0) { > + return ret; > + } > + return ret | BDRV_BLOCK_RAW; > } > > static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs, > diff --git a/include/block/block.h b/include/block/block.h > index f808550..cb7019b 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -84,6 +84,8 @@ typedef struct BlockDevOps { > /* BDRV_BLOCK_DATA: data is read from bs->file or another file > * BDRV_BLOCK_ZERO: sectors read as zero > * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data > + * BDRV_BLOCK_RAW: used internally to indicate that the request > + * was piped through the raw driver Sorry I didn't review this earlier but this flag looks hacky and I'm not confident about merging the patch yet. The patch makes me wonder if the raw_bsd driver should avoid calling bs->file itself: return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); Let block.c:bdrv_co_get_block_status() call down into bs->file. The problem is then the protocol cannot report unallocated sectors with this approach. I think we want to preserve bs' offset while taking the other flags from bs->file (DATA, ZERO). Peter, Paolo: What do you think of this approach? Stefan
On 10/02/2013 08:20 AM, Peter Lieven wrote: > if a raw device like an iscsi target or host device is used > the current implementation makes a second call out to get > the block status of bs->file. however, the raw driver already > has called bdrv_get_block_status on bs->file. > > v4: use a flag to detect the raw driver instead of the strncmp > hack. The v4 designation... > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- ...should appear after the --- separator (useful to reviewers, but doesn't need to be part of the git history). > block.c | 4 ++-- > block/raw_bsd.c | 6 +++++- > include/block/block.h | 3 +++ > 3 files changed, 10 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto: > Sorry I didn't review this earlier but this flag looks hacky and I'm not > confident about merging the patch yet. > > The patch makes me wonder if the raw_bsd driver should avoid calling > bs->file itself: > > return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > > Let block.c:bdrv_co_get_block_status() call down into bs->file. > > The problem is then the protocol cannot report unallocated sectors with > this approach. > > I think we want to preserve bs' offset while taking the other flags from > bs->file (DATA, ZERO). This would cause other changes. For example, a qcow2 with full metadata preallocation (i.e. all L2 tables are there but it points to holes) would not return DATA anymore. I think this is wrong, and especially a change from the old is_allocated API. However, a variant on this idea could be to return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file". Paolo > Peter, Paolo: What do you think of this approach?
Am 02.10.2013 17:13, schrieb Paolo Bonzini: > Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto: >> Sorry I didn't review this earlier but this flag looks hacky and I'm not >> confident about merging the patch yet. >> >> The patch makes me wonder if the raw_bsd driver should avoid calling >> bs->file itself: >> >> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | >> (sector_num << BDRV_SECTOR_BITS); >> >> Let block.c:bdrv_co_get_block_status() call down into bs->file. >> >> The problem is then the protocol cannot report unallocated sectors with >> this approach. >> >> I think we want to preserve bs' offset while taking the other flags from >> bs->file (DATA, ZERO). > This would cause other changes. For example, a qcow2 with full metadata > preallocation (i.e. all L2 tables are there but it points to holes) > would not return DATA anymore. I think this is wrong, and especially a > change from the old is_allocated API. > > However, a variant on this idea could be to return > > BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > > and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file". I am fine with that. Peter
diff --git a/block.c b/block.c index 93e113a..7fa2e43 100644 --- a/block.c +++ b/block.c @@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, if (bs->file && (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && - (ret & BDRV_BLOCK_OFFSET_VALID)) { + (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) { ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS, *pnum, pnum); if (ret2 >= 0) { @@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, } } - return ret; + return ret & ~BDRV_BLOCK_RAW; } /* Coroutine wrapper for bdrv_get_block_status() */ diff --git a/block/raw_bsd.c b/block/raw_bsd.c index d4ace60..a9e0209 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { - return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); + int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum); + if (ret < 0) { + return ret; + } + return ret | BDRV_BLOCK_RAW; } static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs, diff --git a/include/block/block.h b/include/block/block.h index f808550..cb7019b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -84,6 +84,8 @@ typedef struct BlockDevOps { /* BDRV_BLOCK_DATA: data is read from bs->file or another file * BDRV_BLOCK_ZERO: sectors read as zero * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data + * BDRV_BLOCK_RAW: used internally to indicate that the request + * was piped through the raw driver * * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in * bs->file where sector data can be read from as raw data. @@ -105,6 +107,7 @@ typedef struct BlockDevOps { #define BDRV_BLOCK_DATA 1 #define BDRV_BLOCK_ZERO 2 #define BDRV_BLOCK_OFFSET_VALID 4 +#define BDRV_BLOCK_RAW 8 #define BDRV_BLOCK_OFFSET_MASK BDRV_SECTOR_MASK typedef enum {
if a raw device like an iscsi target or host device is used the current implementation makes a second call out to get the block status of bs->file. however, the raw driver already has called bdrv_get_block_status on bs->file. v4: use a flag to detect the raw driver instead of the strncmp hack. Signed-off-by: Peter Lieven <pl@kamp.de> --- block.c | 4 ++-- block/raw_bsd.c | 6 +++++- include/block/block.h | 3 +++ 3 files changed, 10 insertions(+), 3 deletions(-)