qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCHv3 00/20] block: logical block provisioning enhancements
Date: Tue, 08 Oct 2013 10:01:07 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0

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;
    }
}



reply via email to

[Prev in Thread] Current Thread [Next in Thread]