SRP fail-over faster

Introducing srp_dev_loss_tmo module parameter. Creating a timer to clean up connection after srp_dev_loss_tmo expired. During srp_dev_loss_tmo, the qp is in error state, srp will return DID_RESET for outstanding I/O and return FAILED for abort_cmd, reset_lun, and return SUCCESS (without trying reconnect) on reset_host.

Signed-off-by Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Index: ofed_kernel/drivers/infiniband/ulp/srp/ib_srp.c
===================================================================

drivers/infiniband/ulp/srp/ib_srp.c +38 -0

@@ -78,6 +77,13 @@
  MODULE_PARM_DESC(mellanox_workarounds,
                 "Enable workarounds for Mellanox SRP target bugs if != 0");
 
+static int srp_dev_loss_tmo = 60;
+
+module_param(srp_dev_loss_tmo, int, 0444);
+MODULE_PARM_DESC(srp_dev_loss_tmo,
+                "Default number of seconds that srp transport should \
+                 insulate the lost of a remote port (default is 60 secs");
+
  static void srp_add_one(struct ib_device *device);
  static void srp_remove_one(struct ib_device *device);
  static void srp_completion(struct ib_cq *cq, void *target_ptr);

@@ -898,6 +926,48 @@
                                      DMA_FROM_DEVICE);
  }
 
+static void srp_reconnect_work(struct work_struct *work)
+{
+       struct srp_target_port *target =
+               container_of(work, struct srp_target_port, work);
+
+       srp_reconnect_target(target);
+       target->work_in_progress = 0;
+}
+
+static void srp_qp_in_err_timer(unsigned long data)
+{
+       struct srp_target_port *target = (struct srp_target_port *)data;
+       struct srp_request *req, *tmp;
+
+       if (target->state != SRP_TARGET_LIVE)
+               return;
+
+       spin_lock_irq(target->scsi_host->host_lock);
+       list_for_each_entry_safe(req, tmp, &target->req_queue, list)
+               srp_reset_req(target, req);
+       spin_unlock_irq(target->scsi_host->host_lock);
+
+       spin_lock_irq(target->scsi_host->host_lock);
+       if (!target->work_in_progress) {
+               target->work_in_progress = 1;
+               INIT_WORK(&target->work, srp_reconnect_work);
+               schedule_work(&target->work);
+       }
+       spin_unlock_irq(target->scsi_host->host_lock);
+}
+

drivers/infiniband/ulp/srp/ib_srp.c +3 -0

+
  static void srp_completion(struct ib_cq *cq, void *target_ptr)
  {
        struct srp_target_port *target = target_ptr;

@@ -960,11 +980,20 @@
        ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
        while (ib_poll_cq(cq, 1, &wc) > 0) {
                if (wc.status) {
+                       unsigned long flags;
+
                        shost_printk(KERN_ERR, target->scsi_host,
                                     PFX "failed %s status %d\n",

                                    wc.wr_id & SRP_OP_RECV ? "receive" :
"send",
                                    wc.status);
-                       target->qp_in_error = 1;
+                       spin_lock_irqsave(target->scsi_host->host_lock, flags);
+                       if (!target->qp_in_error &&
+                           target->state == SRP_TARGET_LIVE) {
+                               target->qp_in_error = 1;
+                               srp_qp_err_add_timer(target,
+                                                    srp_dev_loss_tmo - 55);
+                       }
+                       spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
                       break;
               }

drivers/infiniband/ulp/srp/ib_srp.c +23 -1

@@ -1274,5 +1299,6 @@
        int attr_mask = 0;
        int comp = 0;
        int opcode = 0;
+       unsigned long flags;
 
        switch (event->event) {

@@ -1301,6 +1381,14 @@
                shost_printk(KERN_ERR, target->scsi_host,
                             PFX "connection closed\n");
 
+               spin_lock_irqsave(target->scsi_host->host_lock, flags);
+               if (!target->qp_in_error &&
+                   target->state == SRP_TARGET_LIVE) {
+                       target->qp_in_error = 1;
+                       srp_qp_err_add_timer(target,
+                                            srp_dev_loss_tmo - 55);
+               }
+               spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
                target->status = 0;
                break;
 

@@ -1443,9 +1529,22 @@
  static int srp_reset_host(struct scsi_cmnd *scmnd)
  {
        struct srp_target_port *target = host_to_target(scmnd->device->host);
+       struct srp_request *req, *tmp;
        int ret = FAILED;
 
-       shost_printk(KERN_ERR, target->scsi_host, PFX "SRP reset_host
 called\n");
+       shost_printk(KERN_ERR, target->scsi_host,
+                    PFX "SRP reset_host called state %d qp_err %d\n",
+                    target->state, target->qp_in_error);
+
+       spin_lock_irq(target->scsi_host->host_lock);
+       if (timer_pending(&target->qp_err_timer) || target->qp_in_error ||
+           target->state != SRP_TARGET_LIVE) {
+               list_for_each_entry_safe(req, tmp, &target->req_queue, list)
+                       srp_reset_req(target, req);
+               spin_unlock_irq(target->scsi_host->host_lock);
+               return SUCCESS;
+       }
+       spin_unlock_irq(target->scsi_host->host_lock);
 
        if (!srp_reconnect_target(target))

               ret = SUCCESS;

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

@@ -2150,6 +2342,9 @@

                         sizeof (struct srp_indirect_buf) + srp_sg_tablesize * 16);

+       if (srp_dev_loss_tmo < 60)
+               srp_dev_loss_tmo = 60;
+
       ret = class_register(&srp_class);
       if (ret) {
               printk(KERN_ERR PFX "couldn't register class infiniband_srp\n");
Index: ofed_kernel/drivers/infiniband/ulp/srp/ib_srp.h
===================================================================

drivers/infiniband/ulp/srp/ib_srp.h +1 -0

@@ -153,12 +159,14 @@
        struct srp_request      req_ring[SRP_SQ_SIZE];
 
        struct work_struct      work;
+       int                     work_in_progress;

+static int srp_dev_loss_tmo = 60;

Quoted 1 line

+static int srp_dev_loss_tmo = 60;

I don't think the name needs to be this abbreviated. We don't necessarily need the srp_ prefix, but probably "device_loss_timeout" is much clearer without being too much longer.

drivers/infiniband/ulp/srp/ib_srp.c line 85

Quoted 5 lines

drivers/infiniband/ulp/srp/ib_srp.c +5 -0

+
+module_param(srp_dev_loss_tmo, int, 0444);
+MODULE_PARM_DESC(srp_dev_loss_tmo,
+		 "Default number of seconds that srp transport should \
+		  insulate the lost of a remote port (default is 60 secs");

I can't understand this description. What does "insulate the lost" of a port mean?

drivers/infiniband/ulp/srp/ib_srp.c line 935

Quoted 7 lines

drivers/infiniband/ulp/srp/ib_srp.c +7 -0

+static void srp_reconnect_work(struct work_struct *work)
+{
+	struct srp_target_port *target =
+		container_of(work, struct srp_target_port, work);
+
+	srp_reconnect_target(target);
+	target->work_in_progress = 0;

surely this is racy... isn't it possible for a context to see work_in_progress as 1, decide not to schedule the work, and then have it set to 0 immediately afterwards by the workqueue context?

drivers/infiniband/ulp/srp/ib_srp.c line 966

Quoted 1 line

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

+		target->qp_err_timer.expires = time * HZ + jiffies;

given that this is only with 1 second resolution, probably makes sense to either make it a deferrable timer or round the timeout to avoid extra wakeups.

drivers/infiniband/ulp/srp/ib_srp.c line 967

Quoted 1 line

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

+		add_timer(&target->qp_err_timer);

I don't see anywhere that this is canceled on module unload etc?

drivers/infiniband/ulp/srp/ib_srp.c line 2346

Quoted 5 lines

drivers/infiniband/ulp/srp/ib_srp.c +4 -0

+				srp_qp_err_add_timer(target,
+						     srp_dev_loss_tmo - 55);
 
+	if (srp_dev_loss_tmo < 60)
+		srp_dev_loss_tmo = 60;

I don't understand the 55 and the 60 here... what are these magic numbers? Wouldn't it make sense for the user to specify the actual timeout that is used (value - 55) rather than the value and then secretly subtracting 55?

- R.

First it does not make sense for user to set it below 60; therefore, it is forced to have 60 and above

Quoted 2 lines

First it does not make sense for user to set it below 60; therefore, it is forced to have 60 and above

Why not? A minute seems to be a really long time given the point of these patches is supposed to be failing over faster. Surely we can tell if a path really failed sooner than 60 seconds on an IB fabric.

- R.

Quoted 3 lines

Quoted 3 lines

First it does not make sense for user to set it below 60; therefore, it is forced to have 60 and above

Quoted 3 lines

Why not? A minute seems to be a really long time given the point of these patches is supposed to be failing over faster. Surely we can tell if a path really failed sooner than 60 seconds on an IB fabric.

When we fail-over, it will cause the luns ownership transfer in target/storage. It's undesirable op unless necessary Target/storage most likely can reboot and come back within 60 seconds We don't want to create the situation of path bouncing

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

First it does not make sense for user to set it below 60; therefore, it is forced to have 60 and above

Why not? A minute seems to be a really long time given the point of these patches is supposed to be failing over faster. Surely we can tell if a path really failed sooner than 60 seconds on an IB fabric.

When we fail-over, it will cause the luns ownership transfer in target/storage. It's undesirable op unless necessary Target/storage most likely can reboot and come back within 60 seconds We don't want to create the situation of path bouncing

OK, I can see why in some (many) situations it makes sense to wait a while before reporting a target as gone. But why do we hard code the policy of a minimum timeout of 60 seconds in the kernel? Why not a minimum of 120 seconds? What if I know my storage is guaranteed to reboot in 2 seconds -- why can't I have a timeout of 5 seconds?

You haven't really explained where the magic number of 60 seconds comes from.

- R.

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

First it does not make sense for user to set it below 60; therefore, it is forced to have 60 and above

Why not? A minute seems to be a really long time given the point of these patches is supposed to be failing over faster. Surely we can tell if a path really failed sooner than 60 seconds on an IB fabric.

When we fail-over, it will cause the luns ownership transfer in target/storage. It's undesirable op unless necessary Target/storage most likely can reboot and come back within 60 seconds We don't want to create the situation of path bouncing

OK, I can see why in some (many) situations it makes sense to wait a while before reporting a target as gone. But why do we hard code the policy of a minimum timeout of 60 seconds in the kernel? Why not a minimum of 120 seconds? What if I know my storage is guaranteed to reboot in 2 seconds -- why can't I have a timeout of 5 seconds?

You haven't really explained where the magic number of 60 seconds comes from.

Quoted 19 lines · Roland Dreier

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

First it does not make sense for user to set it below 60; therefore,

it is forced to have 60 and above

Quoted 8 lines

Quoted 3 lines

Why not? A minute seems to be a really long time given the point of these patches is supposed to be failing over faster. Surely we can tell if a path really failed sooner than 60 seconds on an IB fabric.

When we fail-over, it will cause the luns ownership transfer in target/storage. It's undesirable op unless necessary Target/storage most likely can reboot and come back within 60 seconds We don't want to create the situation of path bouncing

OK, I can see why in some (many) situations it makes sense to wait a while before reporting a target as gone. But why do we hard code the policy of a minimum timeout of 60 seconds in the kernel? Why not a minimum of 120 seconds? What if I know my storage is guaranteed to reboot in 2 seconds -- why can't I have a timeout of 5 seconds?

Quoted 2 lines

You haven't really explained where the magic number of 60 seconds comes from.

On that note, does this have to be a module parameter -- what if I have connections to different devices, that have different reboot guarantees?

And if I really want a timeout of less than 60 seconds, why should I have to patch my kernel?

And if I want to disable this completely?

Quoted 2 lines · David Dillow

And if I want to disable this completely?

Unless these patches are bad and affect the stability of the driver, why do you want to disable it? If you don't use multipath/device-mapper and use /dev/sd**, everything will be same

Quoted 4 lines · Vu Pham

Quoted 2 lines · David Dillow

And if I want to disable this completely?

Quoted 3 lines

Unless these patches are bad and affect the stability of the driver, why do you want to disable it? If you don't use multipath/device-mapper and use /dev/sd**, everything will be same

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

Quoted 7 lines · David Dillow

Quoted 5 lines · Vu Pham

Quoted 3 lines · David Dillow

And if I want to disable this completely?

Quoted 7 lines

Quoted 2 lines

Unless these patches are bad and affect the stability of the driver, why do you want to disable it? If you don't use multipath/device-mapper and use /dev/sd**, everything will be same

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

I've been left with a similar impression for several multipath things I've seen in the past. True active/active multipath setups should have a shorter timeout - there is no penalty for directing more traffic to the 2nd path (the paths should be load balancing existing traffic in the standard case anyhow).

An active/passive configuration might be different...

Certainly an enforced lower limit in the kernel is silly, and a per-device setting does make some sense.

Jason

Quoted 5 lines · Jason Gunthorpe

Quoted 4 lines · David Dillow

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

Quoted 2 lines

Certainly an enforced lower limit in the kernel is silly, and a per-device setting does make some sense.

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Quoted 9 lines · Vu Pham

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · David Dillow

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

Certainly an enforced lower limit in the kernel is silly, and a per-device setting does make some sense.

Quoted 3 lines

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Please put your patches inline for ease of reply...

You still seem to have a 30 second minimum timeout, and no way to disable it altogether.

Quoted 15 lines · David Dillow

Quoted 13 lines · Vu Pham

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · David Dillow

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

Certainly an enforced lower limit in the kernel is silly, and a per-device setting does make some sense.

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Quoted 4 lines

Please put your patches inline for ease of reply...

You still seem to have a 30 second minimum timeout, and no way to disable it altogether.

And if I could read a patch, I'd notice that that was not a minimum, but a default. Still, I have to have a 1 second timeout with no way to disable entirely. Better, but...

Quoted 20 lines · David Dillow

Quoted 18 lines · David Dillow

Quoted 9 lines · Vu Pham

Quoted 7 lines · Jason Gunthorpe

Quoted 5 lines · David Dillow

I use multipath with ALUA, and I don't mind if the link flaps a bit. 60 seconds is near my SCSI timeout of 77 seconds, so it doesn't buy me much. I'd rather multipath be delivering traffic to the backup path than sitting on its thumbs for 60 seconds doing nothing.

Quoted 5 lines

Quoted 3 lines

Certainly an enforced lower limit in the kernel is silly, and a per-device setting does make some sense.

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Please put your patches inline for ease of reply...

Quoted 7 lines

Quoted 3 lines

You still seem to have a 30 second minimum timeout, and no way to disable it altogether.

And if I could read a patch, I'd notice that that was not a minimum, but a default. Still, I have to have a 1 second timeout with no way to disable entirely. Better, but...

Last time I reply to myself tonight, I promise... :/

You also don't seem to use the user supplied setting, but hard code the time to 5 seconds?

David Dillow wrote:

Yes and you can not disable intirely. I'm still looking at benefits/advantages to disable it entirely

Quoted 2 lines

You also don't seem to use the user supplied setting, but hard code the time to 5 seconds?

I use the user supplied setting for local async event on port error where link is broken from host to switch

For case link broken from target port to switch. We detect this case by receiving connection closed or wqe error and when this happen unknown certain seconds already passed by; therefore, I sleep 5 seconds instead of using user supplied value.

To really sleep user supplied number of seconds, we need to register trap to SM and receiving trap for a node leaving the fabric. It requires a lot of changes in srp_daemon (registering to trap, passing event down to srp driver) and srp driver (handling this event)

Quoted 2 lines · Vu Pham

David Dillow wrote:

Quoted 2 lines

Yes and you can not disable intirely. I'm still looking at benefits/advantages to disable it entirely

To me, the advantage is I have a perfectly viable backup path to the storage, and can immediately start issuing commands to it rather than waiting for any timeout. On my systems, 1 second can be up to 1500 MB transferred and a _huge_ number of compute cycles. And I expect those numbers to grow.

Quoted 5 lines

Quoted 3 lines

You also don't seem to use the user supplied setting, but hard code the time to 5 seconds?

I use the user supplied setting for local async event on port error where link is broken from host to switch

Perhaps that part should be in the patch that adds that support, then?

Quoted 2 lines

For case link broken from target port to switch. We detect this case by receiving connection closed or wqe error and when this happen unknown certain seconds already passed by; therefore, I sleep 5 seconds instead of using user supplied value.

This makes a certain amount of sense; I was confused by the two unrelated changes in this patch. I'm still not all that happy about a hard-coded 5 seconds, especially with no explanation about the magic number.

Quoted 4 lines

To really sleep user supplied number of seconds, we need to register trap to SM and receiving trap for a node leaving the fabric. It requires a lot of changes in srp_daemon (registering to trap, passing event down to srp driver) and srp driver (handling this event)

Well, if this were done, then you wouldn't need to sleep at all would you? Just wait for the trap telling you the target rejoined the fabric? Perhaps you'd want a delay before tearing down the target connection, but then that could be part of the user settings above?

Not that I'm sure it is worth it, though.

Quoted 4 lines · David Dillow

Quoted 2 lines · Vu Pham

Yes and you can not disable intirely. I'm still looking at benefits/advantages to disable it entirely

Quoted 6 lines

To me, the advantage is I have a perfectly viable backup path to the storage, and can immediately start issuing commands to it rather than waiting for any timeout. On my systems, 1 second can be up to 1500 MB transferred and a _huge_ number of compute cycles. And I expect those numbers to grow.

You can still do so with these patches applied by using the right device name (ie. /dev/sdXXX)

Quoted 5 lines

Quoted 2 lines

I use the user supplied setting for local async event on port error where link is broken from host to switch

Perhaps that part should be in the patch that adds that support, then?

That's patch #4

Quoted 5 lines

This makes a certain amount of sense; I was confused by the two unrelated changes in this patch. I'm still not all that happy about a hard-coded 5 seconds, especially with no explanation about the magic number.

As I said above, it's not magic at all, it just that certain unknown seconds already passed by, therefore, just pick X seconds to sleep on.

Quoted 11 lines

Quoted 5 lines

To really sleep user supplied number of seconds, we need to register trap to SM and receiving trap for a node leaving the fabric. It requires a lot of changes in srp_daemon (registering to trap, passing event down to srp driver) and srp driver (handling this event)

Well, if this were done, then you wouldn't need to sleep at all would you? Just wait for the trap telling you the target rejoined the fabric? Perhaps you'd want a delay before tearing down the target connection, but then that could be part of the user settings above?

Quoted 2 lines

Not that I'm sure it is worth it, though.

If it's done, you still need to sleep target->device_loss_timeout (instead of some unknown seconds + 5) to tear down connection so that dm-multipath can fail-over.

Quoted 13 lines · Vu Pham

Quoted 11 lines · David Dillow

Quoted 2 lines · Vu Pham

Yes and you can not disable intirely. I'm still looking at benefits/advantages to disable it entirely

To me, the advantage is I have a perfectly viable backup path to the storage, and can immediately start issuing commands to it rather than waiting for any timeout. On my systems, 1 second can be up to 1500 MB transferred and a _huge_ number of compute cycles. And I expect those numbers to grow.

Quoted 2 lines

You can still do so with these patches applied by using the right device name (ie. /dev/sdXXX)

Not in a multipath situation configured for failover. I have to use the multipath device, which will then use the appropriate path as prioritized by ALUA.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I use the user supplied setting for local async event on port error where link is broken from host to switch

Perhaps that part should be in the patch that adds that support, then?

That's patch #4

Sure, and perhaps the part that massages the timeout should be in the patch that introduces it and actually uses it, no?

Quoted 8 lines

Quoted 6 lines

This makes a certain amount of sense; I was confused by the two unrelated changes in this patch. I'm still not all that happy about a hard-coded 5 seconds, especially with no explanation about the magic number.

As I said above, it's not magic at all, it just that certain unknown seconds already passed by, therefore, just pick X seconds to sleep on.

Sorry, this is a common idiom here -- a bare number in source code, with no explanation as to where it came from or why it was picked, is often called a "magic number."

I'm saying you should comment on it, either in the commit message or in a comment in the code. Or better yet, give it a #define and a comment above that definition that says why you picked it.

In other words, don't make someone who comes along after us have to search for this mail thread to figure out that the 5 second sleep was pulled out of thin air.

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

To really sleep user supplied number of seconds, we need to register trap to SM and receiving trap for a node leaving the fabric. It requires a lot of changes in srp_daemon (registering to trap, passing event down to srp driver) and srp driver (handling this event)

Well, if this were done, then you wouldn't need to sleep at all would you? Just wait for the trap telling you the target rejoined the fabric? Perhaps you'd want a delay before tearing down the target connection, but then that could be part of the user settings above?

Not that I'm sure it is worth it, though.

Quoted 2 lines

If it's done, you still need to sleep target->device_loss_timeout (instead of some unknown seconds + 5) to tear down connection so that dm-multipath can fail-over.

Or I can just start failing requests due to knowing they won't get to the target so dm-multipath will use the backup path immediately. I can sleep as long as I want before killing the connection, just in case it comes back, but my commands will still be going to the other path.

David Dillow wrote:
I don't know much about multipath in ALUA mode.
How would multipath driver (in ALUA mode) to switch path? (ie. basing on what criteria?)

Can you switch path manually in user mode (while there are commands stucked in current active path)?

Without this patch, all outstanding I/Os have to go thru error recovery before being returned with error code so that dm-multipath fail-over.

If you want to failing requests right away, you can just set device_loss_timeout=1, others don't want dm-multipath to switch path right away. That's a whole idea of these patches that I submitted

Quoted 4 lines · Vu Pham

David Dillow wrote:
I don't know much about multipath in ALUA mode.
How would multipath driver (in ALUA mode) to switch path? (ie. basing on what criteria?)

ALUA sets the priority of each path, and generally multipath is set to round-robin among all paths of the same priority. So, paths going to the primary controller of a LUN get the best priority and are used preferentially over the backup paths. Once no more paths in a priority group are active, the round-robin selector will fall back to the next highest priority path group.

The multipath core will immediately fail a path if the lower layers propagate an error up to it, such that an I/O request completes in error. If it has failed the path, it will start sending requests down alternate paths without waiting for the queue to drain on the first one.

ALUA is not like RDAC -- in ALUA, all paths are valid to use, but some paths will give better performance. You do not necessarily need to give the array a command to move the LUN to another controller, so there's no reason to wait for a queue to drain.

At least that is the way I understand things, having picked my way through the block layer, multipath core, and device handlers.

Quoted 2 lines

Can you switch path manually in user mode (while there are commands stucked in current active path)?

I've not tried, but give the above I don't see why not.

Quoted 2 lines

Without this patch, all outstanding I/Os have to go thru error recovery before being returned with error code so that dm-multipath fail-over.

I think we're talking about two separate things here -- I agree that the idea of failing IO early when we've lost our local connection, or know the target is not in the fabric, is a good one. I want a fast failure so that I can immediately start using my alternate paths. I'll have to deal with the timeouts on the requests in flight at some point, but they don't hold back independent requests.

The difference of opinion seems to be in how long to wait after being notified of a connection loss -- or the target leaving the fabric -- before we start kicking back errors at the SRP command dispatch handler. I agree that it makes sense to wait a moment before forcing an RDAC path change, as they seem to be slow. But I also want it to get out of the way for my case, when I don't incur much of a penalty to immediately light up my backup path.

Quoted 2 lines

If you want to failing requests right away, you can just set device_loss_timeout=1, others don't want dm-multipath to switch path right away. That's a whole idea of these patches that I submitted

The thing is, I don't want to wait even 1 second to use my backup path, and I don't want all of those requests going into a black hole for that time, forcing me to wait for the SCSI timeout on requests that could have been immediately processed. On our system, 1 second is up to 1500 MB of data transferred over this one connection, and waiting around twiddling our thumbs for a single second can potentially cost 1.3 thousand trillion operations.

It's a big improvement from 3-5 minutes cutting down to 1s and now you talk about device_loss_timeout=0. I'll look at the trade-off to have it; however, to receive and process the async event (port error) already cost you a fair amount of cycles.

Quoted 3 lines · Vu Pham

David Dillow wrote:
I don't know much about multipath in ALUA mode.
How would multipath driver (in ALUA mode) to switch path? (ie. basing on what criteria?)

ALUA sets the priority of each path, and generally multipath is set to round-robin among all paths of the same priority. So, paths going to the primary controller of a LUN get the best priority and are used preferentially over the backup paths. Once no more paths in a priority group are active, the round-robin selector will fall back to the next highest priority path group.

The multipath core will immediately fail a path if the lower layers propagate an error up to it, such that an I/O request completes in error. If it has failed the path, it will start sending requests down alternate paths without waiting for the queue to drain on the first one.

ALUA is not like RDAC -- in ALUA, all paths are valid to use, but some paths will give better performance. You do not necessarily need to give the array a command to move the LUN to another controller, so there's no reason to wait for a queue to drain.

At least that is the way I understand things, having picked my way through the block layer, multipath core, and device handlers.

[ ... ]

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Introducing device_loss_timeout per target granularity. Creating a timer to clean up connection after device_loss_timeout expired. During device_loss_timeout, the QP is in error state, srp will return DID_RESET for outstanding I/Os and return FAILED for abort_cmd, reset_lun, and return SUCCESS (without retrying reconnect) on reset_host

Signed-off-by Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

-----------------------------------------------------------------------

drivers/infiniband/ulp/srp/ib_srp.c |   94 ++++++++++++++++++++++++++++++++++- drivers/infiniband/ulp/srp/ib_srp.h |    3 +

 2 files changed, 95 insertions(+), 2 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

index e44939a..12404d5 100644
@@ -433,6 +433,10 @@ static void srp_remove_work(struct work_struct *work)
                return;
        }

Quoted 36 lines · Vu Pham

[ ... ]

Here is the updated patch which implement the device_loss_timeout for each target instead of module parameter. It also reflects changes from previous feedbacks. Please review

Introducing device_loss_timeout per target granularity. Creating a timer to clean up connection after device_loss_timeout expired. During device_loss_timeout, the QP is in error state, srp will return DID_RESET for outstanding I/Os and return FAILED for abort_cmd, reset_lun, and return SUCCESS (without retrying reconnect) on reset_host

Signed-off-by Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

-----------------------------------------------------------------------

drivers/infiniband/ulp/srp/ib_srp.c | 94 ++++++++++++++++++++++++++++++++++-

 drivers/infiniband/ulp/srp/ib_srp.h |    3 +
 2 files changed, 95 insertions(+), 2 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +4 -0

index e44939a..12404d5 100644
@@ -433,6 +433,10 @@ static void srp_remove_work(struct work_struct *work)
               return;
       }
       target->state = SRP_TARGET_REMOVED;
+
+       if (timer_pending(&target->qp_err_timer))
+               del_timer_sync(&target->qp_err_timer);
+
       spin_unlock_irq(target->scsi_host->host_lock);
 
       spin_lock(&target->srp_host->target_lock);

Quoted 2 lines

[ ... ]

Calling del_timer_sync() while holding a spinlock can cause locking inversion. Has this code been tested on a kernel with LOCKDEP enabled ?

Bart.

drivers/infiniband/ulp/srp/ib_srp.c static void srp_remove_work(struct work_struct *work) · line 440

Quoted 4 lines

drivers/infiniband/ulp/srp/ib_srp.c +3 -0

+	if (timer_pending(&target->qp_err_timer))
+		del_timer_sync(&target->qp_err_timer);
+
 	spin_unlock_irq(target->scsi_host->host_lock);

As was pointed out, I don't think you can do del_timer_sync while holding the lock, since the timer function takes the same lock.

But I don't know that just switching to del_timer without the sync works here ... without the sync then the timeout function could still run any time after the del_timer, even after everything gets freed.

BTW the test of timer_pending isn't needed here... del_timer does the test internally anyway.

I do agree it would be very good to improve the SRP error handling. I have some concerns about the overall design here -- it seems that if we handle connection failure and allow a new connection to proceed while cleaning up asynchronously, then this opens the door to a lot of complexity, and I don't see that complexity handled in this patchset. For example, the new connection could fail too before the old one is done cleaning up, etc, etc and we end up with an arbitrarily large queue of things waiting to clean up. Or maybe it really it is simpler than that.

I think the best way to move this forward would be to post another cleaned up version of your patch set. Please try to reorganize things so each patch is reasonably self contained. Of course your patchset is taking multiple steps to improve things. But as much as possible, please try to avoid combining two things into a single patch, and conversely also try to avoid putting things into a patch that don't make sense without a later patch.

Avoiding policy in the kernel as much as possible in terms of hard-coded timeouts etc is a good goal too.

Also it would help to give each patch a separate descriptive subject, and put as much detail in the changelogs as you can.

Thanks,
Roland