On Thu, 2012-09-06 at 11:04 +0200, Paolo Bonzini wrote: > Il 06/09/2012 03:58, Nicholas A. Bellinger ha scritto: > >> This patch series fixes this problem by using higher-order allocations > >> to build the data scatterlist. The problem is that iscsi assumes that the > >> scatterlist consists of single pages, which is not true anymore. So > >> patch 2 has to introduce some relatively complicated changes to > >> iscsi_map_iovec and iscsi_unmap_iovec. > > > > So enabling multi-page per SGL support is a feature that has been > > dormant within target core for a long time. It's about time that we > > start taking advantage of it again. ;) > > Yeah, I noticed some preparation for it in tcm_fc/tfc_io.c, though too > late (they look a lot like my iscsi changes, it would have saved me some > time!). > > While this is obviously not to be taken lightly, I disagree with making > this a per-fabric choice. With a properly organized (and bisectable) > series, it should be relatively easy to review and to get right. It's a temporary bit that allows us to figure out which fabrics can (safely) be enabled for multi-page SGLs operation for the short term within for-next code. Unless your prepared to commit to fio+writeverify'ing 8x mainline fabric drivers in many different types of fabric dependent I/O combination for high order allocations, I'd still prefer to have some way to disable this optimization in a per fabric basis if we really need too. That way we can just disable a problematic fabric instead of having to revert the whole thing if users run into problems with a specific fabric module late during the cycle. If the other fabric maintainers are OK with enabling this in their code and give their Reviewed-By's + Tested-By's, then I have no problem dropping this extra bit once everything has been converted. > I looked a bit more closely now and there are no changes needed to other > targets (actually there is a change needed in tcm_qla2xxx, but the code > is currently disabled). > > There are however changes to transport_kmap_data_sg needed and a few > other places. > > I definitely agree with your other comments, including making max_order > a DEF_DEV_ATTRIB. In addition, the default max_order should be capped > based on queue_max_sectors(q) if applicable, to avoid hitting this scenario: > > /* > * XXX: if the length the device accepts is shorter than the > * length of the S/G list entry this will cause and > * endless loop. Better hope no driver uses huge pages. > */ > Mmmmm, indeed. Also, I'm not sure that every old SCSI LLD is smart enough to handle high older allocations -> multi-page SGLs either.. -- 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
