From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTSET-00026S-FX for qemu-devel@nongnu.org; Tue, 08 Oct 2013 04:01:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTSEL-0006d0-1V for qemu-devel@nongnu.org; Tue, 08 Oct 2013 04:01:29 -0400 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:40269 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTSEK-0006ck-M1 for qemu-devel@nongnu.org; Tue, 08 Oct 2013 04:01:20 -0400 Message-ID: <5253BBC3.3050203@kamp.de> Date: Tue, 08 Oct 2013 10:01:07 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1380029714-5239-1-git-send-email-pl@kamp.de> <20131007084259.GF6254@stefanha-thinkpad.redhat.com> <525281F6.5070709@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv3 00/20] block: logical block provisioning enhancements List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , Paolo Bonzini Cc: Kevin Wolf , Anthony Liguori , Stefan Hajnoczi , qemu-devel , ronnie sahlberg On 08.10.2013 09:02, Stefan Hajnoczi wrote: > On Mon, Oct 7, 2013 at 11:42 AM, Paolo Bonzini 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; } }