| Message ID | 20181017072004.63084-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PATCHv2] ib_srp: Remove WARN_ON in srp_terminate_io() | expand |
On 10/17/18 12:20 AM, Hannes Reinecke wrote: > The WARN_ON() is pointless as the rport is placed in SDEV_TRANSPORT_OFFLINE > at that time, so no new commands can be submitted via srp_queuecommand() > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Reviewed-by: Jens Axboe <axboe@kernel.dk> > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index 0b34e909505f..5a79444c2f3c 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport) > struct scsi_device *sdev; > int i, j; > > - /* > - * Invoking srp_terminate_io() while srp_queuecommand() is running > - * is not safe. Hence the warning statement below. > - */ > - shost_for_each_device(sdev, shost) > - WARN_ON_ONCE(sdev->request_queue->request_fn_active); > - > for (i = 0; i < target->ch_count; i++) { > ch = &target->ch[i]; Although I had explained before why I think that warning is not pointless, I agree with this change because the legacy block layer is going away. Anyway: Acked-by: Bart Van Assche <bvanassche@acm.org>
On Wed, 2018-10-17 at 07:43 -0700, Bart Van Assche wrote: > On 10/17/18 12:20 AM, Hannes Reinecke wrote: > > The WARN_ON() is pointless as the rport is placed in SDEV_TRANSPORT_OFFLINE > > at that time, so no new commands can be submitted via srp_queuecommand() > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > Reviewed-by: Jens Axboe <axboe@kernel.dk> > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.com> > > --- > > drivers/infiniband/ulp/srp/ib_srp.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > > index 0b34e909505f..5a79444c2f3c 100644 > > --- a/drivers/infiniband/ulp/srp/ib_srp.c > > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > > @@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport) > > struct scsi_device *sdev; > > int i, j; > > > > - /* > > - * Invoking srp_terminate_io() while srp_queuecommand() is running > > - * is not safe. Hence the warning statement below. > > - */ > > - shost_for_each_device(sdev, shost) > > - WARN_ON_ONCE(sdev->request_queue->request_fn_active); > > - > > for (i = 0; i < target->ch_count; i++) { > > ch = &target->ch[i]; > > Although I had explained before why I think that warning is not > pointless, I agree with this change because the legacy block layer is > going away. Anyway: > > Acked-by: Bart Van Assche <bvanassche@acm.org> Thanks, applied to for-next.
On Wed, 2018-10-17 at 11:30 -0400, Doug Ledford wrote: > On Wed, 2018-10-17 at 07:43 -0700, Bart Van Assche wrote: > > On 10/17/18 12:20 AM, Hannes Reinecke wrote: > > > The WARN_ON() is pointless as the rport is placed in SDEV_TRANSPORT_OFFLINE > > > at that time, so no new commands can be submitted via srp_queuecommand() > > > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > > Reviewed-by: Jens Axboe <axboe@kernel.dk> > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.com> > > > --- > > > drivers/infiniband/ulp/srp/ib_srp.c | 7 ------- > > > 1 file changed, 7 deletions(-) > > > > > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > > > index 0b34e909505f..5a79444c2f3c 100644 > > > --- a/drivers/infiniband/ulp/srp/ib_srp.c > > > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > > > @@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport) > > > struct scsi_device *sdev; > > > int i, j; > > > > > > - /* > > > - * Invoking srp_terminate_io() while srp_queuecommand() is running > > > - * is not safe. Hence the warning statement below. > > > - */ > > > - shost_for_each_device(sdev, shost) > > > - WARN_ON_ONCE(sdev->request_queue->request_fn_active); > > > - > > > for (i = 0; i < target->ch_count; i++) { > > > ch = &target->ch[i]; > > > > Although I had explained before why I think that warning is not > > pointless, I agree with this change because the legacy block layer is > > going away. Anyway: > > > > Acked-by: Bart Van Assche <bvanassche@acm.org> > > Thanks, applied to for-next. > FWIW, this introduced a build warning (shost and sdev are now unused variables). I edited the patch to remove the newly unneeded variables.
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 0b34e909505f..5a79444c2f3c 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1334,13 +1334,6 @@ static void srp_terminate_io(struct srp_rport *rport) struct scsi_device *sdev; int i, j; - /* - * Invoking srp_terminate_io() while srp_queuecommand() is running - * is not safe. Hence the warning statement below. - */ - shost_for_each_device(sdev, shost) - WARN_ON_ONCE(sdev->request_queue->request_fn_active); - for (i = 0; i < target->ch_count; i++) { ch = &target->ch[i];