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

[Peter Lieven <pl@kamp.de>]

On 08.10.2013 09:02, Stefan Hajnoczi wrote:
> On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini <pbonzini@redhat.com> 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;
     }
}