* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-19 15:27 ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
@ 2018-05-19 15:36 ` Matthew Wilcox
2018-05-21 17:54 ` Darrick J. Wong
2018-05-19 23:19 ` Theodore Y. Ts'o
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2018-05-19 15:36 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Jeff Layton, Theodore Y. Ts'o, linux-fsdevel, fstests, xfs
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;
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-19 15:36 ` Matthew Wilcox
@ 2018-05-21 17:54 ` Darrick J. Wong
2018-05-22 10:30 ` Jeff Layton
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-21 17:54 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jeff Layton, Theodore Y. Ts'o, linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-21 17:54 ` Darrick J. Wong
@ 2018-05-22 10:30 ` Jeff Layton
2018-05-22 22:09 ` Dave Chinner
0 siblings, 1 reply; 26+ messages in thread
From: Jeff Layton @ 2018-05-22 10:30 UTC (permalink / raw)
To: Darrick J. Wong, Matthew Wilcox
Cc: Theodore Y. Ts'o, linux-fsdevel, fstests, xfs
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>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-22 10:30 ` Jeff Layton
@ 2018-05-22 22:09 ` Dave Chinner
2018-05-23 10:56 ` Jeff Layton
0 siblings, 1 reply; 26+ messages in thread
From: Dave Chinner @ 2018-05-22 22:09 UTC (permalink / raw)
To: Jeff Layton
Cc: Darrick J. Wong, Matthew Wilcox, Theodore Y. Ts'o,
linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-22 22:09 ` Dave Chinner
@ 2018-05-23 10:56 ` Jeff Layton
2018-05-24 3:59 ` Dave Chinner
0 siblings, 1 reply; 26+ messages in thread
From: Jeff Layton @ 2018-05-23 10:56 UTC (permalink / raw)
To: Dave Chinner
Cc: Darrick J. Wong, Matthew Wilcox, Theodore Y. Ts'o,
linux-fsdevel, fstests, xfs
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>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-23 10:56 ` Jeff Layton
@ 2018-05-24 3:59 ` Dave Chinner
0 siblings, 0 replies; 26+ messages in thread
From: Dave Chinner @ 2018-05-24 3:59 UTC (permalink / raw)
To: Jeff Layton
Cc: Darrick J. Wong, Matthew Wilcox, Theodore Y. Ts'o,
linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-19 15:27 ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
2018-05-19 15:36 ` Matthew Wilcox
@ 2018-05-19 23:19 ` Theodore Y. Ts'o
2018-05-20 11:45 ` Jeff Layton
2018-05-22 4:06 ` [PATCH v2] " Darrick J. Wong
2018-05-22 16:43 ` [PATCH v3] " Darrick J. Wong
3 siblings, 1 reply; 26+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-19 23:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Jeff Layton, Matthew Wilcox, linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-19 23:19 ` Theodore Y. Ts'o
@ 2018-05-20 11:45 ` Jeff Layton
2018-05-20 12:58 ` Matthew Wilcox
2018-05-20 17:57 ` Theodore Y. Ts'o
0 siblings, 2 replies; 26+ messages in thread
From: Jeff Layton @ 2018-05-20 11:45 UTC (permalink / raw)
To: Theodore Y. Ts'o, Darrick J. Wong
Cc: Matthew Wilcox, linux-fsdevel, fstests, xfs
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 11:45 ` Jeff Layton
@ 2018-05-20 12:58 ` Matthew Wilcox
2018-05-20 13:18 ` Jeff Layton
2018-05-20 16:29 ` Theodore Y. Ts'o
2018-05-20 17:57 ` Theodore Y. Ts'o
1 sibling, 2 replies; 26+ messages in thread
From: Matthew Wilcox @ 2018-05-20 12:58 UTC (permalink / raw)
To: Jeff Layton
Cc: Theodore Y. Ts'o, Darrick J. Wong, linux-fsdevel, fstests,
xfs
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 12:58 ` Matthew Wilcox
@ 2018-05-20 13:18 ` Jeff Layton
2018-05-20 16:29 ` Theodore Y. Ts'o
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2018-05-20 13:18 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Theodore Y. Ts'o, Darrick J. Wong, linux-fsdevel, fstests,
xfs
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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 12:58 ` Matthew Wilcox
2018-05-20 13:18 ` Jeff Layton
@ 2018-05-20 16:29 ` Theodore Y. Ts'o
2018-05-20 19:20 ` Matthew Wilcox
1 sibling, 1 reply; 26+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-20 16:29 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jeff Layton, Darrick J. Wong, linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 16:29 ` Theodore Y. Ts'o
@ 2018-05-20 19:20 ` Matthew Wilcox
2018-05-20 19:41 ` Theodore Y. Ts'o
0 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2018-05-20 19:20 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Jeff Layton, Darrick J. Wong, linux-fsdevel, fstests, xfs
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);
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 19:20 ` Matthew Wilcox
@ 2018-05-20 19:41 ` Theodore Y. Ts'o
2018-05-21 11:20 ` Jeff Layton
0 siblings, 1 reply; 26+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-20 19:41 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jeff Layton, Darrick J. Wong, linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 19:41 ` Theodore Y. Ts'o
@ 2018-05-21 11:20 ` Jeff Layton
2018-05-21 14:43 ` Theodore Y. Ts'o
0 siblings, 1 reply; 26+ messages in thread
From: Jeff Layton @ 2018-05-21 11:20 UTC (permalink / raw)
To: Theodore Y. Ts'o, Matthew Wilcox
Cc: Darrick J. Wong, linux-fsdevel, fstests, xfs, linux-block
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>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-21 11:20 ` Jeff Layton
@ 2018-05-21 14:43 ` Theodore Y. Ts'o
0 siblings, 0 replies; 26+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-21 14:43 UTC (permalink / raw)
To: Jeff Layton
Cc: Matthew Wilcox, Darrick J. Wong, linux-fsdevel, fstests, xfs,
linux-block
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] fs: clear writeback errors in inode_init_always
2018-05-20 11:45 ` Jeff Layton
2018-05-20 12:58 ` Matthew Wilcox
@ 2018-05-20 17:57 ` Theodore Y. Ts'o
1 sibling, 0 replies; 26+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-20 17:57 UTC (permalink / raw)
To: Jeff Layton; +Cc: Darrick J. Wong, Matthew Wilcox, linux-fsdevel, fstests, xfs
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] fs: clear writeback errors in inode_init_always
2018-05-19 15:27 ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
2018-05-19 15:36 ` Matthew Wilcox
2018-05-19 23:19 ` Theodore Y. Ts'o
@ 2018-05-22 4:06 ` Darrick J. Wong
2018-05-22 10:14 ` Jeff Layton
2018-05-22 12:14 ` Brian Foster
2018-05-22 16:43 ` [PATCH v3] " Darrick J. Wong
3 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-22 4:06 UTC (permalink / raw)
To: xfs; +Cc: Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton
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;
}
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] fs: clear writeback errors in inode_init_always
2018-05-22 4:06 ` [PATCH v2] " Darrick J. Wong
@ 2018-05-22 10:14 ` Jeff Layton
2018-05-22 12:14 ` Brian Foster
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Layton @ 2018-05-22 10:14 UTC (permalink / raw)
To: Darrick J. Wong, xfs; +Cc: Matthew Wilcox, linux-fsdevel, fstests
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>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] fs: clear writeback errors in inode_init_always
2018-05-22 4:06 ` [PATCH v2] " Darrick J. Wong
2018-05-22 10:14 ` Jeff Layton
@ 2018-05-22 12:14 ` Brian Foster
2018-05-22 14:37 ` Darrick J. Wong
1 sibling, 1 reply; 26+ messages in thread
From: Brian Foster @ 2018-05-22 12:14 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] fs: clear writeback errors in inode_init_always
2018-05-22 12:14 ` Brian Foster
@ 2018-05-22 14:37 ` Darrick J. Wong
0 siblings, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-22 14:37 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3] fs: clear writeback errors in inode_init_always
2018-05-19 15:27 ` [PATCH] fs: clear writeback errors in inode_init_always Darrick J. Wong
` (2 preceding siblings ...)
2018-05-22 4:06 ` [PATCH v2] " Darrick J. Wong
@ 2018-05-22 16:43 ` Darrick J. Wong
2018-05-22 18:40 ` Brian Foster
2018-05-22 22:05 ` Dave Chinner
3 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-22 16:43 UTC (permalink / raw)
To: xfs; +Cc: Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton, Brian Foster
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3] fs: clear writeback errors in inode_init_always
2018-05-22 16:43 ` [PATCH v3] " Darrick J. Wong
@ 2018-05-22 18:40 ` Brian Foster
2018-05-22 18:47 ` Darrick J. Wong
2018-05-22 22:05 ` Dave Chinner
1 sibling, 1 reply; 26+ messages in thread
From: Brian Foster @ 2018-05-22 18:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] fs: clear writeback errors in inode_init_always
2018-05-22 18:40 ` Brian Foster
@ 2018-05-22 18:47 ` Darrick J. Wong
0 siblings, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-22 18:47 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] fs: clear writeback errors in inode_init_always
2018-05-22 16:43 ` [PATCH v3] " Darrick J. Wong
2018-05-22 18:40 ` Brian Foster
@ 2018-05-22 22:05 ` Dave Chinner
2018-05-23 3:00 ` Darrick J. Wong
1 sibling, 1 reply; 26+ messages in thread
From: Dave Chinner @ 2018-05-22 22:05 UTC (permalink / raw)
To: Darrick J. Wong
Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton,
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);
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] fs: clear writeback errors in inode_init_always
2018-05-22 22:05 ` Dave Chinner
@ 2018-05-23 3:00 ` Darrick J. Wong
0 siblings, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2018-05-23 3:00 UTC (permalink / raw)
To: Dave Chinner
Cc: xfs, Matthew Wilcox, linux-fsdevel, fstests, Jeff Layton,
Brian Foster
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
^ permalink raw reply [flat|nested] 26+ messages in thread