| Message ID | 20181016153820.29487-3-mfo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve our SAUCE for virtio-scsi reqs counter (fix CPU soft lockup) | expand |
On 16.10.18 17:38, Mauricio Faria de Oliveira wrote: > BugLink: https://bugs.launchpad.net/bugs/1798110 > > The commit a6c038a533f3 ("UBUNTU: SAUCE: (no-up) virtio-scsi: Increment > reqs counter.") changed the upstream signature of virtscsi_pick_vq_mq() > just to pass the 'struct virtio_scsi_target_state *tgt' pointer, which > can be derived from the 'scsi_cmnd *sc' pointer with the scsi_target() > macro (it indeed is, in many sites in this source file.) > > This doesn't functionally matter because that function is static, thus > not called externally, but in order to maintain/reestablish the same > signature as upstream (since it's not really required to change it), > revert that part of the commit and use the scsi_target() macro, as > used elsewhere, to get the 'tgt' pointer from 'sc'. > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> > Tested-by: Mauricio Faria de Oliveira <mfo@canonical.com> > Tested-by: David Coronel <david.coronel@canonical.com> > --- I was thinking about this one a while but in the end I think this does not qualify for SRU. Even less by piggy-backing on a fix for a completely different issue. But even individually it does not solve any bug (not even an issue for external drivers using the interface as the function is static). For those reasons I do not think this should be added. Lets only fix the bug that needs fixing. -Stefan > drivers/scsi/virtio_scsi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 572722e86bef..377ef50d57b4 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -592,10 +592,12 @@ static int virtscsi_queuecommand_single(struct Scsi_Host *sh, > } > > static struct virtio_scsi_vq *virtscsi_pick_vq_mq(struct virtio_scsi *vscsi, > - struct virtio_scsi_target_state *tgt, struct scsi_cmnd *sc) > + struct scsi_cmnd *sc) > { > u32 tag = blk_mq_unique_tag(sc->request); > u16 hwq = blk_mq_unique_tag_to_hwq(tag); > + struct virtio_scsi_target_state *tgt = > + scsi_target(sc->device)->hostdata; > > atomic_inc(&tgt->reqs); > return &vscsi->req_vqs[hwq]; > @@ -647,7 +649,7 @@ static int virtscsi_queuecommand_multi(struct Scsi_Host *sh, > struct virtio_scsi_vq *req_vq; > > if (shost_use_blk_mq(sh)) > - req_vq = virtscsi_pick_vq_mq(vscsi, tgt, sc); > + req_vq = virtscsi_pick_vq_mq(vscsi, sc); > else > req_vq = virtscsi_pick_vq(vscsi, tgt); > >
On Wed, Oct 17, 2018 at 5:20 AM Stefan Bader <stefan.bader@canonical.com> wrote: > > On 16.10.18 17:38, Mauricio Faria de Oliveira wrote: > > BugLink: https://bugs.launchpad.net/bugs/1798110 > > > > The commit a6c038a533f3 ("UBUNTU: SAUCE: (no-up) virtio-scsi: Increment > > reqs counter.") changed the upstream signature of virtscsi_pick_vq_mq() > > just to pass the 'struct virtio_scsi_target_state *tgt' pointer, which > > can be derived from the 'scsi_cmnd *sc' pointer with the scsi_target() > > macro (it indeed is, in many sites in this source file.) > > > > This doesn't functionally matter because that function is static, thus > > not called externally, but in order to maintain/reestablish the same > > signature as upstream (since it's not really required to change it), > > revert that part of the commit and use the scsi_target() macro, as > > used elsewhere, to get the 'tgt' pointer from 'sc'. > > > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> > > Tested-by: Mauricio Faria de Oliveira <mfo@canonical.com> > > Tested-by: David Coronel <david.coronel@canonical.com> > > --- > > I was thinking about this one a while but in the end I think this does not > qualify for SRU. Even less by piggy-backing on a fix for a completely different > issue. But even individually it does not solve any bug (not even an issue for > external drivers using the interface as the function is static). > For those reasons I do not think this should be added. Lets only fix the bug > that needs fixing. Good point. Indeed it's not a fix, so I guess it would only be worth it if it actually caused problems for other patches (i.e., actual fixes) to apply, which is not the case. Thanks for reviewing. > > -Stefan > > > drivers/scsi/virtio_scsi.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > > index 572722e86bef..377ef50d57b4 100644 > > --- a/drivers/scsi/virtio_scsi.c > > +++ b/drivers/scsi/virtio_scsi.c > > @@ -592,10 +592,12 @@ static int virtscsi_queuecommand_single(struct Scsi_Host *sh, > > } > > > > static struct virtio_scsi_vq *virtscsi_pick_vq_mq(struct virtio_scsi *vscsi, > > - struct virtio_scsi_target_state *tgt, struct scsi_cmnd *sc) > > + struct scsi_cmnd *sc) > > { > > u32 tag = blk_mq_unique_tag(sc->request); > > u16 hwq = blk_mq_unique_tag_to_hwq(tag); > > + struct virtio_scsi_target_state *tgt = > > + scsi_target(sc->device)->hostdata; > > > > atomic_inc(&tgt->reqs); > > return &vscsi->req_vqs[hwq]; > > @@ -647,7 +649,7 @@ static int virtscsi_queuecommand_multi(struct Scsi_Host *sh, > > struct virtio_scsi_vq *req_vq; > > > > if (shost_use_blk_mq(sh)) > > - req_vq = virtscsi_pick_vq_mq(vscsi, tgt, sc); > > + req_vq = virtscsi_pick_vq_mq(vscsi, sc); > > else > > req_vq = virtscsi_pick_vq(vscsi, tgt); > > > > > >
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 572722e86bef..377ef50d57b4 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -592,10 +592,12 @@ static int virtscsi_queuecommand_single(struct Scsi_Host *sh, } static struct virtio_scsi_vq *virtscsi_pick_vq_mq(struct virtio_scsi *vscsi, - struct virtio_scsi_target_state *tgt, struct scsi_cmnd *sc) + struct scsi_cmnd *sc) { u32 tag = blk_mq_unique_tag(sc->request); u16 hwq = blk_mq_unique_tag_to_hwq(tag); + struct virtio_scsi_target_state *tgt = + scsi_target(sc->device)->hostdata; atomic_inc(&tgt->reqs); return &vscsi->req_vqs[hwq]; @@ -647,7 +649,7 @@ static int virtscsi_queuecommand_multi(struct Scsi_Host *sh, struct virtio_scsi_vq *req_vq; if (shost_use_blk_mq(sh)) - req_vq = virtscsi_pick_vq_mq(vscsi, tgt, sc); + req_vq = virtscsi_pick_vq_mq(vscsi, sc); else req_vq = virtscsi_pick_vq(vscsi, tgt);
BugLink: https://bugs.launchpad.net/bugs/1798110 The commit a6c038a533f3 ("UBUNTU: SAUCE: (no-up) virtio-scsi: Increment reqs counter.") changed the upstream signature of virtscsi_pick_vq_mq() just to pass the 'struct virtio_scsi_target_state *tgt' pointer, which can be derived from the 'scsi_cmnd *sc' pointer with the scsi_target() macro (it indeed is, in many sites in this source file.) This doesn't functionally matter because that function is static, thus not called externally, but in order to maintain/reestablish the same signature as upstream (since it's not really required to change it), revert that part of the commit and use the scsi_target() macro, as used elsewhere, to get the 'tgt' pointer from 'sc'. Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> Tested-by: Mauricio Faria de Oliveira <mfo@canonical.com> Tested-by: David Coronel <david.coronel@canonical.com> --- drivers/scsi/virtio_scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)