| Message ID | 20200831150124.206267-10-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On Mon, 2020-08-31 at 18:01 +0300, Maxim Levitsky wrote: > Currently scsi_target_emulate_report_luns iterates > over child devices list twice, and there is guarantee, that > it will not be changed meanwhile. > > This reason for two loops is that it needs to know how much memory > to allocate. > > Avoid this by iterating once, and allocating the memory for the output > dynamically with reserving enought memory so that in practice it won't > be reallocated often. Just too many spelling/grammar mistakes in the commit message. Sorry about that. It should be something like that: Currently scsi_target_emulate_report_luns iterates over the child device list twice, and there is no guarantee that this list is the same in both iterations. The reason for iterating twise is that the first iteration calculates how much memory to allocate. However if we use a dynamic array we can avoid iterating twice, and therefore we avoid this race. Best regards, Maxim Levitsky > > Bugzilla for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1866707 > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > hw/scsi/scsi-bus.c | 62 ++++++++++++++++++++++------------------------ > 1 file changed, 29 insertions(+), 33 deletions(-) > > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c > index feab20b76d..150dee2e6a 100644 > --- a/hw/scsi/scsi-bus.c > +++ b/hw/scsi/scsi-bus.c > @@ -438,19 +438,25 @@ struct SCSITargetReq { > static void store_lun(uint8_t *outbuf, int lun) > { > if (lun < 256) { > + /* Simple logical unit addressing method*/ > + outbuf[0] = 0; > outbuf[1] = lun; > - return; > + } else { > + /* Flat space addressing method */ > + outbuf[0] = 0x40 | (lun >> 8); > + outbuf[1] = (lun & 255); > } > - outbuf[1] = (lun & 255); > - outbuf[0] = (lun >> 8) | 0x40; > } > > static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > { > BusChild *kid; > - int i, len, n; > int channel, id; > - bool found_lun0; > + uint8_t tmp[8] = {0}; > + int len = 0; > + > + /* reserve space for 63 LUNs*/ > + GByteArray *buf = g_byte_array_sized_new(512); > > if (r->req.cmd.xfer < 16) { > return false; > @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > } > channel = r->req.dev->channel; > id = r->req.dev->id; > - found_lun0 = false; > - n = 0; > > - rcu_read_lock(); > > - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { > - DeviceState *qdev = kid->child; > - SCSIDevice *dev = SCSI_DEVICE(qdev); > + /* add size (will be updated later to correct value */ > + g_byte_array_append(buf, tmp, 8); > + len += 8; > > - if (dev->channel == channel && dev->id == id) { > - if (dev->lun == 0) { > - found_lun0 = true; > - } > - n += 8; > - } > - } > - if (!found_lun0) { > - n += 8; > - } > - > - scsi_target_alloc_buf(&r->req, n + 8); > + /* add LUN0 */ > + g_byte_array_append(buf, tmp, 8); > + len += 8; > > - len = MIN(n + 8, r->req.cmd.xfer & ~7); > - memset(r->buf, 0, len); > - stl_be_p(&r->buf[0], n); > - i = found_lun0 ? 8 : 16; > + rcu_read_lock(); > QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { > DeviceState *qdev = kid->child; > SCSIDevice *dev = SCSI_DEVICE(qdev); > > - if (dev->channel == channel && dev->id == id) { > - store_lun(&r->buf[i], dev->lun); > - i += 8; > + if (dev->channel == channel && dev->id == id && dev->lun != 0) { > + store_lun(tmp, dev->lun); > + g_byte_array_append(buf, tmp, 8); > + len += 8; > } > } > - > rcu_read_unlock(); > > - assert(i == n + 8); > - r->len = len; > + r->buf_len = len; > + r->buf = g_byte_array_free(buf, FALSE); > + r->len = MIN(len, r->req.cmd.xfer & ~7); > + > + /* store the LUN list length */ > + stl_be_p(&r->buf[0], len - 8); > + > return true; > } >
On Mon, Aug 31, 2020 at 06:01:24PM +0300, Maxim Levitsky wrote: > Currently scsi_target_emulate_report_luns iterates > over child devices list twice, and there is guarantee, that > it will not be changed meanwhile. > > This reason for two loops is that it needs to know how much memory > to allocate. > > Avoid this by iterating once, and allocating the memory for the output > dynamically with reserving enought memory so that in practice it won't > be reallocated often. > > Bugzilla for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1866707 "Buglink:" is the tag name documented in https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message > static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > { > BusChild *kid; > - int i, len, n; > int channel, id; > - bool found_lun0; > + uint8_t tmp[8] = {0}; > + int len = 0; > + > + /* reserve space for 63 LUNs*/ > + GByteArray *buf = g_byte_array_sized_new(512); > > if (r->req.cmd.xfer < 16) { > return false; buf is leaked. > @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > } > channel = r->req.dev->channel; > id = r->req.dev->id; > - found_lun0 = false; > - n = 0; > > - rcu_read_lock(); > > - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { > - DeviceState *qdev = kid->child; > - SCSIDevice *dev = SCSI_DEVICE(qdev); > + /* add size (will be updated later to correct value */ > + g_byte_array_append(buf, tmp, 8); > + len += 8; Can g_byte_array_size() be used instead of keeping a len local variable? Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Tue, 2020-09-08 at 16:27 +0100, Stefan Hajnoczi wrote: > On Mon, Aug 31, 2020 at 06:01:24PM +0300, Maxim Levitsky wrote: > > Currently scsi_target_emulate_report_luns iterates > > over child devices list twice, and there is guarantee, that > > it will not be changed meanwhile. > > > > This reason for two loops is that it needs to know how much memory > > to allocate. > > > > Avoid this by iterating once, and allocating the memory for the output > > dynamically with reserving enought memory so that in practice it won't > > be reallocated often. > > > > Bugzilla for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1866707 > > "Buglink:" is the tag name documented in > https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message Noted > > > static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > > { > > BusChild *kid; > > - int i, len, n; > > int channel, id; > > - bool found_lun0; > > + uint8_t tmp[8] = {0}; > > + int len = 0; > > + > > + /* reserve space for 63 LUNs*/ > > + GByteArray *buf = g_byte_array_sized_new(512); > > > > if (r->req.cmd.xfer < 16) { > > return false; > > buf is leaked. Oops, will fix > > > @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > > } > > channel = r->req.dev->channel; > > id = r->req.dev->id; > > - found_lun0 = false; > > - n = 0; > > > > - rcu_read_lock(); > > > > - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { > > - DeviceState *qdev = kid->child; > > - SCSIDevice *dev = SCSI_DEVICE(qdev); > > + /* add size (will be updated later to correct value */ > > + g_byte_array_append(buf, tmp, 8); > > + len += 8; > > Can g_byte_array_size() be used instead of keeping a len local variable? Glib don't seem to have this function, I checked the docs. Its seems that they want to convert it to GBytes which is basically immutible verion of GByteArray and it does have g_bytes_get_size. I decided that a local variable while ugly is still better that this. I haven't wrote much code that uses Glib, so I might have missed something though. I had read this reference: https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Best regards, Maxim Levitsky
On Wed, Sep 09, 2020 at 11:20:24AM +0300, Maxim Levitsky wrote: > On Tue, 2020-09-08 at 16:27 +0100, Stefan Hajnoczi wrote: > > On Mon, Aug 31, 2020 at 06:01:24PM +0300, Maxim Levitsky wrote: > > > @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) > > > } > > > channel = r->req.dev->channel; > > > id = r->req.dev->id; > > > - found_lun0 = false; > > > - n = 0; > > > > > > - rcu_read_lock(); > > > > > > - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { > > > - DeviceState *qdev = kid->child; > > > - SCSIDevice *dev = SCSI_DEVICE(qdev); > > > + /* add size (will be updated later to correct value */ > > > + g_byte_array_append(buf, tmp, 8); > > > + len += 8; > > > > Can g_byte_array_size() be used instead of keeping a len local variable? > Glib don't seem to have this function, I checked the docs. > Its seems that they want to convert it to GBytes which is basically immutible verion > of GByteArray and it does have g_bytes_get_size. > I decided that a local variable while ugly is still better that this. > > > I haven't wrote much code that uses Glib, so I might have missed something though. > I had read this reference: > https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html Oops, you're right. GByteArray != GBytes. The local variable makes sense. Stefan
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index feab20b76d..150dee2e6a 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -438,19 +438,25 @@ struct SCSITargetReq { static void store_lun(uint8_t *outbuf, int lun) { if (lun < 256) { + /* Simple logical unit addressing method*/ + outbuf[0] = 0; outbuf[1] = lun; - return; + } else { + /* Flat space addressing method */ + outbuf[0] = 0x40 | (lun >> 8); + outbuf[1] = (lun & 255); } - outbuf[1] = (lun & 255); - outbuf[0] = (lun >> 8) | 0x40; } static bool scsi_target_emulate_report_luns(SCSITargetReq *r) { BusChild *kid; - int i, len, n; int channel, id; - bool found_lun0; + uint8_t tmp[8] = {0}; + int len = 0; + + /* reserve space for 63 LUNs*/ + GByteArray *buf = g_byte_array_sized_new(512); if (r->req.cmd.xfer < 16) { return false; @@ -460,46 +466,36 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r) } channel = r->req.dev->channel; id = r->req.dev->id; - found_lun0 = false; - n = 0; - rcu_read_lock(); - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { - DeviceState *qdev = kid->child; - SCSIDevice *dev = SCSI_DEVICE(qdev); + /* add size (will be updated later to correct value */ + g_byte_array_append(buf, tmp, 8); + len += 8; - if (dev->channel == channel && dev->id == id) { - if (dev->lun == 0) { - found_lun0 = true; - } - n += 8; - } - } - if (!found_lun0) { - n += 8; - } - - scsi_target_alloc_buf(&r->req, n + 8); + /* add LUN0 */ + g_byte_array_append(buf, tmp, 8); + len += 8; - len = MIN(n + 8, r->req.cmd.xfer & ~7); - memset(r->buf, 0, len); - stl_be_p(&r->buf[0], n); - i = found_lun0 ? 8 : 16; + rcu_read_lock(); QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) { DeviceState *qdev = kid->child; SCSIDevice *dev = SCSI_DEVICE(qdev); - if (dev->channel == channel && dev->id == id) { - store_lun(&r->buf[i], dev->lun); - i += 8; + if (dev->channel == channel && dev->id == id && dev->lun != 0) { + store_lun(tmp, dev->lun); + g_byte_array_append(buf, tmp, 8); + len += 8; } } - rcu_read_unlock(); - assert(i == n + 8); - r->len = len; + r->buf_len = len; + r->buf = g_byte_array_free(buf, FALSE); + r->len = MIN(len, r->req.cmd.xfer & ~7); + + /* store the LUN list length */ + stl_be_p(&r->buf[0], len - 8); + return true; }
Currently scsi_target_emulate_report_luns iterates over child devices list twice, and there is guarantee, that it will not be changed meanwhile. This reason for two loops is that it needs to know how much memory to allocate. Avoid this by iterating once, and allocating the memory for the output dynamically with reserving enought memory so that in practice it won't be reallocated often. Bugzilla for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1866707 Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- hw/scsi/scsi-bus.c | 62 ++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 33 deletions(-)