Re: Re: Re: [Pv-drivers] 3.7-rc2 regression : file copied to CIFS-mounted directory corrupted

[Jongman Heo <jongman.heo@samsung.com>]

------- Original Message -------
Sender : Jongman Heo<jongman.heo@samsung.com>
Date : 2012-10-24 11:53 (GMT+09:00)
Title : Re: Re: [Pv-drivers] 3.7-rc2 regression : file copied to CIFS-mounted directory corrupted

Hi,

------- Original Message -------
Sender : Eric Dumazet
Date : 2012-10-24 04:39 (GMT+09:00)
Title : Re: [Pv-drivers] 3.7-rc2 regression : file copied to CIFS-mounted directory corrupted

On Tue, 2012-10-23 at 15:50 +0200, Eric Dumazet wrote:

> Only the skb head is handled in the code you copy/pasted.
> 
> You need to generalize that to code in lines ~754
> 
> 
> Then, the number of estimated descriptors is bad :
> 
> /* conservatively estimate # of descriptors to use */
> count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) +
> skb_shinfo(skb)->nr_frags + 1;
> 
> 
> Yes, you need a more precise estimation and vmxnet3_map_pkt() should
> eventually split too big frags.

raw patch would be :

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index ce9d4f2..0ae1bcc 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -744,28 +744,43 @@ vmxnet3_map_pkt(struct sk_buff *skb, struct vmxnet3_tx_ctx *ctx,

for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
+ u32 buf_size;

- tbi = tq->buf_info + tq->tx_ring.next2fill;
- tbi->map_type = VMXNET3_MAP_PAGE;
- tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
- 0, skb_frag_size(frag),
- DMA_TO_DEVICE);
+ buf_offset = 0;
+ len = skb_frag_size(frag);
+ while (len) {
+ tbi = tq->buf_info + tq->tx_ring.next2fill;
+ if (len < VMXNET3_MAX_TX_BUF_SIZE) {
+ buf_size = len;
+ dw2 |= len;
+ } else {
+ buf_size = VMXNET3_MAX_TX_BUF_SIZE;
+ /* spec says that for TxDesc.len, 0 == 2^14 */
+ }
+ tbi->map_type = VMXNET3_MAP_PAGE;
+ tbi->dma_addr = skb_frag_dma_map(&adapter->pdev->dev, frag,
+ buf_offset, buf_size,
+ DMA_TO_DEVICE);

- tbi->len = skb_frag_size(frag);
+ tbi->len = buf_size;

- gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
- BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);
+ gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
+ BUG_ON(gdesc->txd.gen == tq->tx_ring.gen);

- gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
- gdesc->dword[2] = cpu_to_le32(dw2 | skb_frag_size(frag));
- gdesc->dword[3] = 0;
+ gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
+ gdesc->dword[2] = cpu_to_le32(dw2);
+ gdesc->dword[3] = 0;

- dev_dbg(&adapter->netdev->dev,
- "txd[%u]: 0x%llu %u %u\n",
- tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
- le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
- vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
- dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
+ dev_dbg(&adapter->netdev->dev,
+ "txd[%u]: 0x%llu %u %u\n",
+ tq->tx_ring.next2fill, le64_to_cpu(gdesc->txd.addr),
+ le32_to_cpu(gdesc->dword[2]), gdesc->dword[3]);
+ vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
+ dw2 = tq->tx_ring.gen << VMXNET3_TXD_GEN_SHIFT;
+
+ len -= buf_size;
+ buf_offset += buf_size;
+ }
}

ctx->eop_txd = gdesc;
@@ -886,6 +901,18 @@ vmxnet3_prepare_tso(struct sk_buff *skb,
}
}

+static int txd_estimate(const struct sk_buff *skb)
+{
+ int count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) + 1;
+ int i;
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ const struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[i];
+
+ count += VMXNET3_TXD_NEEDED(skb_frag_size(frag));
+ }
+ return count;
+}

/*
  * Transmits a pkt thru a given tq
@@ -914,9 +941,7 @@ vmxnet3_tq_xmit(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
union Vmxnet3_GenericDesc tempTxDesc;
#endif

- /* conservatively estimate # of descriptors to use */
- count = VMXNET3_TXD_NEEDED(skb_headlen(skb)) +
- skb_shinfo(skb)->nr_frags + 1;
+ count = txd_estimate(skb);

ctx.ipv4 = (vlan_get_protocol(skb) == cpu_to_be16(ETH_P_IP));

------- Original Message End -------

Hi, Eric, 
your raw patch seemed to fix the issue. But after ~200 runs, copied file has been corrupted again.

# cmp -l /home/local.bin /mnt/cifs/new.bin | awk '{printf "%08X %02X %02X\n", $1, strtonum(0$2), strtonum(0$3)}' > diff.log

I compared the difference between source and copied file. Size of file is 45872732 bytes (= 0x2BBF65C).
Among them, 4096 (0x1000) bytes are different, from 0x2B96001 to 0x2B97000. Instead of original data, 0x00 was copied in those area.

patch applied on top of 2d1f4c8e ("Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux").

Regards,
Jongman Heo.

------- Original Message End -------

Hi, 

As I said in the previous post, with 2d1f4c8e + Eric's patch, I got the issue mentioned above ; sometimes 4096 bytes are being copied by all zeros. Not easily reproducible, but always happened before running ~500 runs.

But with 3.7-rc3 + the patch, I haven't encountered the issue so far, until ~2000 runs.
Maybe, the root cause is fixed by Eric's patch, and the remaining subtle bug, corrupting 4096 bytes, might be fixed by other commit, between 3.7-rc2 and -rc3.

Thanks,
Jongman Heo.