From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49709) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjSpt-0006j1-QZ for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:54:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VjSpl-0000cY-DT for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:54:17 -0500 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:49508 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjSpl-0000cN-29 for qemu-devel@nongnu.org; Thu, 21 Nov 2013 06:54:09 -0500 Message-ID: <528DF46E.5020902@kamp.de> Date: Thu, 21 Nov 2013 12:54:22 +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> <528DF1CC.7090001@kamp.de> <528DF33F.5000405@redhat.com> In-Reply-To: <528DF33F.5000405@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 Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 21.11.2013 12:49, Paolo Bonzini wrote: > 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. it has both. > > 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. i have seen that, if you insist to keep this patch, you have to change the following to return -ENOTSUP in iscsi_co_discard: if (iTask.status == SCSI_STATUS_CHECK_CONDITION) { /* the target might fail with a check condition if it is not happy with the alignment of the UNMAP request we silently fail in this case */ return 0; } > >> 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. You told that the last revision clarifies this part, so there is a change that has been misinterpreted in the past. What is the actual reason to add this tweak? I would like to strictly map write_zeroes to write same. If it is not supported, write plain zeroes. Peter