[PATCH 0/2] Fix BUG_ON and hang in ext4_da_writepages() in presence of IO errors

[PATCH 0/2] Fix BUG_ON and hang in ext4_da_writepages() in presence of IO errors

[Jan Kara]

  Hi Ted,

  I've noticed ext4 crashes when ext4_da_writepages() is confronted with IO
errors. The two patches in this series fix the issues for me so now the
system survives when the device is removed from under it. Could you please
apply them? Thanks.

								Honza

[PATCH 1/2] ext4: Fix BUG_ON in ext4_da_block_invalidatepages()

[Jan Kara]

When an allocation of blocks fails in mpage_da_map_and_submit() e.g. because of
EIO we call ext4_da_block_invalidatepages() to invalidate pages we cannot write
but we fail to set mpd->io_done. Thus we continue searching for dirty pages and
add them to the current extent in mpd structure. Eventually we try to allocate
blocks for the extent again and strange things start happening. In particular
if the allocation fails again, we try to invalidate pages again and that
triggers BUG_ON in ext4_da_block_invalidatepages().

Fix the issue by setting mpd->io_done after the pages are invalidated.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9f7f9e4..337d9ca 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2314,6 +2314,7 @@ static void mpage_da_map_and_submit(struct mpage_da_data *mpd)
 		/* invalidate all the pages */
 		ext4_da_block_invalidatepages(mpd, next,
 				mpd->b_size >> mpd->inode->i_blkbits);
+		mpd->io_done = 1;
 		return;
 	}
 	BUG_ON(blks == 0);
-- 
1.7.1

[PATCH 2/2] ext4: Redirty page which could not be added to current extent in __mpage_da_writepage()

[Jan Kara]

When a page cannot be added to current extent in __mpage_da_writepage() we
map current extent and send it for IO. Currently, mpage_da_submit_io() also
redirtied and unlocked this page but it's not clear whether this is just a
lucky accident or a well hidden intent. Actually, we get this wrong in the case
when ext4_map_blocks() fails because of EIO and thus mpage_da_submit_io()
doesn't get called and the page is left locked leading to deadlocks.

Fix the issue by explicitely redirtying and unlocking the page in
__mpage_da_writepage() whenever we see the page could not be added to the
current extent.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 337d9ca..aea9963 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2465,6 +2465,7 @@ static int __mpage_da_writepage(struct page *page,
 		 */
 		if (mpd->next_page != mpd->first_page) {
 			mpage_da_map_and_submit(mpd);
+redirty_page:
 			/*
 			 * skip rest of the page in the page_vec
 			 */
@@ -2477,6 +2478,7 @@ static int __mpage_da_writepage(struct page *page,
 		 * Start next extent of pages ...
 		 */
 		mpd->first_page = page->index;
+		mpd->next_page = page->index + 1;
 
 		/*
 		 * ... and blocks
@@ -2486,7 +2488,6 @@ static int __mpage_da_writepage(struct page *page,
 		mpd->b_blocknr = 0;
 	}
 
-	mpd->next_page = page->index + 1;
 	logical = (sector_t) page->index <<
 		  (PAGE_CACHE_SHIFT - inode->i_blkbits);
 
@@ -2494,7 +2495,7 @@ static int __mpage_da_writepage(struct page *page,
 		mpage_add_bh_to_extent(mpd, logical, PAGE_CACHE_SIZE,
 				       (1 << BH_Dirty) | (1 << BH_Uptodate));
 		if (mpd->io_done)
-			return MPAGE_DA_EXTENT_TAIL;
+			goto redirty_page;
 	} else {
 		/*
 		 * Page with regular buffer heads, just add all dirty ones
@@ -2514,7 +2515,7 @@ static int __mpage_da_writepage(struct page *page,
 						       bh->b_size,
 						       bh->b_state);
 				if (mpd->io_done)
-					return MPAGE_DA_EXTENT_TAIL;
+					goto redirty_page;
 			} else if (buffer_dirty(bh) && (buffer_mapped(bh))) {
 				/*
 				 * mapped dirty buffer. We need to update
@@ -2530,6 +2531,7 @@ static int __mpage_da_writepage(struct page *page,
 			logical++;
 		} while ((bh = bh->b_this_page) != head);
 	}
+	mpd->next_page = page->index + 1;
 
 	return 0;
 }
-- 
1.7.1

Re: [PATCH 1/2] ext4: Fix BUG_ON in ext4_da_block_invalidatepages()

[Curt Wohlgemuth]

Hi Jan:

This problem was fixed by the patch I sent on 18 Feb, titled

   [PATCH 1/2] ext4: mark multi-page IO complete on mapping failure

I guess this didn't make it into 2.6.38, but it's in the ext4 patch queue now.

Thanks,
Curt

On Wed, Mar 16, 2011 at 1:52 PM, Jan Kara <jack@suse.cz> wrote:
> When an allocation of blocks fails in mpage_da_map_and_submit() e.g. because of
> EIO we call ext4_da_block_invalidatepages() to invalidate pages we cannot write
> but we fail to set mpd->io_done. Thus we continue searching for dirty pages and
> add them to the current extent in mpd structure. Eventually we try to allocate
> blocks for the extent again and strange things start happening. In particular
> if the allocation fails again, we try to invalidate pages again and that
> triggers BUG_ON in ext4_da_block_invalidatepages().
>
> Fix the issue by setting mpd->io_done after the pages are invalidated.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/inode.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9f7f9e4..337d9ca 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2314,6 +2314,7 @@ static void mpage_da_map_and_submit(struct mpage_da_data *mpd)
>                /* invalidate all the pages */
>                ext4_da_block_invalidatepages(mpd, next,
>                                mpd->b_size >> mpd->inode->i_blkbits);
> +               mpd->io_done = 1;
>                return;
>        }
>        BUG_ON(blks == 0);
> --
> 1.7.1
>
> --
> 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
>
--
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: [PATCH 1/2] ext4: Fix BUG_ON in ext4_da_block_invalidatepages()

[Jan Kara]

  Hi Curt,

On Wed 16-03-11 14:02:02, Curt Wohlgemuth wrote:
> This problem was fixed by the patch I sent on 18 Feb, titled
> 
>    [PATCH 1/2] ext4: mark multi-page IO complete on mapping failure
> 
> I guess this didn't make it into 2.6.38, but it's in the ext4 patch queue now.
  Ah, OK. And I see you also fixed the second issue although my patch IMHO
still makes sense as a cleanup...

								Honza

> On Wed, Mar 16, 2011 at 1:52 PM, Jan Kara <jack@suse.cz> wrote:
> > When an allocation of blocks fails in mpage_da_map_and_submit() e.g. because of
> > EIO we call ext4_da_block_invalidatepages() to invalidate pages we cannot write
> > but we fail to set mpd->io_done. Thus we continue searching for dirty pages and
> > add them to the current extent in mpd structure. Eventually we try to allocate
> > blocks for the extent again and strange things start happening. In particular
> > if the allocation fails again, we try to invalidate pages again and that
> > triggers BUG_ON in ext4_da_block_invalidatepages().
> >
> > Fix the issue by setting mpd->io_done after the pages are invalidated.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext4/inode.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 9f7f9e4..337d9ca 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -2314,6 +2314,7 @@ static void mpage_da_map_and_submit(struct mpage_da_data *mpd)
> >                /* invalidate all the pages */
> >                ext4_da_block_invalidatepages(mpd, next,
> >                                mpd->b_size >> mpd->inode->i_blkbits);
> > +               mpd->io_done = 1;
> >                return;
> >        }
> >        BUG_ON(blks == 0);
> > --
> > 1.7.1
> >
> > --
> > 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
> >
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
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