Hi! > From: Hannes Reinecke <h...@suse.de> > > [ Upstream commit 5faf50e9e9fdc2117c61ff7e20da49cd6a29e0ca ] > > alua_bus_detach() might be running concurrently with alua_rtpg_work(), so > we might trip over h->sdev == NULL and call BUG_ON(). The correct way of > handling it is to not set h->sdev to NULL in alua_bus_detach(), and call > rcu_synchronize() before the final delete to ensure that all concurrent > threads have left the critical section. Then we can get rid of the > BUG_ON() and replace it with a simple if condition.
I don't get it.
In the new version, h->sdev will never be NULL, because it is never
set to NULL. It is will be valid up-to the point when h is freed...
synchronize_rcu() should prevent race with alua_rtpg(), but BUG_ON()s
should stay, to catch any bogosity... Or maybe the if (!h->sdev) tests
should be simply removed. But it does not make sense to silently
continue.
Best regards,
Pavel
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -672,8 +672,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct
> alua_port_group *pg)
> rcu_read_lock();
> list_for_each_entry_rcu(h,
> &tmp_pg->dh_list, node) {
> - /* h->sdev should always be
> valid */
> - BUG_ON(!h->sdev);
> + if (!h->sdev)
> + continue;
> h->sdev->access_state = desc[0];
> }
> rcu_read_unlock();
> @@ -719,7 +719,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct
> alua_port_group *pg)
> pg->expiry = 0;
> rcu_read_lock();
> list_for_each_entry_rcu(h, &pg->dh_list, node) {
> - BUG_ON(!h->sdev);
> + if (!h->sdev)
> + continue;
> h->sdev->access_state =
> (pg->state & SCSI_ACCESS_STATE_MASK);
> if (pg->pref)
> @@ -1160,7 +1161,6 @@ static void alua_bus_detach(struct scsi_device *sdev)
> spin_lock(&h->pg_lock);
> pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
> rcu_assign_pointer(h->pg, NULL);
> - h->sdev = NULL;
> spin_unlock(&h->pg_lock);
> if (pg) {
> spin_lock_irq(&pg->lock);
> @@ -1169,6 +1169,7 @@ static void alua_bus_detach(struct scsi_device *sdev)
> kref_put(&pg->kref, release_port_group);
> }
> sdev->handler_data = NULL;
> + synchronize_rcu();
> kfree(h);
> }
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
signature.asc
Description: Digital signature
