On Wed, 2013-08-21 at 09:14 -0700, Christoph Hellwig wrote: > I don't like the layering here. The re-execution of the same command > for both reading and writing the data from/to the backend device already > looks sketchy here due to doubling work of task attribute handling, the > various state bits, etc. And it will only get more complicated when > the required locking is added. In addition we have all that confusion > about overloading the data direction. > > I think the way this should be handled is: > > > - cmd->execute_cmd gets set to a new sbc_emulate_compare_and_write > - sbc_emulate_compare_and_write does all the setup for the locking, > sets up the read buffer, then calls ops->execute_rw to do the > read. The complete callback does the comparism, then calls > ops->execute_rw to do the write, and the second time we hit > the complete callback we teard down the read buffer, stop the > locking, etc. > I do like this approach better, however calling ops->execute_rw() the second time around does require at least the TRANSPORT_PROCESSING and other transport_state bits from target_execute_cmd() to be set for things to work correctly. Bypassing the aborted + CMD_*STOP checks should be OK for the write submission, and will kick (if necessary) during the final completion. Setting up the read buffer from sbc_emulate_compare_and_write() would require two extra COMPARE_AND_WRITE specific se_cmd elements, so I'm tempted to continue to use the bidi fields for this (because they already exist) with transport_generic_get_mem_bidi(), and use a SCF_COMPARE_AND_WRITE flag to avoid any CDB specific checks in transport_complete_ok(), and reverse dma direction mapping case. > This also avoids bloating the command with another function pointer > or having to change all execute_cmd prototypes. Indeed. --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
