Re: [Qemu-devel] [PATCH 1/3] scsi-disk: Limit zero write request to SCSI_WRITE_SAME_MAX

[Fam Zheng]

On Fri, 04/24 10:50, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 10:33, Fam Zheng wrote:
> > SBC-4 says:
> > 
> >     If the number of logical blocks specified to be unmapped or written
> >     exceeds the value indicated in the MAXIMUM WRITE SAME LENGTH field
> >     in the Block Limits VPD page (see 6.6.4), then the device server
> >     shall terminate the command with CHECK CONDITION status with the
> >     sense key set to ILLEGAL REQUEST and the additional sense code set
> >     to INVALID FIELD IN CDB.
> > 
> > Check the request size to match the spec.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  hw/scsi/scsi-disk.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 54d71f4..b748982 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -1707,6 +1707,11 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq 
> > *r, uint8_t *inbuf)
> >          return;
> >      }
> >  
> > +    if (nb_sectors * (s->qdev.blocksize / 512) * 512 > 
> > SCSI_WRITE_SAME_MAX) {
> > +        scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
> > +        return;
> > +    }
> > +
> >      if (buffer_is_zero(inbuf, s->qdev.blocksize)) {
> >          int flags = (req->cmd.buf[1] & 0x8) ? BDRV_REQ_MAY_UNMAP : 0;
> >  
> > @@ -1726,7 +1731,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq 
> > *r, uint8_t *inbuf)
> >      data->r = r;
> >      data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> >      data->nb_sectors = nb_sectors * (s->qdev.blocksize / 512);
> > -    data->iov.iov_len = MIN(data->nb_sectors * 512, SCSI_WRITE_SAME_MAX);
> > +    data->iov.iov_len = data->nb_sectors * 512;
> >      data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
> >                                                data->iov.iov_len);
> >      qemu_iovec_init_external(&data->qiov, &data->iov, 1);
> > 
> 
> SCSI_WRITE_SAME_MAX is not exported as the MAXIMUM WRITE SAME LENGTH in
> the VPD.  In fact, we don't export anything and prefer to get very large
> requests, because then we can choose ourselves how to split them and
> serialize them.  By contrast, if you have a low limit, some guests
> including Linux will send a lot of concurrent WRITE SAME requests which
> will have a huge latency.
> 
> In addition, SCSI_WRITE_SAME_MAX is 512 *kilo*bytes.  That's really
> little :) as some disks have a multi-*mega*byte unmap granularity.  So
> what is SCSI_WRITE_SAME_MAX?
> 
> Simply, when we're asked to do a WRITE SAME with non-zero payload, we
> build a 512 KiB buffer and fill it repeatedly with the pattern.  Then
> the I/O is split in multiple writes, each covering 512 KiB except
> possibly the last.  Note that non-zero WRITE SAME is not a fast path.
> 
> So this patch is not correct.

OK, I get it. Thanks for explanation.

> However, it shouldn't be a problem for
> the rest of the series.

It is. The request has to be splitted to aligned part and unaligned part
because the latter requires a buffer, as we don't like unbounded allocation.

Fam