On Tue, 2012-07-17 at 09:34 -0700, Roland Dreier wrote: > On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger > <nab@xxxxxxxxxxxxxxx> wrote: > >> Do you have a plan for how to handle this? Do we really want to plumb > >> through another callback to tell the fabric driver to free the command > >> in this case? > > > I need to think more about this ahead of changing it back again for-3.6 > > now that other fabrics have the assumption of target_submit_cmd() would > > not fail. > > > There is a clear case with qla2xxx for just changing it back (we might > > end up doing that just yet) but I wanted to get the other important bits > > into for-next into place first.. > > Sleeping on things, I now feel pretty strongly that having target_submit_cmd > return an error value for "immediate" errors where the command does not > make it into the target core is the right approach. > > Freeing commands is already one of the most confusing and complex parts > of the target code, with "check_stop_free," "cmd_wait_comp" and > "SCF_ACK_KREF." Adding another code flow back to the fabric driver > with yet another set of semantics around freeing a command seems like > it's making things even harder to maintain. > I wasn't talking about adding another release path. I was thinking more about a TFO call to optionally abort HW/SW exchange if we can't accept the command in question. Not to mention, this could end up being useful for other purposes beyond the initial target_submit_cmd() failure case due to active I/O shutdown. (aborts + active I/O shutdown with active SRP WRITEs with ib_srpt come to mind needing something like this..) > On the other hand, every caller of target_submit_cmd() in the tree already > has an error path where it handles problems it detects right before calling > target_submit_cmd(), and so it's trivial to share that for problems detected > inside target_submit_cmd(). If you look at patch 4/7, pretty much the > only changes are like going from > > target_submit_cmd(...); > > to > > if (target_submit_cmd(...)) > goto err; > > and in fact the ->handle_cmd() wrapper that qla_target uses to call > target_submit_cmd via tcm_qla2xxx already has a return value that > qla_target handled properly! > Sure, there is no doubting tcm_qla2xxx's usage of this return value back into qla_target.c code.. > I guess another approach would be to split target_submit_cmd() into > the target_get_sess_cmd() part and the rest of it, and have fabric > drivers handle errors queueing commands but not have to worry about > the submit cmd part failing. But I'm not sure that's any better. > > Andy, any feelings about this one way or another? Christoph? > Ok, so for-3.6 it makes the most sense to just convert this back to propagate up the return code, but only for target_get_sess_cmd() failure case.. That said, I'll go ahead and respin patch #4 on top of for-next, and post the updated patch shortly with the same CC's and ask for ACKs from the necessary folks. --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
