[PATCH v2 0/7] libsas: fix issue of swapping or replacing disks

[PATCH v2 0/7] libsas: fix issue of swapping or replacing disks

[Jason Yan]

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 some issues of swapping disks or replacing a disk with a new one.

This patchset addresses the issues above by these main changes:
1. Let the revalidation first scan all phys, mark all changed phys, then
revalidate in two steps. First check if we need to unregister some devices.
if we need to unregister some devices, tell the upper revalidation that we
need to revalidate again. Second, if no devices need to be unregistered,
discover new devices.
2. For sata disk, checking the ata devices class and id to ensure
the same device after flutter.

v1->v2:
1. Do not raise a new bcast but use a loop in sas_revalidate_domain() to
retry revalidation if we need to revalidate again. So that we can do the 
bcast event processing synchronised to the original event
2. Drop some patches splitted from this patchset:
   https://lkml.org/lkml/2018/9/25/153
3. Drop the SATA PHY connection rate matching patch since John had a better
solution for the device discovery phase.
   https://lkml.org/lkml/2019/1/4/340
4. Re-init negotiated_linkrate when PHY is down.
5. Fix an issue when event in queue reached the limit.

Jason Yan (7):
  scsi: libsas: reset the negotiated_linkrate when phy is down
  scsi: libsas: only clear phy->in_shutdown after shutdown event done
  scsi: libsas: optimize the debug print of the revalidate process
  scsi: libsas: split the replacement of sas disks in two steps
  scsi: libsas: check if the same device when flutter
  scsi: libsas: reset the phy address if discover failed
  scsi: libsas: fix issue of swapping two sas disks

 drivers/ata/libata-core.c          |   3 +-
 drivers/scsi/libsas/sas_ata.c      |  18 +++
 drivers/scsi/libsas/sas_discover.c |  25 ++--
 drivers/scsi/libsas/sas_expander.c | 239 +++++++++++++++++++++++++++----------
 drivers/scsi/libsas/sas_phy.c      |   3 +-
 include/linux/libata.h             |   2 +
 include/scsi/libsas.h              |   6 +-
 7 files changed, 224 insertions(+), 72 deletions(-)

-- 
2.14.4

[PATCH v2 1/7] scsi: libsas: reset the negotiated_linkrate when phy is down

[Jason Yan]

If the device is unplugged or disconnected, the negotiated_linkrate
still can be seen from the userspace by sysfs. This makes people
confused and leaks information of the device last used. So let's reset
the negotiated_linkrate after the phy is down.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_expander.c | 2 ++
 include/scsi/libsas.h              | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 17eb4185f29d..7b0e6dcef6e6 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1904,6 +1904,8 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent,
 				&parent->port->sas_port_del_list);
 		phy->port = NULL;
 	}
+	if (phy->phy)
+		phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
 }
 
 static int sas_discover_bfs_by_root_level(struct domain_device *root,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 3de3b10da19a..420156cea3ee 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -448,6 +448,9 @@ static inline void sas_phy_disconnected(struct asd_sas_phy *phy)
 {
 	phy->oob_mode = OOB_NOT_CONNECTED;
 	phy->linkrate = SAS_LINK_RATE_UNKNOWN;
+
+	if (phy->phy)
+		phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
 }
 
 static inline unsigned int to_sas_gpio_od(int device, int bit)
-- 
2.14.4

Re: [PATCH v2 1/7] scsi: libsas: reset the negotiated_linkrate when phy is down

[John Garry]

On 30/01/2019 08:24, Jason Yan wrote:
> If the device is unplugged or disconnected, the negotiated_linkrate
> still can be seen from the userspace by sysfs. This makes people
> confused and leaks information of the device last used. So let's reset
> the negotiated_linkrate after the phy is down.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 2 ++
>  include/scsi/libsas.h              | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 17eb4185f29d..7b0e6dcef6e6 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1904,6 +1904,8 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent,
>  				&parent->port->sas_port_del_list);
>  		phy->port = NULL;
>  	}
> +	if (phy->phy)
> +		phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;

Does this work for both PHYs which were either directly attached or just 
forming part of a wideport?

Please also note that the expander PHY which was previously attached may 
also show the incorrect value.

>  }
>
>  static int sas_discover_bfs_by_root_level(struct domain_device *root,
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 3de3b10da19a..420156cea3ee 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -448,6 +448,9 @@ static inline void sas_phy_disconnected(struct asd_sas_phy *phy)
>  {
>  	phy->oob_mode = OOB_NOT_CONNECTED;
>  	phy->linkrate = SAS_LINK_RATE_UNKNOWN;
> +

Then idea behind sas_phy_disconnected() was to set root PHY values only:

/* Before calling a notify event, LLDD should use this function
  * when the link is severed (possibly from its tasklet).
  * The idea is that the Class only reads those, while the LLDD,
  * can R/W these (thus avoiding a race)

libsas does set sas_phy negotiated_linkrate (but only for expander 
PHYs), so not the perfect place to set this. I'm nitpicking a bit here.


> +	if (phy->phy)
> +		phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
>  }
>
>  static inline unsigned int to_sas_gpio_od(int device, int bit)

Thanks,

>

Re: [PATCH v2 1/7] scsi: libsas: reset the negotiated_linkrate when phy is down

[Jason Yan]

On 2019/1/30 21:08, John Garry wrote:
> On 30/01/2019 08:24, Jason Yan wrote:
>> If the device is unplugged or disconnected, the negotiated_linkrate
>> still can be seen from the userspace by sysfs. This makes people
>> confused and leaks information of the device last used. So let's reset
>> the negotiated_linkrate after the phy is down.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c | 2 ++
>>  include/scsi/libsas.h              | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c
>> b/drivers/scsi/libsas/sas_expander.c
>> index 17eb4185f29d..7b0e6dcef6e6 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1904,6 +1904,8 @@ static void sas_unregister_devs_sas_addr(struct
>> domain_device *parent,
>>                  &parent->port->sas_port_del_list);
>>          phy->port = NULL;
>>      }
>> +    if (phy->phy)
>> +        phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
>
> Does this work for both PHYs which were either directly attached or just
> forming part of a wideport?
>
> Please also note that the expander PHY which was previously attached may
> also show the incorrect value.

Good catch, all PHYs need to do this, not only the last PHY of the wideport.

>
>>  }
>>
>>  static int sas_discover_bfs_by_root_level(struct domain_device *root,
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index 3de3b10da19a..420156cea3ee 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -448,6 +448,9 @@ static inline void sas_phy_disconnected(struct
>> asd_sas_phy *phy)
>>  {
>>      phy->oob_mode = OOB_NOT_CONNECTED;
>>      phy->linkrate = SAS_LINK_RATE_UNKNOWN;
>> +
>
> Then idea behind sas_phy_disconnected() was to set root PHY values only:
>
> /* Before calling a notify event, LLDD should use this function
>   * when the link is severed (possibly from its tasklet).
>   * The idea is that the Class only reads those, while the LLDD,
>   * can R/W these (thus avoiding a race)
>
> libsas does set sas_phy negotiated_linkrate (but only for expander
> PHYs), so not the perfect place to set this. I'm nitpicking a bit here.
>

I don't want to put it here. I asked chenxiang to fix this in LLDD, but 
he insist libsas to do this. Would you please discuss with chenxiang and 
come to an agreement?

>
>> +    if (phy->phy)
>> +        phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
>>  }
>>
>>  static inline unsigned int to_sas_gpio_od(int device, int bit)
>
> Thanks,
>
>>
>
>
>
>
>
> .
>

Re: [PATCH v2 1/7] scsi: libsas: reset the negotiated_linkrate when phy is down

[John Garry]

On 31/01/2019 01:11, Jason Yan wrote:
>
>
> On 2019/1/30 21:08, John Garry wrote:
>> On 30/01/2019 08:24, Jason Yan wrote:
>>> If the device is unplugged or disconnected, the negotiated_linkrate
>>> still can be seen from the userspace by sysfs. This makes people
>>> confused and leaks information of the device last used. So let's reset
>>> the negotiated_linkrate after the phy is down.
>>>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> CC: John Garry <john.garry@huawei.com>
>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>> CC: Ewan Milne <emilne@redhat.com>
>>> CC: Christoph Hellwig <hch@lst.de>
>>> CC: Tomas Henzl <thenzl@redhat.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> CC: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/libsas/sas_expander.c | 2 ++
>>>  include/scsi/libsas.h              | 3 +++
>>>  2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index 17eb4185f29d..7b0e6dcef6e6 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1904,6 +1904,8 @@ static void sas_unregister_devs_sas_addr(struct
>>> domain_device *parent,
>>>                  &parent->port->sas_port_del_list);
>>>          phy->port = NULL;
>>>      }
>>> +    if (phy->phy)
>>> +        phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
>>
>> Does this work for both PHYs which were either directly attached or just
>> forming part of a wideport?
>>
>> Please also note that the expander PHY which was previously attached may
>> also show the incorrect value.
>
> Good catch, all PHYs need to do this, not only the last PHY of the
> wideport.

Please also consider this:
- For the expander host PHY, it will also go down. I find however on my 
system that this generates no broadcast event, but the I find that the 
event count has changed for that PHY. So the expander PHY's state would 
also be wrong.  So maybe we need to yet again inject a broadcast.

- we should update PHY sysfs entries for device_type, sas_addr, and 
target protocols

>
>>
>>>  }
>>>
>>>  static int sas_discover_bfs_by_root_level(struct domain_device *root,
>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>> index 3de3b10da19a..420156cea3ee 100644
>>> --- a/include/scsi/libsas.h
>>> +++ b/include/scsi/libsas.h
>>> @@ -448,6 +448,9 @@ static inline void sas_phy_disconnected(struct
>>> asd_sas_phy *phy)
>>>  {
>>>      phy->oob_mode = OOB_NOT_CONNECTED;
>>>      phy->linkrate = SAS_LINK_RATE_UNKNOWN;
>>> +
>>
>> Then idea behind sas_phy_disconnected() was to set root PHY values only:
>>
>> /* Before calling a notify event, LLDD should use this function
>>   * when the link is severed (possibly from its tasklet).
>>   * The idea is that the Class only reads those, while the LLDD,
>>   * can R/W these (thus avoiding a race)
>>
>> libsas does set sas_phy negotiated_linkrate (but only for expander
>> PHYs), so not the perfect place to set this. I'm nitpicking a bit here.
>>
>
> I don't want to put it here. I asked chenxiang to fix this in LLDD, but
> he insist libsas to do this. Would you please discuss with chenxiang and
> come to an agreement?

I'm ok with the LLDD.

For sure, adding it here is convenient and would resolve the issue for 
other LLDDs using libsas, but setting it directly in the LLDD seems like 
the right thing to do, since this is what we do for other sas_phy rates.

>
>>
>>> +    if (phy->phy)
>>> +        phy->phy->negotiated_linkrate = SAS_LINK_RATE_UNKNOWN;
>>>  }
>>>
>>>  static inline unsigned int to_sas_gpio_od(int device, int bit)
>>
>> Thanks,
>>
>>>
>>
>>
>>
>>
>>
>> .
>>
>
>
> .
>

[PATCH v2 2/7] scsi: libsas: only clear phy->in_shutdown after shutdown event done

[Jason Yan]

When the event queue is full of phy up and down events and reached the
threshold, we will queue a shutdown-event, and set phy->in_shutdown so
that we will not queue a shutdown-event again. But before the
shutdown-event can be executed, every phy-down event will clear
phy->in_shutdown and a new shutdown-event will be queued. The queue will
be full of these shutdown-events.

Fix this by only clear phy->in_shutdown in sas_phye_shutdown(), that is
after the first shutdown-event has been executed.

Fixes: f12486e06ae8 ("scsi: libsas: shut down the PHY if events reached the threshold")
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_phy.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
index 0374243c85d0..762bb13cca74 100644
--- a/drivers/scsi/libsas/sas_phy.c
+++ b/drivers/scsi/libsas/sas_phy.c
@@ -35,7 +35,6 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	phy->in_shutdown = 0;
 	phy->error = 0;
 	sas_deform_port(phy, 1);
 }
@@ -45,7 +44,6 @@ static void sas_phye_oob_done(struct work_struct *work)
 	struct asd_sas_event *ev = to_asd_sas_event(work);
 	struct asd_sas_phy *phy = ev->phy;
 
-	phy->in_shutdown = 0;
 	phy->error = 0;
 }
 
@@ -127,6 +125,7 @@ static void sas_phye_shutdown(struct work_struct *work)
 	} else
 		pr_notice("phy%02d is not enabled, cannot shutdown\n",
 			  phy->id);
+	phy->in_shutdown = 0;
 }
 
 /* ---------- Phy class registration ---------- */
-- 
2.14.4

Re: [PATCH v2 2/7] scsi: libsas: only clear phy->in_shutdown after shutdown event done

[John Garry]

On 30/01/2019 08:24, Jason Yan wrote:
> When the event queue is full of phy up and down events and reached the
> threshold, we will queue a shutdown-event, and set phy->in_shutdown so
> that we will not queue a shutdown-event again. But before the
> shutdown-event can be executed, every phy-down event will clear
> phy->in_shutdown and a new shutdown-event will be queued. The queue will
> be full of these shutdown-events.
>
> Fix this by only clear phy->in_shutdown in sas_phye_shutdown(), that is
> after the first shutdown-event has been executed.
>

Seems ok as a fix:
Reviewed-by: John Garry <john.garry@huawei.com>

After this fix, could we change to use a static per-PHY shutdown event 
so that we cannot re-queue it? I know that this is going against idea of 
dynamic events, but it's easier than messing with flags like this.

> Fixes: f12486e06ae8 ("scsi: libsas: shut down the PHY if events reached the threshold")
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_phy.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_phy.c b/drivers/scsi/libsas/sas_phy.c
> index 0374243c85d0..762bb13cca74 100644
> --- a/drivers/scsi/libsas/sas_phy.c
> +++ b/drivers/scsi/libsas/sas_phy.c
> @@ -35,7 +35,6 @@ static void sas_phye_loss_of_signal(struct work_struct *work)
>  	struct asd_sas_event *ev = to_asd_sas_event(work);
>  	struct asd_sas_phy *phy = ev->phy;
>
> -	phy->in_shutdown = 0;
>  	phy->error = 0;
>  	sas_deform_port(phy, 1);
>  }
> @@ -45,7 +44,6 @@ static void sas_phye_oob_done(struct work_struct *work)
>  	struct asd_sas_event *ev = to_asd_sas_event(work);
>  	struct asd_sas_phy *phy = ev->phy;
>
> -	phy->in_shutdown = 0;
>  	phy->error = 0;
>  }
>
> @@ -127,6 +125,7 @@ static void sas_phye_shutdown(struct work_struct *work)
>  	} else
>  		pr_notice("phy%02d is not enabled, cannot shutdown\n",
>  			  phy->id);
> +	phy->in_shutdown = 0;
>  }
>
>  /* ---------- Phy class registration ---------- */
>

Re: [PATCH v2 2/7] scsi: libsas: only clear phy->in_shutdown after shutdown event done

[Jason Yan]

On 2019/1/31 0:26, John Garry wrote:
> On 30/01/2019 08:24, Jason Yan wrote:
>> When the event queue is full of phy up and down events and reached the
>> threshold, we will queue a shutdown-event, and set phy->in_shutdown so
>> that we will not queue a shutdown-event again. But before the
>> shutdown-event can be executed, every phy-down event will clear
>> phy->in_shutdown and a new shutdown-event will be queued. The queue will
>> be full of these shutdown-events.
>>
>> Fix this by only clear phy->in_shutdown in sas_phye_shutdown(), that is
>> after the first shutdown-event has been executed.
>>
>
> Seems ok as a fix:
> Reviewed-by: John Garry <john.garry@huawei.com>
>
> After this fix, could we change to use a static per-PHY shutdown event
> so that we cannot re-queue it? I know that this is going against idea of
> dynamic events, but it's easier than messing with flags like this.
>

Thanks, I will consider this.

>> Fixes: f12486e06ae8 ("scsi: libsas: shut down the PHY if events
>> reached the threshold")
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_phy.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_phy.c
>> b/drivers/scsi/libsas/sas_phy.c
>> index 0374243c85d0..762bb13cca74 100644
>> --- a/drivers/scsi/libsas/sas_phy.c
>> +++ b/drivers/scsi/libsas/sas_phy.c
>> @@ -35,7 +35,6 @@ static void sas_phye_loss_of_signal(struct
>> work_struct *work)
>>      struct asd_sas_event *ev = to_asd_sas_event(work);
>>      struct asd_sas_phy *phy = ev->phy;
>>
>> -    phy->in_shutdown = 0;
>>      phy->error = 0;
>>      sas_deform_port(phy, 1);
>>  }
>> @@ -45,7 +44,6 @@ static void sas_phye_oob_done(struct work_struct *work)
>>      struct asd_sas_event *ev = to_asd_sas_event(work);
>>      struct asd_sas_phy *phy = ev->phy;
>>
>> -    phy->in_shutdown = 0;
>>      phy->error = 0;
>>  }
>>
>> @@ -127,6 +125,7 @@ static void sas_phye_shutdown(struct work_struct
>> *work)
>>      } else
>>          pr_notice("phy%02d is not enabled, cannot shutdown\n",
>>                phy->id);
>> +    phy->in_shutdown = 0;
>>  }
>>
>>  /* ---------- Phy class registration ---------- */
>>
>
>
>
> .
>

[PATCH v2 3/7] scsi: libsas: optimize the debug print of the revalidate process

[Jason Yan]

sas_rediscover() returns error code if discover failed for a expander
phy. And sas_ex_revalidate_domain() only returns the last phy's error
code. So when sas_revalidate_domain() prints the return value of the
discover process, we do not know if the revalidation for every phy is
successful or not. We just know the last bcast phy revalidation
succeeded or not.

No need to return a error code for sas_ex_revalidate_domain() and
sas_rediscover(), and just print the debug log for each bcast phy directly
in sas_rediscover().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_discover.c |  7 +++----
 drivers/scsi/libsas/sas_expander.c | 11 ++++++-----
 include/scsi/libsas.h              |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 726ada9b8c79..ffc571a12916 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -500,7 +500,6 @@ static void sas_discover_domain(struct work_struct *work)
 
 static void sas_revalidate_domain(struct work_struct *work)
 {
-	int res = 0;
 	struct sas_discovery_event *ev = to_sas_discovery_event(work);
 	struct asd_sas_port *port = ev->port;
 	struct sas_ha_struct *ha = port->ha;
@@ -521,10 +520,10 @@ static void sas_revalidate_domain(struct work_struct *work)
 
 	if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
 		     ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
-		res = sas_ex_revalidate_domain(ddev);
+		sas_ex_revalidate_domain(ddev);
 
-	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d, res 0x%x\n",
-		 port->id, task_pid_nr(current), res);
+	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
+		 port->id, task_pid_nr(current));
  out:
 	mutex_unlock(&ha->disco_mutex);
 
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 7b0e6dcef6e6..5cd720f93f96 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2062,7 +2062,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
  * first phy,for other phys in this port, we add it to the port to
  * forming the wide-port.
  */
-static int sas_rediscover(struct domain_device *dev, const int phy_id)
+static void sas_rediscover(struct domain_device *dev, const int phy_id)
 {
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
@@ -2090,7 +2090,9 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
 		res = sas_rediscover_dev(dev, phy_id, last);
 	} else
 		res = sas_discover_new(dev, phy_id);
-	return res;
+
+	pr_debug("ex %016llx phy%d discover returned 0x%x\n",
+		 SAS_ADDR(dev->sas_addr), phy_id, res);
 }
 
 /**
@@ -2102,7 +2104,7 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
  * Discover process only interrogates devices in order to discover the
  * domain.
  */
-int sas_ex_revalidate_domain(struct domain_device *port_dev)
+void sas_ex_revalidate_domain(struct domain_device *port_dev)
 {
 	int res;
 	struct domain_device *dev = NULL;
@@ -2117,11 +2119,10 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
 			res = sas_find_bcast_phy(dev, &phy_id, i, true);
 			if (phy_id == -1)
 				break;
-			res = sas_rediscover(dev, phy_id);
+			sas_rediscover(dev, phy_id);
 			i = phy_id + 1;
 		} while (i < ex->num_phys);
 	}
-	return res;
 }
 
 void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 420156cea3ee..e557bcb0c266 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct domain_device *);
 
 void sas_init_ex_attr(void);
 
-int  sas_ex_revalidate_domain(struct domain_device *);
+void sas_ex_revalidate_domain(struct domain_device *);
 
 void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
 void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
-- 
2.14.4

Re: [PATCH v2 3/7] scsi: libsas: optimize the debug print of the revalidate process

[John Garry]

On 30/01/2019 08:24, Jason Yan wrote:
> sas_rediscover() returns error code if discover failed for a expander
> phy. And sas_ex_revalidate_domain() only returns the last phy's error
> code. So when sas_revalidate_domain() prints the return value of the
> discover process, we do not know if the revalidation for every phy is
> successful or not. We just know the last bcast phy revalidation
> succeeded or not.
>
> No need to return a error code for sas_ex_revalidate_domain() and
> sas_rediscover(), and just print the debug log for each bcast phy directly
> in sas_rediscover().

do we want to know about every PHY, or just the PHY where res != 0?

>

I don't see any optimisation here. Maybe an improvement.


> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_discover.c |  7 +++----
>  drivers/scsi/libsas/sas_expander.c | 11 ++++++-----
>  include/scsi/libsas.h              |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 726ada9b8c79..ffc571a12916 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -500,7 +500,6 @@ static void sas_discover_domain(struct work_struct *work)
>
>  static void sas_revalidate_domain(struct work_struct *work)
>  {
> -	int res = 0;
>  	struct sas_discovery_event *ev = to_sas_discovery_event(work);
>  	struct asd_sas_port *port = ev->port;
>  	struct sas_ha_struct *ha = port->ha;
> @@ -521,10 +520,10 @@ static void sas_revalidate_domain(struct work_struct *work)
>
>  	if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>  		     ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
> -		res = sas_ex_revalidate_domain(ddev);
> +		sas_ex_revalidate_domain(ddev);
>
> -	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d, res 0x%x\n",
> -		 port->id, task_pid_nr(current), res);
> +	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
> +		 port->id, task_pid_nr(current));
>   out:
>  	mutex_unlock(&ha->disco_mutex);
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 7b0e6dcef6e6..5cd720f93f96 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -2062,7 +2062,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
>   * first phy,for other phys in this port, we add it to the port to
>   * forming the wide-port.
>   */
> -static int sas_rediscover(struct domain_device *dev, const int phy_id)
> +static void sas_rediscover(struct domain_device *dev, const int phy_id)
>  {
>  	struct expander_device *ex = &dev->ex_dev;
>  	struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
> @@ -2090,7 +2090,9 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
>  		res = sas_rediscover_dev(dev, phy_id, last);
>  	} else
>  		res = sas_discover_new(dev, phy_id);
> -	return res;
> +
> +	pr_debug("ex %016llx phy%d discover returned 0x%x\n",

Hmmm.. this is not just discover, but also rediscover

> +		 SAS_ADDR(dev->sas_addr), phy_id, res);
>  }
>
>  /**
> @@ -2102,7 +2104,7 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
>   * Discover process only interrogates devices in order to discover the
>   * domain.
>   */
> -int sas_ex_revalidate_domain(struct domain_device *port_dev)
> +void sas_ex_revalidate_domain(struct domain_device *port_dev)
>  {
>  	int res;
>  	struct domain_device *dev = NULL;
> @@ -2117,11 +2119,10 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
>  			res = sas_find_bcast_phy(dev, &phy_id, i, true);

this was missed

>  			if (phy_id == -1)
>  				break;
> -			res = sas_rediscover(dev, phy_id);
> +			sas_rediscover(dev, phy_id);
>  			i = phy_id + 1;
>  		} while (i < ex->num_phys);
>  	}
> -	return res;
>  }
>
>  void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 420156cea3ee..e557bcb0c266 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct domain_device *);
>
>  void sas_init_ex_attr(void);
>
> -int  sas_ex_revalidate_domain(struct domain_device *);
> +void sas_ex_revalidate_domain(struct domain_device *);
>
>  void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>

Re: [PATCH v2 3/7] scsi: libsas: optimize the debug print of the revalidate process

[Jason Yan]

On 2019/1/31 0:41, John Garry wrote:
> On 30/01/2019 08:24, Jason Yan wrote:
>> sas_rediscover() returns error code if discover failed for a expander
>> phy. And sas_ex_revalidate_domain() only returns the last phy's error
>> code. So when sas_revalidate_domain() prints the return value of the
>> discover process, we do not know if the revalidation for every phy is
>> successful or not. We just know the last bcast phy revalidation
>> succeeded or not.
>>
>> No need to return a error code for sas_ex_revalidate_domain() and
>> sas_rediscover(), and just print the debug log for each bcast phy
>> directly
>> in sas_rediscover().
>
> do we want to know about every PHY, or just the PHY where res != 0?
>

Here I mean every PHY that raises bcast.

>>
>
> I don't see any optimisation here. Maybe an improvement.
>

Thanks, I will change the wording.

>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_discover.c |  7 +++----
>>  drivers/scsi/libsas/sas_expander.c | 11 ++++++-----
>>  include/scsi/libsas.h              |  2 +-
>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_discover.c
>> b/drivers/scsi/libsas/sas_discover.c
>> index 726ada9b8c79..ffc571a12916 100644
>> --- a/drivers/scsi/libsas/sas_discover.c
>> +++ b/drivers/scsi/libsas/sas_discover.c
>> @@ -500,7 +500,6 @@ static void sas_discover_domain(struct work_struct
>> *work)
>>
>>  static void sas_revalidate_domain(struct work_struct *work)
>>  {
>> -    int res = 0;
>>      struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>      struct asd_sas_port *port = ev->port;
>>      struct sas_ha_struct *ha = port->ha;
>> @@ -521,10 +520,10 @@ static void sas_revalidate_domain(struct
>> work_struct *work)
>>
>>      if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>>               ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
>> -        res = sas_ex_revalidate_domain(ddev);
>> +        sas_ex_revalidate_domain(ddev);
>>
>> -    pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d, res 0x%x\n",
>> -         port->id, task_pid_nr(current), res);
>> +    pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>> +         port->id, task_pid_nr(current));
>>   out:
>>      mutex_unlock(&ha->disco_mutex);
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c
>> b/drivers/scsi/libsas/sas_expander.c
>> index 7b0e6dcef6e6..5cd720f93f96 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -2062,7 +2062,7 @@ static int sas_rediscover_dev(struct
>> domain_device *dev, int phy_id, bool last)
>>   * first phy,for other phys in this port, we add it to the port to
>>   * forming the wide-port.
>>   */
>> -static int sas_rediscover(struct domain_device *dev, const int phy_id)
>> +static void sas_rediscover(struct domain_device *dev, const int phy_id)
>>  {
>>      struct expander_device *ex = &dev->ex_dev;
>>      struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
>> @@ -2090,7 +2090,9 @@ static int sas_rediscover(struct domain_device
>> *dev, const int phy_id)
>>          res = sas_rediscover_dev(dev, phy_id, last);
>>      } else
>>          res = sas_discover_new(dev, phy_id);
>> -    return res;
>> +
>> +    pr_debug("ex %016llx phy%d discover returned 0x%x\n",
>
> Hmmm.. this is not just discover, but also rediscover
>

Yes, will fix.

>> +         SAS_ADDR(dev->sas_addr), phy_id, res);
>>  }
>>
>>  /**
>> @@ -2102,7 +2104,7 @@ static int sas_rediscover(struct domain_device
>> *dev, const int phy_id)
>>   * Discover process only interrogates devices in order to discover the
>>   * domain.
>>   */
>> -int sas_ex_revalidate_domain(struct domain_device *port_dev)
>> +void sas_ex_revalidate_domain(struct domain_device *port_dev)
>>  {
>>      int res;
>>      struct domain_device *dev = NULL;
>> @@ -2117,11 +2119,10 @@ int sas_ex_revalidate_domain(struct
>> domain_device *port_dev)
>>              res = sas_find_bcast_phy(dev, &phy_id, i, true);
>
> this was missed

Yes, the return value of sas_find_bcast_phy() is actually unused, and
inside the function debug info has been printed. So we can directly
make it a void function.

>
>>              if (phy_id == -1)
>>                  break;
>> -            res = sas_rediscover(dev, phy_id);
>> +            sas_rediscover(dev, phy_id);
>>              i = phy_id + 1;
>>          } while (i < ex->num_phys);
>>      }
>> -    return res;
>>  }
>>
>>  void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index 420156cea3ee..e557bcb0c266 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct
>> domain_device *);
>>
>>  void sas_init_ex_attr(void);
>>
>> -int  sas_ex_revalidate_domain(struct domain_device *);
>> +void sas_ex_revalidate_domain(struct domain_device *);
>>
>>  void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>>
>
>
>
> .
>

Re: [PATCH v2 3/7] scsi: libsas: optimize the debug print of the revalidate process

[John Garry]

On 31/01/2019 01:31, Jason Yan wrote:
>
>
> On 2019/1/31 0:41, John Garry wrote:
>> On 30/01/2019 08:24, Jason Yan wrote:
>>> sas_rediscover() returns error code if discover failed for a expander
>>> phy. And sas_ex_revalidate_domain() only returns the last phy's error
>>> code. So when sas_revalidate_domain() prints the return value of the
>>> discover process, we do not know if the revalidation for every phy is
>>> successful or not. We just know the last bcast phy revalidation
>>> succeeded or not.
>>>
>>> No need to return a error code for sas_ex_revalidate_domain() and
>>> sas_rediscover(), and just print the debug log for each bcast phy
>>> directly
>>> in sas_rediscover().
>>
>> do we want to know about every PHY, or just the PHY where res != 0?
>>
>
> Here I mean every PHY that raises bcast.


This may be better added at the sas_rediscover() callsite. And do you 
feel adding this for every bcast phy is useful, or just those whose 
rediscover error'ed?

>
>>>
>>
>> I don't see any optimisation here. Maybe an improvement.
>>
>
> Thanks, I will change the wording.
>
>>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> CC: John Garry <john.garry@huawei.com>
>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>> CC: Ewan Milne <emilne@redhat.com>
>>> CC: Christoph Hellwig <hch@lst.de>
>>> CC: Tomas Henzl <thenzl@redhat.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> CC: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/libsas/sas_discover.c |  7 +++----
>>>  drivers/scsi/libsas/sas_expander.c | 11 ++++++-----
>>>  include/scsi/libsas.h              |  2 +-
>>>  3 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_discover.c
>>> b/drivers/scsi/libsas/sas_discover.c
>>> index 726ada9b8c79..ffc571a12916 100644
>>> --- a/drivers/scsi/libsas/sas_discover.c
>>> +++ b/drivers/scsi/libsas/sas_discover.c
>>> @@ -500,7 +500,6 @@ static void sas_discover_domain(struct work_struct
>>> *work)
>>>
>>>  static void sas_revalidate_domain(struct work_struct *work)
>>>  {
>>> -    int res = 0;
>>>      struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>>      struct asd_sas_port *port = ev->port;
>>>      struct sas_ha_struct *ha = port->ha;
>>> @@ -521,10 +520,10 @@ static void sas_revalidate_domain(struct
>>> work_struct *work)
>>>
>>>      if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>>>               ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
>>> -        res = sas_ex_revalidate_domain(ddev);
>>> +        sas_ex_revalidate_domain(ddev);
>>>
>>> -    pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d, res 0x%x\n",
>>> -         port->id, task_pid_nr(current), res);
>>> +    pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>>> +         port->id, task_pid_nr(current));
>>>   out:
>>>      mutex_unlock(&ha->disco_mutex);
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index 7b0e6dcef6e6..5cd720f93f96 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -2062,7 +2062,7 @@ static int sas_rediscover_dev(struct
>>> domain_device *dev, int phy_id, bool last)
>>>   * first phy,for other phys in this port, we add it to the port to
>>>   * forming the wide-port.
>>>   */
>>> -static int sas_rediscover(struct domain_device *dev, const int phy_id)
>>> +static void sas_rediscover(struct domain_device *dev, const int phy_id)
>>>  {
>>>      struct expander_device *ex = &dev->ex_dev;
>>>      struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
>>> @@ -2090,7 +2090,9 @@ static int sas_rediscover(struct domain_device
>>> *dev, const int phy_id)
>>>          res = sas_rediscover_dev(dev, phy_id, last);
>>>      } else
>>>          res = sas_discover_new(dev, phy_id);
>>> -    return res;
>>> +
>>> +    pr_debug("ex %016llx phy%d discover returned 0x%x\n",
>>
>> Hmmm.. this is not just discover, but also rediscover
>>
>
> Yes, will fix.
>
>>> +         SAS_ADDR(dev->sas_addr), phy_id, res);
>>>  }
>>>
>>>  /**
>>> @@ -2102,7 +2104,7 @@ static int sas_rediscover(struct domain_device
>>> *dev, const int phy_id)
>>>   * Discover process only interrogates devices in order to discover the
>>>   * domain.
>>>   */
>>> -int sas_ex_revalidate_domain(struct domain_device *port_dev)
>>> +void sas_ex_revalidate_domain(struct domain_device *port_dev)
>>>  {
>>>      int res;
>>>      struct domain_device *dev = NULL;
>>> @@ -2117,11 +2119,10 @@ int sas_ex_revalidate_domain(struct
>>> domain_device *port_dev)
>>>              res = sas_find_bcast_phy(dev, &phy_id, i, true);
>>
>> this was missed
>
> Yes, the return value of sas_find_bcast_phy() is actually unused, and
> inside the function debug info has been printed. So we can directly
> make it a void function.

ok, but how about add a comment like:

-if (phy_id == -1)
+if (phy_id == -1) /* no remaining broadcast phy found */

>
>>
>>>              if (phy_id == -1)
>>>                  break;
>>> -            res = sas_rediscover(dev, phy_id);
>>> +            sas_rediscover(dev, phy_id);
>>>              i = phy_id + 1;
>>>          } while (i < ex->num_phys);
>>>      }
>>> -    return res;
>>>  }
>>>
>>>  void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>> index 420156cea3ee..e557bcb0c266 100644
>>> --- a/include/scsi/libsas.h
>>> +++ b/include/scsi/libsas.h
>>> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct
>>> domain_device *);
>>>
>>>  void sas_init_ex_attr(void);
>>>
>>> -int  sas_ex_revalidate_domain(struct domain_device *);
>>> +void sas_ex_revalidate_domain(struct domain_device *);
>>>
>>>  void sas_unregister_domain_devices(struct asd_sas_port *port, int
>>> gone);
>>>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>>>
>>
>>
>>
>> .
>>
>
>
> .
>

[PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[Jason Yan]

Now if a new device replaced a old device, the sas address will change.
We unregister the old device and discover the new device in one
revalidation process. But after we deferred the sas_port_delete(), the
sas port is not deleted when we registering the new port and device.
The sas port cannot be added because the name of the new port is the
same as the old.

Fix this by doing the replacement in two steps. The first revalidation
only delete the old device and trigger a new revalidation. The second
revalidation discover the new device. To keep the event processing
synchronised to the original event, we wrapped a loop and added a new
parameter to see if we should revalidate again.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: chenxiang <chenxiang66@hisilicon.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_discover.c | 20 +++++++++++++++-----
 drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
 include/scsi/libsas.h              |  2 +-
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index ffc571a12916..c825c89fbddd 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -498,12 +498,10 @@ static void sas_discover_domain(struct work_struct *work)
 		 task_pid_nr(current), error);
 }
 
-static void sas_revalidate_domain(struct work_struct *work)
+static void sas_do_revalidate_domain(struct asd_sas_port *port, bool *retry)
 {
-	struct sas_discovery_event *ev = to_sas_discovery_event(work);
-	struct asd_sas_port *port = ev->port;
-	struct sas_ha_struct *ha = port->ha;
 	struct domain_device *ddev = port->port_dev;
+	struct sas_ha_struct *ha = port->ha;
 
 	/* prevent revalidation from finding sata links in recovery */
 	mutex_lock(&ha->disco_mutex);
@@ -520,7 +518,7 @@ static void sas_revalidate_domain(struct work_struct *work)
 
 	if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
 		     ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
-		sas_ex_revalidate_domain(ddev);
+		sas_ex_revalidate_domain(ddev, retry);
 
 	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
 		 port->id, task_pid_nr(current));
@@ -532,6 +530,18 @@ static void sas_revalidate_domain(struct work_struct *work)
 	sas_probe_devices(port);
 }
 
+static void sas_revalidate_domain(struct work_struct *work)
+{
+	struct sas_discovery_event *ev = to_sas_discovery_event(work);
+	struct asd_sas_port *port = ev->port;
+	bool retry;
+
+	do {
+		retry = false;
+		sas_do_revalidate_domain(port, &retry);
+	} while (retry);
+}
+
 /* ---------- Events ---------- */
 
 static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 5cd720f93f96..cdbf8d8a28bf 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1994,7 +1994,8 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
 	return false;
 }
 
-static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
+static int sas_unregister(struct domain_device *dev, int phy_id, bool last,
+			      bool *retry)
 {
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *phy = &ex->ex_phy[phy_id];
@@ -2045,7 +2046,12 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
 		SAS_ADDR(phy->attached_sas_addr));
 	sas_unregister_devs_sas_addr(dev, phy_id, last);
 
-	return sas_discover_new(dev, phy_id);
+	/* force the next revalidation find this phy and bring it up */
+	phy->phy_change_count = -1;
+	ex->ex_change_count = -1;
+	*retry = true;
+
+	return 0;
 }
 
 /**
@@ -2062,7 +2068,8 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
  * 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_rediscover(struct domain_device *dev, const int phy_id,
+			   bool *retry)
 {
 	struct expander_device *ex = &dev->ex_dev;
 	struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
@@ -2087,7 +2094,7 @@ static void sas_rediscover(struct domain_device *dev, const int phy_id)
 				break;
 			}
 		}
-		res = sas_rediscover_dev(dev, phy_id, last);
+		res = sas_unregister(dev, phy_id, last, retry);
 	} else
 		res = sas_discover_new(dev, phy_id);
 
@@ -2098,13 +2105,14 @@ static void sas_rediscover(struct domain_device *dev, const int phy_id)
 /**
  * sas_ex_revalidate_domain - revalidate the domain
  * @port_dev: port domain device.
+ * @retry: do we need to revalidate again
  *
  * 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.
  */
-void sas_ex_revalidate_domain(struct domain_device *port_dev)
+void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry)
 {
 	int res;
 	struct domain_device *dev = NULL;
@@ -2119,7 +2127,7 @@ void sas_ex_revalidate_domain(struct domain_device *port_dev)
 			res = sas_find_bcast_phy(dev, &phy_id, i, true);
 			if (phy_id == -1)
 				break;
-			sas_rediscover(dev, phy_id);
+			sas_rediscover(dev, phy_id, retry);
 			i = phy_id + 1;
 		} while (i < ex->num_phys);
 	}
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index e557bcb0c266..deb75765e555 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct domain_device *);
 
 void sas_init_ex_attr(void);
 
-void sas_ex_revalidate_domain(struct domain_device *);
+void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry);
 
 void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
 void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
-- 
2.14.4

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[John Garry]

On 30/01/2019 08:24, Jason Yan wrote:
> Now if a new device replaced a old device, the sas address will change.

Hmmm... not if it's a SATA disk, which would have some same invented SAS 
address.

> We unregister the old device and discover the new device in one
> revalidation process. But after we deferred the sas_port_delete(), the
> sas port is not deleted when we registering the new port and device.
> The sas port cannot be added because the name of the new port is the
> same as the old.
>
> Fix this by doing the replacement in two steps. The first revalidation
> only delete the old device and trigger a new revalidation. The second
> revalidation discover the new device. To keep the event processing
> synchronised to the original event,

Did I originally suggest this? It seems to needlessly make the code more 
complicated.

we wrapped a loop and added a new
> parameter to see if we should revalidate again.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: chenxiang <chenxiang66@hisilicon.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_discover.c | 20 +++++++++++++++-----
>  drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
>  include/scsi/libsas.h              |  2 +-
>  3 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index ffc571a12916..c825c89fbddd 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -498,12 +498,10 @@ static void sas_discover_domain(struct work_struct *work)
>  		 task_pid_nr(current), error);
>  }
>
> -static void sas_revalidate_domain(struct work_struct *work)
> +static void sas_do_revalidate_domain(struct asd_sas_port *port, bool *retry)
>  {
> -	struct sas_discovery_event *ev = to_sas_discovery_event(work);
> -	struct asd_sas_port *port = ev->port;
> -	struct sas_ha_struct *ha = port->ha;
>  	struct domain_device *ddev = port->port_dev;
> +	struct sas_ha_struct *ha = port->ha;
>
>  	/* prevent revalidation from finding sata links in recovery */
>  	mutex_lock(&ha->disco_mutex);
> @@ -520,7 +518,7 @@ static void sas_revalidate_domain(struct work_struct *work)
>
>  	if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>  		     ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
> -		sas_ex_revalidate_domain(ddev);
> +		sas_ex_revalidate_domain(ddev, retry);
>
>  	pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>  		 port->id, task_pid_nr(current));
> @@ -532,6 +530,18 @@ static void sas_revalidate_domain(struct work_struct *work)
>  	sas_probe_devices(port);
>  }
>
> +static void sas_revalidate_domain(struct work_struct *work)
> +{
> +	struct sas_discovery_event *ev = to_sas_discovery_event(work);
> +	struct asd_sas_port *port = ev->port;
> +	bool retry;
> +
> +	do {
> +		retry = false;
> +		sas_do_revalidate_domain(port, &retry);
> +	} while (retry);
> +}
> +
>  /* ---------- Events ---------- */
>
>  static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work *sw)
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 5cd720f93f96..cdbf8d8a28bf 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1994,7 +1994,8 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
>  	return false;
>  }
>
> -static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
> +static int sas_unregister(struct domain_device *dev, int phy_id, bool last,
> +			      bool *retry)
>  {
>  	struct expander_device *ex = &dev->ex_dev;
>  	struct ex_phy *phy = &ex->ex_phy[phy_id];
> @@ -2045,7 +2046,12 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
>  		SAS_ADDR(phy->attached_sas_addr));
>  	sas_unregister_devs_sas_addr(dev, phy_id, last);
>
> -	return sas_discover_new(dev, phy_id);
> +	/* force the next revalidation find this phy and bring it up */
> +	phy->phy_change_count = -1;
> +	ex->ex_change_count = -1;
> +	*retry = true;

Ohh, sorry to say, but that's a real hack :)

Could we just add a flag for the expander PHY to force a discovery 
instead of this?

I assume that you need to do this as the expander PHY change count will 
not be modified for the next revalidation (so no discovery on that PHY).

> +
> +	return 0;
>  }
>
>  /**
> @@ -2062,7 +2068,8 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id, bool last)
>   * 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_rediscover(struct domain_device *dev, const int phy_id,
> +			   bool *retry)
>  {
>  	struct expander_device *ex = &dev->ex_dev;
>  	struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
> @@ -2087,7 +2094,7 @@ static void sas_rediscover(struct domain_device *dev, const int phy_id)
>  				break;
>  			}
>  		}
> -		res = sas_rediscover_dev(dev, phy_id, last);
> +		res = sas_unregister(dev, phy_id, last, retry);
>  	} else
>  		res = sas_discover_new(dev, phy_id);
>
> @@ -2098,13 +2105,14 @@ static void sas_rediscover(struct domain_device *dev, const int phy_id)
>  /**
>   * sas_ex_revalidate_domain - revalidate the domain
>   * @port_dev: port domain device.
> + * @retry: do we need to revalidate again
>   *
>   * 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.
>   */
> -void sas_ex_revalidate_domain(struct domain_device *port_dev)
> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry)
>  {
>  	int res;
>  	struct domain_device *dev = NULL;
> @@ -2119,7 +2127,7 @@ void sas_ex_revalidate_domain(struct domain_device *port_dev)
>  			res = sas_find_bcast_phy(dev, &phy_id, i, true);
>  			if (phy_id == -1)
>  				break;
> -			sas_rediscover(dev, phy_id);
> +			sas_rediscover(dev, phy_id, retry);
>  			i = phy_id + 1;
>  		} while (i < ex->num_phys);
>  	}
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index e557bcb0c266..deb75765e555 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct domain_device *);
>
>  void sas_init_ex_attr(void);
>
> -void sas_ex_revalidate_domain(struct domain_device *);
> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool *retry);
>
>  void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[Jason Yan]

On 2019/1/31 1:22, John Garry wrote:
> On 30/01/2019 08:24, Jason Yan wrote:
>> Now if a new device replaced a old device, the sas address will change.
>
> Hmmm... not if it's a SATA disk, which would have some same invented SAS
> address.
>

Yes, it's only for a SAS disk.

>> We unregister the old device and discover the new device in one
>> revalidation process. But after we deferred the sas_port_delete(), the
>> sas port is not deleted when we registering the new port and device.
>> The sas port cannot be added because the name of the new port is the
>> same as the old.
>>
>> Fix this by doing the replacement in two steps. The first revalidation
>> only delete the old device and trigger a new revalidation. The second
>> revalidation discover the new device. To keep the event processing
>> synchronised to the original event,
>
> Did I originally suggest this? It seems to needlessly make the code more
> complicated.
>

Yes, my first version was raise a new bcast event, and you said it's not 
synchronised to the original event.  Shall I get back to that approach?

> we wrapped a loop and added a new
>> parameter to see if we should revalidate again.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: chenxiang <chenxiang66@hisilicon.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_discover.c | 20 +++++++++++++++-----
>>  drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
>>  include/scsi/libsas.h              |  2 +-
>>  3 files changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_discover.c
>> b/drivers/scsi/libsas/sas_discover.c
>> index ffc571a12916..c825c89fbddd 100644
>> --- a/drivers/scsi/libsas/sas_discover.c
>> +++ b/drivers/scsi/libsas/sas_discover.c
>> @@ -498,12 +498,10 @@ static void sas_discover_domain(struct
>> work_struct *work)
>>           task_pid_nr(current), error);
>>  }
>>
>> -static void sas_revalidate_domain(struct work_struct *work)
>> +static void sas_do_revalidate_domain(struct asd_sas_port *port, bool
>> *retry)
>>  {
>> -    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>> -    struct asd_sas_port *port = ev->port;
>> -    struct sas_ha_struct *ha = port->ha;
>>      struct domain_device *ddev = port->port_dev;
>> +    struct sas_ha_struct *ha = port->ha;
>>
>>      /* prevent revalidation from finding sata links in recovery */
>>      mutex_lock(&ha->disco_mutex);
>> @@ -520,7 +518,7 @@ static void sas_revalidate_domain(struct
>> work_struct *work)
>>
>>      if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>>               ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
>> -        sas_ex_revalidate_domain(ddev);
>> +        sas_ex_revalidate_domain(ddev, retry);
>>
>>      pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>>           port->id, task_pid_nr(current));
>> @@ -532,6 +530,18 @@ static void sas_revalidate_domain(struct
>> work_struct *work)
>>      sas_probe_devices(port);
>>  }
>>
>> +static void sas_revalidate_domain(struct work_struct *work)
>> +{
>> +    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>> +    struct asd_sas_port *port = ev->port;
>> +    bool retry;
>> +
>> +    do {
>> +        retry = false;
>> +        sas_do_revalidate_domain(port, &retry);
>> +    } while (retry);
>> +}
>> +
>>  /* ---------- Events ---------- */
>>
>>  static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work
>> *sw)
>> diff --git a/drivers/scsi/libsas/sas_expander.c
>> b/drivers/scsi/libsas/sas_expander.c
>> index 5cd720f93f96..cdbf8d8a28bf 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1994,7 +1994,8 @@ static bool dev_type_flutter(enum
>> sas_device_type new, enum sas_device_type old)
>>      return false;
>>  }
>>
>> -static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>> bool last)
>> +static int sas_unregister(struct domain_device *dev, int phy_id, bool
>> last,
>> +                  bool *retry)
>>  {
>>      struct expander_device *ex = &dev->ex_dev;
>>      struct ex_phy *phy = &ex->ex_phy[phy_id];
>> @@ -2045,7 +2046,12 @@ static int sas_rediscover_dev(struct
>> domain_device *dev, int phy_id, bool last)
>>          SAS_ADDR(phy->attached_sas_addr));
>>      sas_unregister_devs_sas_addr(dev, phy_id, last);
>>
>> -    return sas_discover_new(dev, phy_id);
>> +    /* force the next revalidation find this phy and bring it up */
>> +    phy->phy_change_count = -1;
>> +    ex->ex_change_count = -1;
>> +    *retry = true;
>
> Ohh, sorry to say, but that's a real hack :)
>

This is the way sas_resume_port() already in use.

> Could we just add a flag for the expander PHY to force a discovery
> instead of this?
>

of course we can add a flag instead of this, but I don't think it worth
to do this. We have to change the logic of sas_find_bcast_dev() and
sas_find_bcast_phy() to achieve this. Or we have to add a new function
to find out which PHY's flag is set.

> I assume that you need to do this as the expander PHY change count will
> not be modified for the next revalidation (so no discovery on that PHY).
>

To save one instruction(assign), we have to add two(check and assign)?
And how to predict if the PHY change count will be modified or not?
It's unnessesary to do this.

>> +
>> +    return 0;
>>  }
>>
>>  /**
>> @@ -2062,7 +2068,8 @@ static int sas_rediscover_dev(struct
>> domain_device *dev, int phy_id, bool last)
>>   * 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_rediscover(struct domain_device *dev, const int phy_id,
>> +               bool *retry)
>>  {
>>      struct expander_device *ex = &dev->ex_dev;
>>      struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
>> @@ -2087,7 +2094,7 @@ static void sas_rediscover(struct domain_device
>> *dev, const int phy_id)
>>                  break;
>>              }
>>          }
>> -        res = sas_rediscover_dev(dev, phy_id, last);
>> +        res = sas_unregister(dev, phy_id, last, retry);
>>      } else
>>          res = sas_discover_new(dev, phy_id);
>>
>> @@ -2098,13 +2105,14 @@ static void sas_rediscover(struct
>> domain_device *dev, const int phy_id)
>>  /**
>>   * sas_ex_revalidate_domain - revalidate the domain
>>   * @port_dev: port domain device.
>> + * @retry: do we need to revalidate again
>>   *
>>   * 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.
>>   */
>> -void sas_ex_revalidate_domain(struct domain_device *port_dev)
>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>> *retry)
>>  {
>>      int res;
>>      struct domain_device *dev = NULL;
>> @@ -2119,7 +2127,7 @@ void sas_ex_revalidate_domain(struct
>> domain_device *port_dev)
>>              res = sas_find_bcast_phy(dev, &phy_id, i, true);
>>              if (phy_id == -1)
>>                  break;
>> -            sas_rediscover(dev, phy_id);
>> +            sas_rediscover(dev, phy_id, retry);
>>              i = phy_id + 1;
>>          } while (i < ex->num_phys);
>>      }
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index e557bcb0c266..deb75765e555 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct
>> domain_device *);
>>
>>  void sas_init_ex_attr(void);
>>
>> -void sas_ex_revalidate_domain(struct domain_device *);
>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>> *retry);
>>
>>  void sas_unregister_domain_devices(struct asd_sas_port *port, int gone);
>>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>>
>
>
>
> .
>

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[John Garry]

On 31/01/2019 02:04, Jason Yan wrote:
>
>
> On 2019/1/31 1:22, John Garry wrote:
>> On 30/01/2019 08:24, Jason Yan wrote:
>>> Now if a new device replaced a old device, the sas address will change.
>>
>> Hmmm... not if it's a SATA disk, which would have some same invented SAS
>> address.
>>
>
> Yes, it's only for a SAS disk.
>
>>> We unregister the old device and discover the new device in one
>>> revalidation process. But after we deferred the sas_port_delete(), the
>>> sas port is not deleted when we registering the new port and device.
>>> The sas port cannot be added because the name of the new port is the
>>> same as the old.
>>>
>>> Fix this by doing the replacement in two steps. The first revalidation
>>> only delete the old device and trigger a new revalidation. The second
>>> revalidation discover the new device. To keep the event processing
>>> synchronised to the original event,
>>
>> Did I originally suggest this? It seems to needlessly make the code more
>> complicated.
>>
>
> Yes, my first version was raise a new bcast event, and you said it's not
> synchronised to the original event.  Shall I get back to that approach?

Not sure. This patch seems to fix something closely related to that in 
"scsi: libsas: fix issue of swapping two sas disks", which I will check 
further.

EOM

>
>> we wrapped a loop and added a new
>>> parameter to see if we should revalidate again.
>>>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> CC: chenxiang <chenxiang66@hisilicon.com>
>>> CC: John Garry <john.garry@huawei.com>
>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>> CC: Ewan Milne <emilne@redhat.com>
>>> CC: Christoph Hellwig <hch@lst.de>
>>> CC: Tomas Henzl <thenzl@redhat.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> CC: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/libsas/sas_discover.c | 20 +++++++++++++++-----
>>>  drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
>>>  include/scsi/libsas.h              |  2 +-
>>>  3 files changed, 30 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_discover.c
>>> b/drivers/scsi/libsas/sas_discover.c
>>> index ffc571a12916..c825c89fbddd 100644
>>> --- a/drivers/scsi/libsas/sas_discover.c
>>> +++ b/drivers/scsi/libsas/sas_discover.c
>>> @@ -498,12 +498,10 @@ static void sas_discover_domain(struct
>>> work_struct *work)
>>>           task_pid_nr(current), error);
>>>  }
>>>
>>> -static void sas_revalidate_domain(struct work_struct *work)
>>> +static void sas_do_revalidate_domain(struct asd_sas_port *port, bool
>>> *retry)
>>>  {
>>> -    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>> -    struct asd_sas_port *port = ev->port;
>>> -    struct sas_ha_struct *ha = port->ha;
>>>      struct domain_device *ddev = port->port_dev;
>>> +    struct sas_ha_struct *ha = port->ha;
>>>
>>>      /* prevent revalidation from finding sata links in recovery */
>>>      mutex_lock(&ha->disco_mutex);
>>> @@ -520,7 +518,7 @@ static void sas_revalidate_domain(struct
>>> work_struct *work)
>>>
>>>      if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>>>               ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
>>> -        sas_ex_revalidate_domain(ddev);
>>> +        sas_ex_revalidate_domain(ddev, retry);
>>>
>>>      pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>>>           port->id, task_pid_nr(current));
>>> @@ -532,6 +530,18 @@ static void sas_revalidate_domain(struct
>>> work_struct *work)
>>>      sas_probe_devices(port);
>>>  }
>>>
>>> +static void sas_revalidate_domain(struct work_struct *work)
>>> +{
>>> +    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>> +    struct asd_sas_port *port = ev->port;
>>> +    bool retry;
>>> +
>>> +    do {
>>> +        retry = false;
>>> +        sas_do_revalidate_domain(port, &retry);
>>> +    } while (retry);
>>> +}
>>> +
>>>  /* ---------- Events ---------- */
>>>
>>>  static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work
>>> *sw)
>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index 5cd720f93f96..cdbf8d8a28bf 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1994,7 +1994,8 @@ static bool dev_type_flutter(enum
>>> sas_device_type new, enum sas_device_type old)
>>>      return false;
>>>  }
>>>
>>> -static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>>> bool last)
>>> +static int sas_unregister(struct domain_device *dev, int phy_id, bool
>>> last,
>>> +                  bool *retry)
>>>  {
>>>      struct expander_device *ex = &dev->ex_dev;
>>>      struct ex_phy *phy = &ex->ex_phy[phy_id];
>>> @@ -2045,7 +2046,12 @@ static int sas_rediscover_dev(struct
>>> domain_device *dev, int phy_id, bool last)
>>>          SAS_ADDR(phy->attached_sas_addr));
>>>      sas_unregister_devs_sas_addr(dev, phy_id, last);
>>>
>>> -    return sas_discover_new(dev, phy_id);
>>> +    /* force the next revalidation find this phy and bring it up */
>>> +    phy->phy_change_count = -1;
>>> +    ex->ex_change_count = -1;
>>> +    *retry = true;
>>
>> Ohh, sorry to say, but that's a real hack :)
>>
>
> This is the way sas_resume_port() already in use.
>
>> Could we just add a flag for the expander PHY to force a discovery
>> instead of this?
>>
>
> of course we can add a flag instead of this, but I don't think it worth
> to do this. We have to change the logic of sas_find_bcast_dev() and
> sas_find_bcast_phy() to achieve this. Or we have to add a new function
> to find out which PHY's flag is set.
>
>> I assume that you need to do this as the expander PHY change count will
>> not be modified for the next revalidation (so no discovery on that PHY).
>>
>
> To save one instruction(assign), we have to add two(check and assign)?
> And how to predict if the PHY change count will be modified or not?
> It's unnessesary to do this.
>
>>> +
>>> +    return 0;
>>>  }
>>>
>>>  /**
>>> @@ -2062,7 +2068,8 @@ static int sas_rediscover_dev(struct
>>> domain_device *dev, int phy_id, bool last)
>>>   * 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_rediscover(struct domain_device *dev, const int phy_id,
>>> +               bool *retry)
>>>  {
>>>      struct expander_device *ex = &dev->ex_dev;
>>>      struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
>>> @@ -2087,7 +2094,7 @@ static void sas_rediscover(struct domain_device
>>> *dev, const int phy_id)
>>>                  break;
>>>              }
>>>          }
>>> -        res = sas_rediscover_dev(dev, phy_id, last);
>>> +        res = sas_unregister(dev, phy_id, last, retry);
>>>      } else
>>>          res = sas_discover_new(dev, phy_id);
>>>
>>> @@ -2098,13 +2105,14 @@ static void sas_rediscover(struct
>>> domain_device *dev, const int phy_id)
>>>  /**
>>>   * sas_ex_revalidate_domain - revalidate the domain
>>>   * @port_dev: port domain device.
>>> + * @retry: do we need to revalidate again
>>>   *
>>>   * 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.
>>>   */
>>> -void sas_ex_revalidate_domain(struct domain_device *port_dev)
>>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>>> *retry)
>>>  {
>>>      int res;
>>>      struct domain_device *dev = NULL;
>>> @@ -2119,7 +2127,7 @@ void sas_ex_revalidate_domain(struct
>>> domain_device *port_dev)
>>>              res = sas_find_bcast_phy(dev, &phy_id, i, true);
>>>              if (phy_id == -1)
>>>                  break;
>>> -            sas_rediscover(dev, phy_id);
>>> +            sas_rediscover(dev, phy_id, retry);
>>>              i = phy_id + 1;
>>>          } while (i < ex->num_phys);
>>>      }
>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>> index e557bcb0c266..deb75765e555 100644
>>> --- a/include/scsi/libsas.h
>>> +++ b/include/scsi/libsas.h
>>> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct
>>> domain_device *);
>>>
>>>  void sas_init_ex_attr(void);
>>>
>>> -void sas_ex_revalidate_domain(struct domain_device *);
>>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>>> *retry);
>>>
>>>  void sas_unregister_domain_devices(struct asd_sas_port *port, int
>>> gone);
>>>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>>>
>>
>>
>>
>> .
>>
>
>
> .
>

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[John Garry]

On 31/01/2019 10:29, John Garry wrote:
> On 31/01/2019 02:04, Jason Yan wrote:
>>
>>
>> On 2019/1/31 1:22, John Garry wrote:
>>> On 30/01/2019 08:24, Jason Yan wrote:
>>>> Now if a new device replaced a old device, the sas address will change.
>>>
>>> Hmmm... not if it's a SATA disk, which would have some same invented SAS
>>> address.
>>>
>>
>> Yes, it's only for a SAS disk.
>>
>>>> We unregister the old device and discover the new device in one
>>>> revalidation process. But after we deferred the sas_port_delete(), the
>>>> sas port is not deleted when we registering the new port and device.
>>>> The sas port cannot be added because the name of the new port is the
>>>> same as the old.
>>>>
>>>> Fix this by doing the replacement in two steps. The first revalidation
>>>> only delete the old device and trigger a new revalidation. The second
>>>> revalidation discover the new device. To keep the event processing
>>>> synchronised to the original event,

This change seems ok, but please see below regarding generating the 
bcast events.

>>>
>>> Did I originally suggest this? It seems to needlessly make the code more
>>> complicated.
>>>
>>
>> Yes, my first version was raise a new bcast event, and you said it's not
>> synchronised to the original event.  Shall I get back to that approach?
>
> Not sure. This patch seems to fix something closely related to that in
> "scsi: libsas: fix issue of swapping two sas disks", which I will check
> further.
>

An idea:

So, before the libsas changes to generate dynamic events, when libsas 
was processing a particular event type - like a broadcast event - extra 
events generated by the LLDD were discarded by libsas.

The revalidation process attempted to do all revalidation for the domain 
is a single pass, which was ok. This really did not change.

However, in this revalidation pass, we also clear all expander and PHY 
events.

Maybe this is not the right thing to do. Maybe we should just clear a 
single PHY event per pass, since we're processing each broadcast event 
one-by-one.

Today you will notice that if we remove a disk for example, many 
broadcast events are generated, but only the first broadcast event 
actually does any revalidation.

EOM

>
>>
>>> we wrapped a loop and added a new
>>>> parameter to see if we should revalidate again.
>>>>
>>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>>> CC: chenxiang <chenxiang66@hisilicon.com>
>>>> CC: John Garry <john.garry@huawei.com>
>>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>>> CC: Ewan Milne <emilne@redhat.com>
>>>> CC: Christoph Hellwig <hch@lst.de>
>>>> CC: Tomas Henzl <thenzl@redhat.com>
>>>> CC: Dan Williams <dan.j.williams@intel.com>
>>>> CC: Hannes Reinecke <hare@suse.com>
>>>> ---
>>>>  drivers/scsi/libsas/sas_discover.c | 20 +++++++++++++++-----
>>>>  drivers/scsi/libsas/sas_expander.c | 20 ++++++++++++++------
>>>>  include/scsi/libsas.h              |  2 +-
>>>>  3 files changed, 30 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/libsas/sas_discover.c
>>>> b/drivers/scsi/libsas/sas_discover.c
>>>> index ffc571a12916..c825c89fbddd 100644
>>>> --- a/drivers/scsi/libsas/sas_discover.c
>>>> +++ b/drivers/scsi/libsas/sas_discover.c
>>>> @@ -498,12 +498,10 @@ static void sas_discover_domain(struct
>>>> work_struct *work)
>>>>           task_pid_nr(current), error);
>>>>  }
>>>>
>>>> -static void sas_revalidate_domain(struct work_struct *work)
>>>> +static void sas_do_revalidate_domain(struct asd_sas_port *port, bool
>>>> *retry)
>>>>  {
>>>> -    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>>> -    struct asd_sas_port *port = ev->port;
>>>> -    struct sas_ha_struct *ha = port->ha;
>>>>      struct domain_device *ddev = port->port_dev;
>>>> +    struct sas_ha_struct *ha = port->ha;
>>>>
>>>>      /* prevent revalidation from finding sata links in recovery */
>>>>      mutex_lock(&ha->disco_mutex);
>>>> @@ -520,7 +518,7 @@ static void sas_revalidate_domain(struct
>>>> work_struct *work)
>>>>
>>>>      if (ddev && (ddev->dev_type == SAS_FANOUT_EXPANDER_DEVICE ||
>>>>               ddev->dev_type == SAS_EDGE_EXPANDER_DEVICE))
>>>> -        sas_ex_revalidate_domain(ddev);
>>>> +        sas_ex_revalidate_domain(ddev, retry);
>>>>
>>>>      pr_debug("done REVALIDATING DOMAIN on port %d, pid:%d\n",
>>>>           port->id, task_pid_nr(current));
>>>> @@ -532,6 +530,18 @@ static void sas_revalidate_domain(struct
>>>> work_struct *work)
>>>>      sas_probe_devices(port);
>>>>  }
>>>>
>>>> +static void sas_revalidate_domain(struct work_struct *work)
>>>> +{
>>>> +    struct sas_discovery_event *ev = to_sas_discovery_event(work);
>>>> +    struct asd_sas_port *port = ev->port;
>>>> +    bool retry;
>>>> +
>>>> +    do {
>>>> +        retry = false;
>>>> +        sas_do_revalidate_domain(port, &retry);
>>>> +    } while (retry);
>>>> +}
>>>> +
>>>>  /* ---------- Events ---------- */
>>>>
>>>>  static void sas_chain_work(struct sas_ha_struct *ha, struct sas_work
>>>> *sw)
>>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>>> b/drivers/scsi/libsas/sas_expander.c
>>>> index 5cd720f93f96..cdbf8d8a28bf 100644
>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>> @@ -1994,7 +1994,8 @@ static bool dev_type_flutter(enum
>>>> sas_device_type new, enum sas_device_type old)
>>>>      return false;
>>>>  }
>>>>
>>>> -static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>>>> bool last)
>>>> +static int sas_unregister(struct domain_device *dev, int phy_id, bool
>>>> last,
>>>> +                  bool *retry)
>>>>  {
>>>>      struct expander_device *ex = &dev->ex_dev;
>>>>      struct ex_phy *phy = &ex->ex_phy[phy_id];
>>>> @@ -2045,7 +2046,12 @@ static int sas_rediscover_dev(struct
>>>> domain_device *dev, int phy_id, bool last)
>>>>          SAS_ADDR(phy->attached_sas_addr));
>>>>      sas_unregister_devs_sas_addr(dev, phy_id, last);
>>>>
>>>> -    return sas_discover_new(dev, phy_id);
>>>> +    /* force the next revalidation find this phy and bring it up */
>>>> +    phy->phy_change_count = -1;
>>>> +    ex->ex_change_count = -1;
>>>> +    *retry = true;
>>>
>>> Ohh, sorry to say, but that's a real hack :)
>>>
>>
>> This is the way sas_resume_port() already in use.
>>
>>> Could we just add a flag for the expander PHY to force a discovery
>>> instead of this?
>>>
>>
>> of course we can add a flag instead of this, but I don't think it worth
>> to do this. We have to change the logic of sas_find_bcast_dev() and
>> sas_find_bcast_phy() to achieve this. Or we have to add a new function
>> to find out which PHY's flag is set.
>>
>>> I assume that you need to do this as the expander PHY change count will
>>> not be modified for the next revalidation (so no discovery on that PHY).
>>>
>>
>> To save one instruction(assign), we have to add two(check and assign)?
>> And how to predict if the PHY change count will be modified or not?
>> It's unnessesary to do this.
>>
>>>> +
>>>> +    return 0;
>>>>  }
>>>>
>>>>  /**
>>>> @@ -2062,7 +2068,8 @@ static int sas_rediscover_dev(struct
>>>> domain_device *dev, int phy_id, bool last)
>>>>   * 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_rediscover(struct domain_device *dev, const int
>>>> phy_id,
>>>> +               bool *retry)
>>>>  {
>>>>      struct expander_device *ex = &dev->ex_dev;
>>>>      struct ex_phy *changed_phy = &ex->ex_phy[phy_id];
>>>> @@ -2087,7 +2094,7 @@ static void sas_rediscover(struct domain_device
>>>> *dev, const int phy_id)
>>>>                  break;
>>>>              }
>>>>          }
>>>> -        res = sas_rediscover_dev(dev, phy_id, last);
>>>> +        res = sas_unregister(dev, phy_id, last, retry);
>>>>      } else
>>>>          res = sas_discover_new(dev, phy_id);
>>>>
>>>> @@ -2098,13 +2105,14 @@ static void sas_rediscover(struct
>>>> domain_device *dev, const int phy_id)
>>>>  /**
>>>>   * sas_ex_revalidate_domain - revalidate the domain
>>>>   * @port_dev: port domain device.
>>>> + * @retry: do we need to revalidate again
>>>>   *
>>>>   * 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.
>>>>   */
>>>> -void sas_ex_revalidate_domain(struct domain_device *port_dev)
>>>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>>>> *retry)
>>>>  {
>>>>      int res;
>>>>      struct domain_device *dev = NULL;
>>>> @@ -2119,7 +2127,7 @@ void sas_ex_revalidate_domain(struct
>>>> domain_device *port_dev)
>>>>              res = sas_find_bcast_phy(dev, &phy_id, i, true);
>>>>              if (phy_id == -1)
>>>>                  break;
>>>> -            sas_rediscover(dev, phy_id);
>>>> +            sas_rediscover(dev, phy_id, retry);
>>>>              i = phy_id + 1;
>>>>          } while (i < ex->num_phys);
>>>>      }
>>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>>> index e557bcb0c266..deb75765e555 100644
>>>> --- a/include/scsi/libsas.h
>>>> +++ b/include/scsi/libsas.h
>>>> @@ -692,7 +692,7 @@ int  sas_discover_root_expander(struct
>>>> domain_device *);
>>>>
>>>>  void sas_init_ex_attr(void);
>>>>
>>>> -void sas_ex_revalidate_domain(struct domain_device *);
>>>> +void sas_ex_revalidate_domain(struct domain_device *port_dev, bool
>>>> *retry);
>>>>
>>>>  void sas_unregister_domain_devices(struct asd_sas_port *port, int
>>>> gone);
>>>>  void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *);
>>>>
>>>
>>>
>>>
>>> .
>>>
>>
>>
>> .
>>
>
>
>
> .
>

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[Jason Yan]

On 2019/2/1 0:38, John Garry wrote:
> On 31/01/2019 10:29, John Garry wrote:
>> On 31/01/2019 02:04, Jason Yan wrote:
>>>
>>>
>>> On 2019/1/31 1:22, John Garry wrote:
>>>> On 30/01/2019 08:24, Jason Yan wrote:
>>>>> Now if a new device replaced a old device, the sas address will
>>>>> change.
>>>>
>>>> Hmmm... not if it's a SATA disk, which would have some same invented
>>>> SAS
>>>> address.
>>>>
>>>
>>> Yes, it's only for a SAS disk.
>>>
>>>>> We unregister the old device and discover the new device in one
>>>>> revalidation process. But after we deferred the sas_port_delete(), the
>>>>> sas port is not deleted when we registering the new port and device.
>>>>> The sas port cannot be added because the name of the new port is the
>>>>> same as the old.
>>>>>
>>>>> Fix this by doing the replacement in two steps. The first revalidation
>>>>> only delete the old device and trigger a new revalidation. The second
>>>>> revalidation discover the new device. To keep the event processing
>>>>> synchronised to the original event,
>
> This change seems ok, but please see below regarding generating the
> bcast events.
>
>>>>
>>>> Did I originally suggest this? It seems to needlessly make the code
>>>> more
>>>> complicated.
>>>>
>>>
>>> Yes, my first version was raise a new bcast event, and you said it's not
>>> synchronised to the original event.  Shall I get back to that approach?
>>
>> Not sure. This patch seems to fix something closely related to that in
>> "scsi: libsas: fix issue of swapping two sas disks", which I will check
>> further.
>>
>
> An idea:
>
> So, before the libsas changes to generate dynamic events, when libsas
> was processing a particular event type - like a broadcast event - extra
> events generated by the LLDD were discarded by libsas.
>
> The revalidation process attempted to do all revalidation for the domain
> is a single pass, which was ok. This really did not change.
>
> However, in this revalidation pass, we also clear all expander and PHY
> events.
>

Actually we only clean one expander and it's attached PHYs events now.

> Maybe this is not the right thing to do. Maybe we should just clear a
> single PHY event per pass, since we're processing each broadcast event
> one-by-one.
>

Yes, we can do this. But I don't understand how this will fix the issue?
We have this issue now because we have to probe the sas port and/or 
delete the sas port out side of the disco_mutex. So for a specific PHY, 
we cannot add and delete at the same time inside the disco_mutex.

> Today you will notice that if we remove a disk for example, many
> broadcast events are generated, but only the first broadcast event
> actually does any revalidation.
>
> EOM
>

Re: [PATCH v2 4/7] scsi: libsas: split the replacement of sas disks in two steps

[John Garry]

On 01/02/2019 01:58, Jason Yan wrote:
>
>
> On 2019/2/1 0:38, John Garry wrote:
>> On 31/01/2019 10:29, John Garry wrote:
>>> On 31/01/2019 02:04, Jason Yan wrote:
>>>>
>>>>
>>>> On 2019/1/31 1:22, John Garry wrote:
>>>>> On 30/01/2019 08:24, Jason Yan wrote:
>>>>>> Now if a new device replaced a old device, the sas address will
>>>>>> change.
>>>>>
>>>>> Hmmm... not if it's a SATA disk, which would have some same invented
>>>>> SAS
>>>>> address.
>>>>>
>>>>
>>>> Yes, it's only for a SAS disk.
>>>>
>>>>>> We unregister the old device and discover the new device in one
>>>>>> revalidation process. But after we deferred the sas_port_delete(),
>>>>>> the
>>>>>> sas port is not deleted when we registering the new port and device.
>>>>>> The sas port cannot be added because the name of the new port is the
>>>>>> same as the old.
>>>>>>
>>>>>> Fix this by doing the replacement in two steps. The first
>>>>>> revalidation
>>>>>> only delete the old device and trigger a new revalidation. The second
>>>>>> revalidation discover the new device. To keep the event processing
>>>>>> synchronised to the original event,
>>
>> This change seems ok, but please see below regarding generating the
>> bcast events.
>>
>>>>>
>>>>> Did I originally suggest this? It seems to needlessly make the code
>>>>> more
>>>>> complicated.
>>>>>
>>>>
>>>> Yes, my first version was raise a new bcast event, and you said it's
>>>> not
>>>> synchronised to the original event.  Shall I get back to that approach?
>>>
>>> Not sure. This patch seems to fix something closely related to that in
>>> "scsi: libsas: fix issue of swapping two sas disks", which I will check
>>> further.
>>>
>>
>> An idea:
>>
>> So, before the libsas changes to generate dynamic events, when libsas
>> was processing a particular event type - like a broadcast event - extra
>> events generated by the LLDD were discarded by libsas.
>>
>> The revalidation process attempted to do all revalidation for the domain
>> is a single pass, which was ok. This really did not change.
>>
>> However, in this revalidation pass, we also clear all expander and PHY
>> events.
>>
>
> Actually we only clean one expander and it's attached PHYs events now.

ok, fine, it's just for one expander; but we still do clear that one 
expanders events fully.

However we would have to be careful here to ensure that we don't have a 
situation where we still have PHY events pending but no broadcast events 
to trigger the revalidation and subsequent processing.

>
>> Maybe this is not the right thing to do. Maybe we should just clear a
>> single PHY event per pass, since we're processing each broadcast event
>> one-by-one.
>>
>
> Yes, we can do this. But I don't understand how this will fix the issue?

It would solve the problem of having to fixup the expanders events = -1, 
which I mentioned was not so nice.

As for fixing the main problem, I was not against the idea of the other 
change in sas_rediscover_dev() to not call sas_discover_new() when the 
SAS address has changed.

> We have this issue now because we have to probe the sas port and/or
> delete the sas port out side of the disco_mutex. So for a specific PHY,
> we cannot add and delete at the same time inside the disco_mutex.
>
>> Today you will notice that if we remove a disk for example, many
>> broadcast events are generated, but only the first broadcast event
>> actually does any revalidation.
>>
>> EOM
>>
>
>
>
> .
>

[PATCH v2 5/7] scsi: libsas: check if the same device when flutter

[Jason Yan]

The ata device do not have a real sas address. If a ata device is
replaced with another one, the sas address is the same. Now libsas treat
this scenario as flutter and do not delete the old one and discover the
new one. This will cause the data read from or write to the wrong
device.

And also when hotplugging a sata device, libsas entered to the flutter
case and sometimes found the phy attached address is abnormal. The log
is like this:

sas: ex 500e004aaaaaaa1f phy6 originated BROADCAST(CHANGE)
sas: ex 500e004aaaaaaa1f phy06:U:0 attached: 0000000000000000 (no device)
sas: ex 500e004aaaaaaa1f phy 0x6 broadcast flutter

Fix this issue by checking the phy attached address and the ata device's
class and id if they are the same as the origin. The ata class and id is
readed in ata EH process. When ata EH is scheduled, revalidate will be
deferred and a new bcast will be raised.

Tested-by: Chen Liangfei <chenliangfei1@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: John Garry <john.garry@huawei.com>
CC: chenxiang <chenxiang66@hisilicon.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Tejun Heo <tj@kernel.org>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/ata/libata-core.c          |  3 +-
 drivers/scsi/libsas/sas_ata.c      | 18 ++++++++++
 drivers/scsi/libsas/sas_expander.c | 67 ++++++++++++++++++++++++++++++++------
 include/linux/libata.h             |  2 ++
 include/scsi/libsas.h              |  1 +
 5 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b8c3f9e6af89..67e77fa3c63a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4225,7 +4225,7 @@ void ata_std_postreset(struct ata_link *link, unsigned int *classes)
  *	RETURNS:
  *	1 if @dev matches @new_class and @new_id, 0 otherwise.
  */
-static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
+int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 			       const u16 *new_id)
 {
 	const u16 *old_id = dev->id;
@@ -7400,6 +7400,7 @@ EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
 EXPORT_SYMBOL_GPL(ata_do_eh);
 EXPORT_SYMBOL_GPL(ata_std_error_handler);
 
+EXPORT_SYMBOL_GPL(ata_dev_same_device);
 EXPORT_SYMBOL_GPL(ata_cable_40wire);
 EXPORT_SYMBOL_GPL(ata_cable_80wire);
 EXPORT_SYMBOL_GPL(ata_cable_unknown);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 6f93fee2b21b..a9f5523f0347 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -625,6 +625,22 @@ static int sas_get_ata_command_set(struct domain_device *dev)
 	return ata_dev_classify(&tf);
 }
 
+static void sas_ata_store_id(struct domain_device *dev)
+{
+	struct ata_device *ata_dev = sas_to_ata_dev(dev);
+	unsigned char model[ATA_ID_PROD_LEN + 1];
+	unsigned char serial[ATA_ID_SERNO_LEN + 1];
+
+	/* store the ata device's class and id */
+	memcpy(dev->sata_dev.id, ata_dev->id, ATA_ID_WORDS);
+	dev->sata_dev.class = ata_dev->class;
+
+	ata_id_c_string(ata_dev->id, model, ATA_ID_PROD, sizeof(model));
+	ata_id_c_string(ata_dev->id, serial, ATA_ID_SERNO, sizeof(serial));
+
+	sas_ata_printk(KERN_INFO, dev, "model:%s serial:%s\n", model, serial);
+}
+
 void sas_probe_sata(struct asd_sas_port *port)
 {
 	struct domain_device *dev, *n;
@@ -649,6 +665,8 @@ void sas_probe_sata(struct asd_sas_port *port)
 		 */
 		if (!ata_dev_enabled(sas_to_ata_dev(dev)))
 			sas_fail_probe(dev, __func__, -ENODEV);
+		else
+			sas_ata_store_id(dev);
 	}
 
 }
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index cdbf8d8a28bf..6e56ebdc2148 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1994,6 +1994,61 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
 	return false;
 }
 
+/*
+ * we think the device is fluttering so just read the phy state and update
+ * some information of the device, but if some important things changed
+ * such as the sas address, or the linkrate, or the ata devices id and class,
+ * we have to unregister the device and re-probe it.
+ */
+static bool sas_process_flutter(struct domain_device *dev, struct ex_phy *phy,
+				int phy_id, u8 *sas_addr)
+{
+	struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id);
+	enum sas_linkrate linkrate = phy->linkrate;
+	char *action = "";
+
+	sas_ex_phy_discover(dev, phy_id);
+
+	if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
+		action = ", needs recovery";
+	pr_debug("ex %016llx phy%d broadcast flutter%s\n",
+		 SAS_ADDR(dev->sas_addr), phy_id, action);
+
+	if (linkrate != phy->linkrate) {
+		pr_debug("ex %016llx phy%d linkrate changed from %d to %d\n",
+			 SAS_ADDR(dev->sas_addr), phy_id,
+			 linkrate, phy->linkrate);
+		return false;
+	}
+
+	/* the phy attached address will be updated by sas_ex_phy_discover()
+	 * and sometimes become abnormal
+	 */
+	if (SAS_ADDR(phy->attached_sas_addr) != SAS_ADDR(sas_addr) ||
+	    SAS_ADDR(phy->attached_sas_addr) == 0) {
+		/* if attached_sas_addr become abnormal, we must set the
+		 * original address back so that the device can be unregistered
+		 */
+		memcpy(phy->attached_sas_addr, sas_addr, SAS_ADDR_SIZE);
+		pr_debug("phy address(%016llx) abnormal, origin:%016llx\n",
+			 SAS_ADDR(phy->attached_sas_addr),
+			 SAS_ADDR(sas_addr));
+		return false;
+	}
+
+	if (ata_dev) {
+		struct ata_device *adev = sas_to_ata_dev(ata_dev);
+		unsigned int class = ata_dev->sata_dev.class;
+		u16 *id = ata_dev->sata_dev.id;
+
+		/* to see if the disk is replaced with another one */
+		if (!ata_dev_same_device(adev, class, id))
+			return false;
+	}
+
+	return true;
+}
+
 static int sas_unregister(struct domain_device *dev, int phy_id, bool last,
 			      bool *retry)
 {
@@ -2028,16 +2083,8 @@ static int sas_unregister(struct domain_device *dev, int phy_id, bool last,
 		return res;
 	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
 		   dev_type_flutter(type, phy->attached_dev_type)) {
-		struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id);
-		char *action = "";
-
-		sas_ex_phy_discover(dev, phy_id);
-
-		if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
-			action = ", needs recovery";
-		pr_debug("ex %016llx phy 0x%x broadcast flutter%s\n",
-			 SAS_ADDR(dev->sas_addr), phy_id, action);
-		return res;
+		if (sas_process_flutter(dev, phy, phy_id, sas_addr))
+			return res;
 	}
 
 	/* we always have to delete the old device when we went here */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 68133842e6d7..b253cfcdd6ae 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1144,6 +1144,8 @@ extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
 extern int sata_scr_write_flush(struct ata_link *link, int reg, u32 val);
 extern bool ata_link_online(struct ata_link *link);
 extern bool ata_link_offline(struct ata_link *link);
+extern int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
+			     const u16 *new_id);
 #ifdef CONFIG_PM
 extern int ata_host_suspend(struct ata_host *host, pm_message_t mesg);
 extern void ata_host_resume(struct ata_host *host);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index deb75765e555..2da5d085b11a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -164,6 +164,7 @@ struct sata_device {
 	struct ata_host *ata_host;
 	struct smp_resp rps_resp ____cacheline_aligned; /* report_phy_sata_resp */
 	u8     fis[ATA_RESP_FIS_SIZE];
+	u16    id[ATA_ID_WORDS];
 };
 
 struct ssp_device {
-- 
2.14.4

[PATCH v2 6/7] scsi: libsas: reset the phy address if discover failed

[Jason Yan]

When we failed to discover the device, the phy address is still kept
in ex_phy. So when the next time we revalidate this phy the
address and device type is the same, it will be considered as flutter
and will not be discovered again. So the device will not be brought up.

Fix this by reset the phy address to the initial value. Then
in the next revalidation the device will be discovered agian.

Tested-by: Chen Liangfei <chenliangfei1@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: Xiaofei Tan <tanxiaofei@huawei.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_expander.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 6e56ebdc2148..e781941a7088 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1100,6 +1100,13 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 						 i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
 			}
 		}
+	} else {
+		/* if we failed to discover this device, we have to
+		 * reset the expander phy attached address so that we
+		 * will not treat the phy as flutter in the next
+		 * revalidation
+		 */
+		memset(ex_phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
 	}
 
 	return res;
-- 
2.14.4

Re: [PATCH v2 6/7] scsi: libsas: reset the phy address if discover failed

[John Garry]

On 30/01/2019 08:24, Jason Yan wrote:
> When we failed to discover the device, the phy address is still kept
> in ex_phy. So when the next time we revalidate this phy the
> address and device type is the same, it will be considered as flutter
> and will not be discovered again. So the device will not be brought up.
>
> Fix this by reset the phy address to the initial value. Then
> in the next revalidation the device will be discovered agian.

Why fail to discover the device? I wonder in which scenario you have 
seen this, such that it is worth rediscovery.

>
> Tested-by: Chen Liangfei <chenliangfei1@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: Xiaofei Tan <tanxiaofei@huawei.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_expander.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 6e56ebdc2148..e781941a7088 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1100,6 +1100,13 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
>  						 i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
>  			}
>  		}
> +	} else {
> +		/* if we failed to discover this device, we have to
> +		 * reset the expander phy attached address so that we
> +		 * will not treat the phy as flutter in the next
> +		 * revalidation
> +		 */
> +		memset(ex_phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
>  	}
>
>  	return res;
>

Re: [PATCH v2 6/7] scsi: libsas: reset the phy address if discover failed

[Jason Yan]

On 2019/1/31 1:36, John Garry wrote:
> On 30/01/2019 08:24, Jason Yan wrote:
>> When we failed to discover the device, the phy address is still kept
>> in ex_phy. So when the next time we revalidate this phy the
>> address and device type is the same, it will be considered as flutter
>> and will not be discovered again. So the device will not be brought up.
>>
>> Fix this by reset the phy address to the initial value. Then
>> in the next revalidation the device will be discovered agian.
>
> Why fail to discover the device? I wonder in which scenario you have
> seen this, such that it is worth rediscovery.
>

The test guys have seen this for several times, especially after LLDD 
changed the logic of lldd_dev_found and may return error now.


>>
>> Tested-by: Chen Liangfei <chenliangfei1@huawei.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: Xiaofei Tan <tanxiaofei@huawei.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c
>> b/drivers/scsi/libsas/sas_expander.c
>> index 6e56ebdc2148..e781941a7088 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1100,6 +1100,13 @@ static int sas_ex_discover_dev(struct
>> domain_device *dev, int phy_id)
>>                           i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
>>              }
>>          }
>> +    } else {
>> +        /* if we failed to discover this device, we have to
>> +         * reset the expander phy attached address so that we
>> +         * will not treat the phy as flutter in the next
>> +         * revalidation
>> +         */
>> +        memset(ex_phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
>>      }
>>
>>      return res;
>>
>
>
>
> .
>

Re: [PATCH v2 6/7] scsi: libsas: reset the phy address if discover failed

[John Garry]

On 31/01/2019 02:13, Jason Yan wrote:
>
>
> On 2019/1/31 1:36, John Garry wrote:
>> On 30/01/2019 08:24, Jason Yan wrote:
>>> When we failed to discover the device, the phy address is still kept

/s/phy/PHY/

>>> in ex_phy. So when the next time we revalidate this phy the
>>> address and device type is the same, it will be considered as flutter
>>> and will not be discovered again. So the device will not be brought up.
>>>
>>> Fix this by reset the phy address to the initial value. Then
>>> in the next revalidation the device will be discovered agian.
>>
>> Why fail to discover the device? I wonder in which scenario you have
>> seen this, such that it is worth rediscovery.
>>
>
> The test guys have seen this for several times, especially after LLDD
> changed the logic of lldd_dev_found and may return error now.

We're finding that a specific SATA disk stays in STP pending for some 
time and we fail the init in lldd dev found, like you say.

However, I think that we should ensure that this passes, as with your 
change I find we get some looping of dev found and lost until it finally 
recovers.

Regardless, I think that your change on its own is ok, so:
Reviewed-by: John Garry <john.garry@huawei.com>


>
>
>>>
>>> Tested-by: Chen Liangfei <chenliangfei1@huawei.com>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> CC: Xiaofei Tan <tanxiaofei@huawei.com>
>>> CC: John Garry <john.garry@huawei.com>
>>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>>> CC: Ewan Milne <emilne@redhat.com>
>>> CC: Christoph Hellwig <hch@lst.de>
>>> CC: Tomas Henzl <thenzl@redhat.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> CC: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/libsas/sas_expander.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index 6e56ebdc2148..e781941a7088 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1100,6 +1100,13 @@ static int sas_ex_discover_dev(struct
>>> domain_device *dev, int phy_id)
>>>                           i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
>>>              }
>>>          }
>>> +    } else {
>>> +        /* if we failed to discover this device, we have to
>>> +         * reset the expander phy attached address so that we
>>> +         * will not treat the phy as flutter in the next
>>> +         * revalidation
>>> +         */
>>> +        memset(ex_phy->attached_sas_addr, 0, SAS_ADDR_SIZE);
>>>      }
>>>
>>>      return res;
>>>
>>
>>
>>
>> .
>>
>
>
> .
>

[PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[Jason Yan]

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:
  -->phy10: 0 --> delete phy10 domain device
  -->phy11: 5000cca04eb043ad (no change)
revalidation done

revalidation 2:
  -->step 1, check phy10:
  -->phy10: 5000cca04eb043ad   --> add to wide port(port-0:0:11) (phy11
       address is still 5000cca04eb043ad now)

  -->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 <chenliangfei1@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: Xiaofei Tan <tanxiaofei@huawei.com>
CC: chenxiang <chenxiang66@hisilicon.com>
CC: John Garry <john.garry@huawei.com>
CC: Johannes Thumshirn <jthumshirn@suse.de>
CC: Ewan Milne <emilne@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: Tomas Henzl <thenzl@redhat.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Hannes Reinecke <hare@suse.com>
---
 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,
-- 
2.14.4

Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[John Garry]

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 <chenliangfei1@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> CC: Xiaofei Tan <tanxiaofei@huawei.com>
> CC: chenxiang <chenxiang66@hisilicon.com>
> CC: John Garry <john.garry@huawei.com>
> CC: Johannes Thumshirn <jthumshirn@suse.de>
> CC: Ewan Milne <emilne@redhat.com>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Tomas Henzl <thenzl@redhat.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Hannes Reinecke <hare@suse.com>
> ---
>  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,
>

Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[Jason Yan]

On 2019/1/31 1:53, John Garry wrote:
> 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?

'revalidation 1' means one entry in sas_discover_domain().

>
>>   -->phy10: 0 --> delete phy10 domain device
>>   -->phy11: 5000cca04eb043ad (no change)
>
> so is disk 11 still inserted at this stage?

Maybe, but that's what we read from the hardware.

>
>> revalidation done
>>
>> revalidation 2:
>
> is port-0:0:10 deleted now?
>

Yes. But we don't care about it.

>>   -->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.
>

This issue is because we always process the PHYs from 0 to max phy 
number. And please be aware of the real physcial address of the PHY and 
the address stored in the memory is not always the same.
Actually when you checking phy10, phy11 physcial address is not 
5000cca04eb043ad. But the address stored in domain device is still 
5000cca04eb043ad. We have not get a chance to to read it because we are 
processing phy10 now, right?

It's very easy to reproduce. I suggest you to do it yourself and look at 
the logs.

>>
>>   -->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 <chenliangfei1@huawei.com>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> CC: Xiaofei Tan <tanxiaofei@huawei.com>
>> CC: chenxiang <chenxiang66@hisilicon.com>
>> CC: John Garry <john.garry@huawei.com>
>> CC: Johannes Thumshirn <jthumshirn@suse.de>
>> CC: Ewan Milne <emilne@redhat.com>
>> CC: Christoph Hellwig <hch@lst.de>
>> CC: Tomas Henzl <thenzl@redhat.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Hannes Reinecke <hare@suse.com>
>> ---
>>  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,
>>
>
>
>
> .
>

Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[John Garry]

On 31/01/2019 02:55, Jason Yan wrote:
>
>
> On 2019/1/31 1:53, John Garry wrote:
>> 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?
>
> 'revalidation 1' means one entry in sas_discover_domain().
>
>>
>>>   -->phy10: 0 --> delete phy10 domain device
>>>   -->phy11: 5000cca04eb043ad (no change)
>>
>> so is disk 11 still inserted at this stage?
>
> Maybe, but that's what we read from the hardware.
>
>>
>>> revalidation done
>>>
>>> revalidation 2:
>>
>> is port-0:0:10 deleted now?
>>
>
> Yes. But we don't care about it.
>
>>>   -->step 1, check phy10:
>>>   -->phy10: 5000cca04eb043ad   --> add to wide port(port-0:0:11) (phy11
>>>        address is still 5000cca04eb043ad now)

We do not want this to happen and it seems to be the crux of the problem.

As an alternate to your solution, how about check if the PHY is an end 
device. If so, it should not form/join a wideport; that is, apart from 
dual-port disks, which I am not sure about - I think each port still has 
a unique WWN, so should be ok.

>>
>> 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.
>>
>
> This issue is because we always process the PHYs from 0 to max phy
> number. And please be aware of the real physcial address of the PHY and
> the address stored in the memory is not always the same.
> Actually when you checking phy10, phy11 physcial address is not
> 5000cca04eb043ad. But the address stored in domain device is still
> 5000cca04eb043ad. We have not get a chance to to read it because we are
> processing phy10 now, right?
>

I see.

> It's very easy to reproduce. I suggest you to do it yourself and look at
> the logs.
>

I can't physically access the backpane, and this is not the sort of 
thing which is easy to fake by hacking the driver.

And the log which you provided internally does not have much - if any - 
libsas logs to help me understand it.

>>>
>>>   -->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.

Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[Jason Yan]

On 2019/2/1 0:34, John Garry wrote:
> On 31/01/2019 02:55, Jason Yan wrote:
>>
>>
>> On 2019/1/31 1:53, John Garry wrote:
>>> 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?
>>
>> 'revalidation 1' means one entry in sas_discover_domain().
>>
>>>
>>>>   -->phy10: 0 --> delete phy10 domain device
>>>>   -->phy11: 5000cca04eb043ad (no change)
>>>
>>> so is disk 11 still inserted at this stage?
>>
>> Maybe, but that's what we read from the hardware.
>>
>>>
>>>> revalidation done
>>>>
>>>> revalidation 2:
>>>
>>> is port-0:0:10 deleted now?
>>>
>>
>> Yes. But we don't care about it.
>>
>>>>   -->step 1, check phy10:
>>>>   -->phy10: 5000cca04eb043ad   --> add to wide port(port-0:0:11) (phy11
>>>>        address is still 5000cca04eb043ad now)
>
> We do not want this to happen and it seems to be the crux of the problem.
>
> As an alternate to your solution, how about check if the PHY is an end
> device. If so, it should not form/join a wideport; that is, apart from
> dual-port disks, which I am not sure about - I think each port still has
> a unique WWN, so should be ok.
>

If the PHY do not join a wideport, then it have to form a wideport of 
it's own. I'm not sure if we can have two ports with the same address 
and do not break anything?

>>>
>>> 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.
>>>
>>
>> This issue is because we always process the PHYs from 0 to max phy
>> number. And please be aware of the real physcial address of the PHY and
>> the address stored in the memory is not always the same.
>> Actually when you checking phy10, phy11 physcial address is not
>> 5000cca04eb043ad. But the address stored in domain device is still
>> 5000cca04eb043ad. We have not get a chance to to read it because we are
>> processing phy10 now, right?
>>
>
> I see.
>
>> It's very easy to reproduce. I suggest you to do it yourself and look at
>> the logs.
>>
>
> I can't physically access the backpane, and this is not the sort of
> thing which is easy to fake by hacking the driver.
>
> And the log which you provided internally does not have much - if any -
> libsas logs to help me understand it.
>
>>>>
>>>>   -->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.
>
>
> .
>

Re: [PATCH v2 7/7] scsi: libsas: fix issue of swapping two sas disks

[John Garry]

On 01/02/2019 02:04, Jason Yan wrote:
>
>
> On 2019/2/1 0:34, John Garry wrote:
>> On 31/01/2019 02:55, Jason Yan wrote:
>>>
>>>
>>> On 2019/1/31 1:53, John Garry wrote:
>>>> 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?
>>>
>>> 'revalidation 1' means one entry in sas_discover_domain().
>>>
>>>>
>>>>>   -->phy10: 0 --> delete phy10 domain device
>>>>>   -->phy11: 5000cca04eb043ad (no change)
>>>>
>>>> so is disk 11 still inserted at this stage?
>>>
>>> Maybe, but that's what we read from the hardware.
>>>
>>>>
>>>>> revalidation done
>>>>>
>>>>> revalidation 2:
>>>>
>>>> is port-0:0:10 deleted now?
>>>>
>>>
>>> Yes. But we don't care about it.
>>>
>>>>>   -->step 1, check phy10:
>>>>>   -->phy10: 5000cca04eb043ad   --> add to wide port(port-0:0:11)
>>>>> (phy11
>>>>>        address is still 5000cca04eb043ad now)
>>
>> We do not want this to happen and it seems to be the crux of the problem.
>>
>> As an alternate to your solution, how about check if the PHY is an end
>> device. If so, it should not form/join a wideport; that is, apart from
>> dual-port disks, which I am not sure about - I think each port still has
>> a unique WWN, so should be ok.
>>
>
> If the PHY do not join a wideport, then it have to form a wideport of
> it's own. I'm not sure if we can have two ports with the same address
> and do not break anything?

I'm not sure, but port-0:0:11 should be deleted from step 2, just after 
this step, below.

Thanks,
John

>
>>>>
>>>> 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.
>>>>
>>>
>>> This issue is because we always process the PHYs from 0 to max phy
>>> number. And please be aware of the real physcial address of the PHY and
>>> the address stored in the memory is not always the same.
>>> Actually when you checking phy10, phy11 physcial address is not
>>> 5000cca04eb043ad. But the address stored in domain device is still
>>> 5000cca04eb043ad. We have not get a chance to to read it because we are
>>> processing phy10 now, right?
>>>
>>
>> I see.
>>
>>> It's very easy to reproduce. I suggest you to do it yourself and look at
>>> the logs.
>>>
>>
>> I can't physically access the backpane, and this is not the sort of
>> thing which is easy to fake by hacking the driver.
>>
>> And the log which you provided internally does not have much - if any -
>> libsas logs to help me understand it.
>>
>>>>>
>>>>>   -->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.
>>
>>
>> .
>>
>
>
> .
>