From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3BBDCC282D7 for ; Wed, 30 Jan 2019 17:54:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 07AA02087F for ; Wed, 30 Jan 2019 17:54:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732748AbfA3RyV (ORCPT ); Wed, 30 Jan 2019 12:54:21 -0500 Received: from szxga05-in.huawei.com ([45.249.212.191]:3249 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726464AbfA3RyU (ORCPT ); Wed, 30 Jan 2019 12:54:20 -0500 Received: from DGGEMS406-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id C401B6265E6F41B059AC; Thu, 31 Jan 2019 01:54:17 +0800 (CST) Received: from [127.0.0.1] (10.202.226.43) by DGGEMS406-HUB.china.huawei.com (10.3.19.206) with Microsoft SMTP Server id 14.3.408.0; Thu, 31 Jan 2019 01:54:08 +0800 Subject: Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks To: Jason Yan , , References: <20190130082412.9357-1-yanaijie@huawei.com> <20190130082412.9357-8-yanaijie@huawei.com> CC: , , , , , , , , , , , , Xiaofei Tan , Ewan Milne , Tomas Henzl From: John Garry Message-ID: <368786a4-9c18-7d5b-e62b-5dcdc634d3e6@huawei.com> Date: Wed, 30 Jan 2019 17:53:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20190130082412.9357-8-yanaijie@huawei.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.226.43] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/01/2019 08:24, Jason Yan wrote: > The work flow of revalidation now is scanning expander phy by the > sequence of the phy and check if the phy have changed. This will leads > to an issue of swapping two sas disks on one expander. > > Assume we have two sas disks, connected with expander phy10 and phy11: > > phy10: 5000cca04eb1001d port-0:0:10 > phy11: 5000cca04eb043ad port-0:0:11 > > Swap these two disks, and imaging the following scenario: > > revalidation 1: What does "revalidation 1" actually mean? > -->phy10: 0 --> delete phy10 domain device > -->phy11: 5000cca04eb043ad (no change) so is disk 11 still inserted at this stage? > revalidation done > > revalidation 2: is port-0:0:10 deleted now? > -->step 1, check phy10: > -->phy10: 5000cca04eb043ad --> add to wide port(port-0:0:11) (phy11 > address is still 5000cca04eb043ad now) So this should not happen. How are you physcially swapping them such that phy11 address is still 5000cca04eb043ad? I don't see how this would be true at revalidation 1. > > -->step 2, check phy11: > -->phy11: 0 --> phy11 address is 0 now, but it's part of wide > port(port-0:0:11), the domain device will not be deleted. > revalidation done > > revalidation 3: > -->phy10, 5000cca04eb043ad (no change) > -->phy11: 5000cca04eb1001d --> try to add port-0:0:11 but failed, > port-0:0:11 already exist, trigger a warning as follows > revalidation done > > [14790.189699] sysfs: cannot create duplicate filename > '/devices/pci0000:74/0000:74:02.0/host0/port-0:0/expander-0:0/port-0:0:11' > [14790.201081] CPU: 25 PID: 5031 Comm: kworker/u192:3 Not tainted > 4.16.0-rc1-191134-g138f084-dirty #228 > [14790.210199] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 EC UEFI > Nemo 2.0 RC0 - B303 05/16/2018 > [14790.219323] Workqueue: 0000:74:02.0_disco_q sas_revalidate_domain > [14790.225404] Call trace: > [14790.227842] dump_backtrace+0x0/0x18c > [14790.231492] show_stack+0x14/0x1c > [14790.234798] dump_stack+0x88/0xac > [14790.238101] sysfs_warn_dup+0x64/0x7c > [14790.241751] sysfs_create_dir_ns+0x90/0xa0 > [14790.245835] kobject_add_internal+0xa0/0x284 > [14790.250092] kobject_add+0xb8/0x11c > [14790.253570] device_add+0xe8/0x598 > [14790.256960] sas_port_add+0x24/0x50 > [14790.260436] sas_ex_discover_devices+0xb10/0xc30 > [14790.265040] sas_ex_revalidate_domain+0x1d8/0x518 > [14790.269731] sas_revalidate_domain+0x12c/0x154 > [14790.274163] process_one_work+0x128/0x2b0 > [14790.278160] worker_thread+0x14c/0x408 > [14790.281897] kthread+0xfc/0x128 > [14790.285026] ret_from_fork+0x10/0x18 > [14790.288598] ------------[ cut here ]------------ > > At last, the disk 5000cca04eb1001d is lost. > > The basic idea of fix this issue is to let the revalidation first scan > all phys, and then unregisterring devices. Only when no devices need to > be unregisterred, go to the next step to discover new devices. If there > are devices need unregister, unregister those devices and tell the > revalidation event processor to retry again. The next revalidation will > process the discovering of the new devices. > > Tested-by: Chen Liangfei > Signed-off-by: Jason Yan > CC: Xiaofei Tan > CC: chenxiang > CC: John Garry > CC: Johannes Thumshirn > CC: Ewan Milne > CC: Christoph Hellwig > CC: Tomas Henzl > CC: Dan Williams > CC: Hannes Reinecke > --- > drivers/scsi/libsas/sas_expander.c | 146 +++++++++++++++++++++++++------------ > 1 file changed, 100 insertions(+), 46 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c > index e781941a7088..073bb5c6e353 100644 > --- a/drivers/scsi/libsas/sas_expander.c > +++ b/drivers/scsi/libsas/sas_expander.c > @@ -2056,7 +2056,7 @@ static bool sas_process_flutter(struct domain_device *dev, struct ex_phy *phy, > return true; > } > > -static int sas_unregister(struct domain_device *dev, int phy_id, bool last, > +static int sas_ex_unregister(struct domain_device *dev, int phy_id, bool last, > bool *retry) > { > struct expander_device *ex = &dev->ex_dev; > @@ -2108,21 +2108,8 @@ static int sas_unregister(struct domain_device *dev, int phy_id, bool last, > return 0; > } > > -/** > - * sas_rediscover - revalidate the domain. > - * @dev:domain device to be detect. > - * @phy_id: the phy id will be detected. > - * > - * NOTE: this process _must_ quit (return) as soon as any connection > - * errors are encountered. Connection recovery is done elsewhere. > - * Discover process only interrogates devices in order to discover the > - * domain.For plugging out, we un-register the device only when it is > - * the last phy in the port, for other phys in this port, we just delete it > - * from the port.For inserting, we do discovery when it is the > - * first phy,for other phys in this port, we add it to the port to > - * forming the wide-port. > - */ > -static void sas_rediscover(struct domain_device *dev, const int phy_id, > + > +static void sas_ex_unregister_device(struct domain_device *dev, const int phy_id, > bool *retry) > { > struct expander_device *ex = &dev->ex_dev; > @@ -2131,31 +2118,70 @@ static void sas_rediscover(struct domain_device *dev, const int phy_id, > int i; > bool last = true; /* is this the last phy of the port */ > > - pr_debug("ex %016llx phy%d originated BROADCAST(CHANGE)\n", > - SAS_ADDR(dev->sas_addr), phy_id); > - > - if (SAS_ADDR(changed_phy->attached_sas_addr) != 0) { > - for (i = 0; i < ex->num_phys; i++) { > - struct ex_phy *phy = &ex->ex_phy[i]; > + for (i = 0; i < ex->num_phys; i++) { > + struct ex_phy *phy = &ex->ex_phy[i]; > > - if (i == phy_id) > - continue; > - if (SAS_ADDR(phy->attached_sas_addr) == > - SAS_ADDR(changed_phy->attached_sas_addr)) { > - pr_debug("phy%d part of wide port with phy%d\n", > - phy_id, i); > - last = false; > - break; > - } > + if (i == phy_id) > + continue; > + if (SAS_ADDR(phy->attached_sas_addr) == > + SAS_ADDR(changed_phy->attached_sas_addr)) { > + pr_debug("phy%d part of wide port with phy%d\n", > + phy_id, i); > + last = false; > + break; > } > - res = sas_unregister(dev, phy_id, last, retry); > - } else > - res = sas_discover_new(dev, phy_id); > + } > + res = sas_ex_unregister(dev, phy_id, last, retry); > > pr_debug("ex %016llx phy%d discover returned 0x%x\n", > SAS_ADDR(dev->sas_addr), phy_id, res); > } > > +static int sas_ex_try_unregister(struct domain_device *dev, u8 *changed_phy, > + int nr, bool *retry) > +{ > + struct expander_device *ex = &dev->ex_dev; > + int unregistered = 0; > + struct ex_phy *phy; > + int i; > + > + for (i = 0; i < nr; i++) { > + pr_debug("ex %016llx phy%d originated BROADCAST(CHANGE)\n", > + SAS_ADDR(dev->sas_addr), changed_phy[i]); > + > + phy = &ex->ex_phy[changed_phy[i]]; > + > + if (SAS_ADDR(phy->attached_sas_addr) == 0) > + continue; > + > + sas_ex_unregister_device(dev, changed_phy[i], retry); > + changed_phy[i] = 0xff; > + unregistered++; > + } > + return unregistered; > +} > + > +static void sas_ex_register(struct domain_device *dev, u8 *changed_phy, > + int nr) > +{ > + struct expander_device *ex = &dev->ex_dev; > + struct ex_phy *phy; > + int res = 0; > + int i; > + > + for (i = 0; i < nr; i++) { > + if (changed_phy[i] == 0xff) > + continue; > + > + phy = &ex->ex_phy[changed_phy[i]]; > + > + res = sas_discover_new(dev, changed_phy[i]); > + > + pr_debug("ex %016llx phy%d register returned 0x%x\n", > + SAS_ADDR(dev->sas_addr), changed_phy[i], res); > + } > +} > + > /** > * sas_ex_revalidate_domain - revalidate the domain > * @port_dev: port domain device. > @@ -2170,21 +2196,49 @@ void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry) > { > int res; > struct domain_device *dev = NULL; > + u8 changed_phy[MAX_EXPANDER_PHYS]; > + struct expander_device *ex; > + int unregistered = 0; > + int phy_id; > + int nr = 0; > + int i = 0; > > res = sas_find_bcast_dev(port_dev, &dev); > - if (res == 0 && dev) { > - struct expander_device *ex = &dev->ex_dev; > - int i = 0, phy_id; > - > - do { > - phy_id = -1; > - res = sas_find_bcast_phy(dev, &phy_id, i, true); > - if (phy_id == -1) > - break; > - sas_rediscover(dev, phy_id, retry); > - i = phy_id + 1; > - } while (i < ex->num_phys); > + if (res || !dev) > + return; > + > + memset(changed_phy, 0xff, MAX_EXPANDER_PHYS); > + ex = &dev->ex_dev; > + > + do { > + phy_id = -1; > + res = sas_find_bcast_phy(dev, &phy_id, i, true); > + if (phy_id == -1) > + break; > + changed_phy[nr++] = phy_id; > + i = phy_id + 1; > + } while (i < dev->ex_dev.num_phys); > + > + if (nr == 0) > + return; > + > + unregistered = sas_ex_try_unregister(dev, changed_phy, nr, retry); > + > + if (unregistered > 0) { > + struct ex_phy *phy; > + > + for (i = 0; i < nr; i++) { > + if (changed_phy[i] == 0xff) > + continue; > + phy = &ex->ex_phy[changed_phy[i]]; > + phy->phy_change_count = -1; > + } > + ex->ex_change_count = -1; > + *retry = true; > + return; > } > + > + sas_ex_register(dev, changed_phy, nr); > } > > void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost, >