[Bug 32972] New: EXT4 causes corrupt BitTorrent downloads

[Bug 32972] New: EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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: fs_ext4@kernel-bugs.osdl.org
        ReportedBy: damien@grassart.com
                CC: feng.tang@intel.com
        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 <feng.tang@intel.com>
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 <feng.tang@intel.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f44307a..dec10e2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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.

Re: [Bug 32972] New: EXT4 causes corrupt BitTorrent downloads

[Yongqiang Yang]

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);
	}



On Sun, Apr 10, 2011 at 9:44 PM,  <bugzilla-daemon@bugzilla.kernel.org> 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: fs_ext4@kernel-bugs.osdl.org
>        ReportedBy: damien@grassart.com
>                CC: feng.tang@intel.com
>        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 <feng.tang@intel.com>
> 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 <feng.tang@intel.com>
>    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index f44307a..dec10e2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 32972] New: EXT4 causes corrupt BitTorrent downloads

[Ted Ts'o]

On Sun, Apr 10, 2011 at 10:30:13PM +0800, Yongqiang Yang wrote:
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug 32972] New: EXT4 causes corrupt BitTorrent downloads

[Yongqiang Yang]

On Mon, Apr 11, 2011 at 9:46 AM, Ted Ts'o <tytso@mit.edu> wrote:
> On Sun, Apr 10, 2011 at 10:30:13PM +0800, Yongqiang Yang wrote:
>> 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....
Agree.  this way, the comment for unwritten case is also much clearer.

Yongqiang
>
>                                                - Ted
>



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #1 from Anonymous Emailer <anonymous@kernel-bugs.osdl.org>  2011-04-10 14:30:37 ---
Reply-To: xiaoqiangnk@gmail.com

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);
    }



On Sun, Apr 10, 2011 at 9:44 PM,  <bugzilla-daemon@bugzilla.kernel.org> 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: fs_ext4@kernel-bugs.osdl.org
>        ReportedBy: damien@grassart.com
>                CC: feng.tang@intel.com
>        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 <feng.tang@intel.com>
> 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 <feng.tang@intel.com>
>    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index f44307a..dec10e2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #2 from Damien Grassart <damien@grassart.com>  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

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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #3 from Theodore Tso <tytso@mit.edu>  2011-04-11 01:46:57 ---
On Sun, Apr 10, 2011 at 10:30:13PM +0800, Yongqiang Yang wrote:
> 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

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #4 from Yongqiang Yang <xiaoqiangnk@gmail.com>  2011-04-11 01:50:38 ---
On Mon, Apr 11, 2011 at 9:46 AM, Ted Ts'o <tytso@mit.edu> wrote:
> On Sun, Apr 10, 2011 at 10:30:13PM +0800, Yongqiang Yang wrote:
>> 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....
Agree.  this way, the comment for unwritten case is also much clearer.

Yongqiang
>
>                                                - Ted
>

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #5 from Feng Tang <feng.tang@intel.com>  2011-04-11 02:26:43 ---

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

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

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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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


Rafael J. Wysocki <rjw@sisk.pl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |florian@mickler.org,
                   |                            |maciej.rutecki@gmail.com,
                   |                            |rjw@sisk.pl
             Blocks|                            |32012




--- Comment #6 from Rafael J. Wysocki <rjw@sisk.pl>  2011-04-11 23:10:40 ---
First-Bad-Commit : 6de9843dab3f2a1d4d66d80aa9e5782f80977d20

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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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





--- Comment #7 from Florian Mickler <florian@mickler.org>  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 <tytso@mit.edu>
Date:   Sun Apr 10 22:30:07 2011 -0400

    ext4: fix data corruption regression by reverting commit 6de9843dab3f

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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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


Florian Mickler <florian@mickler.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |CODE_FIX




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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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


Florian Mickler <florian@mickler.org> changed:

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




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

[Bug 32972] EXT4 causes corrupt BitTorrent downloads

[bugzilla-daemon]

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


Giacomo Catenazzi <cate@cateee.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cate@cateee.net




--- Comment #8 from Giacomo Catenazzi <cate@cateee.net>  2011-04-13 19:03:34 ---
*** Bug 33112 has been marked as a duplicate of this bug. ***

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