James Bottomley wrote:

On Fri, 2008-12-12 at 22:25 +0300, Vladislav Bolkhovitin wrote:

quoted

quoted

One thing that leaps immediately to mind is that you could isolate this
to the net layer by putting it in skb_frag_struct.  However, such a move
would require a proper API for this in net ...

To have net_priv analog in skb was the first idea I was tried. But I 
quickly gave up, because it would required that all the pages in each 
skb_frag_struct be from the same originator, i.e. with the same 
net_priv. It is unpractical to change all the operations with skb's to 
forbid merging them, if they have different net_priv. There are too many 
such places in very not obvious code pieces.

Actually, I said carry in skb_frag_struct not skb ... that allows for
merging of skbs with different page sources.  The API changes would have
to allow setting at this level.

Possibly, you are right, but the amount of changes would be very big, 
while the net_priv patch I did is just 309 lines long including all the 
descriptions and comments. And it's really simple and straightforward.

quoted

quoted

right now it looks like
you're using the struct page addition to carry this information from
SCSI to net, which is a bit of a layering violation.

I don't think there is any layering violation here. Just lower layer 
notifies upper layer that transmission of a page has finished. It's done 
a bit not straightforward, but still basically the same as, for 
instance, on_free_cmd() callbacks which SCST core uses to notify target 
drivers and dev handlers that the corresponding command is about to be 
freed, so they can free associated with it data as well.

The way you transmit the information you want notification is done by a
private tag in struct page, so you're carrying the information on an
object that belongs to neither layer ... that's the violation.  It's
essentially an extension of the net API that goes via the mm layer.

I understand your fears, but I think there can be another view on this. 
There are 2 layers: TCP and iSCSI. ISCSI asks TCP to send data and TCP 
performs that. ISCSI communicates with TCP using sendpage() function, it 
asks TCP to send pages. So, the unit of communications is page. When TCP 
finished transmitting a page, it notifies iSCSI that the page was 
transmitted. Now iSCSI needs to find out its own command, associated 
with that page. It does that using page->net_priv, which keeps pointer 
to the associated iSCSI command. I don't think there is any layer 
violation here, because iSCSI and TCP communicate using pages, not any 
other objects. If they communicated using, e.g., skb, no doubts, it 
would be a layers violation.

But, since iSCSI and TCP communicate using pages, the situation is the 
same as between SCST core and a target driver in the on_free_cmd() 
example I used before. When SCST core notifies the target driver using 
on_free_cmd() callback that a command finished and is about to be freed, 
the target driver also needs to find out its own associated with the 
command data and it does that using tgt_priv pointer in struct scst_cmd. 
Similarly, for instance, struct scsi_cmnd has host_scribble pointer for 
low level drivers.

Thanks,
Vlad