On Fri, 11 May 2012 13:55:03 -0700, Nicholas A. Bellinger wrote: > On Fri, 2012-05-11 at 10:38 -0400, Jörn Engel wrote: > > This patch isn't done yet. I'm having enough trouble to untangle the > > qla2xxx side alone, so all other transports are ignored for now and > > some break on build. But ignoring that issue, is it correct(er) to > > always return 1 from tcm_qla2xxx_check_stop_free()? > > Correct, this is intended for users of target_submit_cmd() w/ > TARGET_SCF_ACK_KREF, and used by the response callback accounting done > within transport_cmd_check_stop() -> TFO->check_stop_free(). > > > It clearly doesn't make any sense to check the return value of kref_put. > > However, it is unclear to me what purpose this return value is supposed > > to serve. It seems to be indicating whether the command in question has > > been freed. Well, it has in the sense that the kfref is gone and it is > > no longer safe to access it. > > You'll recall this was originally added to the target-core response path > to signal the final release of the descriptor between the target-core > response accountting, and HW fabric ACK generated by qla2xxx to release > qla_tgt_cmd descriptor memory. > > So the way things currently work, always returning 1 here for > ->check_stop_free() with TARGET_SCF_ACK_KREF usage is certainly not the > right thing to do.. > > Are you intending to try to get rid of the TFO->check_stop_free() return > value all together, or something else..? My goal is to not look at the return code of kref_put. Documentation/kref does have one semi-valid example of using the return value, but I don't believe it applies to the code discussed here. All other uses only introduce subtle races that usually work ok and sometimes fail spectacularly. I care more deeply as one of my coworkers has created a testcase that manages to tickle spectacular failure fairly consistently, putting me on a mad rampage against anything that could possibly race. ;) Jörn -- Somewhere around the year 2000 there was this turningpoint when it became cheaper to collect information than to understand it. -- Freeman Dyson -- 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
