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

[Peter Lieven]

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