Re: [PATCH v3 5/7] libsas: add a new workqueue to run probe/destruct discovery event

[John Garry]

On 10/07/2017 08:06, Yijing Wang wrote:
> Sometimes, we want sync libsas probe or destruct in sas discovery work,
> like when libsas revalidate domain. We need to split probe and destruct
> work from the scsi host workqueue.
>
> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx>
> CC: John Garry <john.garry@xxxxxxxxxx>
> CC: Johannes Thumshirn <jthumshirn@xxxxxxx>
> CC: Ewan Milne <emilne@xxxxxxxxxx>
> CC: Christoph Hellwig <hch@xxxxxx>
> CC: Tomas Henzl <thenzl@xxxxxxxxxx>
> CC: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>  drivers/scsi/libsas/sas_discover.c | 13 ++++++++++++-
>  drivers/scsi/libsas/sas_init.c     |  8 ++++++++
>  include/scsi/libsas.h              |  1 +
>  3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 5d4a3a8..a25d648 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -559,7 +559,18 @@ static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
>  	 * not racing against draining
>  	 */
>  	sas_port_get(port);
> -	ret = scsi_queue_work(ha->core.shost, &sw->work);
> +	/*
> +	 * discovery event probe and destruct would be called in other
> +	 * discovery event like discover domain and revalidate domain
> +	 * events, in some cases, we need to sync execute probe and destruct
> +	 * events, so run probe and destruct discover events except in a new
> +	 * workqueue.

Can we just use ha->disc_q for all discovery events for simplicity? 
Would this solve the disco mutex you try to solve in patch 7/7?

> +	 */
> +	if (ev->type == DISCE_PROBE || ev->type == DISCE_DESTRUCT)
> +		ret = queue_work(ha->disc_q, &sw->work);
> +	else
> +		ret = scsi_queue_work(ha->core.shost, &sw->work);
> +
>  	if (ret != 1)

Uhh, we are mixing bool and int here... but we're forced to by 
scsi_queue_work()

>  		sas_port_put(port);
>  }
> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index 2f3b736..df1d78b 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -152,6 +152,13 @@ int sas_register_ha(struct sas_ha_struct *sas_ha)
>  	if (!sas_ha->event_q)
>  		goto Undo_ports;
>
> +	snprintf(name, 64, "%s_disc_q", dev_name(sas_ha->dev));
> +	sas_ha->disc_q = create_singlethread_workqueue(name);
> +	if(!sas_ha->disc_q) {
> +		destroy_workqueue(sas_ha->event_q);
> +		goto Undo_ports;
> +	}
> +
>  	INIT_LIST_HEAD(&sas_ha->eh_done_q);
>  	INIT_LIST_HEAD(&sas_ha->eh_ata_q);
>
> @@ -187,6 +194,7 @@ int sas_unregister_ha(struct sas_ha_struct *sas_ha)
>  	__sas_drain_work(sas_ha);
>  	mutex_unlock(&sas_ha->drain_mutex);
>  	destroy_workqueue(sas_ha->event_q);
> +	destroy_workqueue(sas_ha->disc_q);
>
>  	return 0;
>  }
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index c2ef05e..4bcb9fe 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -406,6 +406,7 @@ struct sas_ha_struct {
>  	struct device *dev;	  /* should be set */
>  	struct module *lldd_module; /* should be set */
>  	struct workqueue_struct *event_q;
> +	struct workqueue_struct *disc_q;
>
>  	u8 *sas_addr;		  /* must be set */
>  	u8 hashed_sas_addr[HASHED_SAS_ADDR_SIZE];