Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751112AbdGMCgr (ORCPT ); Wed, 12 Jul 2017 22:36:47 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:2100 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbdGMCgp (ORCPT ); Wed, 12 Jul 2017 22:36:45 -0400 Subject: Re: [PATCH v3 5/7] libsas: add a new workqueue to run probe/destruct discovery event To: John Garry , , References: <1499670369-44143-1-git-send-email-wangyijing@huawei.com> <1499670369-44143-6-git-send-email-wangyijing@huawei.com> CC: , , , , , , , , , , , , , , , , , , Johannes Thumshirn , Linuxarm From: wangyijing Message-ID: <5966DC91.1090200@huawei.com> Date: Thu, 13 Jul 2017 10:36:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.177.23.4] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.5966DCB7.00B7,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 486c6dfc302571d64753b4441ae1c833 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4250 Lines: 123 在 2017/7/13 0:50, 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 >> CC: John Garry >> CC: Johannes Thumshirn >> CC: Ewan Milne >> CC: Christoph Hellwig >> CC: Tomas Henzl >> CC: Dan Williams >> --- >> 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? No, since we could queue sas discovery probe/destruct event in sas discovery work(like sas_revalidate_domain) sas_revalidate_domain sas_ex_revalidate_domain sas_rediscover sas_rediscover_dev sas_unregister_devs_sas_addr sas_unregister_dev sas_discover_event(dev->port, DISCE_DESTRUCT) sas_discover_new sas_ex_discover_devices sas_ex_discover_dev sas_ex_discover_end_dev sas_discover_end_dev sas_discover_event(dev->port, DISCE_PROBE) So we could not sync probe or destruct sas discovery event in sas_revalidate_domain if they are queued in a same workqueue, or it would block for ever. > >> + */ >> + 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() Eagle eye :), I could check the return value separately. Thanks! Yijing. > >> 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]; >> > > > > . >