Skip to contentSkip to conversation

New: EXT4 causes corrupt BitTorrent downloads

Bug 32972 [email protected] 3 participants 14 messages Apr 10, 2011 – Apr 13, 2011 ext4 file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

https://bugzilla.kernel.org/show_bug.cgi?id=32972

Summary: EXT4 causes corrupt BitTorrent downloads
Product: File System
Version: 2.5
Kernel Version: 2.6.39-rc2+
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: ext4
AssignedTo: [email protected]
ReportedBy: [email protected]
CC: [email protected]
Regression: Yes

Using the Transmission BitTorrent client (version 2.11) with the most recent kernel (2.6.39-rc2+), any torrent I try to download is corrupt. By using Transmission's "Verify Local Data" functionality, I am able to reproduce this consistently. During verification, the percent complete will reverse itself as the verification process invalidates pieces that were just downloaded.

I was able to bisect the issue and track it down to this commit:

commit 6de9843dab3f2a1d4d66d80aa9e5782f80977d20

Author: Feng Tang <[email protected]>
Date:   Wed Mar 23 14:05:03 2011 -0400

ext4: remove redundant set_buffer_mapped() in ext4_da_get_block_prep()

The map_bh() call will have already set the buffer_head to mapped.

Signed-off-by Feng Tang <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>
fs/ext4/inode.c +0 -0
index f44307a..dec10e2 100644
@@ -2502,7 +2502,6 @@ static int ext4_da_get_block_prep(struct inode *inode,

sector_t iblock,

                 * for partial write.
                 */
                set_buffer_new(bh);
-               set_buffer_mapped(bh);
        }
        return 0;

}

I confirmed that reverting this commit makes the problem go away.

Thanks,
-Damien

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
#1 lore raw

Ah, it is a wrong fix.

original code:

	map_bh(bh, inode->i_sb, map.m_pblk);
	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

	if (buffer_unwritten(bh)) {
		/* A delayed write to unwritten bh should be marked
		 * new and mapped.  Mapped ensures that we don't do
		 * get_block multiple times when we write to the same
		 * offset and new ensures that we do proper zero out
		 * for partial write.
		 */
		set_buffer_new(bh);
		set_buffer_mapped(bh);
	}

after the patch:

	map_bh(bh, inode->i_sb, map.m_pblk);
	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

	if (buffer_unwritten(bh)) {
		/* A delayed write to unwritten bh should be marked
		 * new and mapped.  Mapped ensures that we don't do
		 * get_block multiple times when we write to the same
		 * offset and new ensures that we do proper zero out
		 * for partial write.
		 */
		set_buffer_new(bh);
	}

Actually, mapped flag is cleared by statement:

	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

So the right code should be：

	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;
	map_bh(bh, inode->i_sb, map.m_pblk);
	if (buffer_unwritten(bh)) {
		/* A delayed write to unwritten bh should be marked
		 * new and mapped.  Mapped ensures that we don't do
		 * get_block multiple times when we write to the same
		 * offset and new ensures that we do proper zero out
		 * for partial write.
		 */
		set_buffer_new(bh);
	}
Quoted 67 lines · On Sun, Apr 10, 2011 at 9:44 PM, <[email protected]> wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=32972

          Summary: EXT4 causes corrupt BitTorrent downloads
          Product: File System
          Version: 2.5
   Kernel Version: 2.6.39-rc2+
         Platform: All
       OS/Version: Linux
             Tree: Mainline
           Status: NEW
         Severity: high
         Priority: P1
        Component: ext4
       AssignedTo: [email protected]
       ReportedBy: [email protected]
               CC: [email protected]
       Regression: Yes

Using the Transmission BitTorrent client (version 2.11) with the most recent kernel (2.6.39-rc2+), any torrent I try to download is corrupt. By using Transmission's "Verify Local Data" functionality, I am able to reproduce this consistently. During verification, the percent complete will reverse itself as the verification process invalidates pieces that were just downloaded.

I was able to bisect the issue and track it down to this commit:

commit 6de9843dab3f2a1d4d66d80aa9e5782f80977d20
Author: Feng Tang <[email protected]>
Date:   Wed Mar 23 14:05:03 2011 -0400

   ext4: remove redundant set_buffer_mapped() in ext4_da_get_block_prep()

   The map_bh() call will have already set the buffer_head to mapped.

Signed-off-by Feng Tang <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>
fs/ext4/inode.c +0 -1
index f44307a..dec10e2 100644
@@ -2502,7 +2502,6 @@ static int ext4_da_get_block_prep(struct inode *inode,
 sector_t iblock,
                 * for partial write.
                 */
                set_buffer_new(bh);
-               set_buffer_mapped(bh);
        }
        return 0;

 }

I confirmed that reverting this commit makes the problem go away.

Thanks,
-Damien

--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 6 lines
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#2 lore raw
Quoted 13 lines · Yongqiang Yang

So the right code should be：

	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;
	map_bh(bh, inode->i_sb, map.m_pblk);
	if (buffer_unwritten(bh)) {
		/* A delayed write to unwritten bh should be marked
		 * new and mapped.  Mapped ensures that we don't do
		 * get_block multiple times when we write to the same
		 * offset and new ensures that we do proper zero out
		 * for partial write.
		 */
		set_buffer_new(bh);
	}

Actually, I'm much more comfortable backing out commit 6de9843da entirely. The above is *not* equivalent to what we had before --- consider the case where ext4_map_blocks returns !EXT4_MAP_MAPPED && !EXT4_MAP_UNWRITTEN.

I don't *think* this should happen in the case where ext4_map_blocks returns a value > 0, but the fact that it's not obvious, means I'd much rather keep things the way that they are. It's not like dropping the set_buffer_mapped(bh) was saving anything measurable anyway....

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 20 lines · Ted Ts'o
Quoted 13 lines · Yongqiang Yang

So the right code should be：

      bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags; map_bh(bh, inode->i_sb, map.m_pblk);
      if (buffer_unwritten(bh)) {
              /* A delayed write to unwritten bh should be marked
               * new and mapped.  Mapped ensures that we don't do
               * get_block multiple times when we write to the same
               * offset and new ensures that we do proper zero out
               * for partial write.
               */
              set_buffer_new(bh);
      }

Actually, I'm much more comfortable backing out commit 6de9843da entirely.  The above is *not* equivalent to what we had before --- consider the case where ext4_map_blocks returns !EXT4_MAP_MAPPED && !EXT4_MAP_UNWRITTEN.

Quoted 4 lines

I don't *think* this should happen in the case where ext4_map_blocks returns a value > 0, but the fact that it's not obvious, means I'd much rather keep things the way that they are.  It's not like dropping the set_buffer_mapped(bh) was saving anything measurable anyway....

Agree. this way, the comment for unwritten case is also much clearer.

Yongqiang

Quoted 3 lines

                                               - Ted

Signature · 6 lines
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#4 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #1 from Anonymous Emailer <[email protected]> 2011-04-10 14:30:37 ---
Reply-To: [email protected]

Ah, it is a wrong fix.

original code:

    map_bh(bh, inode->i_sb, map.m_pblk);
    bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

    if (buffer_unwritten(bh)) {
        /* A delayed write to unwritten bh should be marked
         * new and mapped.  Mapped ensures that we don't do
         * get_block multiple times when we write to the same
         * offset and new ensures that we do proper zero out
         * for partial write.
         */
        set_buffer_new(bh);
        set_buffer_mapped(bh);
    }

after the patch:

    map_bh(bh, inode->i_sb, map.m_pblk);
    bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

    if (buffer_unwritten(bh)) {
        /* A delayed write to unwritten bh should be marked
         * new and mapped.  Mapped ensures that we don't do
         * get_block multiple times when we write to the same
         * offset and new ensures that we do proper zero out
         * for partial write.
         */
        set_buffer_new(bh);
    }

Actually, mapped flag is cleared by statement:

    bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;

So the right code should be：

    bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;
    map_bh(bh, inode->i_sb, map.m_pblk);
    if (buffer_unwritten(bh)) {
        /* A delayed write to unwritten bh should be marked
         * new and mapped.  Mapped ensures that we don't do
         * get_block multiple times when we write to the same
         * offset and new ensures that we do proper zero out
         * for partial write.
         */
        set_buffer_new(bh);
    }
Quoted 67 lines · On Sun, Apr 10, 2011 at 9:44 PM, <[email protected]> wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=32972

          Summary: EXT4 causes corrupt BitTorrent downloads
          Product: File System
          Version: 2.5
   Kernel Version: 2.6.39-rc2+
         Platform: All
       OS/Version: Linux
             Tree: Mainline
           Status: NEW
         Severity: high
         Priority: P1
        Component: ext4
       AssignedTo: [email protected]
       ReportedBy: [email protected]
               CC: [email protected]
       Regression: Yes

Using the Transmission BitTorrent client (version 2.11) with the most recent kernel (2.6.39-rc2+), any torrent I try to download is corrupt. By using Transmission's "Verify Local Data" functionality, I am able to reproduce this consistently. During verification, the percent complete will reverse itself as the verification process invalidates pieces that were just downloaded.

I was able to bisect the issue and track it down to this commit:

commit 6de9843dab3f2a1d4d66d80aa9e5782f80977d20
Author: Feng Tang <[email protected]>
Date:   Wed Mar 23 14:05:03 2011 -0400

   ext4: remove redundant set_buffer_mapped() in ext4_da_get_block_prep()

   The map_bh() call will have already set the buffer_head to mapped.

Signed-off-by Feng Tang <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>
fs/ext4/inode.c +0 -1
index f44307a..dec10e2 100644
@@ -2502,7 +2502,6 @@ static int ext4_da_get_block_prep(struct inode *inode,
 sector_t iblock,
                 * for partial write.
                 */
                set_buffer_new(bh);
-               set_buffer_mapped(bh);
        }
        return 0;

 }

I confirmed that reverting this commit makes the problem go away.

Thanks,
-Damien

--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 6 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#5 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #2 from Damien Grassart <[email protected]> 2011-04-10 16:18:13 --- Hi, thanks for the quick response. I've just tested and confirmed that your fix works.

Also, should that comment inside the conditional be updated (or moved up) as well? Otherwise it seems to be out of sync with the code.

Thanks,
-Damien

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#6 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #3 from Theodore Tso <[email protected]> 2011-04-11 01:46:57 ---

Quoted 13 lines · Yongqiang Yang

So the right code should be：

	bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags;
	map_bh(bh, inode->i_sb, map.m_pblk);
	if (buffer_unwritten(bh)) {
		/* A delayed write to unwritten bh should be marked
		 * new and mapped.  Mapped ensures that we don't do
		 * get_block multiple times when we write to the same
		 * offset and new ensures that we do proper zero out
		 * for partial write.
		 */
		set_buffer_new(bh);
	}

Actually, I'm much more comfortable backing out commit 6de9843da entirely. The above is *not* equivalent to what we had before --- consider the case where ext4_map_blocks returns !EXT4_MAP_MAPPED && !EXT4_MAP_UNWRITTEN.

I don't *think* this should happen in the case where ext4_map_blocks returns a value > 0, but the fact that it's not obvious, means I'd much rather keep things the way that they are. It's not like dropping the set_buffer_mapped(bh) was saving anything measurable anyway....

- Ted

Signature · 6 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#7 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #4 from Yongqiang Yang <[email protected]> 2011-04-11 01:50:38 ---

Quoted 20 lines · Ted Ts'o
Quoted 13 lines · Yongqiang Yang

So the right code should be：

      bh->b_state = (bh->b_state & ~EXT4_MAP_FLAGS) | map.m_flags; map_bh(bh, inode->i_sb, map.m_pblk);
      if (buffer_unwritten(bh)) {
              /* A delayed write to unwritten bh should be marked
               * new and mapped.  Mapped ensures that we don't do
               * get_block multiple times when we write to the same
               * offset and new ensures that we do proper zero out
               * for partial write.
               */
              set_buffer_new(bh);
      }

Actually, I'm much more comfortable backing out commit 6de9843da entirely.  The above is *not* equivalent to what we had before --- consider the case where ext4_map_blocks returns !EXT4_MAP_MAPPED && !EXT4_MAP_UNWRITTEN.

Quoted 4 lines

I don't *think* this should happen in the case where ext4_map_blocks returns a value > 0, but the fact that it's not obvious, means I'd much rather keep things the way that they are.  It's not like dropping the set_buffer_mapped(bh) was saving anything measurable anyway....

Agree. this way, the comment for unwritten case is also much clearer.

Yongqiang

Quoted 3 lines

                                               - Ted

Signature · 6 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#8 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #5 from Feng Tang <[email protected]> 2011-04-11 02:26:43 ---

Quoted 6 lines

Actually, I'm much more comfortable backing out commit 6de9843da entirely. The above is *not* equivalent to what we had before --- consider the case where ext4_map_blocks returns !EXT4_MAP_MAPPED && !EXT4_MAP_UNWRITTEN.

Quoted 6 lines

I don't *think* this should happen in the case where ext4_map_blocks returns a value > 0, but the fact that it's not obvious, means I'd much rather keep things the way that they are. It's not like dropping the set_buffer_mapped(bh) was saving anything measurable anyway....

- Ted

Agree for the revert and sorry for the inconvenience brought by my patch. I walked across the code when debugging a problem, and thought the patch can clean the code a little :(

Thanks,
Feng

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#9 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

Rafael J. Wysocki <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

                 CC|                            |[email protected],
                   |                            |[email protected],
                   |                            |[email protected]
             Blocks|                            |32012

--- Comment #6 from Rafael J. Wysocki <[email protected]> 2011-04-11 23:10:40 --- First-Bad-Commit : 6de9843dab3f2a1d4d66d80aa9e5782f80977d20

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#10 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

--- Comment #7 from Florian Mickler <[email protected]> 2011-04-12 09:20:42 --- A patch referencing this bug report has been merged in v2.6.39-rc3:

commit c8205636029fc869278c55b7336053b3e7ae3ef4

Author: Theodore Ts'o <[email protected]>
Date:   Sun Apr 10 22:30:07 2011 -0400

ext4: fix data corruption regression by reverting commit 6de9843dab3f

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#11 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

Florian Mickler <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

             Status|NEW                         |RESOLVED
         Resolution|                            |CODE_FIX
Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#12 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

Florian Mickler <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |CLOSED

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
author
#13 lore raw

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

In reply to [email protected]

https://bugzilla.kernel.org/show_bug.cgi?id=32972

Giacomo Catenazzi <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

--- Comment #8 from Giacomo Catenazzi <[email protected]> 2011-04-13 19:03:34 --- *** Bug 33112 has been marked as a duplicate of this bug. ***

Signature · 3 lines
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.