Re: enclosure: fix sysfs symlinks creation when using multipath

[Douglas Miller]

On 06/20/2017 06:38 AM, Maurizio Lombardi wrote:


Dne 16.6.2017 v 18:08 Douglas Miller napsal(a):

Just to respond to James' question on the cause. What I observed was a race 
condition between udevd (ses_init()) and a worker thread (do_scan_async()),
where the worker thread is creating the directories that are the target of the 
symlinks being created by udevd.
Something was happening when udevd caught up with the worker thread (so the 
target directory did not exist) and it seemed the worker thread either got 
preempted or
else just could not stay ahead of udevd. This means that udevd started failing 
to create symlinks even though the worker thread eventually got them all 
created.
I did observe what appeared to be preemption, as the creation of directories 
stopped until udevd finished failing all the (rest of the) symlinks.
Although there may have been other explanations for what I saw.

I am able to pass my testing with this patch. I don't see an official submit of 
this patch, but will respond to it when I see one.

Thanks Douglas for testing it, I will resubmit the patch if no one has any 
objections.

Maurizio.

I did not see any additional comments, and no objections. Is it time to 
submit the new patch?


Thanks,
Doug

Re: enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Dne 16.6.2017 v 18:08 Douglas Miller napsal(a):
> Just to respond to James' question on the cause. What I observed was a race 
> condition between udevd (ses_init()) and a worker thread (do_scan_async()),
> where the worker thread is creating the directories that are the target of 
> the symlinks being created by udevd.
> Something was happening when udevd caught up with the worker thread (so the 
> target directory did not exist) and it seemed the worker thread either got 
> preempted or
> else just could not stay ahead of udevd. This means that udevd started 
> failing to create symlinks even though the worker thread eventually got them 
> all created.
> I did observe what appeared to be preemption, as the creation of directories 
> stopped until udevd finished failing all the (rest of the) symlinks.
> Although there may have been other explanations for what I saw.
> 

> I am able to pass my testing with this patch. I don't see an official submit 
> of this patch, but will respond to it when I see one.

Thanks Douglas for testing it, I will resubmit the patch if no one has any 
objections.

Maurizio.

Re: enclosure: fix sysfs symlinks creation when using multipath

[Douglas Miller]

On 06/16/2017 10:41 AM, Douglas Miller wrote:

On 03/16/2017 01:49 PM, James Bottomley wrote:

On Wed, 2017-03-15 at 19:39 -0400, Martin K. Petersen wrote:

Maurizio Lombardi  writes:


With multipath, it may happen that the same device is passed to
enclosure_add_device() multiple times and that the
enclosure_add_links() function fails to create the symlinks because
the device's sysfs directory entry is still NULL.  In this case,
the
links will never be created because all the subsequent calls to
enclosure_add_device() will immediately fail with EEXIST.

James?

Well I don't think the patch is the correct way to do this.  The
problem is that if we encounter an error creating the links, we
shouldn't add the device to the enclosure.  There's no need of a
links_created variable (see below).

However, more interesting is why the link creation failed in the first
place.  The device clearly seems to exist because it was added to sysfs
at time index 19.2 and the enclosure didn't try to use it until 60.0.
  Can you debug this a bit more, please?  I can't see anything specific
to multipath in the trace, so whatever this is looks like it could
happen in the single path case as well.

James

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..ae89082 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device 
*edev, int component,

   struct device *dev)
  {
  struct enclosure_component *cdev;
+int err;
if (!edev || component >= edev->components)
  return -EINVAL;
@@ -384,12 +385,15 @@ int enclosure_add_device(struct 
enclosure_device *edev, int component,

  if (cdev->dev == dev)
  return -EEXIST;
  -if (cdev->dev)
+if (cdev->dev) {
  enclosure_remove_links(cdev);
-
-put_device(cdev->dev);
-cdev->dev = get_device(dev);
-return enclosure_add_links(cdev);
+put_device(cdev->dev);
+cdev->dev = NULL;
+}
+err = enclosure_add_links(cdev);
+if (!err)
+cdev->dev = get_device(dev);
+return err;
  }
  EXPORT_SYMBOL_GPL(enclosure_add_device);
After stumbling across the NULL pointer panic, I was able to use 
Maurizio's second patch below:


diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..6ac07ea 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device 
*edev, int component,

 struct device *dev)
 {
struct enclosure_component *cdev;
+   int err;

if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,17 @@ int enclosure_add_device(struct enclosure_device 
*edev, int component,

if (cdev->dev == dev)
return -EEXIST;

-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
+   put_device(cdev->dev);
+   }
cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   err = enclosure_add_links(cdev);
+   if (err) {
+   cdev->dev = NULL;
+   put_device(cdev->dev);
+   }
+   return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);


I am able to pass my testing with this patch. I don't see an official 
submit of this patch, but will respond to it when I see one. Again, I 
am seeing the problem even without multipath.


Just to respond to James' question on the cause. What I observed was a 
race condition between udevd (ses_init()) and a worker thread 
(do_scan_async()), where the worker thread is creating the directories 
that are the target of the symlinks being created by udevd. Something 
was happening when udevd caught up with the worker thread (so the target 
directory did not exist) and it seemed the worker thread either got 
preempted or else just could not stay ahead of udevd. This means that 
udevd started failing to create symlinks even though the worker thread 
eventually got them all created. I did observe what appeared to be 
preemption, as the creation of directories stopped until udevd finished 
failing all the (rest of the) symlinks. Although there may have been 
other explanations for what I saw.

Re: enclosure: fix sysfs symlinks creation when using multipath

[Douglas Miller]

On 03/16/2017 01:49 PM, James Bottomley wrote:

On Wed, 2017-03-15 at 19:39 -0400, Martin K. Petersen wrote:

Maurizio Lombardi  writes:


With multipath, it may happen that the same device is passed to
enclosure_add_device() multiple times and that the
enclosure_add_links() function fails to create the symlinks because
the device's sysfs directory entry is still NULL.  In this case,
the
links will never be created because all the subsequent calls to
enclosure_add_device() will immediately fail with EEXIST.

James?

Well I don't think the patch is the correct way to do this.  The
problem is that if we encounter an error creating the links, we
shouldn't add the device to the enclosure.  There's no need of a
links_created variable (see below).

However, more interesting is why the link creation failed in the first
place.  The device clearly seems to exist because it was added to sysfs
at time index 19.2 and the enclosure didn't try to use it until 60.0.
  Can you debug this a bit more, please?  I can't see anything specific
to multipath in the trace, so whatever this is looks like it could
happen in the single path case as well.

James

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..ae89082 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int 
component,
 struct device *dev)
  {
struct enclosure_component *cdev;
+   int err;
  
  	if (!edev || component >= edev->components)

return -EINVAL;
@@ -384,12 +385,15 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
if (cdev->dev == dev)
return -EEXIST;
  
-	if (cdev->dev)

+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
-   cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   put_device(cdev->dev);
+   cdev->dev = NULL;
+   }
+   err = enclosure_add_links(cdev);
+   if (!err)
+   cdev->dev = get_device(dev);
+   return err;
  }
  EXPORT_SYMBOL_GPL(enclosure_add_device);
  
After stumbling across the NULL pointer panic, I was able to use 
Maurizio's second patch below:


diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..6ac07ea 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device 
*edev, int component,

 struct device *dev)
 {
struct enclosure_component *cdev;
+   int err;

if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,17 @@ int enclosure_add_device(struct enclosure_device 
*edev, int component,

if (cdev->dev == dev)
return -EEXIST;

-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
+   put_device(cdev->dev);
+   }
cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   err = enclosure_add_links(cdev);
+   if (err) {
+   cdev->dev = NULL;
+   put_device(cdev->dev);
+   }
+   return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);


I am able to pass my testing with this patch. I don't see an official 
submit of this patch, but will respond to it when I see one. Again, I am 
seeing the problem even without multipath.

Re: [RFC] enclosure: fix sysfs symlinks creation when using multipath

[Douglas Miller]

On 06/16/2017 07:48 AM, Maurizio Lombardi wrote:


Dne 16.6.2017 v 14:40 Douglas Miller napsal(a):

I'd like to add that we are seeing this problem with singlepath installations 
and need to get this fixed upstream as soon as possible. RHEL new product 
contains this fix and is working for us, but we need to be able to offer other 
distros as well. I am currently running this patch on a custom-built Ubuntu 
16.04.2 kernel and it is fixing the problem there.

What needs to be done to get this patch accepted?


Note that James proposed a different patch to fix this bug.

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..ae89082 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int 
component,
 struct device *dev)
  {
struct enclosure_component *cdev;
+   int err;

if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,15 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
if (cdev->dev == dev)
return -EEXIST;

-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
-   cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   put_device(cdev->dev);
+   cdev->dev = NULL;
+   }
+   err = enclosure_add_links(cdev);
+   if (!err)
+   cdev->dev = get_device(dev);
+   return err;
  }
  EXPORT_SYMBOL_GPL(enclosure_add_device);



I will test this out. Thanks.

Re: [RFC] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Dne 16.6.2017 v 14:40 Douglas Miller napsal(a):
> 
> I'd like to add that we are seeing this problem with singlepath installations 
> and need to get this fixed upstream as soon as possible. RHEL new product 
> contains this fix and is working for us, but we need to be able to offer 
> other distros as well. I am currently running this patch on a custom-built 
> Ubuntu 16.04.2 kernel and it is fixing the problem there.
> 
> What needs to be done to get this patch accepted?
> 

Note that James proposed a different patch to fix this bug.

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..ae89082 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int 
component,
 struct device *dev)
 {
struct enclosure_component *cdev;
+   int err;
 
if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,15 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
if (cdev->dev == dev)
return -EEXIST;
 
-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
-   cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   put_device(cdev->dev);
+   cdev->dev = NULL;
+   }
+   err = enclosure_add_links(cdev);
+   if (!err)
+   cdev->dev = get_device(dev);
+   return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);
 

Re: [RFC] enclosure: fix sysfs symlinks creation when using multipath

[Douglas Miller]

On 02/07/2017 08:08 AM, Maurizio Lombardi wrote:

With multipath, it may happen that the same device is passed
to enclosure_add_device() multiple times and that the enclosure_add_links()
function fails to create the symlinks because the device's sysfs
directory entry is still NULL.
In this case, the links will never be created because all the subsequent
calls to enclosure_add_device() will immediately fail with EEXIST.

This patch modifies the code so the driver will detect this condition
and will retry to create the symlinks when enclosure_add_device() is called.

Signed-off-by: Maurizio Lombardi 
---
  drivers/misc/enclosure.c  | 16 ++--
  include/linux/enclosure.h |  1 +
  2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..a856c98 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
 struct device *dev)
  {
struct enclosure_component *cdev;
+   int error;
  
  	if (!edev || component >= edev->components)

return -EINVAL;
  
  	cdev = >component[component];
  
-	if (cdev->dev == dev)

+   if (cdev->dev == dev) {
+   if (!cdev->links_created) {
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   }
return -EEXIST;
+   }
  
  	if (cdev->dev)

enclosure_remove_links(cdev);
  
  	put_device(cdev->dev);

cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   else
+   cdev->links_created = 0;
+   return error;
  }
  EXPORT_SYMBOL_GPL(enclosure_add_device);
  
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h

index a4cf57c..c3bdc4c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -97,6 +97,7 @@ struct enclosure_component {
struct device cdev;
struct device *dev;
enum enclosure_component_type type;
+   int links_created;
int number;
int fault;
int active;


Tested-by: Douglas Miller 

I'd like to add that we are seeing this problem with singlepath 
installations and need to get this fixed upstream as soon as possible. 
RHEL new product contains this fix and is working for us, but we need to 
be able to offer other distros as well. I am currently running this 
patch on a custom-built Ubuntu 16.04.2 kernel and it is fixing the 
problem there.


What needs to be done to get this patch accepted?

Thanks,
Doug

Re: [PATCH] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Dne 21.3.2017 v 10:58 Maurizio Lombardi napsal(a):

> I will ask our customer to test your patch,
> there is only a small problem: you can't set cdev->dev = NULL
> and then call enclosure_add_links(cdev) because you will end up dereferencing
> a NULL pointer.
> I suggest a slightly different patch:
> 
> @@ -375,6 +379,7 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>struct device *dev)
>  {
>   struct enclosure_component *cdev;
> + int err;
>  
>   if (!edev || component >= edev->components)
>   return -EINVAL;
> @@ -384,12 +389,18 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>   if (cdev->dev == dev)
>   return -EEXIST;
>  
> - if (cdev->dev)
> + if (cdev->dev) {
>   enclosure_remove_links(cdev);
> -
> - put_device(cdev->dev);
> + put_device(cdev->dev);
> + }
>   cdev->dev = get_device(dev);
> - return enclosure_add_links(cdev);
> +
> + err = enclosure_add_links(cdev);
> + if (err) {
> + put_device(cdev->dev);
> + cdev->dev = NULL;
> + }
> + return err;
>  }
>  EXPORT_SYMBOL_GPL(enclosure_add_device);


Our customer confirms that this patch solves his issues with enclosures' 
symlinks.

Re: [PATCH] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Dne 16.3.2017 v 19:49 James Bottomley napsal(a): 
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 65fed71..ae89082 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>struct device *dev)
>  {
>   struct enclosure_component *cdev;
> + int err;
>  
>   if (!edev || component >= edev->components)
>   return -EINVAL;
> @@ -384,12 +385,15 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>   if (cdev->dev == dev)
>   return -EEXIST;
>  
> - if (cdev->dev)
> + if (cdev->dev) {
>   enclosure_remove_links(cdev);
> -
> - put_device(cdev->dev);
> - cdev->dev = get_device(dev);
> - return enclosure_add_links(cdev);
> + put_device(cdev->dev);
> + cdev->dev = NULL;
> + }
> + err = enclosure_add_links(cdev);
> + if (!err)
> + cdev->dev = get_device(dev);
> + return err;
>  }
>  EXPORT_SYMBOL_GPL(enclosure_add_device);
>  
>


I will ask our customer to test your patch,
there is only a small problem: you can't set cdev->dev = NULL
and then call enclosure_add_links(cdev) because you will end up dereferencing
a NULL pointer.
I suggest a slightly different patch:

@@ -375,6 +379,7 @@ int enclosure_add_device(struct enclosure_device *edev, int 
component,
 struct device *dev)
 {
struct enclosure_component *cdev;
+   int err;
 
if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +389,18 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
if (cdev->dev == dev)
return -EEXIST;
 
-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
+   put_device(cdev->dev);
+   }
cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+
+   err = enclosure_add_links(cdev);
+   if (err) {
+   put_device(cdev->dev);
+   cdev->dev = NULL;
+   }
+   return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);
 
Thanks,
Maurizio.
 

Re: [PATCH] enclosure: fix sysfs symlinks creation when using multipath

[James Bottomley]

On Wed, 2017-03-15 at 19:39 -0400, Martin K. Petersen wrote:
> Maurizio Lombardi  writes:
> 
> > With multipath, it may happen that the same device is passed to
> > enclosure_add_device() multiple times and that the
> > enclosure_add_links() function fails to create the symlinks because
> > the device's sysfs directory entry is still NULL.  In this case,
> > the
> > links will never be created because all the subsequent calls to
> > enclosure_add_device() will immediately fail with EEXIST.
> 
> James?

Well I don't think the patch is the correct way to do this.  The
problem is that if we encounter an error creating the links, we
shouldn't add the device to the enclosure.  There's no need of a
links_created variable (see below).

However, more interesting is why the link creation failed in the first
place.  The device clearly seems to exist because it was added to sysfs
at time index 19.2 and the enclosure didn't try to use it until 60.0. 
 Can you debug this a bit more, please?  I can't see anything specific
to multipath in the trace, so whatever this is looks like it could
happen in the single path case as well.

James

---

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..ae89082 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,6 +375,7 @@ int enclosure_add_device(struct enclosure_device *edev, int 
component,
 struct device *dev)
 {
struct enclosure_component *cdev;
+   int err;
 
if (!edev || component >= edev->components)
return -EINVAL;
@@ -384,12 +385,15 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
if (cdev->dev == dev)
return -EEXIST;
 
-   if (cdev->dev)
+   if (cdev->dev) {
enclosure_remove_links(cdev);
-
-   put_device(cdev->dev);
-   cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   put_device(cdev->dev);
+   cdev->dev = NULL;
+   }
+   err = enclosure_add_links(cdev);
+   if (!err)
+   cdev->dev = get_device(dev);
+   return err;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);
 

Re: [PATCH] enclosure: fix sysfs symlinks creation when using multipath

[Martin K. Petersen]

Maurizio Lombardi  writes:

> With multipath, it may happen that the same device is passed to
> enclosure_add_device() multiple times and that the
> enclosure_add_links() function fails to create the symlinks because
> the device's sysfs directory entry is still NULL.  In this case, the
> links will never be created because all the subsequent calls to
> enclosure_add_device() will immediately fail with EEXIST.

James?

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

With multipath, it may happen that the same device is passed
to enclosure_add_device() multiple times and that the enclosure_add_links()
function fails to create the symlinks because the device's sysfs
directory entry is still NULL.
In this case, the links will never be created because all the subsequent
calls to enclosure_add_device() will immediately fail with EEXIST.

This is an example of what happens:

[   19.251902] scsi 0:0:27:0: Direct-Access SEAGATE  ST8000NM0075 E002 
PQ: 0 ANSI: 6
[   19.261874] scsi 0:0:27:0: SSP: handle(0x0027), 
sas_addr(0x5000c50085826dd6), phy(34), device_name(0x5000c50085826dd4)
[   19.274656] scsi 0:0:27:0: SSP: enclosure_logical_id(0x500093d001be4000), 
slot(57)
[   19.283944] scsi 0:0:27:0: SSP: enclosure level(0x), connector name( 
)
[   19.292933] scsi 0:0:27:0: qdepth(254), tagged(1), simple(0), ordered(0), 
scsi_l
[...]
[   60.066524] enclosure_add_device(0:0:27:0) called, enclosure_add_links() 
failed with error -2
[...]
[   75.119146] sd 0:0:27:0: Attached scsi generic sg27 type 0
[   75.126722] sd 0:0:27:0: [sdaa] 15628053168 512-byte logical blocks: (8.00 
TB/7.27 TiB)
[   75.126723] sd 0:0:27:0: [sdaa] 4096-byte physical blocks
[   75.129059] sd 0:0:27:0: [sdaa] Write Protect is off
[   75.129061] sd 0:0:27:0: [sdaa] Mode Sense: db 00 10 08
[   75.130417] sd 0:0:27:0: [sdaa] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[   75.158088] sd 0:0:27:0: [sdaa] Attached SCSI disk
[...]
[   75.192722] enclosure_add_device(0:0:27:0) called, device already exists

This patch modifies the code so the driver will detect this condition
and will retry to create the symlinks when enclosure_add_device() is called.

Signed-off-by: Maurizio Lombardi 
---
 drivers/misc/enclosure.c  | 16 ++--
 include/linux/enclosure.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..a856c98 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
 struct device *dev)
 {
struct enclosure_component *cdev;
+   int error;
 
if (!edev || component >= edev->components)
return -EINVAL;
 
cdev = >component[component];
 
-   if (cdev->dev == dev)
+   if (cdev->dev == dev) {
+   if (!cdev->links_created) {
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   }
return -EEXIST;
+   }
 
if (cdev->dev)
enclosure_remove_links(cdev);
 
put_device(cdev->dev);
cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   else
+   cdev->links_created = 0;
+   return error;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a4cf57c..c3bdc4c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -97,6 +97,7 @@ struct enclosure_component {
struct device cdev;
struct device *dev;
enum enclosure_component_type type;
+   int links_created;
int number;
int fault;
int active;
-- 
Maurizio Lombardi

Re: [PATCH RFC] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

Hi James,

have you noticed this patch?


Dne 7.2.2017 v 15:08 Maurizio Lombardi napsal(a):
> With multipath, it may happen that the same device is passed
> to enclosure_add_device() multiple times and that the enclosure_add_links()
> function fails to create the symlinks because the device's sysfs
> directory entry is still NULL.
> In this case, the links will never be created because all the subsequent
> calls to enclosure_add_device() will immediately fail with EEXIST.
> 
> This patch modifies the code so the driver will detect this condition
> and will retry to create the symlinks when enclosure_add_device() is called.
> 
> Signed-off-by: Maurizio Lombardi 
> ---
>  drivers/misc/enclosure.c  | 16 ++--
>  include/linux/enclosure.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 65fed71..a856c98 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev, 
> int component,
>struct device *dev)
>  {
>   struct enclosure_component *cdev;
> + int error;
>  
>   if (!edev || component >= edev->components)
>   return -EINVAL;
>  
>   cdev = >component[component];
>  
> - if (cdev->dev == dev)
> + if (cdev->dev == dev) {
> + if (!cdev->links_created) {
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + }
>   return -EEXIST;
> + }
>  
>   if (cdev->dev)
>   enclosure_remove_links(cdev);
>  
>   put_device(cdev->dev);
>   cdev->dev = get_device(dev);
> - return enclosure_add_links(cdev);
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + else
> + cdev->links_created = 0;
> + return error;
>  }
>  EXPORT_SYMBOL_GPL(enclosure_add_device);
>  
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index a4cf57c..c3bdc4c 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -97,6 +97,7 @@ struct enclosure_component {
>   struct device cdev;
>   struct device *dev;
>   enum enclosure_component_type type;
> + int links_created;
>   int number;
>   int fault;
>   int active;
> 

[PATCH RFC] enclosure: fix sysfs symlinks creation when using multipath

[Maurizio Lombardi]

With multipath, it may happen that the same device is passed
to enclosure_add_device() multiple times and that the enclosure_add_links()
function fails to create the symlinks because the device's sysfs
directory entry is still NULL.
In this case, the links will never be created because all the subsequent
calls to enclosure_add_device() will immediately fail with EEXIST.

This patch modifies the code so the driver will detect this condition
and will retry to create the symlinks when enclosure_add_device() is called.

Signed-off-by: Maurizio Lombardi 
---
 drivers/misc/enclosure.c  | 16 ++--
 include/linux/enclosure.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..a856c98 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev, 
int component,
 struct device *dev)
 {
struct enclosure_component *cdev;
+   int error;
 
if (!edev || component >= edev->components)
return -EINVAL;
 
cdev = >component[component];
 
-   if (cdev->dev == dev)
+   if (cdev->dev == dev) {
+   if (!cdev->links_created) {
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   }
return -EEXIST;
+   }
 
if (cdev->dev)
enclosure_remove_links(cdev);
 
put_device(cdev->dev);
cdev->dev = get_device(dev);
-   return enclosure_add_links(cdev);
+   error = enclosure_add_links(cdev);
+   if (!error)
+   cdev->links_created = 1;
+   else
+   cdev->links_created = 0;
+   return error;
 }
 EXPORT_SYMBOL_GPL(enclosure_add_device);
 
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a4cf57c..c3bdc4c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -97,6 +97,7 @@ struct enclosure_component {
struct device cdev;
struct device *dev;
enum enclosure_component_type type;
+   int links_created;
int number;
int fault;
int active;
-- 
Maurizio Lombardi