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

[Peter Lieven <pl@kamp.de>]

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