diff mbox

Btrfs: fix regression in lock_delalloc_pages

Message ID 1488853256-10867-1-git-send-email-bo.li.liu@oracle.com (mailing list archive)
State Accepted
Headers show

Commit Message

Liu Bo March 7, 2017, 2:20 a.m. UTC
The bug is a regression after commit
(da2c7009f6ca "btrfs: teach __process_pages_contig about PAGE_LOCK operation")
and commit
(76c0021db8fd "Btrfs: use helper to simplify lock/unlock pages").

So if the dirty pages which are under writeback got truncated partially
before we lock the dirty pages, we couldn't find all pages mapping to the
delalloc range, and the bug didn't return an error so it kept going on and
found that the delalloc range got truncated and got to unlock the dirty
pages, and then the ASSERT could caught the error, and showed

-----------------------------------------------------------------------------
assertion failed: page_ops & PAGE_LOCK, file: fs/btrfs/extent_io.c, line: 1716
-----------------------------------------------------------------------------

This fixes the bug by returning the proper -EAGAIN.

Cc: David Sterba <dsterba@suse.com>
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/extent_io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Qu Wenruo March 7, 2017, 5:54 a.m. UTC | #1
At 03/07/2017 10:20 AM, Liu Bo wrote:
> The bug is a regression after commit
> (da2c7009f6ca "btrfs: teach __process_pages_contig about PAGE_LOCK operation")
> and commit
> (76c0021db8fd "Btrfs: use helper to simplify lock/unlock pages").
>
> So if the dirty pages which are under writeback got truncated partially
> before we lock the dirty pages, we couldn't find all pages mapping to the
> delalloc range, and the bug didn't return an error so it kept going on and
> found that the delalloc range got truncated and got to unlock the dirty
> pages, and then the ASSERT could caught the error, and showed
>
> -----------------------------------------------------------------------------
> assertion failed: page_ops & PAGE_LOCK, file: fs/btrfs/extent_io.c, line: 1716
> -----------------------------------------------------------------------------

We also triggered such bug when running xfstests btrfs/070, although 
possibility is somewhat low.

However since it's more about possibility, any fsstress may trigger it 
though.

Thanks,
Qu

>
> This fixes the bug by returning the proper -EAGAIN.
>
> Cc: David Sterba <dsterba@suse.com>
> Reported-by: Dave Jones <davej@codemonkey.org.uk>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/extent_io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 28e8192..8df7974 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1714,7 +1714,8 @@ static int __process_pages_contig(struct address_space *mapping,
>  			 * can we find nothing at @index.
>  			 */
>  			ASSERT(page_ops & PAGE_LOCK);
> -			return ret;
> +			err = -EAGAIN;
> +			goto out;
>  		}
>
>  		for (i = 0; i < ret; i++) {
>


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liu Bo March 7, 2017, 7:55 p.m. UTC | #2
On Tue, Mar 07, 2017 at 01:54:31PM +0800, Qu Wenruo wrote:
> 
> 
> At 03/07/2017 10:20 AM, Liu Bo wrote:
> > The bug is a regression after commit
> > (da2c7009f6ca "btrfs: teach __process_pages_contig about PAGE_LOCK operation")
> > and commit
> > (76c0021db8fd "Btrfs: use helper to simplify lock/unlock pages").
> > 
> > So if the dirty pages which are under writeback got truncated partially
> > before we lock the dirty pages, we couldn't find all pages mapping to the
> > delalloc range, and the bug didn't return an error so it kept going on and
> > found that the delalloc range got truncated and got to unlock the dirty
> > pages, and then the ASSERT could caught the error, and showed
> > 
> > -----------------------------------------------------------------------------
> > assertion failed: page_ops & PAGE_LOCK, file: fs/btrfs/extent_io.c, line: 1716
> > -----------------------------------------------------------------------------
> 
> We also triggered such bug when running xfstests btrfs/070, although
> possibility is somewhat low.
> 
> However since it's more about possibility, any fsstress may trigger it
> though.
>

Yes, I think buffer writes with O_SYNC and some concurrent truncate
could get us to ASSERT.

Thanks,

-liubo

> Thanks,
> Qu
> 
> > 
> > This fixes the bug by returning the proper -EAGAIN.
> > 
> > Cc: David Sterba <dsterba@suse.com>
> > Reported-by: Dave Jones <davej@codemonkey.org.uk>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/extent_io.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 28e8192..8df7974 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -1714,7 +1714,8 @@ static int __process_pages_contig(struct address_space *mapping,
> >  			 * can we find nothing at @index.
> >  			 */
> >  			ASSERT(page_ops & PAGE_LOCK);
> > -			return ret;
> > +			err = -EAGAIN;
> > +			goto out;
> >  		}
> > 
> >  		for (i = 0; i < ret; i++) {
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 28e8192..8df7974 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1714,7 +1714,8 @@  static int __process_pages_contig(struct address_space *mapping,
 			 * can we find nothing at @index.
 			 */
 			ASSERT(page_ops & PAGE_LOCK);
-			return ret;
+			err = -EAGAIN;
+			goto out;
 		}
 
 		for (i = 0; i < ret; i++) {