[Top][All Lists]

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

Re: [Qemu-devel] [PATCH v2 13/20] block/iscsi: use UNMAP to write zeroes

From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH v2 13/20] block/iscsi: use UNMAP to write zeroes if LBPRZ=1
Date: Thu, 21 Nov 2013 12:54:22 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0

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 &&
+    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.


reply via email to

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