Re: [Qemu-devel] [PATCH 5/5] qcow2: Implement .bdrv_co_pwritev

[Kevin Wolf <kwolf@redhat.com>]

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

Am 03.06.2016 um 22:13 hat Eric Blake geschrieben:
> On 06/03/2016 11:21 AM, Kevin Wolf wrote:
> > This changes qcow2 to implement the byte-based .bdrv_co_pwritev
> > interface rather than the sector-based old one.
> > 
> > As preallocation uses the same allocation function as normal writes, and
> > the interface of that function needs to be changed, it is converted in
> > the same patch.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2-cluster.c | 13 ++++----
> >  block/qcow2.c         | 82 ++++++++++++++++++++++++---------------------------
> >  block/qcow2.h         |  3 +-
> >  trace-events          |  6 ++--
> >  4 files changed, 49 insertions(+), 55 deletions(-)
> > 
> 
> > +++ b/block/qcow2-cluster.c
> > @@ -1261,7 +1261,8 @@ fail:
> >   * Return 0 on success and -errno in error cases
> >   */
> >  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > -    int *num, uint64_t *host_offset, QCowL2Meta **m)
> > +                               unsigned int *bytes, uint64_t *host_offset,
> > +                               QCowL2Meta **m)
> >  {
> 
> >  
> > -    *num -= remaining >> BDRV_SECTOR_BITS;
> > -    assert(*num > 0);
> > +    *bytes -= remaining;
> > +    assert(*bytes > 0);
> 
> The assertion is now weaker.  Beforehand, num could go negative.  But
> bytes cannot, all it can do is go to 0.  If we wrap around, the
> assertion won't catch it.  Do you want to strengthen it by also
> asserting that bytes < INT_MAX?

Is it useful to assert this, though? We have tons of such loops and we
never assert that cur_bytes <= bytes (which is what you would be
checking effectively) because it's quite obvious.

This assertion was meant to document the less obvious fact that we
always make some progress and never return 0 bytes.

> > +++ b/block/qcow2.c
> > @@ -1544,23 +1544,20 @@ fail:
> >      return ret;
> >  }
> >  
> > -static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> > -                           int64_t sector_num,
> > -                           int remaining_sectors,
> > -                           QEMUIOVector *qiov)
> > +static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs,
> > +        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> 
> Worth consistent indentation, while touching it?

Will fix.

> > +    while (bytes != 0) {
> >  
> >          l2meta = NULL;
> >  
> >          trace_qcow2_writev_start_part(qemu_coroutine_self());
> > -        index_in_cluster = sector_num & (s->cluster_sectors - 1);
> > -        cur_nr_sectors = remaining_sectors;
> > -        if (bs->encrypted &&
> > -            cur_nr_sectors >
> > -            QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster) {
> > -            cur_nr_sectors =
> > -                QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors - index_in_cluster;
> > +        offset_in_cluster = offset_into_cluster(s, offset);
> > +        cur_bytes = MIN(bytes, INT_MAX);
> > +        if (bs->encrypted) {
> > +            cur_bytes = MIN(cur_bytes,
> > +                            QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> > +                            - offset_in_cluster);
> 
> Again, if the block layer learns to auto-fragment, then we should just
> inform the block layer about QCOW_MAX_CRYPT_CLUSTERS as our max_transfer
> limit.
> 
> 
> > @@ -1634,10 +1628,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> >          qemu_co_mutex_unlock(&s->lock);
> >          BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
> >          trace_qcow2_writev_data(qemu_coroutine_self(),
> > -                                (cluster_offset >> 9) + index_in_cluster);
> > -        ret = bdrv_co_writev(bs->file->bs,
> > -                             (cluster_offset >> 9) + index_in_cluster,
> > -                             cur_nr_sectors, &hd_qiov);
> > +                                cluster_offset + offset_in_cluster);
> > +        ret = bdrv_co_pwritev(bs->file->bs,
> > +                              cluster_offset + offset_in_cluster,
> > +                              cur_bytes, &hd_qiov, 0);
> 
> s/0/flags/, even if .supported_write_flags stays 0 for now.

I don't agree. That might make sense for a passthrough driver like raw,
but I don't see a reason to guess for qcow2 that passing flags down is
the right thing to do. The only flag that we currently have (FUA) isn't
correctly implemented by passing it to the lower layer because qcow2
metadata must be considered.

So I would leave changing flags here for the patch that adds some flag
to .supported_write_flags.

> > @@ -2059,7 +2053,7 @@ static int preallocate(BlockDriverState *bs)
> >          uint8_t buf[BDRV_SECTOR_SIZE];
> >          memset(buf, 0, BDRV_SECTOR_SIZE);
> >          ret = bdrv_write(bs->file->bs,
> > -                         (host_offset >> BDRV_SECTOR_BITS) + num - 1,
> > +                         ((host_offset + cur_bytes) >> BDRV_SECTOR_BITS) - 1,
> >                           buf, 1);
> 
> Do we still have to call bdrv_write(), or can we convert this to a
> byte-based interface call?

Okay, I'll make it a one-byte bdrv_pwrite().

Kevin

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