[PATCH] fs-writeback: handle errors in sync_sb_inodes()

[PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Guillaume Chazarain]

Currently it is possible for some errors to be detected at write-back
time but not reported to the program as shown by the following script
using the included make_file.c.

---------8<---------8<---------8<---------8<---------8<---------8<---------
#!/bin/sh

# We binary search the size of a file in 40M filesystem that can cause
# the missed error.
MIN=5000000
MAX=50000000
rm fs.40M
dd if=/dev/zero of=fs.40M bs=40M count=0 seek=1 status=noxfer
#mkfs.ext2 -F fs.40M
mkfs.ext3 -F fs.40M
#mkfs.jfs -q fs.40M
#mkfs.reiserfs -fq fs.40M
#mkfs.xfs fs.40M

attempt()
{
	SIZE=$1
	RES=0
	./make_file valid_file $SIZE
	mount fs.40M /mnt -o loop
	if ! ./make_file /mnt/not_enough_space $SIZE; then
		# We could not create the file as the requested size
		# was clearly too big
		RES=1
	fi
	umount /mnt

	if [ $RES -eq 0 ]; then
		mount fs.40M /mnt -o loop
		if cmp valid_file /mnt/not_enough_space; then
			# The file was too small, it fitted in the filesystem
			RES=-1
		fi
		umount /mnt
	fi

	if [ $RES -eq 0 ]; then
		echo "Undetected ENOSPC with SIZE=$SIZE"
		exit
	fi

	return $RES
}
while [ $((MAX - MIN)) -gt 1 ]; do
	SIZE=$(((MIN + MAX) / 2))
	attempt $SIZE
	RES=$?
	if [ $RES -eq 1 ]; then
		MAX=$SIZE
	else
		MIN=$SIZE
	fi
done

echo "Could not reproduce the problem"

---------8<---------8<---------8<---------8<---------8<---------8<---------
/* make_file.c */
#include <unistd.h>
#include <sys/fcntl.h>
#include <sys/mman.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	int size, fd;
	char *mapping;
	if (argc != 3) {
		fprintf(stderr, "Usage: %s FILE SIZE\n", argv[0]);
		return 1;
	}
	size = atoi(argv[2]);

	fd = open(argv[1], O_RDWR | O_CREAT, 0600);
	if (fd < 0) {
		perror(argv[1]);
		return 1;
	}
	if (ftruncate(fd, size) < 0) {
		perror("ftruncate");
		return 1;
	}
	mapping = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0);
	if (mapping == MAP_FAILED) {
		perror("mmap");
		return 1;
	}
	memset(mapping, 0xFF, size);

	/* Force a write-back */
	sync();

	if (msync(mapping, size, MS_SYNC) < 0) {
		perror("msync");
		return 1;
	}
	if (close(fd) < 0) {
		perror("close");
		return 1;
	}
	printf("%s: successfully written %d bytes\n", argv[1], size);
	return 0;
}
---------8<---------8<---------8<---------8<---------8<---------8<---------
make_file.c mmaps a hole, performs some writeback (memset + sync) and
then expects to find some error code in msync(). The script mounts a
40M loopback filesystem and does a binary search to find the size of
a file big enough to provoke a ENOSPC, but small enough to show the
error not being detected at msync() time.

The error window is large enough for such a size to be quickly found, but with
this patch, no such file size can be found.

All mmap capable filesystems I tested are affected (ext2, ext3,
jfs, reiserfs, xfs). XFS is special in that it survives the test thanks
to the page_mkwrite() work, i.e. it SIGBUS during memset. Anyway, this
behavious solves ENOSPC but does nothing for EIO.

The offending code is in fs/fs-writeback.c:

sync_sb_inodes(...) ()
{
	...
	__writeback_single_inode(inode, wbc);
	...
}
__writeback_single_inode() gets the error from mapping->flags, clears it
and returns it. But sync_sb_inodes() ignores this return value. In -mm
there is sync_sb_inodes-propagate-errors.patch that propagates the
error from __writeback_single_inode upwards in the call stack. IMHO,
this propagation is useless because:

- the error is combined from the errors in all the synced inodes, so it
just tells that some inode in a specific fs got an error,
- nobody in the call stack is interested in this error: certainly not
pdflush, or 'void sync(2)'.

Signed-off-by: Guillaume Chazarain <guichaz@yahoo.fr>
---

 fs/fs-writeback.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 0fca820..88bb3c4 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -417,6 +417,7 @@ sync_sb_inodes(struct super_block *sb, struct writeback_control *wbc)
 		struct address_space *mapping = inode->i_mapping;
 		struct backing_dev_info *bdi = mapping->backing_dev_info;
 		long pages_skipped;
+		int err;
 
 		if (!bdi_cap_writeback_dirty(bdi)) {
 			redirty_tail(inode);
@@ -461,7 +462,8 @@ sync_sb_inodes(struct super_block *sb, struct writeback_control *wbc)
 		BUG_ON(inode->i_state & I_FREEING);
 		__iget(inode);
 		pages_skipped = wbc->pages_skipped;
-		__writeback_single_inode(inode, wbc);
+		err = __writeback_single_inode(inode, wbc);
+		mapping_set_error(mapping, err);
 		if (wbc->sync_mode == WB_SYNC_HOLD) {
 			inode->dirtied_when = jiffies;
 			list_move(&inode->i_list, &sb->s_dirty);

[PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Guillaume Chazarain]

Currently it is possible for some errors to be detected at write-back
time but not reported to the program as shown by the following script
using the included make_file.c. Undetected ENOSPC or EIO can obviously lead to
data corruption.

---------8<---------8<---------8<---------8<---------8<---------8<---------
#!/bin/bash

# We binary search the size of a file in a 40M filesystem that can cause
# the missed error.
MIN=5000000
MAX=50000000

rm fs.40M
dd if=/dev/zero of=fs.40M bs=40M count=0 seek=1 status=noxfer
#mkfs.ext2 -F fs.40M
mkfs.ext3 -F fs.40M
#mkfs.jfs -q fs.40M
#mkfs.reiserfs -fq fs.40M
#mkfs.xfs fs.40M # SIGBUS in memset() with patch
# XFS => OK for ENOSPC, but not for EIO

attempt()
{
	SIZE=$1
	RES=0

	mount fs.40M /mnt -o loop
	if ! ./make_file /mnt/not_enough_space $SIZE; then
		# We could not create the file as the requested size
		# was clearly too big
		echo "/mnt/not_enough_space: not enough space for $SIZE bytes"
		RES=1
	fi
	umount /mnt

	if [ $RES -eq 0 ]; then
		# We think we could create the file, let's see if that's true
		mount fs.40M /mnt -o loop
		cmp <(tr '\000' '\377' < /dev/zero | head -c$SIZE) \
		    /mnt/not_enough_space
		RES=$?
		umount /mnt

		if [ $RES -eq 0 ]; then
			# The file was small enough to fit in the filesystem
			RES=-1
		else
			echo "Undetected ENOSPC with SIZE=$SIZE"
			exit
		fi
	fi

	return $RES
}

while [ $((MAX - MIN)) -gt 1 ]; do
	SIZE=$(((MIN + MAX) / 2))
	attempt $SIZE
	RES=$?
	if [ $RES -eq 1 ]; then
		MAX=$SIZE
	else
		MIN=$SIZE
	fi
done

echo "Could not reproduce the problem"
---------8<---------8<---------8<---------8<---------8<---------8<---------
/* make_file.c */
#include <unistd.h>
#include <sys/fcntl.h>
#include <sys/mman.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	int size, fd;
	char *mapping;
	if (argc != 3) {
		fprintf(stderr, "Usage: %s FILE SIZE\n", argv[0]);
		return 1;
	}
	size = atoi(argv[2]);

	fd = open(argv[1], O_RDWR | O_CREAT, 0600);
	if (fd < 0) {
		perror(argv[1]);
		return 1;
	}
	if (ftruncate(fd, size) < 0) {
		perror("ftruncate");
		return 1;
	}
	mapping = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0);
	if (mapping == MAP_FAILED) {
		perror("mmap");
		return 1;
	}
	memset(mapping, 0xFF, size);

	/* Force a write-back */
	sync();

	if (msync(mapping, size, MS_SYNC) < 0) {
		perror("msync");
		return 1;
	}
	if (close(fd) < 0) {
		perror("close");
		return 1;
	}
	printf("%s: successfully written %d bytes\n", argv[1], size);
	return 0;
}
---------8<---------8<---------8<---------8<---------8<---------8<---------

make_file.c mmaps a hole, performs some writeback (memset + sync) and
then expects to find some error code in msync(). The script mounts a
40M loopback filesystem and does a binary search to find the size of
a file big enough to provoke a ENOSPC, but small enough to show the
error not being detected at msync() time.

The error window is large enough for such a size to be quickly found, but with
this patch, no such file size can be found.

All mmap capable filesystems I tested are affected (ext2, ext3,
jfs, reiserfs, xfs). XFS is special in that it survives the test thanks
to the page_mkwrite() work, i.e. it SIGBUS during memset. Anyway, this XFS
behaviour solves ENOSPC but does nothing for EIO.

The offending code is in fs/fs-writeback.c:

sync_sb_inodes(...)
{
	...
	__writeback_single_inode(inode, wbc);
	...
}

__writeback_single_inode() gets the error from mapping->flags, clears it
and returns it. But sync_sb_inodes() ignores this return value. sync_sb_inodes()
should leave the error in each mapping instead of propagating it through the
call stack for the following reasons:

- the propagated error would combine the errors in all the synced inodes, so it
would just tell that some inode in a specific fs got an error,
- nobody in the call stack would be interested by this error: certainly not
pdflush, or 'void sync(2)'.

Signed-off-by: Guillaume Chazarain <guichaz@gmail.com>
---

 fs/fs-writeback.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 495214d..0e6881e 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
 		struct address_space *mapping = inode->i_mapping;
 		struct backing_dev_info *bdi = mapping->backing_dev_info;
 		long pages_skipped;
+		int err;
 
 		if (!bdi_cap_writeback_dirty(bdi)) {
 			redirty_tail(inode);
@@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
 		BUG_ON(inode->i_state & I_FREEING);
 		__iget(inode);
 		pages_skipped = wbc->pages_skipped;
-		__writeback_single_inode(inode, wbc);
+		err = __writeback_single_inode(inode, wbc);
+		mapping_set_error(mapping, err);
 		if (wbc->sync_mode == WB_SYNC_HOLD) {
 			inode->dirtied_when = jiffies;
 			list_move(&inode->i_list, &sb->s_dirty);

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Andrew Morton]

On Fri, 23 May 2008 22:34:29 +0200 Guillaume Chazarain <guichaz@gmail.com> wrote:

> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 495214d..0e6881e 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
>  		struct address_space *mapping = inode->i_mapping;
>  		struct backing_dev_info *bdi = mapping->backing_dev_info;
>  		long pages_skipped;
> +		int err;
>  
>  		if (!bdi_cap_writeback_dirty(bdi)) {
>  			redirty_tail(inode);
> @@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
>  		BUG_ON(inode->i_state & I_FREEING);
>  		__iget(inode);
>  		pages_skipped = wbc->pages_skipped;
> -		__writeback_single_inode(inode, wbc);
> +		err = __writeback_single_inode(inode, wbc);
> +		mapping_set_error(mapping, err);
>  		if (wbc->sync_mode == WB_SYNC_HOLD) {
>  			inode->dirtied_when = jiffies;
>  			list_move(&inode->i_list, &sb->s_dirty);

ho hum, I've forgotten why I didn't like this.  Let's give it a run.

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Guillaume Chazarain]

On Wed, May 28, 2008 at 7:38 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> ho hum, I've forgotten why I didn't like this.

http://lkml.org/lkml/2007/1/2/238

Hope that helps :-)

-- 
Guillaume

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Dave Chinner]

On Tue, May 27, 2008 at 10:38:32PM -0700, Andrew Morton wrote:
> On Fri, 23 May 2008 22:34:29 +0200 Guillaume Chazarain <guichaz@gmail.com> wrote:
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 495214d..0e6881e 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
> >  		struct address_space *mapping = inode->i_mapping;
> >  		struct backing_dev_info *bdi = mapping->backing_dev_info;
> >  		long pages_skipped;
> > +		int err;
> >  
> >  		if (!bdi_cap_writeback_dirty(bdi)) {
> >  			redirty_tail(inode);
> > @@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
> >  		BUG_ON(inode->i_state & I_FREEING);
> >  		__iget(inode);
> >  		pages_skipped = wbc->pages_skipped;
> > -		__writeback_single_inode(inode, wbc);
> > +		err = __writeback_single_inode(inode, wbc);
> > +		mapping_set_error(mapping, err);
> >  		if (wbc->sync_mode == WB_SYNC_HOLD) {
> >  			inode->dirtied_when = jiffies;
> >  			list_move(&inode->i_list, &sb->s_dirty);
> 
> ho hum, I've forgotten why I didn't like this.  Let's give it a run.

IIRC, it'll produce lots of spurious EIO errors under writeback on
XFS as XFS will return EAGAIN if we've been asked for a non-blocking
flush and we would have blocked....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Andrew Morton]

On Wed, 28 May 2008 18:25:50 +1000 Dave Chinner <david@fromorbit.com> wrote:

> On Tue, May 27, 2008 at 10:38:32PM -0700, Andrew Morton wrote:
> > On Fri, 23 May 2008 22:34:29 +0200 Guillaume Chazarain <guichaz@gmail.com> wrote:
> > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > index 495214d..0e6881e 100644
> > > --- a/fs/fs-writeback.c
> > > +++ b/fs/fs-writeback.c
> > > @@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > >  		struct address_space *mapping = inode->i_mapping;
> > >  		struct backing_dev_info *bdi = mapping->backing_dev_info;
> > >  		long pages_skipped;
> > > +		int err;
> > >  
> > >  		if (!bdi_cap_writeback_dirty(bdi)) {
> > >  			redirty_tail(inode);
> > > @@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > >  		BUG_ON(inode->i_state & I_FREEING);
> > >  		__iget(inode);
> > >  		pages_skipped = wbc->pages_skipped;
> > > -		__writeback_single_inode(inode, wbc);
> > > +		err = __writeback_single_inode(inode, wbc);
> > > +		mapping_set_error(mapping, err);
> > >  		if (wbc->sync_mode == WB_SYNC_HOLD) {
> > >  			inode->dirtied_when = jiffies;
> > >  			list_move(&inode->i_list, &sb->s_dirty);
> > 
> > ho hum, I've forgotten why I didn't like this.  Let's give it a run.
> 
> IIRC, it'll produce lots of spurious EIO errors under writeback on
> XFS as XFS will return EAGAIN if we've been asked for a non-blocking
> flush and we would have blocked....

OIC.  This, I guess...

--- a/fs/fs-writeback.c~fs-writeback-handle-errors-in-sync_sb_inodes-fix
+++ a/fs/fs-writeback.c
@@ -500,7 +500,8 @@ void generic_sync_sb_inodes(struct super
 		__iget(inode);
 		pages_skipped = wbc->pages_skipped;
 		err = __writeback_single_inode(inode, wbc);
-		mapping_set_error(mapping, err);
+		if (err == -EIO || err == -ENOSPC) /* xfs can return -EAGAIN */
+			mapping_set_error(mapping, err);
 		if (wbc->sync_mode == WB_SYNC_HOLD) {
 			inode->dirtied_when = jiffies;
 			list_move(&inode->i_list, &sb->s_dirty);
_

I do so need to find time to work out where this all really has gone wrong.

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Dave Chinner]

On Wed, May 28, 2008 at 01:35:49AM -0700, Andrew Morton wrote:
> On Wed, 28 May 2008 18:25:50 +1000 Dave Chinner <david@fromorbit.com> wrote:
> > On Tue, May 27, 2008 at 10:38:32PM -0700, Andrew Morton wrote:
> > > On Fri, 23 May 2008 22:34:29 +0200 Guillaume Chazarain <guichaz@gmail.com> wrote:
> > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > > index 495214d..0e6881e 100644
> > > > --- a/fs/fs-writeback.c
> > > > +++ b/fs/fs-writeback.c
> > > > @@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > > >  		struct address_space *mapping = inode->i_mapping;
> > > >  		struct backing_dev_info *bdi = mapping->backing_dev_info;
> > > >  		long pages_skipped;
> > > > +		int err;
> > > >  
> > > >  		if (!bdi_cap_writeback_dirty(bdi)) {
> > > >  			redirty_tail(inode);
> > > > @@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > > >  		BUG_ON(inode->i_state & I_FREEING);
> > > >  		__iget(inode);
> > > >  		pages_skipped = wbc->pages_skipped;
> > > > -		__writeback_single_inode(inode, wbc);
> > > > +		err = __writeback_single_inode(inode, wbc);
> > > > +		mapping_set_error(mapping, err);
> > > >  		if (wbc->sync_mode == WB_SYNC_HOLD) {
> > > >  			inode->dirtied_when = jiffies;
> > > >  			list_move(&inode->i_list, &sb->s_dirty);
> > > 
> > > ho hum, I've forgotten why I didn't like this.  Let's give it a run.
> > 
> > IIRC, it'll produce lots of spurious EIO errors under writeback on
> > XFS as XFS will return EAGAIN if we've been asked for a non-blocking
> > flush and we would have blocked....
> 
> OIC.  This, I guess...
> 
> --- a/fs/fs-writeback.c~fs-writeback-handle-errors-in-sync_sb_inodes-fix
> +++ a/fs/fs-writeback.c
> @@ -500,7 +500,8 @@ void generic_sync_sb_inodes(struct super
>  		__iget(inode);
>  		pages_skipped = wbc->pages_skipped;
>  		err = __writeback_single_inode(inode, wbc);
> -		mapping_set_error(mapping, err);
> +		if (err == -EIO || err == -ENOSPC) /* xfs can return -EAGAIN */
> +			mapping_set_error(mapping, err);

Rapidly gets messy. XFS can also return at least EDQUOT and EUCLEAN
(i.e. EFSCORRUPTED) from here as well, and it's likely that there's
other real errors I can't think of right now as well.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs-writeback: handle errors in sync_sb_inodes()

[Andrew Morton]

On Wed, 28 May 2008 18:43:13 +1000 Dave Chinner <david@fromorbit.com> wrote:

> On Wed, May 28, 2008 at 01:35:49AM -0700, Andrew Morton wrote:
> > On Wed, 28 May 2008 18:25:50 +1000 Dave Chinner <david@fromorbit.com> wrote:
> > > On Tue, May 27, 2008 at 10:38:32PM -0700, Andrew Morton wrote:
> > > > On Fri, 23 May 2008 22:34:29 +0200 Guillaume Chazarain <guichaz@gmail.com> wrote:
> > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > > > > index 495214d..0e6881e 100644
> > > > > --- a/fs/fs-writeback.c
> > > > > +++ b/fs/fs-writeback.c
> > > > > @@ -454,6 +454,7 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > > > >  		struct address_space *mapping = inode->i_mapping;
> > > > >  		struct backing_dev_info *bdi = mapping->backing_dev_info;
> > > > >  		long pages_skipped;
> > > > > +		int err;
> > > > >  
> > > > >  		if (!bdi_cap_writeback_dirty(bdi)) {
> > > > >  			redirty_tail(inode);
> > > > > @@ -498,7 +499,8 @@ void generic_sync_sb_inodes(struct super_block *sb,
> > > > >  		BUG_ON(inode->i_state & I_FREEING);
> > > > >  		__iget(inode);
> > > > >  		pages_skipped = wbc->pages_skipped;
> > > > > -		__writeback_single_inode(inode, wbc);
> > > > > +		err = __writeback_single_inode(inode, wbc);
> > > > > +		mapping_set_error(mapping, err);
> > > > >  		if (wbc->sync_mode == WB_SYNC_HOLD) {
> > > > >  			inode->dirtied_when = jiffies;
> > > > >  			list_move(&inode->i_list, &sb->s_dirty);
> > > > 
> > > > ho hum, I've forgotten why I didn't like this.  Let's give it a run.
> > > 
> > > IIRC, it'll produce lots of spurious EIO errors under writeback on
> > > XFS as XFS will return EAGAIN if we've been asked for a non-blocking
> > > flush and we would have blocked....
> > 
> > OIC.  This, I guess...
> > 
> > --- a/fs/fs-writeback.c~fs-writeback-handle-errors-in-sync_sb_inodes-fix
> > +++ a/fs/fs-writeback.c
> > @@ -500,7 +500,8 @@ void generic_sync_sb_inodes(struct super
> >  		__iget(inode);
> >  		pages_skipped = wbc->pages_skipped;
> >  		err = __writeback_single_inode(inode, wbc);
> > -		mapping_set_error(mapping, err);
> > +		if (err == -EIO || err == -ENOSPC) /* xfs can return -EAGAIN */
> > +			mapping_set_error(mapping, err);
> 
> Rapidly gets messy. XFS can also return at least EDQUOT and EUCLEAN
> (i.e. EFSCORRUPTED) from here as well, and it's likely that there's
> other real errors I can't think of right now as well.
> 

argh.  Who said it was allowed to do that :(

Give up.  We (ie: I) need to fix this one properly.