On Mon, Jun 13, 2011 at 02:51:18PM -0700, Nicholas A. Bellinger wrote: > + list_del(&cmd->se_qf_node); > + atomic_dec(&dev->dev_qf_count); > + smp_mb__after_atomic_dec(); > + spin_unlock_irq(&dev->qf_cmd_lock); When you unlock right after an atomic_dec there should be no need for a memory barrier. > + > + printk(KERN_INFO "Processing %s cmd: %p QUEUE_FULL in work queue" > + " context: %s\n", cmd->se_tfo->get_fabric_name(), cmd, > + (cmd->t_state == TRANSPORT_COMPLETE_OK) ? "COMPLETE_OK" : > + (cmd->t_state == TRANSPORT_COMPLETE_QF_WP) ? "WRITE_PENDING" > + : "UNKNOWN"); Please remove this printk again. Printing tons of verbose information during normal operation is a complete no-go. These look like fine candidates for tracepoints. > + /* > + * The SCF_EMULATE_QUEUE_FULL flag will be cleared once se_cmd > + * has been added to head of queue > + */ > + transport_add_cmd_to_queue(cmd, cmd->t_state); I'd just pass 0 instead of t->state, as that avoids a useless roundtrip on t_state_lock. > + spin_lock_irq(&dev->qf_cmd_lock); > + cmd->se_cmd_flags |= SCF_EMULATE_QUEUE_FULL; > + cmd->transport_qf_callback = qf_callback; What's the point of setting the callback in the command? Callbacks in the command mean serious memory bloat given how many of them we have. I can't see anything that speaks against having this in the fabric ops. > + list_add_tail(&cmd->se_qf_node, &cmd->se_dev->qf_cmd_list); > + atomic_inc(&dev->dev_qf_count); > + smp_mb__after_atomic_inc(); > + spin_unlock_irq(&cmd->se_dev->qf_cmd_lock); Again, this shouldn't need a barrier. > + schedule_work(&cmd->se_dev->qf_work_queue); So this is simply scheduled using the system workqueues. Why is it handled different from normal execution? I don't quite get why it's offloaded to start with. Can't we just add it to the pending list for the per-dev thread from this context directly? > + SCF_EMULATE_QUEUE_FULL = 0x02000000, Why _EMULATE? It's not more emulated than anything else in a target. Also please document all flags that you add. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
