On Tue, 2011-09-13 at 23:08 +0200, Christoph Hellwig wrote: > This series cleans up various lose ends when freeing struct se_cmds. > > There are two things I stumbled upon when doing this which rather confuse > me: > > (1) what exactly is the purpose of the check_stop_free fabric method. > Generally this ends up as a transport_generic_free_cmd with just > a few notable differences: > > - srpt decrements an internal reference count, which ends > up calling transport_generic_free_cmd once it hits zero > - tcm_fc does not ignore TMR requests like all others > - qla2xxxx cals transport_generic_free_cmd for tmr request, > but does not do it for normal requests > ->check_stop_free() was originally added for tcm_loop, which is able to release it's descriptor in the main callback path, eg: scsi_cmnd->scsi_done is called directly within ->queue_data_in() and ->queue_status, and we don't have to wait for a fabric acknowledge before releasing.. tcm_fc is using ->check_stop_free() in the same manner as tcm_loop, to directly release se_cmd immediately via transport_generic_free_cmd() > I seems rather counter productive to not have a common model > here, and I don't quite understand what qla2xxx and the srp > target are trying to archive > The problem is a race between ->check_stop_free() being called in transport_cmd_check_stop(), and fabric modules releasing se_cmd before the completion of ->->check_stop_free() This is currently why srpt and qla2xxx do these extra checks, and we would benefit from internalizing this logic into target core. > (2) what the point of SCF_SE_LUN_CMD is. As a first a !se_cmd->se_lun > check should give us the same information. But more importantly > why we would ever allow a command without a LUN to stay around, > and more importantly be on core target lists that could lead to > a free. This was originally used by iscsi-target to identify which descriptors had a se_lun association, but eventually bled into target-core. In modern target core code this ends up signaling when a se_cmd is associated with a TMR, but is for all intensive purposes unnecessary in target-core. --nab -- 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
