[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid wasted work i
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid wasted work in bdrv_is_allocated() |
Date: |
Wed, 5 Jul 2017 20:07:25 +0800 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Wed, 07/05 06:56, Eric Blake wrote:
> On 07/04/2017 02:06 AM, Fam Zheng wrote:
> > On Mon, 07/03 17:14, Eric Blake wrote:
> >> @@ -1717,6 +1718,10 @@ int64_t coroutine_fn
> >> bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
> >> * Drivers not implementing the functionality are assumed to not support
> >> * backing files, hence all their sectors are reported as allocated.
> >> *
> >> + * If 'allocation' is true, the caller only cares about allocation
> >> + * status; this is a hint that a larger 'pnum' result is more
> >> + * important than including BDRV_BLOCK_OFFSET_VALID in the return.
> >> + *
> >
> > This is slightly unintuitive. I would guess "allocation == false" means "I
> > don't
> > care about the allocation status" but it actually is "I don't care about the
> > consecutiveness". The "only" semantics is missing in the parameter name.
> >
> > Maybe rename it to "consecutive" and invert the logic? I.e. "consecutive ==
> > true" means BDRV_BLOCK_OFFSET_VALID is wanted.
>
> Reasonable idea; other [shorter] names I've been toying with:
> strict
> mapping
> precise
>
> any of which, if true (set true by bdrv_get_block_status), means that I
> care more about BDRV_BLOCK_OFFSET_VALID and validity for learning host
> offsets, if false it means I'm okay getting a larger *pnum even if it
> extends over disjoint host offsets; or:
>
> fast
>
> which if true (set true by bdrv_is_allocated) means I want a larger
> *pnum even if I have to sacrifice accuracy by omitting
> BDRV_BLOCK_OFFSET_VALID, because I'm trying to reduce effort spent.
>
> >
> > Sorry for bikeshedding.
>
> Not a problem, I also had some double-takes in writing my own code
> trying to remember which way I wanted the 'allocation' boolean to be
> set, so coming up with a more intuitive name/default state in order to
> help legibility is worth it. Do any of my above suggestions sound better?
>
I'd vote for "mapping" because it has a close connection with offset (as in
BDRV_BLOCK_OFFSET_VALID).
Or simply call it "offset" and if false, never return BDRV_BLOCK_OFFSET_VALID.
Fam
Re: [Qemu-devel] [PATCH v2 03/15] block: Add flag to avoid wasted work in bdrv_is_allocated(), Eric Blake, 2017/07/05
[Qemu-devel] [PATCH v2 02/15] block: Allow NULL file for bdrv_get_block_status(), Eric Blake, 2017/07/03
[Qemu-devel] [PATCH v2 05/15] qcow2: Switch is_zero_sectors() to byte-based, Eric Blake, 2017/07/03
[Qemu-devel] [PATCH v2 06/15] block: Switch bdrv_make_zero() to byte-based, Eric Blake, 2017/07/03
[Qemu-devel] [PATCH v2 07/15] qemu-img: Switch get_block_status() to byte-based, Eric Blake, 2017/07/03