* [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() @ 2022-11-17 18:36 Bart Van Assche 2022-11-17 18:36 ` [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" Bart Van Assche ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Bart Van Assche @ 2022-11-17 18:36 UTC (permalink / raw) To: Martin K . Petersen; +Cc: linux-scsi, Bart Van Assche Hi Martin, This patch series reworks how the SCSI ALUA device handler calls scsi_device_put(). Please consider this patch series for the next merge window. Thanks, Bart. Bart Van Assche (2): scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" scsi: alua: Call scsi_device_put() from non-atomic context drivers/scsi/device_handler/scsi_dh_alua.c | 46 ++++++++++++---------- 1 file changed, 25 insertions(+), 21 deletions(-) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-17 18:36 [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Bart Van Assche @ 2022-11-17 18:36 ` Bart Van Assche 2022-11-18 6:11 ` Sachin Sant 2022-11-17 18:36 ` [PATCH 2/2] scsi: alua: Call scsi_device_put() from non-atomic context Bart Van Assche ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Bart Van Assche @ 2022-11-17 18:36 UTC (permalink / raw) To: Martin K . Petersen Cc: linux-scsi, Bart Van Assche, Hannes Reinecke, Martin Wilck, Sachin Sant, Benjamin Block 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(-) 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); } ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-17 18:36 ` [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" Bart Van Assche @ 2022-11-18 6:11 ` Sachin Sant 2022-11-18 15:07 ` Bart Van Assche 0 siblings, 1 reply; 10+ messages in thread From: Sachin Sant @ 2022-11-18 6:11 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, linux-scsi, Hannes Reinecke, Martin Wilck, Benjamin Block > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-18 6:11 ` Sachin Sant @ 2022-11-18 15:07 ` Bart Van Assche 2022-11-18 16:03 ` Sachin Sant 0 siblings, 1 reply; 10+ messages in thread From: Bart Van Assche @ 2022-11-18 15:07 UTC (permalink / raw) To: Sachin Sant Cc: Martin K . Petersen, linux-scsi, Hannes Reinecke, Martin Wilck, Benjamin Block 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/)? Thanks, Bart. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-18 15:07 ` Bart Van Assche @ 2022-11-18 16:03 ` Sachin Sant 2022-11-18 18:54 ` Bart Van Assche 0 siblings, 1 reply; 10+ messages in thread From: Sachin Sant @ 2022-11-18 16:03 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, linux-scsi, Hannes Reinecke, Martin Wilck, Benjamin Block > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-18 16:03 ` Sachin Sant @ 2022-11-18 18:54 ` Bart Van Assche 2022-11-19 9:46 ` Sachin Sant 0 siblings, 1 reply; 10+ messages in thread From: Bart Van Assche @ 2022-11-18 18:54 UTC (permalink / raw) To: Sachin Sant Cc: Martin K . Petersen, linux-scsi, Hannes Reinecke, Martin Wilck, Benjamin Block 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" 2022-11-18 18:54 ` Bart Van Assche @ 2022-11-19 9:46 ` Sachin Sant 0 siblings, 0 replies; 10+ messages in thread From: Sachin Sant @ 2022-11-19 9:46 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, linux-scsi, Hannes Reinecke, Martin Wilck, Benjamin Block > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] scsi: alua: Call scsi_device_put() from non-atomic context 2022-11-17 18:36 [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Bart Van Assche 2022-11-17 18:36 ` [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" Bart Van Assche @ 2022-11-17 18:36 ` Bart Van Assche 2022-11-26 0:34 ` [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Martin K. Petersen 2022-12-01 3:45 ` Martin K. Petersen 3 siblings, 0 replies; 10+ messages in thread From: Bart Van Assche @ 2022-11-17 18:36 UTC (permalink / raw) To: Martin K . Petersen Cc: linux-scsi, Bart Van Assche, Martin Wilck, Hannes Reinecke, Sachin Sant, Benjamin Block, Dan Carpenter Since commit f93ed747e2c7 ("scsi: core: Release SCSI devices synchronously"), scsi_device_put() might sleep. Avoid calling it from alua_rtpg_queue() with the pg_lock held. The lock only pretects h->pg, anyway. To avoid the pg being freed under us, because of a race with another thread, take a temporary reference. In alua_rtpg_queue(), verify that the pg still belongs to the sdev being passed before actually queueing the RTPG. This patch fixes the following smatch warning: drivers/scsi/device_handler/scsi_dh_alua.c:1013 alua_rtpg_queue() warn: sleeping in atomic context alua_check_vpd() <- disables preempt -> alua_rtpg_queue() -> scsi_device_put() Cc: Martin Wilck <mwilck@suse.com> Cc: Hannes Reinecke <hare@suse.de> Cc: Sachin Sant <sachinp@linux.ibm.com> Cc: Benjamin Block <bblock@linux.ibm.com> Suggested-by: Martin Wilck <mwilck@suse.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/device_handler/scsi_dh_alua.c | 27 +++++++++++++++------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index bd4ee294f5c7..49cc18a87473 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -354,6 +354,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h, "%s: port group %x rel port %x\n", ALUA_DH_NAME, group_id, rel_port); + kref_get(&pg->kref); + /* Check for existing port group references */ spin_lock(&h->pg_lock); old_pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock)); @@ -373,11 +375,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); - alua_rtpg_queue(rcu_dereference_protected(h->pg, - lockdep_is_held(&h->pg_lock)), - sdev, NULL, true); spin_unlock(&h->pg_lock); + alua_rtpg_queue(pg, sdev, NULL, true); + kref_put(&pg->kref, release_port_group); + if (old_pg) kref_put(&old_pg->kref, release_port_group); @@ -986,6 +988,9 @@ static bool alua_rtpg_queue(struct alua_port_group *pg, { int start_queue = 0; unsigned long flags; + + might_sleep(); + if (WARN_ON_ONCE(!pg) || scsi_device_get(sdev)) return false; @@ -996,11 +1001,17 @@ static bool alua_rtpg_queue(struct alua_port_group *pg, force = true; } if (pg->rtpg_sdev == NULL) { - pg->interval = 0; - pg->flags |= ALUA_PG_RUN_RTPG; - kref_get(&pg->kref); - pg->rtpg_sdev = sdev; - start_queue = 1; + struct alua_dh_data *h = sdev->handler_data; + + rcu_read_lock(); + if (h && rcu_dereference(h->pg) == pg) { + pg->interval = 0; + pg->flags |= ALUA_PG_RUN_RTPG; + kref_get(&pg->kref); + pg->rtpg_sdev = sdev; + start_queue = 1; + } + rcu_read_unlock(); } else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force) { pg->flags |= ALUA_PG_RUN_RTPG; /* Do not queue if the worker is already running */ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() 2022-11-17 18:36 [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Bart Van Assche 2022-11-17 18:36 ` [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" Bart Van Assche 2022-11-17 18:36 ` [PATCH 2/2] scsi: alua: Call scsi_device_put() from non-atomic context Bart Van Assche @ 2022-11-26 0:34 ` Martin K. Petersen 2022-12-01 3:45 ` Martin K. Petersen 3 siblings, 0 replies; 10+ messages in thread From: Martin K. Petersen @ 2022-11-26 0:34 UTC (permalink / raw) To: Bart Van Assche; +Cc: Martin K . Petersen, linux-scsi Bart, > This patch series reworks how the SCSI ALUA device handler calls > scsi_device_put(). Please consider this patch series for the next > merge window. Applied to 6.2/scsi-staging, thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() 2022-11-17 18:36 [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Bart Van Assche ` (2 preceding siblings ...) 2022-11-26 0:34 ` [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Martin K. Petersen @ 2022-12-01 3:45 ` Martin K. Petersen 3 siblings, 0 replies; 10+ messages in thread From: Martin K. Petersen @ 2022-12-01 3:45 UTC (permalink / raw) To: Bart Van Assche; +Cc: Martin K . Petersen, linux-scsi On Thu, 17 Nov 2022 10:36:24 -0800, Bart Van Assche wrote: > This patch series reworks how the SCSI ALUA device handler calls > scsi_device_put(). Please consider this patch series for the next merge > window. > > Thanks, > > Bart. > > [...] Applied to 6.2/scsi-queue, thanks! [1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" https://git.kernel.org/mkp/scsi/c/a500c4cc06cd [2/2] scsi: alua: Call scsi_device_put() from non-atomic context https://git.kernel.org/mkp/scsi/c/50759b881e1d -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-12-01 3:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-17 18:36 [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Bart Van Assche 2022-11-17 18:36 ` [PATCH 1/2] scsi: alua: Revert "Move a scsi_device_put() call out of alua_check_vpd()" Bart Van Assche 2022-11-18 6:11 ` Sachin Sant 2022-11-18 15:07 ` Bart Van Assche 2022-11-18 16:03 ` Sachin Sant 2022-11-18 18:54 ` Bart Van Assche 2022-11-19 9:46 ` Sachin Sant 2022-11-17 18:36 ` [PATCH 2/2] scsi: alua: Call scsi_device_put() from non-atomic context Bart Van Assche 2022-11-26 0:34 ` [PATCH 0/2] Rework how the ALUA driver calls scsi_device_put() Martin K. Petersen 2022-12-01 3:45 ` Martin K. Petersen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).