From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48802)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1VjSlI-00059u-Ti
	for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:49:38 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1VjSlC-0007fX-VD
	for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:49:32 -0500
Received: from mx1.redhat.com ([209.132.183.28]:4841)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1VjSlC-0007fR-Nf
	for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:49:26 -0500
Message-ID: <528DF33F.5000405@redhat.com>
Date: Thu, 21 Nov 2013 12:49:19 +0100
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1384880863-10434-1-git-send-email-pbonzini@redhat.com>
	<1384880863-10434-14-git-send-email-pbonzini@redhat.com>
	<528DF1CC.7090001@kamp.de>
In-Reply-To: <528DF1CC.7090001@kamp.de>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 13/20] block/iscsi: use UNMAP to write
	zeroes if LBPRZ=1
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Lieven <pl@kamp.de>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

Il 21/11/2013 12:43, Peter Lieven ha scritto:
>>
>>   @@ -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.

Does that target have LBPRZ and LBPU but not LBPWS?  Note that I'm still
preferring WRITE SAME to UNMAP if both are available.

If so, then this patch is indeed problematic.  Otherwise, it's just
making the same assumptions that Linux has been making forever.

> Additionally in our semantic a discard request may silently fail.

That doesn't matter, the silent failure is handled in block.c.  Here I'm
calling iscsi_co_discard, not bdrv_co_discard.  If it returns ENOTSUP,
it is passed up to bdrv_co_do_write_zeroes which will fall back to writes.

> In general this could lead to data corruption
> due to broken implementations.

A broken implementation could also have LBPWS=1 and execute only the
aligned parts of a WRITE SAME with UNMAP request.

Paolo