| Message ID | 20221117183626.2656196-2-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" | expand |
> On 18-Nov-2022, at 12:06 AM, Bart Van Assche <bvanassche@acm.org> wrote: > > There is a bug in commit 0b25e17e9018 ("scsi: alua: Move a > scsi_device_put() call out of alua_check_vpd()"): that patch may cause > alua_rtpg_queue() callers to call scsi_device_put() even if that function > should not be called. Revert that commit to prepare for a different > solution. > > Cc: Hannes Reinecke <hare@suse.de> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Sachin Sant <sachinp@linux.ibm.com> > Cc: Benjamin Block <bblock@linux.ibm.com> > Reported-by: Sachin Sant <sachinp@linux.ibm.com> > Reported-by: Benjamin Block <bblock@linux.ibm.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++-------------- > 1 file changed, 8 insertions(+), 15 deletions(-) Thanks for the patch. Tested it on top of next-20221117. The reported warning is not seen. Tested-by: Sachin Sant <sachinp@linux.ibm.com <mailto:sachinp@linux.ibm.com>> - Sachin
> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote: > > On 11/17/22 22:11, Sachin Sant wrote: >> Tested it on top of next-20221117. The reported warning is not seen. >> Tested-by: Sachin Sant <sachinp@linux.ibm.com <mailto:sachinp@linux.ibm.com>> > > Hi Sachin, > > Thank you for the help with testing. > > Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)? I tested with both the patches applied on top of next-20221117. - Sachin
On 11/18/22 08:03, Sachin Sant wrote: >> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote: >> Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)? > > I tested with both the patches applied on top of next-20221117. Thank you Sachin for having confirmed this. In the future when testing an entire patch series, consider replying with "Tested-by:" to the cover letter instead of the first patch. I think that is the conventional way to indicate that a patch series has been tested in its entirety instead of a single patch from a series. Bart.
> On 19-Nov-2022, at 12:24 AM, Bart Van Assche <bvanassche@acm.org> wrote: > > On 11/18/22 08:03, Sachin Sant wrote: >>> On 18-Nov-2022, at 8:37 PM, Bart Van Assche <bvanassche@acm.org> wrote: >>> Can you also test patch 2/2 from this series (https://lore.kernel.org/all/20221117183626.2656196-3-bvanassche@acm.org/)? >> I tested with both the patches applied on top of next-20221117. > > Thank you Sachin for having confirmed this. In the future when testing an > entire patch series, consider replying with "Tested-by:" to the cover letter > instead of the first patch. I think that is the conventional way to indicate > that a patch series has been tested in its entirety instead of a single > patch from a series. > I did not receive the cover letter so replied to the first patch. I should have explicitly called out that I have tested both the patches. Sorry about the confusion. - Sachin
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 693cd827e138..bd4ee294f5c7 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -324,7 +324,6 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h, struct alua_port_group *pg, *old_pg = NULL; bool pg_updated = false; unsigned long flags; - bool put_sdev; group_id = scsi_vpd_tpg_id(sdev, &rel_port); if (group_id < 0) { @@ -374,14 +373,11 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h, list_add_rcu(&h->node, &pg->dh_list); spin_unlock_irqrestore(&pg->lock, flags); - put_sdev = alua_rtpg_queue(rcu_dereference_protected(h->pg, + alua_rtpg_queue(rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock)), sdev, NULL, true); spin_unlock(&h->pg_lock); - if (put_sdev) - scsi_device_put(sdev); - if (old_pg) kref_put(&old_pg->kref, release_port_group); @@ -982,10 +978,9 @@ static void alua_rtpg_work(struct work_struct *work) * RTPG already has been scheduled. * * Returns true if and only if alua_rtpg_work() will be called asynchronously. - * That function is responsible for calling @qdata->fn(). If this function - * returns true, the caller is responsible for invoking scsi_device_put(@sdev). + * That function is responsible for calling @qdata->fn(). */ -static bool __must_check alua_rtpg_queue(struct alua_port_group *pg, +static bool alua_rtpg_queue(struct alua_port_group *pg, struct scsi_device *sdev, struct alua_queue_data *qdata, bool force) { @@ -1024,6 +1019,8 @@ static bool __must_check alua_rtpg_queue(struct alua_port_group *pg, else kref_put(&pg->kref, release_port_group); } + if (sdev) + scsi_device_put(sdev); return true; } @@ -1130,12 +1127,10 @@ static int alua_activate(struct scsi_device *sdev, rcu_read_unlock(); mutex_unlock(&h->init_mutex); - if (alua_rtpg_queue(pg, sdev, qdata, true)) { - scsi_device_put(sdev); + if (alua_rtpg_queue(pg, sdev, qdata, true)) fn = NULL; - } else { + else err = SCSI_DH_DEV_OFFLINED; - } kref_put(&pg->kref, release_port_group); out: if (fn) @@ -1161,9 +1156,7 @@ static void alua_check(struct scsi_device *sdev, bool force) return; } rcu_read_unlock(); - - if (alua_rtpg_queue(pg, sdev, NULL, force)) - scsi_device_put(sdev); + alua_rtpg_queue(pg, sdev, NULL, force); kref_put(&pg->kref, release_port_group); }
There is a bug in commit 0b25e17e9018 ("scsi: alua: Move a scsi_device_put() call out of alua_check_vpd()"): that patch may cause alua_rtpg_queue() callers to call scsi_device_put() even if that function should not be called. Revert that commit to prepare for a different solution. Cc: Hannes Reinecke <hare@suse.de> Cc: Martin Wilck <mwilck@suse.com> Cc: Sachin Sant <sachinp@linux.ibm.com> Cc: Benjamin Block <bblock@linux.ibm.com> Reported-by: Sachin Sant <sachinp@linux.ibm.com> Reported-by: Benjamin Block <bblock@linux.ibm.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/device_handler/scsi_dh_alua.c | 23 ++++++++-------------- 1 file changed, 8 insertions(+), 15 deletions(-)