On Tue, 2016-01-12 at 16:07 +0100, Christoph Hellwig wrote: > > -static int core_set_queue_depth_for_node( > > - struct se_portal_group *tpg, > > - struct se_node_acl *acl) > > +static void > > +target_set_nacl_queue_depth(struct se_portal_group *tpg, > > + struct se_node_acl *acl, u32 queue_depth) > > { > > + acl->queue_depth = queue_depth; > > + > > if (!acl->queue_depth) { > > - pr_err("Queue depth for %s Initiator Node: %s is 0," > > + pr_warn("Queue depth for %s Initiator Node: %s is 0," > > "defaulting to 1.\n", tpg->se_tpg_tfo->get_fabric_name(), > > acl->initiatorname); > > acl->queue_depth = 1; > > } > > - > > - return 0; > > } > > These changes seem unrelated to the rest, care to explain them or > preferably split them out? With this patch in place, this function is now also called by core_tpg_set_initiator_node_queue_depth(), where previously it was called only during target_alloc_node_acl(). Might as well drop the ignored return while we're at it.. > > > int core_tpg_set_initiator_node_queue_depth( > > struct se_portal_group *tpg, > > - unsigned char *initiatorname, > > + struct se_node_acl *acl, > > u32 queue_depth, > > int force) > > please drop th force parameter as it's always 1. > Done. > > { > > + LIST_HEAD(sess_list); > > + struct se_session *sess, *sess_tmp; > > unsigned long flags; > > + int rc; > > > > + /* > > + * User has requested to change the queue depth for a Initiator Node. > > + * Change the value in the Node's struct se_node_acl, and call > > + * target_set_nacl_queue_depth() to set the new queue depth. > > + */ > > + target_set_nacl_queue_depth(tpg, acl, queue_depth); > > > > + spin_lock_irqsave(&acl->nacl_sess_lock, flags); > > + list_for_each_entry_safe(sess, sess_tmp, &acl->acl_sess_list, > > + sess_acl_list) { > > + if (sess->sess_tearing_down != 0) > > continue; > > > > if (!force) { > > @@ -401,71 +387,36 @@ int core_tpg_set_initiator_node_queue_depth( > > " operational. To forcefully change the queue" > > " depth and force session reinstatement" > > " use the \"force=1\" parameter.\n", > > + tpg->se_tpg_tfo->get_fabric_name(), > > + acl->initiatorname); > > + spin_unlock_irqrestore(&acl->nacl_sess_lock, flags); > > return -EEXIST; > > } > > + if (!target_get_session(sess)) > > continue; > > + spin_unlock_irqrestore(&acl->nacl_sess_lock, flags); > > > > + * Finally call tpg->se_tpg_tfo->close_session() to force session > > + * reinstatement to occur if there is an active session for the > > + * $FABRIC_MOD Initiator Node in question. > > */ > > + rc = tpg->se_tpg_tfo->shutdown_session(sess); > > + target_put_session(sess); > > + if (!rc) { > > + spin_lock_irqsave(&acl->nacl_sess_lock, flags); > > + continue; > > + } > > + target_put_session(sess); > > + spin_lock_irqsave(&acl->nacl_sess_lock, flags); > > } > > + spin_unlock_irqrestore(&acl->nacl_sess_lock, flags); > > It seems at thus point there is no need for ->shutdown_session, it > could be folded into ->close_session in a follow on patch. > Not exactly. It's the final target_put_session() -> kref_put() upon se_sess->sess_kref that invokes TFO->close_session(). > > -void target_get_session(struct se_session *se_sess) > > +int target_get_session(struct se_session *se_sess) > > { > > - kref_get(&se_sess->sess_kref); > > + return kref_get_unless_zero(&se_sess->sess_kref); > > } > > EXPORT_SYMBOL(target_get_session); > > I'd be much happier to have a separate prep patch for this.. Since this will need to hit stable at some point, it likely needs to stay with the original bug-fix. -- 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