Re: [Qemu-devel] [PATCH v3 03/20] file-posix: Switch to .bdrv_co_block_status()

[Fam Zheng <famz@redhat.com>]

On Wed, 09/20 08:47, Eric Blake wrote:
> On 09/20/2017 04:57 AM, Fam Zheng wrote:
> > On Thu, 09/14 09:40, Eric Blake wrote:
> >> We are gradually moving away from sector-based interfaces, towards
> >> byte-based.  Update the file protocol driver accordingly.  In mapping
> >> mode, note that the entire file is reported as allocated, so we can
> >> take a shortcut and skip lseek().
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >>
> 
> >>
> >> -    ret = find_allocation(bs, start, &data, &hole);
> >> +    if (!mapping) {
> >> +        *pnum = bytes;
> >> +        *file = bs;
> >> +        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
> >> +            (offset & BDRV_BLOCK_OFFSET_MASK);
> >> +    }
> > 
> > I may be missing something, because the last time I tried to understand the
> > rationale behind "mapping" was already some time ago: shouldn't we still
> > distinguish hole and data? What will omitting BDRV_BLOCK_ZERO help?
> 
> Hmm, the commit message is slightly off (in part, because I switched the
> sense of the bool flag between series revisions, but did not properly
> update the commit text to match).  In mapping mode, we want to return as
> much information as possible (the client is something like 'qemu-img
> map'), including where the holes lie.  But when we are NOT in mapping
> mode, we care more about learning which portions of the file are
> described in the current layer of the backing chain, rather than
> delegating to another layer, regardless of whether the read will see
> data or zeroes.  By the time we are at the POSIX file protocol layer, we
> know that every byte in the file system/block device has a 1:1 mapping
> to the bytes that the guest will read (we do not delegate to any backing
> file), so we can simply report the entire remainder of the file as
> allocated without worrying about holes.

Thanks, it would be good if this explanation can be added to the comment of
"mapping" parameter, so it's easy to understand the actual intention in the
future.

> 
> Here's where the mapping flag was added and semantics documented (in
> series 3; whereas the current email is series 4):
> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03542.html
> 
> So what I really need to do is fix the commit message to read:
> 
> In mapping mode, we must report as much information as possible about
> where holes can be found; but when we don't care about mapping, the user
> is more interested in how much of the guest view will come from the
> current layer rather than delegating to some other BDS, and we can take
> the shortcut that all of the remainder of the file fits that
> description, and therefore take a shortcut and skip lseek() for a larger
> *pnum result.
> 
> (the same comment probably applies to several other patches in the series)
> 

Fam