On Mon, Nov 24, 2014 at 01:34:30PM +0800, Jason Wang wrote: > >> + copied = iov_iter_get_pages(from, pages, ~0U, MAX_SKB_FRAGS, &start); > > Why is this condition needed, given we told iov_iter_get_pages() to > > limit to MAX_SKB_FRAGS pages? > > We don't want to send truncated packets and there's no other way to put > those pages since it was not in the frag array. No, his point is that it could never happen. It could, actually - what's confusing here (and that's inherited from zerocopy_from_iovec()) is that 'i' is a lousy name for that variable. It's actually "how many fragments have we already put there?" and it is not reset when we go into the next iteration of outer loop. FWIW, I've just renamed it into 'frag', put if (frag == MAX_SKB_FRAGS) return -EMSGSIZE; *before* iov_iter_get_pages(), passing MAX_SKB_FRAGS - frag as the limit on number of pages in that call. Voila - logics with put_page() disappears and the inner loop is less obfuscated. There was another bug in that function - iov_iter_get_pages() does *not* advance the iterator; the caller needs to do iov_iter_advance() itself. Also fixed... -- 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
