On Thu, Jun 09, 2011 at 12:25:11PM -0700, Andy Grover wrote: > ? I don't quite follow. Let's use handling REPORT LUNS > (target_core_device.c transport_core_report_lun_response()) as an example. > > First, I don't see in the spec where the REPORT LUNS data-in allocation > length is limited. We currently limit to TRANSPORT_MAX_LUNS_PER_TPG > (256) but if this is innocently raised beyond 512, each being 8 bytes, > then the 4k page will be insufficient and it will break. Should we base > MAX_LUNS on our allocation size? Most importantly we need better checks than a BUG_ON when this gets triggered. Second we'll either need to limit our output based on what's available, or make sure we can deal with multiple pages. For REPORT_LUNS the second option shouldn't actually be too hard. > Second, we are allocating the pages, so how could straddling two pages > be an issue? We're not sharing any buffers with the initiator, only the > fabric, potentially. But what about the tcm_loop? I have a hunch all > these changes will have broken it, but maybe we fix it up at the end. For "real" fabrics that is true, but for tcm_loop and vhost which run on the same machine the exact S/G list format is controlled by the initiator. > > > Alternatively the command emulation could have local buffers and copy > > them to the S/G list, which works fine especially for commands with > > small output that can be on the stack. > > Hmm, so this would be pretty close to keeping t_task_buf around > internally, but making sure that fabrics & backstores only dealt with > S/G lists that mapped to the kmalloced t_task_buf? More or less. Except that I wouldn't keep t_task_buf in the task, but local to the emulation. > > This doesn't look correct. We only used to call > > transport_generic_get_mem when SCF_SCSI_DATA_SG_IO_CDB or > > SCF_SCSI_CONTROL_SG_IO_CDB where set here, and now we do it > > undconditionally. Given that we always set > > SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC for these the code does in fact seem > > dead. > > iscsi always sets TO_MEM_NOALLOC, but that's because it calls > map_mem_to_cmd. Other fabrics still rely on the core to alloc memory. > > Regarding the above code, so the 4 cases *were* DATA_SG, CONTROL_SG, > CONTROL_NONSG, and NON_DATA. This patch eliminated CONTROL_NONSG, the > first two already called get_mem, and it's safe to call it for NON_DATA > because data_length is 0 and transport_generic_get_mem will just return > success, so I think this is ok. Indeed it should work for NON_DATA, and we'll still need it for the SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC case which I had missed - I blindly assumed everyone calls transport_generic_map_mem_to_cmd. So I think you're right, and I had the inconsistency of the entry points into the core code even more. I think it's getting time we write some kerneldoc and/or a proper howto for the exported functions. -- 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
