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

[Kevin Wolf]

Am 25.05.2016 um 16:28 hat Eric Blake geschrieben:
> On 05/25/2016 08:07 AM, Kevin Wolf wrote:
> > Am 25.05.2016 um 00:25 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).
> >>
> >> Signed-off-by: Eric Blake <address@hidden>
> >> ---
> >>  block/qed.c | 25 +++++++++++++------------
> >>  1 file changed, 13 insertions(+), 12 deletions(-)
> >>
> 
> >> @@ -1443,10 +1443,10 @@ static int coroutine_fn 
> >> bdrv_qed_co_write_zeroes(BlockDriverState *bs,
> >>
> >>      /* Refuse if there are untouched backing file sectors */
> 
> The comment wasn't very helpful, so I may rewort it, too
> (s/untouched/unaligned/, or something like that)

I think "unaligned" is not what the comment means. What would an
unaligned sector be anyway?

The idea is probably like in qcow2 that if there is no backing file, the
rest of the cluster already reads as zeros, so we can overwrite it.

> >>      if (bs->backing) {
> >> -        if (qed_offset_into_cluster(s, sector_num * BDRV_SECTOR_SIZE) != 
> >> 0) {
> >> +        if (qed_offset_into_cluster(s, offset) != 0) {
> >>              return -ENOTSUP;
> >>          }
> >> -        if (qed_offset_into_cluster(s, nb_sectors * BDRV_SECTOR_SIZE) != 
> >> 0) {
> >> +        if (qed_offset_into_cluster(s, count) != 0) {
> >>              return -ENOTSUP;
> >>          }
> >>      }
> > 
> > Unaligned requests are only emulated if there is no backing file...
> > 
> >> @@ -1454,12 +1454,13 @@ static int coroutine_fn 
> >> bdrv_qed_co_write_zeroes(BlockDriverState *bs,
> >>      /* Zero writes start without an I/O buffer.  If a buffer becomes 
> >> necessary
> >>       * then it will be allocated during request processing.
> >>       */
> >> -    iov.iov_base = NULL,
> >> -    iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
> >> +    iov.iov_base = NULL;
> >> +    iov.iov_len = count;
> >>
> >>      qemu_iovec_init_external(&qiov, &iov, 1);
> >> -    blockacb = qed_aio_setup(bs, sector_num, &qiov, nb_sectors,
> >> -                             qed_co_write_zeroes_cb, &cb,
> >> +    blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov,
> >> +                             count >> BDRV_SECTOR_BITS,
> > 
> > ...so offset and count can still be unaligned here and we end up zeroing
> > out the wrong part of the sector. I guess we need to return -ENOTSUP for
> > all sub-sector requests, even without a backing file.
> 
> Hmm. Wouldn't it be nicer if we could guarantee that blk_pwrite_zeroes()
> will never call bdrv_co_pwrite_zeroes() with less than
> request_alignment?

If we want this restriction, the right place is to implement it is in
bdrv_co_pwrite_zeroes() before calling into the driver, not on the
BlockBackend level.

> That is, if the block layer takes care of
> read-modify-write for any unaligned byte offset less than
> request_alignment, then the driver only has to worry about sector
> alignment.  Except qed.c doesn't seem to set request_alignment, but is
> just relying on io.c currently setting it to MAX(BDRV_SECTOR_SIZE,
> bs->bl.request_alignment) everywhere. (And the fact that
> request_alignment is a sibling rather than a member to BlockLimits bl is
> awkward.)

As explained on IRC, the block driver shouldn't care about sector
alignment. 512 is just an arbitrary number that some interfaces
misguidedly happen to use as their unit for parameters. The QED request
struct is one of them, but there is no real reason for it. There is no
metadata that we have at a sector granularity.

So I would avoid baking assumptions of bad drivers into new interfaces.

> Maybe we want three limits in BlockLimits, rather than two: the current
> max_pwrite_zeroes does a good job at saying how small blk_pwrite_zeroes
> must fragment large requests, and pwrite_zeroes_alignment does a good
> job at saying how large a request must be to potentially punch a hole,
> but at least in the case of qcow2, where we want to optimize a partial
> write to potentially zeroing an entire cluster, we still want to limit
> things to sector boundaries when checking for whether the rest of the
> cluster already reads as zeroes, whether or not we also want to support
> request_alignment of 1 instead of 512.

For qcow2, the only reason that sectors are involved in the optimisation
is that that's the granularity of bdrv_get_block_status(). Once that is
fixed, qcow2 can use bytes for its optimisation.

Until then, if we decided that we don't want to check the full cluster
any more (clusters are always aligned to sectors) but only the area that
is not overwritten, it would have to round start and end of the request
to the sector bounary.

> There are other drivers that I touched in this series that were relying
> on the fact that the block layer currently guarantees sector alignment,
> and maybe they should be setting request_alignment, or maybe we want to
> add yet another BlockLimit member.  So even if we want normal read/write
> to allow request_alignment of 1 in the case where we don't need the
> block layer to do a read-modify-write, I'm still wondering whether we
> want the write_zeroes engine to have a different minimum alignment and
> ALWAYS hand off to normal read-modify-write for anything smaller.

Do you have an example where the restriction to full sectors is
fundamental and not just a shortcoming of the implementation? As long as
it's only the latter, I think using -ENOTSUP to deal with it until we
fix it for good is fine.

Kevin

pgpMGLL3iklCY.pgp
Description: PGP signature