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. 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
