Re: [Qemu-devel] [PATCH 4/4] qemu-img: conditionally discard target on convert

[Peter Lieven]

On 12.09.2013 10:55, Paolo Bonzini wrote:
> Il 12/09/2013 10:52, Peter Lieven ha scritto:
> > On 18.07.2013 16:14, Paolo Bonzini wrote:
> > > Il 18/07/2013 15:55, ronnie sahlberg ha scritto:
> > > > > > bdrv->write_zeroes will use writesame16 and set the unmap flag only if
> > > > > > BDRV_MAY_DISCARD == 1 and BDRV_O_UNMAP == 1 and lbprz == 1.
> > > > When you use WRITESAME16 you can ignore the lbprz flag.
> > > > Just send a WRITESAME16 command with one block of data that is all
> > > > set to zero.
> > > > If the unmap flag is set and if unmapped blocks read back the same as
> > > > the block you provided (all zero)
> > > > then it will unmap those blocks, where possible.
> > > True, so the unmap flag can be set iff BDRV_MAY_DISCARD == 1.  block.c
> > > can take care of checking BDRV_O_UNMAP.
> > Would you add BDRV_MAY_DISCARD to BdrvRequestFlags?
> >
> > This would require making BdrvRequestFlags public.
Is the BdrvRequestFlags the right place to put BRDV_REQ_MAY_UNMAP?
> > Before I start implementation I would still like to verify if we need this
> > flag. E.g. in which case would we do not want to optimize writing zeroes
> > via discard if the user has set BDRV_O_UNMAP?
> For example if the bdrv_write_zeroes is coming from a WRITE SAME command
> (sent by the guest to an emulated SCSI disk).  In this case, we should
> not discard if the UNMAP bit is zero in the request.
>
> The WRITE SAME implementation in scsi-disk.c is old and broken, but the
> block layer should provide enough API to make it right.
Its actually calling discard regardless if the the buffer contents are zero.
So the outcome of this call is undefined.

    case WRITE_SAME_16:
        nb_sectors = scsi_data_cdb_length(r->req.cmd.buf);
        if (bdrv_is_read_only(s->qdev.conf.bs)) {
            scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
            return 0;
        }
        if (!check_lba_range(s, r->req.cmd.lba, nb_sectors)) {
            goto illegal_lba;
        }

        /*
         * We only support WRITE SAME with the unmap bit set for now.
         */
        if (!(req->cmd.buf[1] & 0x8)) {
            goto illegal_request;
        }

        /* The request is used as the AIO opaque value, so add a ref.  */
        scsi_req_ref(&r->req);
        r->req.aiocb = bdrv_aio_discard(s->qdev.conf.bs,
                                        r->req.cmd.lba * (s->qdev.blocksize / 
512),
                                        nb_sectors * (s->qdev.blocksize / 512),
                                        scsi_aio_complete, r);
        return 0;


Peter