* [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size @ 2018-03-22 7:38 Fam Zheng 2018-03-22 9:11 ` Paolo Bonzini 2018-03-22 12:19 ` Daniel Henrique Barboza 0 siblings, 2 replies; 6+ messages in thread From: Fam Zheng @ 2018-03-22 7:38 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Fam Zheng, eblake Some backends report big max_io_sectors. Making min_io_size the same value in this case will make it impossible for guest to align memory, therefore the disk may not be usable at all. Change the default behavior (when min_io_size and opt_io_size are not specified in the command line), do not assume max_io_sectors is a good value for opt_io_size and min_io_size, use 512 instead. Reported-by: David Gibson <dgibson@redhat.com> Signed-off-by: Fam Zheng <famz@redhat.com> --- hw/scsi/scsi-disk.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 5b7a48f5a5..76e3c9eaa4 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) /* min_io_size and opt_io_size can't be greater than * max_io_sectors */ - min_io_size = - MIN_NON_ZERO(min_io_size, max_io_sectors); - opt_io_size = - MIN_NON_ZERO(opt_io_size, max_io_sectors); + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); } /* required VPD size with unmap support */ buflen = 0x40; -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size 2018-03-22 7:38 [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size Fam Zheng @ 2018-03-22 9:11 ` Paolo Bonzini 2018-03-22 12:19 ` Daniel Henrique Barboza 1 sibling, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2018-03-22 9:11 UTC (permalink / raw) To: Fam Zheng, qemu-devel On 22/03/2018 08:38, Fam Zheng wrote: > Some backends report big max_io_sectors. Making min_io_size the same > value in this case will make it impossible for guest to align memory, > therefore the disk may not be usable at all. > > Change the default behavior (when min_io_size and opt_io_size are not > specified in the command line), do not assume max_io_sectors is a good > value for opt_io_size and min_io_size, use 512 instead. > > Reported-by: David Gibson <dgibson@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > hw/scsi/scsi-disk.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 5b7a48f5a5..76e3c9eaa4 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > /* min_io_size and opt_io_size can't be greater than > * max_io_sectors */ > - min_io_size = > - MIN_NON_ZERO(min_io_size, max_io_sectors); > - opt_io_size = > - MIN_NON_ZERO(opt_io_size, max_io_sectors); > + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); > + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); There are a few easily fixed issues with your chosen defaults, though the problem obviously makes sense: 1) the values are in sectors - since you chose 512, it's not clear if you meant it to be 512 bytes or 512 sectors. :) 512 sectors (256 KiB or 2 MiB depending on logical block size) is still too much for the min_io_size. The min_io_size default (if it is 0) is the physical block size, so I think we should make the min_io_size either 0 or the physical block size. 2) For the opt_io_size, 256 KiB on the other hand is probably too little. On my laptop (NVMe disk) a transfer size of 8 MiB is twice as fast compared to a transfer size of 256 KiB, and 16 MiB or 32 MiB is a little faster too. I would either leave zero as the default, or pick something around 16-32 MiB. Thanks, Paolo > } > /* required VPD size with unmap support */ > buflen = 0x40; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size 2018-03-22 7:38 [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size Fam Zheng 2018-03-22 9:11 ` Paolo Bonzini @ 2018-03-22 12:19 ` Daniel Henrique Barboza 2018-03-26 7:26 ` Fam Zheng 1 sibling, 1 reply; 6+ messages in thread From: Daniel Henrique Barboza @ 2018-03-22 12:19 UTC (permalink / raw) To: Fam Zheng, qemu-devel; +Cc: Paolo Bonzini Hi, On 03/22/2018 04:38 AM, Fam Zheng wrote: > Some backends report big max_io_sectors. Making min_io_size the same > value in this case will make it impossible for guest to align memory, > therefore the disk may not be usable at all. > > Change the default behavior (when min_io_size and opt_io_size are not > specified in the command line), do not assume max_io_sectors is a good > value for opt_io_size and min_io_size, use 512 instead. > > Reported-by: David Gibson <dgibson@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > hw/scsi/scsi-disk.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 5b7a48f5a5..76e3c9eaa4 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > /* min_io_size and opt_io_size can't be greater than > * max_io_sectors */ > - min_io_size = > - MIN_NON_ZERO(min_io_size, max_io_sectors); > - opt_io_size = > - MIN_NON_ZERO(opt_io_size, max_io_sectors); > + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); > + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); > } This code you're changing was added in d082d16a5c ("consider bl->max_transfer .."). I've borrowed this logic from scsi-generic.c, scsi_read_complete: if (s->type == TYPE_DISK && r->req.cmd.buf[0] == INQUIRY && r->req.cmd.buf[2] == 0xb0) { uint32_t max_transfer = blk_get_max_transfer(s->conf.blk) / s->blocksize; assert(max_transfer); stl_be_p(&r->buf[8], max_transfer); /* Also take care of the opt xfer len. */ stl_be_p(&r->buf[12], MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12]))); } Unless I've misunderstood the bug, you will want to change this code too. Otherwise you'll fix it with emulated disks but it might appear when using SCSI passthrough. Thanks, Daniel > /* required VPD size with unmap support */ > buflen = 0x40; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size 2018-03-22 12:19 ` Daniel Henrique Barboza @ 2018-03-26 7:26 ` Fam Zheng 2018-03-27 3:44 ` David Gibson 0 siblings, 1 reply; 6+ messages in thread From: Fam Zheng @ 2018-03-26 7:26 UTC (permalink / raw) To: Daniel Henrique Barboza, dgibson; +Cc: qemu-devel, Paolo Bonzini On Thu, 03/22 09:19, Daniel Henrique Barboza wrote: > Hi, > > On 03/22/2018 04:38 AM, Fam Zheng wrote: > > Some backends report big max_io_sectors. Making min_io_size the same > > value in this case will make it impossible for guest to align memory, > > therefore the disk may not be usable at all. > > > > Change the default behavior (when min_io_size and opt_io_size are not > > specified in the command line), do not assume max_io_sectors is a good > > value for opt_io_size and min_io_size, use 512 instead. > > > > Reported-by: David Gibson <dgibson@redhat.com> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > hw/scsi/scsi-disk.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > > index 5b7a48f5a5..76e3c9eaa4 100644 > > --- a/hw/scsi/scsi-disk.c > > +++ b/hw/scsi/scsi-disk.c > > @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > > > /* min_io_size and opt_io_size can't be greater than > > * max_io_sectors */ > > - min_io_size = > > - MIN_NON_ZERO(min_io_size, max_io_sectors); > > - opt_io_size = > > - MIN_NON_ZERO(opt_io_size, max_io_sectors); > > + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); > > + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); > > } > > This code you're changing was added in d082d16a5c ("consider > bl->max_transfer .."). > I've borrowed this logic from scsi-generic.c, scsi_read_complete: > > if (s->type == TYPE_DISK && > r->req.cmd.buf[0] == INQUIRY && > r->req.cmd.buf[2] == 0xb0) { > uint32_t max_transfer = > blk_get_max_transfer(s->conf.blk) / s->blocksize; > > assert(max_transfer); > stl_be_p(&r->buf[8], max_transfer); > /* Also take care of the opt xfer len. */ > stl_be_p(&r->buf[12], > MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12]))); > } > > > Unless I've misunderstood the bug, you will want to change this code too. > Otherwise > you'll fix it with emulated disks but it might appear when using SCSI > passthrough. I am assuming (because I don't have a reproducer myself) what matters is min_io_size here. David, could you help test if you see the same problem with "-device scsi-block"? If we I'll patch scsi-generic.c in v2 too. Fam ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size 2018-03-26 7:26 ` Fam Zheng @ 2018-03-27 3:44 ` David Gibson 2018-03-27 16:28 ` Fam Zheng 0 siblings, 1 reply; 6+ messages in thread From: David Gibson @ 2018-03-27 3:44 UTC (permalink / raw) To: Fam Zheng; +Cc: Daniel Henrique Barboza, qemu-devel, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 4286 bytes --] On Mon, 26 Mar 2018 15:26:39 +0800 Fam Zheng <famz@redhat.com> wrote: > On Thu, 03/22 09:19, Daniel Henrique Barboza wrote: > > Hi, > > > > On 03/22/2018 04:38 AM, Fam Zheng wrote: > > > Some backends report big max_io_sectors. Making min_io_size the same > > > value in this case will make it impossible for guest to align memory, > > > therefore the disk may not be usable at all. > > > > > > Change the default behavior (when min_io_size and opt_io_size are not > > > specified in the command line), do not assume max_io_sectors is a good > > > value for opt_io_size and min_io_size, use 512 instead. > > > > > > Reported-by: David Gibson <dgibson@redhat.com> > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > --- > > > hw/scsi/scsi-disk.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > > > index 5b7a48f5a5..76e3c9eaa4 100644 > > > --- a/hw/scsi/scsi-disk.c > > > +++ b/hw/scsi/scsi-disk.c > > > @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > > > > > /* min_io_size and opt_io_size can't be greater than > > > * max_io_sectors */ > > > - min_io_size = > > > - MIN_NON_ZERO(min_io_size, max_io_sectors); > > > - opt_io_size = > > > - MIN_NON_ZERO(opt_io_size, max_io_sectors); > > > + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); > > > + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); > > > } > > > > This code you're changing was added in d082d16a5c ("consider > > bl->max_transfer .."). > > I've borrowed this logic from scsi-generic.c, scsi_read_complete: > > > > if (s->type == TYPE_DISK && > > r->req.cmd.buf[0] == INQUIRY && > > r->req.cmd.buf[2] == 0xb0) { > > uint32_t max_transfer = > > blk_get_max_transfer(s->conf.blk) / s->blocksize; > > > > assert(max_transfer); > > stl_be_p(&r->buf[8], max_transfer); > > /* Also take care of the opt xfer len. */ > > stl_be_p(&r->buf[12], > > MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12]))); > > } > > > > > > Unless I've misunderstood the bug, you will want to change this code too. > > Otherwise > > you'll fix it with emulated disks but it might appear when using SCSI > > passthrough. > > I am assuming (because I don't have a reproducer myself) Sorry, I should have given you specific reproduce instructions. You don't need a POWER host - I've verified that the bug trips under TCG. 1. Grab a RHEL ppc64le install image (other installers could well also hit it, but I haven't tried them) 2. Build current qemu master, including the ppc64-softmmu target 3. Create a fresh new guest disk image qemu-img create -f qcow2 disk.qcow2 20G 4. Attempt to install the new guest: $QEMU -nodefaults -nographic -machine pseries \ -cpu POWER8 -smp 1 -m 1G \ -chardev stdio,id=conmon,mux=on,signal=off \ -device spapr-vty,chardev=conmon \ -mon conmon \ -device virtio-scsi-pci,id=scsi \ -drive file=disk.qcow2,if=none,format=qcow2,id=hd \ -device scsi-disk,drive=hd,bus=scsi.0 \ -drive file=RHEL-7.4-20170711.0-Server-ppc64le-dvd1.iso,format=raw,media=cdrom,if=none,id=cd \ -device scsi-cd,drive=cd,bus=scsi.0 That's using the RHEL7.4 GA image, a recent 7.5 snapshot also works as may others. > what matters is > min_io_size here. > > David, could you help test if you see the same problem with "-device > scsi-block"? If we I'll patch scsi-generic.c in v2 too. I'm not sure exactly what you want me to check here? You mean putting the guest disk on a scsi-block instead of scsi-disk? That's a bit more fiddly, since I have to find a block device to back it instead of an image. -- David Gibson <dgibson@redhat.com> Principal Software Engineer, Virtualization, Red Hat [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size 2018-03-27 3:44 ` David Gibson @ 2018-03-27 16:28 ` Fam Zheng 0 siblings, 0 replies; 6+ messages in thread From: Fam Zheng @ 2018-03-27 16:28 UTC (permalink / raw) To: David Gibson; +Cc: Daniel Henrique Barboza, qemu-devel, Paolo Bonzini On Tue, 03/27 14:44, David Gibson wrote: > On Mon, 26 Mar 2018 15:26:39 +0800 > Fam Zheng <famz@redhat.com> wrote: > > > On Thu, 03/22 09:19, Daniel Henrique Barboza wrote: > > > Hi, > > > > > > On 03/22/2018 04:38 AM, Fam Zheng wrote: > > > > Some backends report big max_io_sectors. Making min_io_size the same > > > > value in this case will make it impossible for guest to align memory, > > > > therefore the disk may not be usable at all. > > > > > > > > Change the default behavior (when min_io_size and opt_io_size are not > > > > specified in the command line), do not assume max_io_sectors is a good > > > > value for opt_io_size and min_io_size, use 512 instead. > > > > > > > > Reported-by: David Gibson <dgibson@redhat.com> > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > --- > > > > hw/scsi/scsi-disk.c | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > > > > index 5b7a48f5a5..76e3c9eaa4 100644 > > > > --- a/hw/scsi/scsi-disk.c > > > > +++ b/hw/scsi/scsi-disk.c > > > > @@ -714,10 +714,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > > > > > > > /* min_io_size and opt_io_size can't be greater than > > > > * max_io_sectors */ > > > > - min_io_size = > > > > - MIN_NON_ZERO(min_io_size, max_io_sectors); > > > > - opt_io_size = > > > > - MIN_NON_ZERO(opt_io_size, max_io_sectors); > > > > + min_io_size = MIN(min_io_size ? : 512, max_io_sectors); > > > > + opt_io_size = MIN(opt_io_size ? : 512, max_io_sectors); > > > > } > > > > > > This code you're changing was added in d082d16a5c ("consider > > > bl->max_transfer .."). > > > I've borrowed this logic from scsi-generic.c, scsi_read_complete: > > > > > > if (s->type == TYPE_DISK && > > > r->req.cmd.buf[0] == INQUIRY && > > > r->req.cmd.buf[2] == 0xb0) { > > > uint32_t max_transfer = > > > blk_get_max_transfer(s->conf.blk) / s->blocksize; > > > > > > assert(max_transfer); > > > stl_be_p(&r->buf[8], max_transfer); > > > /* Also take care of the opt xfer len. */ > > > stl_be_p(&r->buf[12], > > > MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12]))); > > > } > > > > > > > > > Unless I've misunderstood the bug, you will want to change this code too. > > > Otherwise > > > you'll fix it with emulated disks but it might appear when using SCSI > > > passthrough. > > > > I am assuming (because I don't have a reproducer myself) > > Sorry, I should have given you specific reproduce instructions. You > don't need a POWER host - I've verified that the bug trips under TCG. > > 1. Grab a RHEL ppc64le install image (other installers could well > also hit it, but I haven't tried them) > 2. Build current qemu master, including the ppc64-softmmu target > 3. Create a fresh new guest disk image > qemu-img create -f qcow2 disk.qcow2 20G > 4. Attempt to install the new guest: > $QEMU -nodefaults -nographic -machine pseries \ > -cpu POWER8 -smp 1 -m 1G \ > -chardev stdio,id=conmon,mux=on,signal=off \ > -device spapr-vty,chardev=conmon \ > -mon conmon \ > -device virtio-scsi-pci,id=scsi \ > -drive file=disk.qcow2,if=none,format=qcow2,id=hd \ > -device scsi-disk,drive=hd,bus=scsi.0 \ > -drive file=RHEL-7.4-20170711.0-Server-ppc64le-dvd1.iso,format=raw,media=cdrom,if=none,id=cd \ > -device scsi-cd,drive=cd,bus=scsi.0 > > That's using the RHEL7.4 GA image, a recent 7.5 snapshot also works as may others. Thanks, your reproducer works. So I've verified that fixing min_io_size alone will eliminate the problem. So there is no such problem for scsi-block. Of course aligning opt_io_size up to max_io_size is dubious but as far as fixing guest I/O, I think touching up scsi-disk is okay. I'll address Paolo's comments and post v2. Fam ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-27 16:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-22 7:38 [Qemu-devel] [PATCH] scsi-disk: Don't enlarge min_io_size to max_io_size Fam Zheng 2018-03-22 9:11 ` Paolo Bonzini 2018-03-22 12:19 ` Daniel Henrique Barboza 2018-03-26 7:26 ` Fam Zheng 2018-03-27 3:44 ` David Gibson 2018-03-27 16:28 ` Fam Zheng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.