From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47650) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjSfE-0002DD-C7 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:43:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VjSf3-0005rP-Kn for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:43:16 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:54584 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjSf3-0005rF-94 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:43:05 -0500 Message-ID: <528DF1CC.7090001@kamp.de> Date: Thu, 21 Nov 2013 12:43:08 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1384880863-10434-1-git-send-email-pbonzini@redhat.com> <1384880863-10434-14-git-send-email-pbonzini@redhat.com> In-Reply-To: <1384880863-10434-14-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 13/20] block/iscsi: use UNMAP to write zeroes if LBPRZ=1 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com On 19.11.2013 18:07, Paolo Bonzini wrote: > The latest revision of SCSI SBC clarifies the semantics of LBPRZ > in a way that lets the iscsi driver remove the distinction between > bdrv_unallocated_blocks_are_zero and bdrv_can_write_zeroes_with_unmap. > See Table 8: > > "[If] the LBA became mapped as the result of an autonomous transition, > and no write command has specified that LBA since the LBA was mapped, > [a read operation returns the] logical block data that would be returned > if that autonomous transition had not occurred and the LBA had remained > unmapped." > > Thus, we can assume that even after an UNMAP command, unallocated blocks > return zero. The distinction may remain for other drivers, for example > qcow2 or qed or vmdk. > > Signed-off-by: Paolo Bonzini > --- > block/iscsi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 93fee6d..63b451d 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -984,6 +984,9 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, > > if ((flags & BDRV_REQ_MAY_UNMAP) && !iscsilun->lbp.lbpws) { > /* WRITE SAME with UNMAP is not supported by the target */ > + if (iscsilun->lbp.lbpu && iscsilun->lbprz) { > + return iscsi_co_discard(bs, sector_num, nb_sectors); > + } > return -ENOTSUP; > } > > @@ -1579,7 +1582,7 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) > { > IscsiLun *iscsilun = bs->opaque; > bdi->unallocated_blocks_are_zero = !!iscsilun->lbprz; > - bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws; > + bdi->can_write_zeroes_with_unmap = !!iscsilun->lbprz; > return 0; > } > I would definetly not do that! I have seen at least my target to execute only parts of a discard request. Additionally in our semantic a discard request may silently fail. In general this could lead to data corruption due to broken implementations. Peter