[PATCH 0/3] fcoe: Fixup device_del() inversion

[PATCH 0/3] fcoe: Fixup device_del() inversion

[Hannes Reinecke]

Hi all,

when shutting down an FCoE target system one is greeted with a flood
of kernel warnings about

sysfs group ffffffff81eff0e0 not found for kobject 'XXX'

Reason here is that the fcoe driver tries to remove devices from
a workqueue, but the NETDEV_UNREGISTER notification expects any
calls to be synchronous as it will tear down the device once
the notifications return.
With this patch device removals are done synchronously, so the
correct ordering is respected and the warnings are gone.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()
  fcoe: separate out fcoe_vport_remove()
  fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

 drivers/scsi/fcoe/fcoe.c | 81 ++++++++++++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 30 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()

[Hannes Reinecke]

No functional change.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 85f9a3e..135bdcf 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -501,11 +501,6 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	struct net_device *netdev = fcoe->netdev;
 	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 
-	rtnl_lock();
-	if (!fcoe->removed)
-		fcoe_interface_remove(fcoe);
-	rtnl_unlock();
-
 	/* Release the self-reference taken during fcoe_interface_create() */
 	/* tear-down the FCoE controller */
 	fcoe_ctlr_destroy(fip);
@@ -2140,6 +2135,11 @@ static void fcoe_destroy_work(struct work_struct *work)
 	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 
 	fcoe_if_destroy(port->lport);
+
+	rtnl_lock();
+	if (!fcoe->removed)
+		fcoe_interface_remove(fcoe);
+	rtnl_unlock();
 	fcoe_interface_cleanup(fcoe);
 
 	mutex_unlock(&fcoe_config_mutex);
@@ -2254,6 +2254,8 @@ static int _fcoe_create(struct net_device *netdev, enum fip_mode fip_mode,
 		printk(KERN_ERR "fcoe: Failed to create interface (%s)\n",
 		       netdev->name);
 		rc = -EIO;
+		if (!fcoe->removed)
+			fcoe_interface_remove(fcoe);
 		rtnl_unlock();
 		fcoe_interface_cleanup(fcoe);
 		mutex_unlock(&fcoe_config_mutex);
-- 
1.8.5.6

Re: [PATCH 1/3] fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()

[Lee Duncan]

On 08/31/2017 02:19 AM, Hannes Reinecke wrote:
> No functional change.

Nit: Then why do it? Perhaps the description can say why such a change
is being done?

> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/fcoe/fcoe.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 85f9a3e..135bdcf 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -501,11 +501,6 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
>  	struct net_device *netdev = fcoe->netdev;
>  	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
>  
> -	rtnl_lock();
> -	if (!fcoe->removed)
> -		fcoe_interface_remove(fcoe);
> -	rtnl_unlock();
> -
>  	/* Release the self-reference taken during fcoe_interface_create() */
>  	/* tear-down the FCoE controller */
>  	fcoe_ctlr_destroy(fip);
> @@ -2140,6 +2135,11 @@ static void fcoe_destroy_work(struct work_struct *work)
>  	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
>  
>  	fcoe_if_destroy(port->lport);
> +
> +	rtnl_lock();
> +	if (!fcoe->removed)
> +		fcoe_interface_remove(fcoe);
> +	rtnl_unlock();
>  	fcoe_interface_cleanup(fcoe);
>  
>  	mutex_unlock(&fcoe_config_mutex);
> @@ -2254,6 +2254,8 @@ static int _fcoe_create(struct net_device *netdev, enum fip_mode fip_mode,
>  		printk(KERN_ERR "fcoe: Failed to create interface (%s)\n",
>  		       netdev->name);
>  		rc = -EIO;
> +		if (!fcoe->removed)
> +			fcoe_interface_remove(fcoe);
>  		rtnl_unlock();
>  		fcoe_interface_cleanup(fcoe);
>  		mutex_unlock(&fcoe_config_mutex);
> 

Looks good to me (other than the comment nit).

Reviewed-by: Lee Duncan <lduncan@suse.com>
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 1/3] fcoe: move fcoe_interface_remove() out of fcoe_interface_cleanup()

[Johannes Thumshirn]

With an updates patch description (see Lee's mail) 

Acked-by: Johannes Thumshirn <jth@kernel.org>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/3] fcoe: separate out fcoe_vport_remove()

[Hannes Reinecke]

Separate out fcoe_vport_remove() from fcoe_destroy_work().
No functional change.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe.c | 55 +++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 135bdcf..617348f 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -155,7 +155,7 @@ static struct fc_seq *fcoe_elsct_send(struct fc_lport *,
 static void fcoe_set_vport_symbolic_name(struct fc_vport *);
 static void fcoe_set_port_id(struct fc_lport *, u32, struct fc_frame *);
 static void fcoe_fcf_get_vlan_id(struct fcoe_fcf_device *);
-
+static void fcoe_vport_remove(struct fc_lport *);
 
 static struct fcoe_sysfs_function_template fcoe_sysfs_templ = {
 	.set_fcoe_ctlr_mode = fcoe_ctlr_mode,
@@ -2103,30 +2103,10 @@ static void fcoe_destroy_work(struct work_struct *work)
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_port *port;
 	struct fcoe_interface *fcoe;
-	struct Scsi_Host *shost;
-	struct fc_host_attrs *fc_host;
-	unsigned long flags;
-	struct fc_vport *vport;
-	struct fc_vport *next_vport;
 
 	port = container_of(work, struct fcoe_port, destroy_work);
-	shost = port->lport->host;
-	fc_host = shost_to_fc_host(shost);
-
-	/* Loop through all the vports and mark them for deletion */
-	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(vport, next_vport, &fc_host->vports, peers) {
-		if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING)) {
-			continue;
-		} else {
-			vport->flags |= FC_VPORT_DELETING;
-			queue_work(fc_host_work_q(shost),
-				   &vport->vport_delete_work);
-		}
-	}
-	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	flush_workqueue(fc_host_work_q(shost));
+	fcoe_vport_remove(port->lport);
 
 	mutex_lock(&fcoe_config_mutex);
 
@@ -2747,6 +2727,37 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
 }
 
 /**
+ * fcoe_vport_remove() - remove attached vports
+ * @lport: lport for which the vports should be removed
+ */
+static void fcoe_vport_remove(struct fc_lport *lport)
+{
+	struct Scsi_Host *shost;
+	struct fc_host_attrs *fc_host;
+	unsigned long flags;
+	struct fc_vport *vport;
+	struct fc_vport *next_vport;
+
+	shost = lport->host;
+	fc_host = shost_to_fc_host(shost);
+
+	/* Loop through all the vports and mark them for deletion */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(vport, next_vport, &fc_host->vports, peers) {
+		if (vport->flags & (FC_VPORT_DEL | FC_VPORT_CREATING)) {
+			continue;
+		} else {
+			vport->flags |= FC_VPORT_DELETING;
+			queue_work(fc_host_work_q(shost),
+				   &vport->vport_delete_work);
+		}
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	flush_workqueue(fc_host_work_q(shost));
+}
+
+/**
  * fcoe_vport_disable() - change vport state
  * @vport: vport to bring online/offline
  * @disable: should the vport be disabled?
-- 
1.8.5.6

Re: [PATCH 2/3] fcoe: separate out fcoe_vport_remove()

[Lee Duncan]

On 08/31/2017 02:19 AM, Hannes Reinecke wrote:
> Separate out fcoe_vport_remove() from fcoe_destroy_work().
> No functional change.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/fcoe/fcoe.c | 55 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> ...
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 2/3] fcoe: separate out fcoe_vport_remove()

[Johannes Thumshirn]

Acked-by: Johannes Thumshirn <jth@kernel.org>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/3] fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

[Hannes Reinecke]

When a NETDEV_UNREGISTER notification is received the network
device is _deleted_ after the callback returns.
So we cannot use a workqueue here, as this would cause an
inversion when removing the device as the netdev is already gone.
This manifests with a nasty warning during shutdown:

sysfs group ffffffff81eff0e0 not found for kobject 'fc_host7'

So open-code fcoe_destroy_work() when receiving the notification
to avoid this inversion.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 617348f..77adced 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1009,6 +1009,8 @@ static inline int fcoe_em_config(struct fc_lport *lport)
  * fcoe_if_destroy() - Tear down a SW FCoE instance
  * @lport: The local port to be destroyed
  *
+ * Locking: Must be called with the RTNL mutex held.
+ *
  */
 static void fcoe_if_destroy(struct fc_lport *lport)
 {
@@ -1030,14 +1032,12 @@ static void fcoe_if_destroy(struct fc_lport *lport)
 	/* Free existing transmit skbs */
 	fcoe_clean_pending_queue(lport);
 
-	rtnl_lock();
 	if (!is_zero_ether_addr(port->data_src_addr))
 		dev_uc_del(netdev, port->data_src_addr);
 	if (lport->vport)
 		synchronize_net();
 	else
 		fcoe_interface_remove(fcoe);
-	rtnl_unlock();
 
 	/* Free queued packets for the per-CPU receive threads */
 	fcoe_percpu_clean(lport);
@@ -1898,7 +1898,14 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	case NETDEV_UNREGISTER:
 		list_del(&fcoe->list);
 		port = lport_priv(ctlr->lp);
-		queue_work(fcoe_wq, &port->destroy_work);
+		fcoe_vport_remove(lport);
+		mutex_lock(&fcoe_config_mutex);
+		fcoe_if_destroy(lport);
+		if (!fcoe->removed)
+			fcoe_interface_remove(fcoe);
+		fcoe_interface_cleanup(fcoe);
+		mutex_unlock(&fcoe_config_mutex);
+		fcoe_ctlr_device_delete(fcoe_ctlr_to_ctlr_dev(ctlr));
 		goto out;
 		break;
 	case NETDEV_FEAT_CHANGE:
@@ -2114,9 +2121,8 @@ static void fcoe_destroy_work(struct work_struct *work)
 	ctlr = fcoe_to_ctlr(fcoe);
 	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 
-	fcoe_if_destroy(port->lport);
-
 	rtnl_lock();
+	fcoe_if_destroy(port->lport);
 	if (!fcoe->removed)
 		fcoe_interface_remove(fcoe);
 	rtnl_unlock();
@@ -2720,7 +2726,9 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
 	mutex_unlock(&n_port->lp_mutex);
 
 	mutex_lock(&fcoe_config_mutex);
+	rtnl_lock();
 	fcoe_if_destroy(vn_port);
+	rtnl_unlock();
 	mutex_unlock(&fcoe_config_mutex);
 
 	return 0;
-- 
1.8.5.6

Re: [PATCH 3/3] fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

[Lee Duncan]

On 08/31/2017 02:19 AM, Hannes Reinecke wrote:
> When a NETDEV_UNREGISTER notification is received the network
> device is _deleted_ after the callback returns.
> So we cannot use a workqueue here, as this would cause an
> inversion when removing the device as the netdev is already gone.
> This manifests with a nasty warning during shutdown:
> 
< ...
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 3/3] fcoe: open-code fcoe_destroy_work() for NETDEV_UNREGISTER

[Johannes Thumshirn]

Acked-by: Johannes Thumshirn <jth@kernel.org>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 0/3] fcoe: Fixup device_del() inversion

[Martin K. Petersen]

Hannes,

> when shutting down an FCoE target system one is greeted with a flood
> of kernel warnings about
>
> sysfs group ffffffff81eff0e0 not found for kobject 'XXX'
>
> Reason here is that the fcoe driver tries to remove devices from
> a workqueue, but the NETDEV_UNREGISTER notification expects any
> calls to be synchronous as it will tear down the device once
> the notifications return.
> With this patch device removals are done synchronously, so the
> correct ordering is respected and the warnings are gone.

Please address the comments from Lee and Johannes and repost. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering