[SRU,Xenial,2/2] UBUNTU: SAUCE: (no-up) virtio-scsi: (cosmetic) reestablish virtscsi_pick_vq_mq() signature
diff mbox series

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)
Related show

Commit Message

Mauricio Faria de Oliveira Oct. 16, 2018, 3:38 p.m. UTC
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(-)

Comments

Stefan Bader Oct. 17, 2018, 8:20 a.m. UTC | #1
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);
>  
>
Mauricio Faria de Oliveira Oct. 17, 2018, 11:50 a.m. UTC | #2
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);
> >
> >
>
>

Patch
diff mbox series

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);