| Message ID | 20200925172604.2142227-9-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote: > From: Maxim Levitsky <mlevitsk@redhat.com> > > Add scsi_device_get which finds the scsi device > and takes a reference to it. > > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Message-Id: <20200913160259.32145-8-mlevitsk@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > Compared to Maxim's patch, I am avoiding the extra argument > to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD() > out of do_scsi_device_find itself. Which is a good idea, although my mindset was like, I got a device, lets just grab a ref to it before it disappears and then do whatever I want. The extra argument was ugly no doubt though. Best regards, Maxim Levitsky > > hw/scsi/scsi-bus.c | 11 +++++++++++ > include/hw/scsi/scsi.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c > index 7599113efe..eda8cb7e70 100644 > --- a/hw/scsi/scsi-bus.c > +++ b/hw/scsi/scsi-bus.c > @@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) > return do_scsi_device_find(bus, channel, id, lun, false); > } > > +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun) > +{ > + SCSIDevice *d; > + RCU_READ_LOCK_GUARD(); > + d = do_scsi_device_find(bus, channel, id, lun, false); > + if (d) { > + object_ref(d); > + } > + return d; > +} > + > static void scsi_device_realize(SCSIDevice *s, Error **errp) > { > SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s); > diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h > index 7a55cdbd74..09fa5c9d2a 100644 > --- a/include/hw/scsi/scsi.h > +++ b/include/hw/scsi/scsi.h > @@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed); > int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size, > uint8_t *buf, uint8_t buf_size); > SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun); > +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun); > > /* scsi-generic.c. */ > extern const SCSIReqOps scsi_generic_req_ops;
On 30/09/20 16:32, Maxim Levitsky wrote: >> Compared to Maxim's patch, I am avoiding the extra argument >> to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD() >> out of do_scsi_device_find itself. > Which is a good idea, although my mindset was like, I got a device, > lets just grab a ref to it before it disappears and then do > whatever I want. Understood, but "I got a device, I know I'm under RCU so it can't disappear" is more efficient and just as common. This also explains the difference in patch 7. Paolo
On Wed, 2020-09-30 at 19:46 +0200, Paolo Bonzini wrote: > On 30/09/20 16:32, Maxim Levitsky wrote: > > > Compared to Maxim's patch, I am avoiding the extra argument > > > to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD() > > > out of do_scsi_device_find itself. > > Which is a good idea, although my mindset was like, I got a device, > > lets just grab a ref to it before it disappears and then do > > whatever I want. > > Understood, but "I got a device, I know I'm under RCU so it can't > disappear" is more efficient and just as common. This also explains the > difference in patch 7. Fair point. I am still learing to correctly use RCU. Best regards, Maxim Levitsky > > Paolo >
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 7599113efe..eda8cb7e70 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun) return do_scsi_device_find(bus, channel, id, lun, false); } +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun) +{ + SCSIDevice *d; + RCU_READ_LOCK_GUARD(); + d = do_scsi_device_find(bus, channel, id, lun, false); + if (d) { + object_ref(d); + } + return d; +} + static void scsi_device_realize(SCSIDevice *s, Error **errp) { SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s); diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index 7a55cdbd74..09fa5c9d2a 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -190,6 +190,7 @@ int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed); int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size, uint8_t *buf, uint8_t buf_size); SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun); +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun); /* scsi-generic.c. */ extern const SCSIReqOps scsi_generic_req_ops;