Re: [Qemu-devel] [PATCH v2 09/13] qed: Convert to bdrv_co_pwrite_zeroes()

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

Am 02.06.2016 um 14:40 hat Eric Blake geschrieben:
> On 06/02/2016 05:16 AM, Kevin Wolf wrote:
> > Am 01.06.2016 um 23:10 hat Eric Blake geschrieben:
> >> Another step on our continuing quest to switch to byte-based
> >> interfaces.
> >>
> >> Kill an abuse of the comma operator while at it (fortunately,
> >> the semantics were still right).  Also, the test for requests
> >> not aligned to clusters should be applied always, not just
> >> when a backing file is present.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  block/qed.c | 33 +++++++++++++++------------------
> >>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> >> -        }
> >> +    /* Fall back if the request is not */
> > 
> > ...aligned?
> 
> Yes, thanks.
> 
> > 
> >> +    if (qed_offset_into_cluster(s, offset) ||
> >> +        qed_offset_into_cluster(s, count)) {
> >> +        return -ENOTSUP;
> >>      }
> > 
> > This is obviously correct and almost as obviously suboptimal compared to
> > the original version (we need cluster alignment with a backing file, but
> > without a backing file, sector alignment would be enough).
> 
> Does QED support per-sector zeroing, or is it only per-cluster zero like
> qcow2?
> 
> /me checks the spec
> 
> only per-cluster zeroing
> 
> > 
> > But as this is QED, which is only supported for compatibility these
> > days, simpler if slightly suboptimal code is okay with me.
> 
> Widening a request to cluster boundaries is only possible if you know
> the cluster is otherwise unallocated or already reads as zeroes, and
> unless qed tracks zero sectors (as opposed to zero clusters), I argue
> that this was actually a bug fix, even when the request was
> sector-aligned, since we lack the check for cluster remains
> unallocated/zero before widening, the way qcow2 does it.  Sub-optimal
> but safe is better than incorrectly optimized.

It actually checks whether the cluster is already allocated; in that
case, qed implements a fallback itself. So I think it was working. Just
as usual with the callback based qed code, it's not very easy to read.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]