commit b4678df184b causing xfstests regressions

commit b4678df184b causing xfstests regressions

[Theodore Y. Ts'o]

Hi Matthew,

Commit b4678df184b: "errseq: Always report a writeback error once"
appears to be causing xfstests regressions.  For ext4, running
"gce-xfstests -c 4k -g auto" will result in reliable shared/298
failures which go away if I revert b4678df184b.

Darrick has also reported occasional generic/047 failures, which I
have seen at least once as well.  I believe two are linked, because
after instrumenting mke2fs in shared/298, the failure is happening
after creating a new 300 MB file:

dd if=/dev/zero of=$img_file bs=1M count=300 &> /dev/null

creating a new loop device

loop_dev=$(_create_loop_device $img_file)

... and then run mke2fs on that loop device.

The instrumentation of mke2fs shows that the first fsync() on
/dev/loop0 (in lib/ext2fs/closefs.c) which is failing with EIO.

I haven't had a chance to really drill down on it, but I think what is
going on is there is some former test which exercises an error path
(using dm_error, or some such), and somehow the errseq_t for the loop
device isn't getting reset, or the inode for the underlying backing
file, had an unitialized errseq_t.

Can you take a closer look at this?

Thanks,

					- Ted

Re: commit b4678df184b causing xfstests regressions

[Matthew Wilcox]

On Fri, May 18, 2018 at 06:50:37PM -0400, Theodore Y. Ts'o wrote:
> Hi Matthew,
> 
> Commit b4678df184b: "errseq: Always report a writeback error once"
> appears to be causing xfstests regressions.  For ext4, running
> "gce-xfstests -c 4k -g auto" will result in reliable shared/298
> failures which go away if I revert b4678df184b.

Thanks; I'll take a look.  Monday is a holiday in Canada, so it may be
Tuesday before I get to it.

> Darrick has also reported occasional generic/047 failures, which I
> have seen at least once as well.  I believe two are linked, because
> after instrumenting mke2fs in shared/298, the failure is happening
> after creating a new 300 MB file:
> 
> dd if=/dev/zero of=$img_file bs=1M count=300 &> /dev/null
> 
> creating a new loop device
> 
> loop_dev=$(_create_loop_device $img_file)
> 
> ... and then run mke2fs on that loop device.
> 
> The instrumentation of mke2fs shows that the first fsync() on
> /dev/loop0 (in lib/ext2fs/closefs.c) which is failing with EIO.
> 
> I haven't had a chance to really drill down on it, but I think what is
> going on is there is some former test which exercises an error path
> (using dm_error, or some such), and somehow the errseq_t for the loop
> device isn't getting reset, or the inode for the underlying backing
> file, had an unitialized errseq_t.
> 
> Can you take a closer look at this?
> 
> Thanks,
> 
> 					- Ted
> 
> 

Re: commit b4678df184b causing xfstests regressions

[Jeff Layton]

On Fri, 2018-05-18 at 18:50 -0400, Theodore Y. Ts'o wrote:
> Hi Matthew,
> 
> Commit b4678df184b: "errseq: Always report a writeback error once"
> appears to be causing xfstests regressions.  For ext4, running
> "gce-xfstests -c 4k -g auto" will result in reliable shared/298
> failures which go away if I revert b4678df184b.
> 
> Darrick has also reported occasional generic/047 failures, which I
> have seen at least once as well.  I believe two are linked, because
> after instrumenting mke2fs in shared/298, the failure is happening
> after creating a new 300 MB file:
> 
> dd if=/dev/zero of=$img_file bs=1M count=300 &> /dev/null
> 
> creating a new loop device
> 
> loop_dev=$(_create_loop_device $img_file)
> 
> ... and then run mke2fs on that loop device.
> 
> The instrumentation of mke2fs shows that the first fsync() on
> /dev/loop0 (in lib/ext2fs/closefs.c) which is failing with EIO.
> 
> I haven't had a chance to really drill down on it, but I think what is
> going on is there is some former test which exercises an error path
> (using dm_error, or some such), and somehow the errseq_t for the loop
> device isn't getting reset, or the inode for the underlying backing
> file, had an unitialized errseq_t.
> 
> Can you take a closer look at this?
> 
> Thanks,
> 
> 					- Ted
> 

Thanks Ted. I'm not that familiar with the loopdev code, but after
giving it a quick look, I suspect that you're correct. We probably need
to do something like reset the loop device's bd_inode->i_mapping->wb_err 
back to zero when we detach the file that backs it.

I wonder if we could roll a test that would do:

create a scratch fs on a dm-error dev with a file on it
set up a loop device on that file
have the backing device of the scratch file throw errors
write to the device
detach loop device
clear dm-error condition
delete file and recreate it
attach same loop device to new file
fsync loop device

My suspicion is that that last fsync would throw an error now and it
wouldn't have before.
-- 
Jeff Layton <jlayton@kernel.org>

Re: commit b4678df184b causing xfstests regressions

[Darrick J. Wong]

On Sat, May 19, 2018 at 09:09:46AM -0400, Jeff Layton wrote:
> On Fri, 2018-05-18 at 18:50 -0400, Theodore Y. Ts'o wrote:
> > Hi Matthew,
> > 
> > Commit b4678df184b: "errseq: Always report a writeback error once"
> > appears to be causing xfstests regressions.  For ext4, running
> > "gce-xfstests -c 4k -g auto" will result in reliable shared/298
> > failures which go away if I revert b4678df184b.
> > 
> > Darrick has also reported occasional generic/047 failures, which I
> > have seen at least once as well.  I believe two are linked, because
> > after instrumenting mke2fs in shared/298, the failure is happening
> > after creating a new 300 MB file:
> > 
> > dd if=/dev/zero of=$img_file bs=1M count=300 &> /dev/null
> > 
> > creating a new loop device
> > 
> > loop_dev=$(_create_loop_device $img_file)
> > 
> > ... and then run mke2fs on that loop device.
> > 
> > The instrumentation of mke2fs shows that the first fsync() on
> > /dev/loop0 (in lib/ext2fs/closefs.c) which is failing with EIO.
> > 
> > I haven't had a chance to really drill down on it, but I think what is
> > going on is there is some former test which exercises an error path
> > (using dm_error, or some such), and somehow the errseq_t for the loop
> > device isn't getting reset, or the inode for the underlying backing
> > file, had an unitialized errseq_t.
> > 
> > Can you take a closer look at this?
> > 
> > Thanks,
> > 
> > 					- Ted
> > 
> 
> Thanks Ted. I'm not that familiar with the loopdev code, but after
> giving it a quick look, I suspect that you're correct. We probably need
> to do something like reset the loop device's bd_inode->i_mapping->wb_err 
> back to zero when we detach the file that backs it.
> 
> I wonder if we could roll a test that would do:
> 
> create a scratch fs on a dm-error dev with a file on it
> set up a loop device on that file
> have the backing device of the scratch file throw errors
> write to the device
> detach loop device
> clear dm-error condition
> delete file and recreate it
> attach same loop device to new file
> fsync loop device
> 
> My suspicion is that that last fsync would throw an error now and it
> wouldn't have before.

I /think/ it's because inode_init_always doesn't clear mapping->wb_err
(even though it clears mapping->flags) when recycling struct inodes.
Will send patch shortly.

--D

> -- 
> Jeff Layton <jlayton@kernel.org>

[PATCH] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In inode_init_always(), we clear the inode mapping flags, which clears
any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
also clear wb_err, which means that old mapping errors can leak through
to new inodes.

This is crucial for the XFS inode allocation path because we recycle old
in-core inodes and we do not want error state from an old file to leak
into the new file.  This bug was discovered by running generic/036 and
generic/047 in a loop and noticing that the EIOs generated by the
collision of direct and buffered writes in generic/036 would survive the
remount between 036 and 047, and get reported to the fsyncs (on
different files on a reformatted fs!) in generic/047.

Since we're changing the semantics of inode_init_always, we must also
change xfs_reinit_inode to retain the writeback error state when we go
to recover an inode that has been torn down in the vfs but not yet
disposed of by XFS.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/inode.c          |    1 +
 fs/xfs/xfs_icache.c |    2 ++
 2 files changed, 3 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 13ceb98c3bd3..3b55391072f3 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	mapping->a_ops = &empty_aops;
 	mapping->host = inode;
 	mapping->flags = 0;
+	mapping->wb_err = 0;
 	atomic_set(&mapping->i_mmap_writable, 0);
 	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
 	mapping->private_data = NULL;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 164350d91efc..f4d672cb60a4 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -298,6 +298,7 @@ xfs_reinit_inode(
 	uint64_t	version = inode_peek_iversion(inode);
 	umode_t		mode = inode->i_mode;
 	dev_t		dev = inode->i_rdev;
+	errseq_t	old_err = inode->i_mapping->wb_err;
 
 	error = inode_init_always(mp->m_super, inode);
 
@@ -306,6 +307,7 @@ xfs_reinit_inode(
 	inode_set_iversion_queried(inode, version);
 	inode->i_mode = mode;
 	inode->i_rdev = dev;
+	inode->i_mapping->wb_err = old_err;
 	return error;
 }
 

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Matthew Wilcox]

On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not

typo of ENOSPC in case you do a new version

> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files on a reformatted fs!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.

Don't you also need to preserve inode->i_mapping->flags across the
reinitialisation for the users which aren't yet using ->wb_err?

> +++ b/fs/xfs/xfs_icache.c
> @@ -298,6 +298,7 @@ xfs_reinit_inode(
>  	uint64_t	version = inode_peek_iversion(inode);
>  	umode_t		mode = inode->i_mode;
>  	dev_t		dev = inode->i_rdev;
> +	errseq_t	old_err = inode->i_mapping->wb_err;
>  
>  	error = inode_init_always(mp->m_super, inode);
>  
> @@ -306,6 +307,7 @@ xfs_reinit_inode(
>  	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
> +	inode->i_mapping->wb_err = old_err;
>  	return error;
>  }
>  

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Theodore Y. Ts'o]

On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files on a reformatted fs!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

This may fix the generic/047 failure, but alas, it does not address
the shared/298 failure.

Jeff's theory that we may need to clear the errseq_t information after
detaching the loop device makes sense to me; and in the case of the
loop device, we wouldn't be initializing the inode, so your patch
wouldn't do anything about that particular case.

						- Ted

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Sat, 2018-05-19 at 19:19 -0400, Theodore Y. Ts'o wrote:
> On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In inode_init_always(), we clear the inode mapping flags, which clears
> > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > also clear wb_err, which means that old mapping errors can leak through
> > to new inodes.
> > 
> > This is crucial for the XFS inode allocation path because we recycle old
> > in-core inodes and we do not want error state from an old file to leak
> > into the new file.  This bug was discovered by running generic/036 and
> > generic/047 in a loop and noticing that the EIOs generated by the
> > collision of direct and buffered writes in generic/036 would survive the
> > remount between 036 and 047, and get reported to the fsyncs (on
> > different files on a reformatted fs!) in generic/047.
> > 
> > Since we're changing the semantics of inode_init_always, we must also
> > change xfs_reinit_inode to retain the writeback error state when we go
> > to recover an inode that has been torn down in the vfs but not yet
> > disposed of by XFS.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This may fix the generic/047 failure, but alas, it does not address
> the shared/298 failure.
> 
> Jeff's theory that we may need to clear the errseq_t information after
> detaching the loop device makes sense to me; and in the case of the
> loop device, we wouldn't be initializing the inode, so your patch
> wouldn't do anything about that particular case.
> 
> 

I poked at this a bit this morning and found that if I ran generic/361
twice in a row that I'd see a failure similar to what you were seeing.
This patch seems to fix it for me.

Ted, could you test this against your reproducer? If this works for you
I'll plan to flesh out the patch description and get this into -next and
eventually to Linus before the next release.

Thanks,
Jeff

-------------------------8<--------------------

[PATCH] loop: clear wb_err in bd_inode when detaching backing file

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 drivers/block/loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e31655d96..55cf554bc914 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1068,6 +1068,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	if (bdev) {
 		bdput(bdev);
 		invalidate_bdev(bdev);
+		bdev->bd_inode->i_mapping->wb_err = 0;
 	}
 	set_capacity(lo->lo_disk, 0);
 	loop_sysfs_exit(lo);
-- 
2.17.0

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Matthew Wilcox]

On Sun, May 20, 2018 at 07:45:31AM -0400, Jeff Layton wrote:
> [PATCH] loop: clear wb_err in bd_inode when detaching backing file

Is this the right thing to do?  Putting the test-suite aside for the
moment, if I have a loop device on a file and I hit a real error on the
storage backing that file, I don't see why detaching the loop device
from the file should clear the error.

I'm really tempted to say that we should fix the test-suite to consume
the error; once it's been read by at least one reader, it won't go back
to zero, but neither will it show up for new readers.

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Sun, 2018-05-20 at 05:58 -0700, Matthew Wilcox wrote:
> On Sun, May 20, 2018 at 07:45:31AM -0400, Jeff Layton wrote:
> > [PATCH] loop: clear wb_err in bd_inode when detaching backing file
> 
> Is this the right thing to do?  Putting the test-suite aside for the
> moment, if I have a loop device on a file and I hit a real error on the
> storage backing that file, I don't see why detaching the loop device
> from the file should clear the error.
> 
> I'm really tempted to say that we should fix the test-suite to consume
> the error; once it's been read by at least one reader, it won't go back
> to zero, but neither will it show up for new readers.

This is a bit of a grey area, for sure. FWIW, I looked at doing this in
invalidate_bdev, but backed off since I wasn't sure about the other
callers.

I still think it probably is the right thing to do. I'd think that
detaching the backing store is sort of an indicator that you really
don't care about it anymore (including any stored errors concerning it).

OTOH, my track record on predicting what applications care about is
pretty abysmal. I'll go with whatever the consensus is here...
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Theodore Y. Ts'o]

On Sun, May 20, 2018 at 05:58:43AM -0700, Matthew Wilcox wrote:
> On Sun, May 20, 2018 at 07:45:31AM -0400, Jeff Layton wrote:
> > [PATCH] loop: clear wb_err in bd_inode when detaching backing file
> 
> Is this the right thing to do?  Putting the test-suite aside for the
> moment, if I have a loop device on a file and I hit a real error on the
> storage backing that file, I don't see why detaching the loop device
> from the file should clear the error.
> 
> I'm really tempted to say that we should fix the test-suite to consume
> the error; once it's been read by at least one reader, it won't go back
> to zero, but neither will it show up for new readers.

You can't detach the backing store if there are any open file
descriptors (or if there is another loop device keeping the loop
device open, or if there is a file system mounted on it, etc.).

If you can detach the backing store, once you detach the backing
store, the loop device is *gone*.  The user of /dev/loop0 will very
likely be a completely different backing store, so returning an error
simply doesn't make any sense.  There is a very good chance it will be
a completely different backing store, with potentially a different
user, so returning a carried over error is just going confuse, annoy,
and anger the user.....

					- Ted

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Theodore Y. Ts'o]

On Sun, May 20, 2018 at 07:45:31AM -0400, Jeff Layton wrote:
> 
> I poked at this a bit this morning and found that if I ran generic/361
> twice in a row that I'd see a failure similar to what you were seeing.
> This patch seems to fix it for me.
> 
> Ted, could you test this against your reproducer? If this works for you
> I'll plan to flesh out the patch description and get this into -next and
> eventually to Linus before the next release.

Yup, this fixes shared/298 for me.   Many thanks!!

						- Ted

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Matthew Wilcox]

On Sun, May 20, 2018 at 12:29:54PM -0400, Theodore Y. Ts'o wrote:
> On Sun, May 20, 2018 at 05:58:43AM -0700, Matthew Wilcox wrote:
> > On Sun, May 20, 2018 at 07:45:31AM -0400, Jeff Layton wrote:
> > > [PATCH] loop: clear wb_err in bd_inode when detaching backing file
> > 
> > Is this the right thing to do?  Putting the test-suite aside for the
> > moment, if I have a loop device on a file and I hit a real error on the
> > storage backing that file, I don't see why detaching the loop device
> > from the file should clear the error.
> > 
> > I'm really tempted to say that we should fix the test-suite to consume
> > the error; once it's been read by at least one reader, it won't go back
> > to zero, but neither will it show up for new readers.
> 
> You can't detach the backing store if there are any open file
> descriptors (or if there is another loop device keeping the loop
> device open, or if there is a file system mounted on it, etc.).
> 
> If you can detach the backing store, once you detach the backing
> store, the loop device is *gone*.  The user of /dev/loop0 will very
> likely be a completely different backing store, so returning an error
> simply doesn't make any sense.  There is a very good chance it will be
> a completely different backing store, with potentially a different
> user, so returning a carried over error is just going confuse, annoy,
> and anger the user.....

Oh!  I misunderstood.  I thought that bd_inode->i_mapping pointed to
lo_backing_file->f_mapping.

So how about this to preserve the error _in the file with the error_?

	if (bdev) {
		bdput(bdev);
		invalidate_bdev(bdev);
+		filp->f_mapping->wb_err = bdev->bd_inode->i_mapping->wb_err;
+		bdev->bd_inode->i_mapping->wb_err = 0;
	}
	set_capacity(lo->lo_disk, 0);
	loop_sysfs_exit(lo);

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Theodore Y. Ts'o]

On Sun, May 20, 2018 at 12:20:09PM -0700, Matthew Wilcox wrote:
> Oh!  I misunderstood.  I thought that bd_inode->i_mapping pointed to
> lo_backing_file->f_mapping.
> 
> So how about this to preserve the error _in the file with the error_?
> 
> 	if (bdev) {
> 		bdput(bdev);
> 		invalidate_bdev(bdev);
> +		filp->f_mapping->wb_err = bdev->bd_inode->i_mapping->wb_err;
> +		bdev->bd_inode->i_mapping->wb_err = 0;
> 	}
> 	set_capacity(lo->lo_disk, 0);
> 	loop_sysfs_exit(lo);

I don't think it's necessary.  wb_err on the underlying file would
have already been set when the error was set.  So if someone tried
calling fsync(2) on the underlying file, it would have collected the
error already.  Re-setting the error when we detach the loop device
doesn't seem to make any sense.

					- Ted

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Sun, 2018-05-20 at 15:41 -0400, Theodore Y. Ts'o wrote:
> On Sun, May 20, 2018 at 12:20:09PM -0700, Matthew Wilcox wrote:
> > Oh!  I misunderstood.  I thought that bd_inode->i_mapping pointed to
> > lo_backing_file->f_mapping.
> > 
> > So how about this to preserve the error _in the file with the error_?
> > 
> > 	if (bdev) {
> > 		bdput(bdev);
> > 		invalidate_bdev(bdev);
> > +		filp->f_mapping->wb_err = bdev->bd_inode->i_mapping->wb_err;
> > +		bdev->bd_inode->i_mapping->wb_err = 0;
> > 	}
> > 	set_capacity(lo->lo_disk, 0);
> > 	loop_sysfs_exit(lo);
> 
> I don't think it's necessary.  wb_err on the underlying file would
> have already been set when the error was set.  So if someone tried
> calling fsync(2) on the underlying file, it would have collected the
> error already.  Re-setting the error when we detach the loop device
> doesn't seem to make any sense.
> 

(cc'ing linux-block)

Yes, the error would have been set on the original file already. As long
as lo_req_flush or __loop_update_dio weren't called before we detach the
device, it should still be possible to call fsync on the original fd and
get back the error.

As a side note, it looks like __loop_update_dio will discard an error
from vfs_fsync, so certain ioctls against a loop device can cause errors
to be lost. It seems like those ought to get propagated to userland or
to the blockdev's mapping somehow.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Theodore Y. Ts'o]

On Mon, May 21, 2018 at 07:20:05AM -0400, Jeff Layton wrote:
> 
> As a side note, it looks like __loop_update_dio will discard an error
> from vfs_fsync, so certain ioctls against a loop device can cause errors
> to be lost. It seems like those ought to get propagated to userland or
> to the blockdev's mapping somehow.

I'm not sure it's worth it.  All of the ioctls that call
loop_update_dio are used just to configure the loop device.  In
practice they are never called once the loop device is actually
running.  It might be a better choice is to just define that errors
get reset if there is any attempt to reconfigure the loop device.

One of these ioctls change the offset that a loop device maps onto the
backing file.  So for example, while offset 0 of the loop device might
have corresponds beginning of the backing file, after executing the
ioctl, offset 0 of the loop device no corresponds to offset 2MB of the
backing store.  This might happen if we are resetting the loop device
to point at the first partition of a disk image, for example.  I
really don't think that preserving the error status when we are doing
that kind of radical configuration makes sense.

Or for example, when we change the block size of the loop device; if
the underlying backing store is an IBM Mainframe DASD with a 2k block
size, the reason why the error was signalled was because there was an
attempt to write a 1k block onto a 2k block device.  So again,
resetting the error status of the loop device is the right thing to
do.

The final thing that's worth perhaps exploring is whether or not we
need all of these knobs and ioctls.  In particular, LOOP_CHANGE_FD is
used by losetup.  It exists because back in prehistory, some
distribution installer needed it for some obscure corner case.  If I
remember correctly, it had to do with switching out the initramfs
floppy disk with the root file system floppy disk.  We've wasted
developer bandwidth trying to deal with syzbot complaints about that
ioctl, and it's not clear it was worth the effort, because it's not
clear any real code actually _needs_ that ioctl.  It might have been
easier to comment out the ioctl, and see if anyone screamed.

Given that loop device is maintianed mostly on the margins, and it's
not clear that lots of complicated error handling during setup is
really necessary, this is basically a please to keep things simple, or
at least no more complicated than it has to be.

Cheers,

						- Ted

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In inode_init_always(), we clear the inode mapping flags, which clears
> > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> 
> typo of ENOSPC in case you do a new version

Fixed, thanks.

> > also clear wb_err, which means that old mapping errors can leak through
> > to new inodes.
> > 
> > This is crucial for the XFS inode allocation path because we recycle old
> > in-core inodes and we do not want error state from an old file to leak
> > into the new file.  This bug was discovered by running generic/036 and
> > generic/047 in a loop and noticing that the EIOs generated by the
> > collision of direct and buffered writes in generic/036 would survive the
> > remount between 036 and 047, and get reported to the fsyncs (on
> > different files on a reformatted fs!) in generic/047.
> > 
> > Since we're changing the semantics of inode_init_always, we must also
> > change xfs_reinit_inode to retain the writeback error state when we go
> > to recover an inode that has been torn down in the vfs but not yet
> > disposed of by XFS.
> 
> Don't you also need to preserve inode->i_mapping->flags across the
> reinitialisation for the users which aren't yet using ->wb_err?

At first I thought (mistakenly) that xfs had been completely converted,
but digging further we still use the old filemap_check_errors.  It seems
reasonable to me that if we're going to resurrect an incore inode then
we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.

--D

> > +++ b/fs/xfs/xfs_icache.c
> > @@ -298,6 +298,7 @@ xfs_reinit_inode(
> >  	uint64_t	version = inode_peek_iversion(inode);
> >  	umode_t		mode = inode->i_mode;
> >  	dev_t		dev = inode->i_rdev;
> > +	errseq_t	old_err = inode->i_mapping->wb_err;
> >  
> >  	error = inode_init_always(mp->m_super, inode);
> >  
> > @@ -306,6 +307,7 @@ xfs_reinit_inode(
> >  	inode_set_iversion_queried(inode, version);
> >  	inode->i_mode = mode;
> >  	inode->i_rdev = dev;
> > +	inode->i_mapping->wb_err = old_err;
> >  	return error;
> >  }
> >  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In inode_init_always(), we clear the inode mapping flags, which clears
any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
also clear wb_err, which means that old mapping errors can leak through
to new inodes.

This is crucial for the XFS inode allocation path because we recycle old
in-core inodes and we do not want error state from an old file to leak
into the new file.  This bug was discovered by running generic/036 and
generic/047 in a loop and noticing that the EIOs generated by the
collision of direct and buffered writes in generic/036 would survive the
remount between 036 and 047, and get reported to the fsyncs (on
different files!) in generic/047.

Since we're changing the semantics of inode_init_always, we must also
change xfs_reinit_inode to retain the writeback error state when we go
to recover an inode that has been torn down in the vfs but not yet
disposed of by XFS.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
---
 fs/inode.c          |    1 +
 fs/xfs/xfs_icache.c |    9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 13ceb98c3bd3..3b55391072f3 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	mapping->a_ops = &empty_aops;
 	mapping->host = inode;
 	mapping->flags = 0;
+	mapping->wb_err = 0;
 	atomic_set(&mapping->i_mmap_writable, 0);
 	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
 	mapping->private_data = NULL;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 164350d91efc..d01f9544ff01 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -298,6 +298,10 @@ xfs_reinit_inode(
 	uint64_t	version = inode_peek_iversion(inode);
 	umode_t		mode = inode->i_mode;
 	dev_t		dev = inode->i_rdev;
+	errseq_t	old_err = inode->i_mapping->wb_err;
+	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
+	bool		as_enospc = test_bit(AS_ENOSPC,
+					     &inode->i_mapping->flags);
 
 	error = inode_init_always(mp->m_super, inode);
 
@@ -306,6 +310,11 @@ xfs_reinit_inode(
 	inode_set_iversion_queried(inode, version);
 	inode->i_mode = mode;
 	inode->i_rdev = dev;
+	inode->i_mapping->wb_err = old_err;
+	if (as_eio)
+		set_bit(AS_EIO, &inode->i_mapping->flags);
+	if (as_enospc)
+		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
 	return error;
 }
 

Re: [PATCH v2] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Mon, 2018-05-21 at 21:06 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> ---
>  fs/inode.c          |    1 +
>  fs/xfs/xfs_icache.c |    9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 13ceb98c3bd3..3b55391072f3 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>  	mapping->a_ops = &empty_aops;
>  	mapping->host = inode;
>  	mapping->flags = 0;
> +	mapping->wb_err = 0;
>  	atomic_set(&mapping->i_mmap_writable, 0);
>  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>  	mapping->private_data = NULL;
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 164350d91efc..d01f9544ff01 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -298,6 +298,10 @@ xfs_reinit_inode(
>  	uint64_t	version = inode_peek_iversion(inode);
>  	umode_t		mode = inode->i_mode;
>  	dev_t		dev = inode->i_rdev;
> +	errseq_t	old_err = inode->i_mapping->wb_err;
> +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> +	bool		as_enospc = test_bit(AS_ENOSPC,
> +					     &inode->i_mapping->flags);
>  
>  	error = inode_init_always(mp->m_super, inode);
>  
> @@ -306,6 +310,11 @@ xfs_reinit_inode(
>  	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
> +	inode->i_mapping->wb_err = old_err;
> +	if (as_eio)
> +		set_bit(AS_EIO, &inode->i_mapping->flags);
> +	if (as_enospc)
> +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
>  	return error;
>  }
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Mon, 2018-05-21 at 10:54 -0700, Darrick J. Wong wrote:
> On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> > On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > In inode_init_always(), we clear the inode mapping flags, which clears
> > > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > 
> > typo of ENOSPC in case you do a new version
> 
> Fixed, thanks.
> 
> > > also clear wb_err, which means that old mapping errors can leak through
> > > to new inodes.
> > > 
> > > This is crucial for the XFS inode allocation path because we recycle old
> > > in-core inodes and we do not want error state from an old file to leak
> > > into the new file.  This bug was discovered by running generic/036 and
> > > generic/047 in a loop and noticing that the EIOs generated by the
> > > collision of direct and buffered writes in generic/036 would survive the
> > > remount between 036 and 047, and get reported to the fsyncs (on
> > > different files on a reformatted fs!) in generic/047.
> > > 
> > > Since we're changing the semantics of inode_init_always, we must also
> > > change xfs_reinit_inode to retain the writeback error state when we go
> > > to recover an inode that has been torn down in the vfs but not yet
> > > disposed of by XFS.
> > 
> > Don't you also need to preserve inode->i_mapping->flags across the
> > reinitialisation for the users which aren't yet using ->wb_err?
> 
> At first I thought (mistakenly) that xfs had been completely converted,
> but digging further we still use the old filemap_check_errors.  It seems
> reasonable to me that if we're going to resurrect an incore inode then
> we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.
> 
> 

Yes, I think the patch you sent makes sense.

Most of the XFS callers are using filemap_write_and_wait{_range}, and
most of those seem to be called from ioctls (->setattr op being the
notable exception).

We could (in principle) pass a pointer to file->f_wb_err to most of
those callers, and use that to check for errors instead of looking for
AS_EIO/AS_ENOSPC. We wouldn't want to advance the error cursor for those
(as that should only be done in the context of an fsync), but it might
be more reliable than using the flags here.

> > > +++ b/fs/xfs/xfs_icache.c
> > > @@ -298,6 +298,7 @@ xfs_reinit_inode(
> > >  	uint64_t	version = inode_peek_iversion(inode);
> > >  	umode_t		mode = inode->i_mode;
> > >  	dev_t		dev = inode->i_rdev;
> > > +	errseq_t	old_err = inode->i_mapping->wb_err;
> > >  
> > >  	error = inode_init_always(mp->m_super, inode);
> > >  
> > > @@ -306,6 +307,7 @@ xfs_reinit_inode(
> > >  	inode_set_iversion_queried(inode, version);
> > >  	inode->i_mode = mode;
> > >  	inode->i_rdev = dev;
> > > +	inode->i_mapping->wb_err = old_err;
> > >  	return error;
> > >  }
> > >  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] fs: clear writeback errors in inode_init_always

[Brian Foster]

On Mon, May 21, 2018 at 09:06:31PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> ---
>  fs/inode.c          |    1 +
>  fs/xfs/xfs_icache.c |    9 +++++++++
>  2 files changed, 10 insertions(+)
> 
...
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 164350d91efc..d01f9544ff01 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -298,6 +298,10 @@ xfs_reinit_inode(
>  	uint64_t	version = inode_peek_iversion(inode);
>  	umode_t		mode = inode->i_mode;
>  	dev_t		dev = inode->i_rdev;
> +	errseq_t	old_err = inode->i_mapping->wb_err;
> +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> +	bool		as_enospc = test_bit(AS_ENOSPC,
> +					     &inode->i_mapping->flags);
>  
>  	error = inode_init_always(mp->m_super, inode);
>  
> @@ -306,6 +310,11 @@ xfs_reinit_inode(
>  	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
> +	inode->i_mapping->wb_err = old_err;
> +	if (as_eio)
> +		set_bit(AS_EIO, &inode->i_mapping->flags);
> +	if (as_enospc)
> +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);

I'm wondering how safe this is. Can't the associated on-disk inode have
been unlinked and reallocated anew across this kind of reinit of the
in-core ip?

Brian

>  	return error;
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

On Tue, May 22, 2018 at 08:14:01AM -0400, Brian Foster wrote:
> On Mon, May 21, 2018 at 09:06:31PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In inode_init_always(), we clear the inode mapping flags, which clears
> > any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> > also clear wb_err, which means that old mapping errors can leak through
> > to new inodes.
> > 
> > This is crucial for the XFS inode allocation path because we recycle old
> > in-core inodes and we do not want error state from an old file to leak
> > into the new file.  This bug was discovered by running generic/036 and
> > generic/047 in a loop and noticing that the EIOs generated by the
> > collision of direct and buffered writes in generic/036 would survive the
> > remount between 036 and 047, and get reported to the fsyncs (on
> > different files!) in generic/047.
> > 
> > Since we're changing the semantics of inode_init_always, we must also
> > change xfs_reinit_inode to retain the writeback error state when we go
> > to recover an inode that has been torn down in the vfs but not yet
> > disposed of by XFS.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> > ---
> >  fs/inode.c          |    1 +
> >  fs/xfs/xfs_icache.c |    9 +++++++++
> >  2 files changed, 10 insertions(+)
> > 
> ...
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 164350d91efc..d01f9544ff01 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -298,6 +298,10 @@ xfs_reinit_inode(
> >  	uint64_t	version = inode_peek_iversion(inode);
> >  	umode_t		mode = inode->i_mode;
> >  	dev_t		dev = inode->i_rdev;
> > +	errseq_t	old_err = inode->i_mapping->wb_err;
> > +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> > +	bool		as_enospc = test_bit(AS_ENOSPC,
> > +					     &inode->i_mapping->flags);
> >  
> >  	error = inode_init_always(mp->m_super, inode);
> >  
> > @@ -306,6 +310,11 @@ xfs_reinit_inode(
> >  	inode_set_iversion_queried(inode, version);
> >  	inode->i_mode = mode;
> >  	inode->i_rdev = dev;
> > +	inode->i_mapping->wb_err = old_err;
> > +	if (as_eio)
> > +		set_bit(AS_EIO, &inode->i_mapping->flags);
> > +	if (as_enospc)
> > +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
> 
> I'm wondering how safe this is. Can't the associated on-disk inode have
> been unlinked and reallocated anew across this kind of reinit of the
> in-core ip?

Oops, yeah, xfs_ialloc ought to clear those error states unconditionally
when allocating a new on-disk inode.

--D

> Brian
> 
> >  	return error;
> >  }
> >  
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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 fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In inode_init_always(), we clear the inode mapping flags, which clears
any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
also clear wb_err, which means that old mapping errors can leak through
to new inodes.

This is crucial for the XFS inode allocation path because we recycle old
in-core inodes and we do not want error state from an old file to leak
into the new file.  This bug was discovered by running generic/036 and
generic/047 in a loop and noticing that the EIOs generated by the
collision of direct and buffered writes in generic/036 would survive the
remount between 036 and 047, and get reported to the fsyncs (on
different files!) in generic/047.

Since we're changing the semantics of inode_init_always, we must also
change xfs_reinit_inode to retain the writeback error state when we go
to recover an inode that has been torn down in the vfs but not yet
disposed of by XFS.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
---
v3: clear error state when allocating new inode
v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
---
 fs/inode.c          |    1 +
 fs/xfs/xfs_icache.c |    9 +++++++++
 fs/xfs/xfs_inode.c  |    5 +++++
 3 files changed, 15 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 13ceb98c3bd3..3b55391072f3 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	mapping->a_ops = &empty_aops;
 	mapping->host = inode;
 	mapping->flags = 0;
+	mapping->wb_err = 0;
 	atomic_set(&mapping->i_mmap_writable, 0);
 	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
 	mapping->private_data = NULL;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 164350d91efc..d01f9544ff01 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -298,6 +298,10 @@ xfs_reinit_inode(
 	uint64_t	version = inode_peek_iversion(inode);
 	umode_t		mode = inode->i_mode;
 	dev_t		dev = inode->i_rdev;
+	errseq_t	old_err = inode->i_mapping->wb_err;
+	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
+	bool		as_enospc = test_bit(AS_ENOSPC,
+					     &inode->i_mapping->flags);
 
 	error = inode_init_always(mp->m_super, inode);
 
@@ -306,6 +310,11 @@ xfs_reinit_inode(
 	inode_set_iversion_queried(inode, version);
 	inode->i_mode = mode;
 	inode->i_rdev = dev;
+	inode->i_mapping->wb_err = old_err;
+	if (as_eio)
+		set_bit(AS_EIO, &inode->i_mapping->flags);
+	if (as_enospc)
+		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 02eae5059231..6c47ea3e577b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -835,6 +835,11 @@ xfs_ialloc(
 			inode->i_mode |= S_ISGID;
 	}
 
+	/* Reset all vfs error state. */
+	inode->i_mapping->wb_err = 0;
+	clear_bit(AS_EIO, &inode->i_mapping->flags);
+	clear_bit(AS_ENOSPC, &inode->i_mapping->flags);
+
 	/*
 	 * If the group ID of the new file does not match the effective group
 	 * ID or one of the supplementary group IDs, the S_ISGID bit is cleared

Re: [PATCH v3] fs: clear writeback errors in inode_init_always

[Brian Foster]

On Tue, May 22, 2018 at 09:43:59AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> ---
> v3: clear error state when allocating new inode
> v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> ---
>  fs/inode.c          |    1 +
>  fs/xfs/xfs_icache.c |    9 +++++++++
>  fs/xfs/xfs_inode.c  |    5 +++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 13ceb98c3bd3..3b55391072f3 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>  	mapping->a_ops = &empty_aops;
>  	mapping->host = inode;
>  	mapping->flags = 0;
> +	mapping->wb_err = 0;
>  	atomic_set(&mapping->i_mmap_writable, 0);
>  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>  	mapping->private_data = NULL;
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 164350d91efc..d01f9544ff01 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -298,6 +298,10 @@ xfs_reinit_inode(
>  	uint64_t	version = inode_peek_iversion(inode);
>  	umode_t		mode = inode->i_mode;
>  	dev_t		dev = inode->i_rdev;
> +	errseq_t	old_err = inode->i_mapping->wb_err;
> +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> +	bool		as_enospc = test_bit(AS_ENOSPC,
> +					     &inode->i_mapping->flags);
>  
>  	error = inode_init_always(mp->m_super, inode);
>  
> @@ -306,6 +310,11 @@ xfs_reinit_inode(
>  	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
> +	inode->i_mapping->wb_err = old_err;
> +	if (as_eio)
> +		set_bit(AS_EIO, &inode->i_mapping->flags);
> +	if (as_enospc)
> +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 02eae5059231..6c47ea3e577b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -835,6 +835,11 @@ xfs_ialloc(
>  			inode->i_mode |= S_ISGID;
>  	}
>  
> +	/* Reset all vfs error state. */

I'd prefer a comment that reminds why we have to do this. E.g.,
something like:

	/*
	 * In-core inode reinit carries over vfs error state. Reset it
	 * so we don't leak error state from a previous generation of
	 * the inode.
	 */

... but feel free to reword that.

> +	inode->i_mapping->wb_err = 0;
> +	clear_bit(AS_EIO, &inode->i_mapping->flags);
> +	clear_bit(AS_ENOSPC, &inode->i_mapping->flags);
> +

I also wonder whether it might be cleaner to just reset this stuff in
xfs_ifree() where we kill the i_mode and bump the gen and whatnot, but
afaict this should work too. With the comment fix, at least:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	/*
>  	 * If the group ID of the new file does not match the effective group
>  	 * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

On Tue, May 22, 2018 at 02:40:09PM -0400, Brian Foster wrote:
> On Tue, May 22, 2018 at 09:43:59AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In inode_init_always(), we clear the inode mapping flags, which clears
> > any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> > also clear wb_err, which means that old mapping errors can leak through
> > to new inodes.
> > 
> > This is crucial for the XFS inode allocation path because we recycle old
> > in-core inodes and we do not want error state from an old file to leak
> > into the new file.  This bug was discovered by running generic/036 and
> > generic/047 in a loop and noticing that the EIOs generated by the
> > collision of direct and buffered writes in generic/036 would survive the
> > remount between 036 and 047, and get reported to the fsyncs (on
> > different files!) in generic/047.
> > 
> > Since we're changing the semantics of inode_init_always, we must also
> > change xfs_reinit_inode to retain the writeback error state when we go
> > to recover an inode that has been torn down in the vfs but not yet
> > disposed of by XFS.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > v3: clear error state when allocating new inode
> > v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> > ---
> >  fs/inode.c          |    1 +
> >  fs/xfs/xfs_icache.c |    9 +++++++++
> >  fs/xfs/xfs_inode.c  |    5 +++++
> >  3 files changed, 15 insertions(+)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 13ceb98c3bd3..3b55391072f3 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> >  	mapping->a_ops = &empty_aops;
> >  	mapping->host = inode;
> >  	mapping->flags = 0;
> > +	mapping->wb_err = 0;
> >  	atomic_set(&mapping->i_mmap_writable, 0);
> >  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
> >  	mapping->private_data = NULL;
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 164350d91efc..d01f9544ff01 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -298,6 +298,10 @@ xfs_reinit_inode(
> >  	uint64_t	version = inode_peek_iversion(inode);
> >  	umode_t		mode = inode->i_mode;
> >  	dev_t		dev = inode->i_rdev;
> > +	errseq_t	old_err = inode->i_mapping->wb_err;
> > +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> > +	bool		as_enospc = test_bit(AS_ENOSPC,
> > +					     &inode->i_mapping->flags);
> >  
> >  	error = inode_init_always(mp->m_super, inode);
> >  
> > @@ -306,6 +310,11 @@ xfs_reinit_inode(
> >  	inode_set_iversion_queried(inode, version);
> >  	inode->i_mode = mode;
> >  	inode->i_rdev = dev;
> > +	inode->i_mapping->wb_err = old_err;
> > +	if (as_eio)
> > +		set_bit(AS_EIO, &inode->i_mapping->flags);
> > +	if (as_enospc)
> > +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
> >  	return error;
> >  }
> >  
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 02eae5059231..6c47ea3e577b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -835,6 +835,11 @@ xfs_ialloc(
> >  			inode->i_mode |= S_ISGID;
> >  	}
> >  
> > +	/* Reset all vfs error state. */
> 
> I'd prefer a comment that reminds why we have to do this. E.g.,
> something like:
> 
> 	/*
> 	 * In-core inode reinit carries over vfs error state. Reset it
> 	 * so we don't leak error state from a previous generation of
> 	 * the inode.
> 	 */

I settled on:

	/*
	 * In-core inode reinit does not clear vfs error state.  Reset
	 * it so we don't leak error state from a previous generation of
	 * the inode.
	 */

> 
> ... but feel free to reword that.
> 
> > +	inode->i_mapping->wb_err = 0;
> > +	clear_bit(AS_EIO, &inode->i_mapping->flags);
> > +	clear_bit(AS_ENOSPC, &inode->i_mapping->flags);
> > +
> 
> I also wonder whether it might be cleaner to just reset this stuff in
> xfs_ifree() where we kill the i_mode and bump the gen and whatnot, but
> afaict this should work too. With the comment fix, at least:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for the review!

--D

> 
> >  	/*
> >  	 * If the group ID of the new file does not match the effective group
> >  	 * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] fs: clear writeback errors in inode_init_always

[Dave Chinner]

On Tue, May 22, 2018 at 09:43:59AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In inode_init_always(), we clear the inode mapping flags, which clears
> any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> also clear wb_err, which means that old mapping errors can leak through
> to new inodes.
> 
> This is crucial for the XFS inode allocation path because we recycle old
> in-core inodes and we do not want error state from an old file to leak
> into the new file.  This bug was discovered by running generic/036 and
> generic/047 in a loop and noticing that the EIOs generated by the
> collision of direct and buffered writes in generic/036 would survive the
> remount between 036 and 047, and get reported to the fsyncs (on
> different files!) in generic/047.
> 
> Since we're changing the semantics of inode_init_always, we must also
> change xfs_reinit_inode to retain the writeback error state when we go
> to recover an inode that has been torn down in the vfs but not yet
> disposed of by XFS.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> ---
> v3: clear error state when allocating new inode
> v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> ---
>  fs/inode.c          |    1 +
>  fs/xfs/xfs_icache.c |    9 +++++++++
>  fs/xfs/xfs_inode.c  |    5 +++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 13ceb98c3bd3..3b55391072f3 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>  	mapping->a_ops = &empty_aops;
>  	mapping->host = inode;
>  	mapping->flags = 0;
> +	mapping->wb_err = 0;
>  	atomic_set(&mapping->i_mmap_writable, 0);
>  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>  	mapping->private_data = NULL;
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 164350d91efc..d01f9544ff01 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -298,6 +298,10 @@ xfs_reinit_inode(
>  	uint64_t	version = inode_peek_iversion(inode);
>  	umode_t		mode = inode->i_mode;
>  	dev_t		dev = inode->i_rdev;
> +	errseq_t	old_err = inode->i_mapping->wb_err;
> +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> +	bool		as_enospc = test_bit(AS_ENOSPC,
> +					     &inode->i_mapping->flags);
>  
>  	error = inode_init_always(mp->m_super, inode);
>  
> @@ -306,6 +310,11 @@ xfs_reinit_inode(
>  	inode_set_iversion_queried(inode, version);
>  	inode->i_mode = mode;
>  	inode->i_rdev = dev;
> +	inode->i_mapping->wb_err = old_err;
> +	if (as_eio)
> +		set_bit(AS_EIO, &inode->i_mapping->flags);
> +	if (as_enospc)
> +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);

I just don't see this as valid.

The inode has already been removed from the VFS cache when this
happens (i.e. it's gone through iput_final()->evict->destroy_inode)
and so any VFS-specific state is already invalid.

We're retaining XFS-specific inode information across the recycling
of the xfs_inode (i.e. stuff that doesn't change *on-disk* across
recycling an inode, but is reset by inode_init_always()). This is a
creating a new life cycle for the VFS inode (i.e. I_NEW is set), so 
*there is no VFS state retained* across this operation from it's
previous life.

Trying to do this is just going to lead us into nasty layering
violations where the VFS state has to be hacked around just in case
this recycling of the previous life cycle's state was the wrong
thing to do.

> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 02eae5059231..6c47ea3e577b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -835,6 +835,11 @@ xfs_ialloc(
>  			inode->i_mode |= S_ISGID;
>  	}
>  
> +	/* Reset all vfs error state. */
> +	inode->i_mapping->wb_err = 0;
> +	clear_bit(AS_EIO, &inode->i_mapping->flags);
> +	clear_bit(AS_ENOSPC, &inode->i_mapping->flags);

Just like this. This is a sure sign we're doing the wrong thing in
xfs_reinit_inode(). VFS inode lifecycle state is trashed at
->destroy_inode. It does not persist into new instantiations of VFS
inode state, regardless of that new inode lifecycle gets to the
I_NEW state...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Dave Chinner]

On Tue, May 22, 2018 at 06:30:40AM -0400, Jeff Layton wrote:
> On Mon, 2018-05-21 at 10:54 -0700, Darrick J. Wong wrote:
> > On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> > > On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > In inode_init_always(), we clear the inode mapping flags, which clears
> > > > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > > 
> > > typo of ENOSPC in case you do a new version
> > 
> > Fixed, thanks.
> > 
> > > > also clear wb_err, which means that old mapping errors can leak through
> > > > to new inodes.
> > > > 
> > > > This is crucial for the XFS inode allocation path because we recycle old
> > > > in-core inodes and we do not want error state from an old file to leak
> > > > into the new file.  This bug was discovered by running generic/036 and
> > > > generic/047 in a loop and noticing that the EIOs generated by the
> > > > collision of direct and buffered writes in generic/036 would survive the
> > > > remount between 036 and 047, and get reported to the fsyncs (on
> > > > different files on a reformatted fs!) in generic/047.
> > > > 
> > > > Since we're changing the semantics of inode_init_always, we must also
> > > > change xfs_reinit_inode to retain the writeback error state when we go
> > > > to recover an inode that has been torn down in the vfs but not yet
> > > > disposed of by XFS.
> > > 
> > > Don't you also need to preserve inode->i_mapping->flags across the
> > > reinitialisation for the users which aren't yet using ->wb_err?
> > 
> > At first I thought (mistakenly) that xfs had been completely converted,
> > but digging further we still use the old filemap_check_errors.  It seems
> > reasonable to me that if we're going to resurrect an incore inode then
> > we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.
> > 
> > 
> 
> Yes, I think the patch you sent makes sense.
> 
> Most of the XFS callers are using filemap_write_and_wait{_range}, and
> most of those seem to be called from ioctls (->setattr op being the
> notable exception).
> 
> We could (in principle) pass a pointer to file->f_wb_err to most of
> those callers, and use that to check for errors instead of looking for
> AS_EIO/AS_ENOSPC. We wouldn't want to advance the error cursor for those
> (as that should only be done in the context of an fsync), but it might
> be more reliable than using the flags here.

Why wouldn't we advance the error pointer? The data writeback error
caused an operation to fail and the error has been reported to
userspace, so how is that different to fsync() failing and reporting
the error to userspace?

This seems like a slippery slope of inconsistency to me, where data
writeback errors are only reported once on fsync(), but can be
reported multiple times through different filesystem operations...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3] fs: clear writeback errors in inode_init_always

[Darrick J. Wong]

On Wed, May 23, 2018 at 08:05:52AM +1000, Dave Chinner wrote:
> On Tue, May 22, 2018 at 09:43:59AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In inode_init_always(), we clear the inode mapping flags, which clears
> > any retained error (AS_EIO, AS_ENOSPC) bits.  Unfortunately, we do not
> > also clear wb_err, which means that old mapping errors can leak through
> > to new inodes.
> > 
> > This is crucial for the XFS inode allocation path because we recycle old
> > in-core inodes and we do not want error state from an old file to leak
> > into the new file.  This bug was discovered by running generic/036 and
> > generic/047 in a loop and noticing that the EIOs generated by the
> > collision of direct and buffered writes in generic/036 would survive the
> > remount between 036 and 047, and get reported to the fsyncs (on
> > different files!) in generic/047.
> > 
> > Since we're changing the semantics of inode_init_always, we must also
> > change xfs_reinit_inode to retain the writeback error state when we go
> > to recover an inode that has been torn down in the vfs but not yet
> > disposed of by XFS.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > v3: clear error state when allocating new inode
> > v2: retain AS_EIO/AS_ENOSPC across xfs inode reinit
> > ---
> >  fs/inode.c          |    1 +
> >  fs/xfs/xfs_icache.c |    9 +++++++++
> >  fs/xfs/xfs_inode.c  |    5 +++++
> >  3 files changed, 15 insertions(+)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 13ceb98c3bd3..3b55391072f3 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -178,6 +178,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> >  	mapping->a_ops = &empty_aops;
> >  	mapping->host = inode;
> >  	mapping->flags = 0;
> > +	mapping->wb_err = 0;
> >  	atomic_set(&mapping->i_mmap_writable, 0);
> >  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
> >  	mapping->private_data = NULL;
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 164350d91efc..d01f9544ff01 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -298,6 +298,10 @@ xfs_reinit_inode(
> >  	uint64_t	version = inode_peek_iversion(inode);
> >  	umode_t		mode = inode->i_mode;
> >  	dev_t		dev = inode->i_rdev;
> > +	errseq_t	old_err = inode->i_mapping->wb_err;
> > +	bool		as_eio = test_bit(AS_EIO, &inode->i_mapping->flags);
> > +	bool		as_enospc = test_bit(AS_ENOSPC,
> > +					     &inode->i_mapping->flags);
> >  
> >  	error = inode_init_always(mp->m_super, inode);
> >  
> > @@ -306,6 +310,11 @@ xfs_reinit_inode(
> >  	inode_set_iversion_queried(inode, version);
> >  	inode->i_mode = mode;
> >  	inode->i_rdev = dev;
> > +	inode->i_mapping->wb_err = old_err;
> > +	if (as_eio)
> > +		set_bit(AS_EIO, &inode->i_mapping->flags);
> > +	if (as_enospc)
> > +		set_bit(AS_ENOSPC, &inode->i_mapping->flags);
> 
> I just don't see this as valid.
> 
> The inode has already been removed from the VFS cache when this
> happens (i.e. it's gone through iput_final()->evict->destroy_inode)
> and so any VFS-specific state is already invalid.
> 
> We're retaining XFS-specific inode information across the recycling
> of the xfs_inode (i.e. stuff that doesn't change *on-disk* across
> recycling an inode, but is reset by inode_init_always()). This is a
> creating a new life cycle for the VFS inode (i.e. I_NEW is set), so 
> *there is no VFS state retained* across this operation from it's
> previous life.
> 
> Trying to do this is just going to lead us into nasty layering
> violations where the VFS state has to be hacked around just in case
> this recycling of the previous life cycle's state was the wrong
> thing to do.
> 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 02eae5059231..6c47ea3e577b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -835,6 +835,11 @@ xfs_ialloc(
> >  			inode->i_mode |= S_ISGID;
> >  	}
> >  
> > +	/* Reset all vfs error state. */
> > +	inode->i_mapping->wb_err = 0;
> > +	clear_bit(AS_EIO, &inode->i_mapping->flags);
> > +	clear_bit(AS_ENOSPC, &inode->i_mapping->flags);
> 
> Just like this. This is a sure sign we're doing the wrong thing in
> xfs_reinit_inode(). VFS inode lifecycle state is trashed at
> ->destroy_inode. It does not persist into new instantiations of VFS
> inode state, regardless of that new inode lifecycle gets to the
> I_NEW state...

(Just reiterating a conversation we had on irc...)

So basically as far as the vfs is concerned the inode is gone and done
and destroyed, and it has no expectation of state being preserved.
Therefore, this patch can be trimmed to the inode_init_always part.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Jeff Layton]

On Wed, 2018-05-23 at 08:09 +1000, Dave Chinner wrote:
> On Tue, May 22, 2018 at 06:30:40AM -0400, Jeff Layton wrote:
> > On Mon, 2018-05-21 at 10:54 -0700, Darrick J. Wong wrote:
> > > On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> > > > On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > In inode_init_always(), we clear the inode mapping flags, which clears
> > > > > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > > > 
> > > > typo of ENOSPC in case you do a new version
> > > 
> > > Fixed, thanks.
> > > 
> > > > > also clear wb_err, which means that old mapping errors can leak through
> > > > > to new inodes.
> > > > > 
> > > > > This is crucial for the XFS inode allocation path because we recycle old
> > > > > in-core inodes and we do not want error state from an old file to leak
> > > > > into the new file.  This bug was discovered by running generic/036 and
> > > > > generic/047 in a loop and noticing that the EIOs generated by the
> > > > > collision of direct and buffered writes in generic/036 would survive the
> > > > > remount between 036 and 047, and get reported to the fsyncs (on
> > > > > different files on a reformatted fs!) in generic/047.
> > > > > 
> > > > > Since we're changing the semantics of inode_init_always, we must also
> > > > > change xfs_reinit_inode to retain the writeback error state when we go
> > > > > to recover an inode that has been torn down in the vfs but not yet
> > > > > disposed of by XFS.
> > > > 
> > > > Don't you also need to preserve inode->i_mapping->flags across the
> > > > reinitialisation for the users which aren't yet using ->wb_err?
> > > 
> > > At first I thought (mistakenly) that xfs had been completely converted,
> > > but digging further we still use the old filemap_check_errors.  It seems
> > > reasonable to me that if we're going to resurrect an incore inode then
> > > we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.
> > > 
> > > 
> > 
> > Yes, I think the patch you sent makes sense.
> > 
> > Most of the XFS callers are using filemap_write_and_wait{_range}, and
> > most of those seem to be called from ioctls (->setattr op being the
> > notable exception).
> > 
> > We could (in principle) pass a pointer to file->f_wb_err to most of
> > those callers, and use that to check for errors instead of looking for
> > AS_EIO/AS_ENOSPC. We wouldn't want to advance the error cursor for those
> > (as that should only be done in the context of an fsync), but it might
> > be more reliable than using the flags here.
> 
> Why wouldn't we advance the error pointer? The data writeback error
> caused an operation to fail and the error has been reported to
> userspace, so how is that different to fsync() failing and reporting
> the error to userspace?
> 
> This seems like a slippery slope of inconsistency to me, where data
> writeback errors are only reported once on fsync(), but can be
> reported multiple times through different filesystem operations...
> 

If I get back an error on ioctl, why should I think that that has
anything to do with writeback? For example, suppose I do:

write = 0
ioctl = -EIO
fsync = 0

The ioctl returns an error but the fsync returns 0. It would be
reasonable to assume that my writes had made it to disk.

I think that we really do want to ensure that fsync always returns an
error if writeback failed since the last fsync. That was sort of the
point of the errseq_t work in the first place.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] fs: clear writeback errors in inode_init_always

[Dave Chinner]

On Wed, May 23, 2018 at 06:56:00AM -0400, Jeff Layton wrote:
> On Wed, 2018-05-23 at 08:09 +1000, Dave Chinner wrote:
> > On Tue, May 22, 2018 at 06:30:40AM -0400, Jeff Layton wrote:
> > > On Mon, 2018-05-21 at 10:54 -0700, Darrick J. Wong wrote:
> > > > On Sat, May 19, 2018 at 08:36:19AM -0700, Matthew Wilcox wrote:
> > > > > On Sat, May 19, 2018 at 08:27:00AM -0700, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > 
> > > > > > In inode_init_always(), we clear the inode mapping flags, which clears
> > > > > > any retained error (AS_EIO, AS_ENOSC) bits.  Unfortunately, we do not
> > > > > 
> > > > > typo of ENOSPC in case you do a new version
> > > > 
> > > > Fixed, thanks.
> > > > 
> > > > > > also clear wb_err, which means that old mapping errors can leak through
> > > > > > to new inodes.
> > > > > > 
> > > > > > This is crucial for the XFS inode allocation path because we recycle old
> > > > > > in-core inodes and we do not want error state from an old file to leak
> > > > > > into the new file.  This bug was discovered by running generic/036 and
> > > > > > generic/047 in a loop and noticing that the EIOs generated by the
> > > > > > collision of direct and buffered writes in generic/036 would survive the
> > > > > > remount between 036 and 047, and get reported to the fsyncs (on
> > > > > > different files on a reformatted fs!) in generic/047.
> > > > > > 
> > > > > > Since we're changing the semantics of inode_init_always, we must also
> > > > > > change xfs_reinit_inode to retain the writeback error state when we go
> > > > > > to recover an inode that has been torn down in the vfs but not yet
> > > > > > disposed of by XFS.
> > > > > 
> > > > > Don't you also need to preserve inode->i_mapping->flags across the
> > > > > reinitialisation for the users which aren't yet using ->wb_err?
> > > > 
> > > > At first I thought (mistakenly) that xfs had been completely converted,
> > > > but digging further we still use the old filemap_check_errors.  It seems
> > > > reasonable to me that if we're going to resurrect an incore inode then
> > > > we should try to hang on to AS_EIO/AS_ENOSPC for as long as we can.
> > > > 
> > > > 
> > > 
> > > Yes, I think the patch you sent makes sense.
> > > 
> > > Most of the XFS callers are using filemap_write_and_wait{_range}, and
> > > most of those seem to be called from ioctls (->setattr op being the
> > > notable exception).
> > > 
> > > We could (in principle) pass a pointer to file->f_wb_err to most of
> > > those callers, and use that to check for errors instead of looking for
> > > AS_EIO/AS_ENOSPC. We wouldn't want to advance the error cursor for those
> > > (as that should only be done in the context of an fsync), but it might
> > > be more reliable than using the flags here.
> > 
> > Why wouldn't we advance the error pointer? The data writeback error
> > caused an operation to fail and the error has been reported to
> > userspace, so how is that different to fsync() failing and reporting
> > the error to userspace?
> > 
> > This seems like a slippery slope of inconsistency to me, where data
> > writeback errors are only reported once on fsync(), but can be
> > reported multiple times through different filesystem operations...
> > 
> 
> If I get back an error on ioctl, why should I think that that has
> anything to do with writeback? For example, suppose I do:
> 
> write = 0
> ioctl = -EIO
> fsync = 0
> 
> The ioctl returns an error but the fsync returns 0. It would be
> reasonable to assume that my writes had made it to disk.
> 
> I think that we really do want to ensure that fsync always returns an
> error if writeback failed since the last fsync. That was sort of the
> point of the errseq_t work in the first place.

This needs documentation explaining all these intricacies. Because
I'm certain that we'll get it wrong again if the expected behaviour
isn't all spelled out very clearly.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com