On Tue, Oct 5, 2010 at 7:52 AM, Joe Eykholt <[email protected]> wrote:
> On 9/26/10 6:58 AM, Hillf Danton wrote:
>> The seemingly problematic operation related to fragmented sock buffer
>> is fixed based upon skb_store_bits.
>
> BTW, your clock is wrong, or something delayed this mail from you
> by almost a week. This caused it to get put very deep in my mailbox.
>
> IYou're trying to pad fragmented skbuffs, which
> we never needed to do before because all frames that weren't
> a multiple of 4 bytes were pre-allocated and not fragmented.
>
> If so, you can take out the code in fc_frame_alloc that currently
> allocates and pre-zeros the pad/fill bytes entirely, since this
> code does the padding for you, but you do have the error handling
> problem, which is partially why we did the pre-allocation.
>
> What makes you add this ability to pad fragmented buffers now?
> The only time the length isn't a multiple of 4 is for certain
> CT operations and ELS ECHO responses and for FCP write data
> sometimes (tapes mostly). There may be other cases that I'm
> unaware of. Except for the FCP write case, I think we can
> pre-allocate stuff. In the FCP write case, I think we must
> allocate a padding s/g list element to handle that, and the LLD
> can do that along with any tail it adds (like the FCoE trailer).
> For now, we just don't use s/g lists for write data if the length
> is not a multiple of 4 bytes. The code in fc_fcp_send_data()
> checks that.
>
>> Signed-off-by: Hillf Danton <[email protected]>
>> ---
>>
>> --- o/linux-2.6.36-rc4/drivers/scsi/libfc/fc_libfc.c 2010-09-13
>> 07:07:38.000000000 +0800
>> +++ m/linux-2.6.36-rc4/drivers/scsi/libfc/fc_libfc.c 2010-09-26
>> 21:35:12.000000000 +0800
>> @@ -21,6 +21,7 @@
>> #include <linux/types.h>
>> #include <linux/scatterlist.h>
>> #include <linux/crc32.h>
>> +#include <linux/skbuff.h>
>>
>> #include <scsi/libfc.h>
>> #include <scsi/fc_encode.h>
>> @@ -134,6 +135,34 @@ u32 fc_copy_buffer_to_sglist(void *buf,
>> return copy_len;
>> }
>>
>> +static int fc_pad_frame(struct fc_frame *fp, u8 pad_value, u32 pad_len)
>> +{
>> + struct sk_buff *skb = fp_skb(fp);
>> + int offset = (int)skb->len;
>> + u64 data;
>> + u8 *buf;
>> + int rc;
>> +
>> + skb_put(skb, pad_len);
>
> Note that skb_put() will panic if there isn't room to add the pad,
> or if the frame isn't linear, which is OK, but I think it also
> means we will never return an error.
>
I correct skb_put() in the second version delivered. And the second
version was canceled for posting but too late for delivering, after
rereading fc_frame.c which does nice for alignment of 4 even the
alignment is checked somewhere and padded.
I will take more care before delivering, and thank you for reply.
Hillf
> But, skb_put() returns the address of the data we're putting, and I
> don't think you want to kmalloc it below.
>
> Actually, I think this is pretty broken and the linear case is
> handled correctly by the original code and we don't need to
> be this general. We're only going to pad by 1, 2, or 3 bytes of zeros.
>
>> +
>> + if (pad_len > sizeof(data)) {
>> + buf = kmalloc(pad_len, GFP_KERNEL);
>> + if (unlikely(!buf))
>> + return -ENOMEM;
>> + memset(buf, pad_value, pad_len);
>> + } else {
>> + memset(&data, pad_value, sizeof(data));
>> + buf = (u8 *)&data;
>> + }
>> +
>> + rc = skb_store_bits(skb, offset, buf, (int)pad_len);
>> +
>> + if (buf != (u8 *)&data)
>> + kfree(buf);
>> +
>> + return rc;
>> +}
>> +
>> /**
>> * fc_fill_hdr() - fill FC header fields based on request
>> * @fp: reply frame containing header to be filled in
>> @@ -157,8 +186,10 @@ void fc_fill_hdr(struct fc_frame *fp, co
>> if (f_ctl & FC_FC_END_SEQ) {
>> fill = -fr_len(fp) & 3;
>> if (fill) {
>> - /* TODO, this may be a problem with fragmented skb */
>> - memset(skb_put(fp_skb(fp), fill), 0, fill);
>> + int err = fc_pad_frame(fp, 0, fill);
>> + if (unlikely(err)) {
>> + /* howto report it? */;
>> + }
>> f_ctl |= fill;
>> }
>> fr_eof(fp) = FC_EOF_T;
>> _______________________________________________
>> devel mailing list
>> [email protected]
>> http://www.open-fcoe.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
