Re: [Qemu-devel] [PATCHv3 00/20] block: logical block provisioning enhancements

[Peter Lieven]

On 08.10.2013 09:02, Stefan Hajnoczi wrote:
> On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini <address@hidden> wrote:
> > Il 07/10/2013 10:42, Stefan Hajnoczi ha scritto:
> > > Could you make bdrv_co_write_zeroes() always use UNMAP, if possible, and
> > > avoid adding the new BDRV_REQ_MAY_UNMAP flag?  While reading the first
> > > few patches in this series I wondered why there is a need to expose
> > > flags at all...
> > >
> > > Sometimes it is useful to distinguish between zeroing at the image
> > > format level from discarding at the device level, but I don't think we
> > > make use of that yet.  I'd prefer to keep the interface simple for now
> > > and add flags later, if necessary.
> > >
> > > Or maybe I just missed something ;)
> > The flag is needed to implement the right semantics for the SCSI WRITE
> > SAME command, which are:
> >
> > - if the UNMAP bit is off, always write the sectors (that's
> > bdrv_aio_write_zeroes without BDRV_REQ_MAY_UNMAP if the payload is zero,
> > otherwise it's emulated with bdrv_aio_writev)
> >
> > - if the target can "discard and write the specified payload", you can
> > discard, else you must write the sectors with the correct payload
> > (that's bdrv_aio_write_zeroes with BDRV_REQ_MAY_UNMAP).
> >
> > Contrast this with the UNMAP command, which does not make any guarantee
> > on the content of the sectors after the command is completed (a few
> > months ago we agreed that, even if you have discard_zeroes=true in the
> > target, it is fine for UNMAP to do nothing).
> Okay, then let's keep the patches to expose the flag.
Okay, then I can keep those.

Can you give a short hint if my approach with brdv_make_empty is what
you want? I would like to not change the parameters, so use BDRV_REQ_MAY_UNMAP
unconditionally.

int bdrv_make_empty(BlockDriverState *bs)
{
    int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
    int64_t ret, nb_sectors, sector_num = 0;
    int n;

    if (bs->drv->bdrv_make_empty) {
        return bs->drv->bdrv_make_empty(bs);
    }

    for (;;) {
        nb_sectors = target_size - sector_num;
        if (nb_sectors <= 0) {
            return 0;
        }
        if (nb_sectors > INT_MAX) {
            nb_sectors = INT_MAX;
        }
        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
        if (ret & BDRV_BLOCK_ZERO) {
            sector_num += n;
            continue;
        }
        ret = bdrv_write_zeroes(bs, sector_num, n, BDRV_REQ_MAY_UNMAP);
        if (ret < 0) {
            error_report("error writing zeroes at sector %" PRId64 ": %s",
                         sector_num, strerror(-ret));
            return ret;
        }
        sector_num += n;
    }
}