[GIT PULL] XFS update for 2.6.23 - revert a commit

[GIT PULL] XFS update for 2.6.23 - revert a commit

[Tim Shimmin]

Hi Linus,

A problem has been found for the XFS commit b394e43e995d08821588a22561c6a71a63b4ff27
and it needs to be reverted.
It has the potential for worse corruption than what it is meant to fix.

Please pull from the for-linus branch:
    git pull git://oss.sgi.com:8090/xfs/xfs-2.6.git for-linus

Lachlan's description:
    This fix is for a problem that has been in XFS since day one.
    [XFS] Avoid replaying inode buffer initialisation log items if on-disk version is newer.

    It tries to fix an issue where log replay is replaying an inode cluster
    initialisation transaction that should not be replayed because the inode
    cluster on disk is more up to date.  Since we don't log file sizes (we
    rely on inode flushing to get them to disk) then we can't just replay
    all the transations in the log and expect the inode to be completely
    restored.  We lose file size updates.  Unfortunately this fix is causing
    more (serious) problems than it is fixing so please don't push this one
    back just yet.

This will update the following files:

 fs/xfs/xfs_buf_item.h    |    5 ----
 fs/xfs/xfs_log_recover.c |   51 ++-------------------------------------------
 fs/xfs/xfs_trans_buf.c   |    1 -
 3 files changed, 3 insertions(+), 54 deletions(-)

through these commits:

commit 053c59a0a7234bac669992f5b8b933b7d7fc189d
Author: Tim Shimmin <tes@chook.melbourne.sgi.com>
Date:   Mon Oct 1 16:39:37 2007 +1000

    Revert "[XFS] Avoid replaying inode buffer initialisation log items if on-disk version is newer."
    
    This reverts commit b394e43e995d08821588a22561c6a71a63b4ff27.
    
    SGI-PV: 969656
    SGI-Modid: xfs-linux-melb:xfs-kern:29804a
    
    Signed-off-by: Lachlan McIlroy <lachlan@sgi.com>
    Signed-off-by: Tim Shimmin <tes@sgi.com>

--Tim

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Eric Sandeen]

Tim Shimmin wrote:
> Hi Linus,
> 
> A problem has been found for the XFS commit b394e43e995d08821588a22561c6a71a63b4ff27
> and it needs to be reverted.
> It has the potential for worse corruption than what it is meant to fix.


Whoops... that's what I get for picking it up too soon for fedora I guess!

Any background on the newly-found problem, for those of us in the peanut
gallery?

Thanks,

-Eric

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Eric Sandeen]

Eric Sandeen wrote:
> Tim Shimmin wrote:
>> Hi Linus,
>>
>> A problem has been found for the XFS commit b394e43e995d08821588a22561c6a71a63b4ff27
>> and it needs to be reverted.
>> It has the potential for worse corruption than what it is meant to fix.
> 
> 
> Whoops... that's what I get for picking it up too soon for fedora I guess!
> 
> Any background on the newly-found problem, for those of us in the peanut
> gallery?

Oh, I'm sorry, the problem description is already there.  I thought that
was the original commit description when I first saw it.

-Eric

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Eric Sandeen]

Eric Sandeen wrote:
> Eric Sandeen wrote:
>> Tim Shimmin wrote:
>>> Hi Linus,
>>>
>>> A problem has been found for the XFS commit b394e43e995d08821588a22561c6a71a63b4ff27
>>> and it needs to be reverted.
>>> It has the potential for worse corruption than what it is meant to fix.
>>
>> Whoops... that's what I get for picking it up too soon for fedora I guess!
>>
>> Any background on the newly-found problem, for those of us in the peanut
>> gallery?
> 
> Oh, I'm sorry, the problem description is already there.  I thought that
> was the original commit description when I first saw it.

*sigh* or maybe not.  ok, no more email 'til I've had my coffee.

-Eric

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Linus Torvalds]

On Mon, 1 Oct 2007, Tim Shimmin wrote:
> 
> Lachlan's description:
>     This fix is for a problem that has been in XFS since day one.
>     [XFS] Avoid replaying inode buffer initialisation log items if on-disk version is newer.
	...

Why wasn't this in the commit logs? Now it just says it was reverted, with 
no actual reasoning *why*. And it apparently wasn't so obvious that no 
such reasoning is needed.

Gah. I ended up amending the commit and updating it with the comment.

		Linus

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Timothy Shimmin]

Linus Torvalds wrote:
> 
> On Mon, 1 Oct 2007, Tim Shimmin wrote:
>> Lachlan's description:
>>     This fix is for a problem that has been in XFS since day one.
>>     [XFS] Avoid replaying inode buffer initialisation log items if on-disk version is newer.
> 	...
> 
> Why wasn't this in the commit logs? Now it just says it was reverted, with 
> no actual reasoning *why*. And it apparently wasn't so obvious that no 
> such reasoning is needed.
> 
> Gah. I ended up amending the commit and updating it with the comment.
> 
> 		Linus

Yeah, sorry about that.
It occurred to me later on.
I initially converted the sgi-mod (undo mod) to a git commit and
then discovered git-revert and thought, hey I'll use that
instead but then forgot to add all the text into it.
D'oh. Thanks for the fixup.

--Tim

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Timothy Shimmin]

Eric Sandeen wrote:
> Tim Shimmin wrote:
>> Hi Linus,
>>
>> A problem has been found for the XFS commit b394e43e995d08821588a22561c6a71a63b4ff27
>> and it needs to be reverted.
>> It has the potential for worse corruption than what it is meant to fix.
> 
> 
> Whoops... that's what I get for picking it up too soon for fedora I guess!
> 
> Any background on the newly-found problem, for those of us in the peanut
> gallery?
> 
> Thanks,
> 
> -Eric

Hi Eric,

Lachlan worked this problem so he can probably provide more details.
My understanding is that we were having a problem with the log replay
replaying newly allocated inodes (inodes from buffer items) over the top
of buffers which were actually more up-to-date than what was logged.
The code used a heuristic to determine if the buffer had been written
to for the inode (by checking on magic#, mode and gen# - not going to
comment on this).
Anyway, it comes down to either copying over the inode buf data or not
copying it over (doing or not doing the log replay).
The change could cause the buffer to be not overwritten in replay
where previously it would be.
We want this to not happen as part of the fix and doing the right thing
and not by mistake and failing to replay when we need it to be replayed.
I presume the latter is what is happening.
The symptoms of this is what Lachlan has discovered in QA on a debug
kernel and he can provide the details.

I believe this started from not logging the inode size changes
(as is consistent with the logging model) for performance reasons,
and so we can't rely on inode log items coming up on log replay
to fix things up.

BTW, we currently have 3 ways of logging an inode:
1. in an item buffer and marked as an inode
2. in an item buffer and not marked as an inode
3. in an inode item

and 3 places where they get replayed:
1. xlog_recover_do_inode_buffer - for di_next_unlinked pointer recovery
2. xlog_recover_do_reg_buffer - for newly allocated inode recovery
3. xlog_recover_do_inode_trans - for general inode recovery

The fix was in #2.


Ughh.

--Tim

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Lachlan McIlroy]

Timothy Shimmin wrote:
> Eric Sandeen wrote:
>> Tim Shimmin wrote:
>>> Hi Linus,
>>>
>>> A problem has been found for the XFS commit 
>>> b394e43e995d08821588a22561c6a71a63b4ff27
>>> and it needs to be reverted.
>>> It has the potential for worse corruption than what it is meant to fix.
>>
>>
>> Whoops... that's what I get for picking it up too soon for fedora I 
>> guess!
>>
>> Any background on the newly-found problem, for those of us in the peanut
>> gallery?
>>
>> Thanks,
>>
>> -Eric
> 
> Hi Eric,
> 
> Lachlan worked this problem so he can probably provide more details.
> My understanding is that we were having a problem with the log replay
> replaying newly allocated inodes (inodes from buffer items) over the top
> of buffers which were actually more up-to-date than what was logged.
> The code used a heuristic to determine if the buffer had been written
> to for the inode (by checking on magic#, mode and gen# - not going to
> comment on this).
> Anyway, it comes down to either copying over the inode buf data or not
> copying it over (doing or not doing the log replay).
> The change could cause the buffer to be not overwritten in replay
> where previously it would be.
> We want this to not happen as part of the fix and doing the right thing
> and not by mistake and failing to replay when we need it to be replayed.
> I presume the latter is what is happening.
> The symptoms of this is what Lachlan has discovered in QA on a debug
> kernel and he can provide the details.
> 
> I believe this started from not logging the inode size changes
> (as is consistent with the logging model) for performance reasons,
> and so we can't rely on inode log items coming up on log replay
> to fix things up.
> 
> BTW, we currently have 3 ways of logging an inode:
> 1. in an item buffer and marked as an inode
> 2. in an item buffer and not marked as an inode
> 3. in an inode item
> 
> and 3 places where they get replayed:
> 1. xlog_recover_do_inode_buffer - for di_next_unlinked pointer recovery
> 2. xlog_recover_do_reg_buffer - for newly allocated inode recovery
> 3. xlog_recover_do_inode_trans - for general inode recovery
> 
> The fix was in #2.
> 
> 
> Ughh.

Yeah that about sums it up.  In an attempt to prevent log replay of inodes
in cases when we shouldn't replay we also prevented log replay of inodes in
cases when we should replay.  We end up with directories that refer to inodes
that were not replayed and we read existing data off disk.  That existing
data is usually previous instances of inodes.  We had cases of regular files
turning into directories and inode version mismatches.

Lachlan

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Justin Piszcz]

On Tue, 2 Oct 2007, Lachlan McIlroy wrote:

> Timothy Shimmin wrote:
>> Eric Sandeen wrote:
>>> Tim Shimmin wrote:
>>>> Hi Linus,
>>>> 
>>>> A problem has been found for the XFS commit 
>>>> b394e43e995d08821588a22561c6a71a63b4ff27
>>>> and it needs to be reverted.
>>>> It has the potential for worse corruption than what it is meant to fix.
>>> 
>>> 
>>> Whoops... that's what I get for picking it up too soon for fedora I guess!
>>> 
>>> Any background on the newly-found problem, for those of us in the peanut
>>> gallery?
>>> 
>>> Thanks,
>>> 
>>> -Eric
>> 
>> Hi Eric,
>> 
>> Lachlan worked this problem so he can probably provide more details.
>> My understanding is that we were having a problem with the log replay
>> replaying newly allocated inodes (inodes from buffer items) over the top
>> of buffers which were actually more up-to-date than what was logged.
>> The code used a heuristic to determine if the buffer had been written
>> to for the inode (by checking on magic#, mode and gen# - not going to
>> comment on this).
>> Anyway, it comes down to either copying over the inode buf data or not
>> copying it over (doing or not doing the log replay).
>> The change could cause the buffer to be not overwritten in replay
>> where previously it would be.
>> We want this to not happen as part of the fix and doing the right thing
>> and not by mistake and failing to replay when we need it to be replayed.
>> I presume the latter is what is happening.
>> The symptoms of this is what Lachlan has discovered in QA on a debug
>> kernel and he can provide the details.
>> 
>> I believe this started from not logging the inode size changes
>> (as is consistent with the logging model) for performance reasons,
>> and so we can't rely on inode log items coming up on log replay
>> to fix things up.
>> 
>> BTW, we currently have 3 ways of logging an inode:
>> 1. in an item buffer and marked as an inode
>> 2. in an item buffer and not marked as an inode
>> 3. in an inode item
>> 
>> and 3 places where they get replayed:
>> 1. xlog_recover_do_inode_buffer - for di_next_unlinked pointer recovery
>> 2. xlog_recover_do_reg_buffer - for newly allocated inode recovery
>> 3. xlog_recover_do_inode_trans - for general inode recovery
>> 
>> The fix was in #2.
>> 
>> 
>> Ughh.
>
> Yeah that about sums it up.  In an attempt to prevent log replay of inodes
> in cases when we shouldn't replay we also prevented log replay of inodes in
> cases when we should replay.  We end up with directories that refer to inodes
> that were not replayed and we read existing data off disk.  That existing
> data is usually previous instances of inodes.  We had cases of regular files
> turning into directories and inode version mismatches.
>
> Lachlan
>
In 2.6.23-rc8?

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[David Chinner]

On Tue, Oct 02, 2007 at 05:03:45PM +1000, Lachlan McIlroy wrote:
> Yeah that about sums it up.  In an attempt to prevent log replay of inodes
> in cases when we shouldn't replay we also prevented log replay of inodes in
> cases when we should replay.  We end up with directories that refer to 
> inodes
> that were not replayed and we read existing data off disk.  That existing
> data is usually previous instances of inodes.  We had cases of regular files
> turning into directories and inode version mismatches.

The issue that started tripping this was stale inodes from a
previous filesystem being detected as valid clusters of inodes.
Hence inodes magically changed from what they were supposed to
be and that caused all sorts of problems.

FWIW, in a past life XFS was supposed to store the uuid of the
filesystem in it's inodes to prevent exactly this sort of confusion,
but that was considered expendable and disappeared in version 2
inodes when XFS grew 32bit link counts and project ID's. The padding
is still there waiting to be used.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Timothy Shimmin]

Hi Justin,

Justin Piszcz wrote:
> 
> 
> On Tue, 2 Oct 2007, Lachlan McIlroy wrote:
> 
>> Yeah that about sums it up.  In an attempt to prevent log replay of 
>> inodes
>> in cases when we shouldn't replay we also prevented log replay of 
>> inodes in
>> cases when we should replay.  We end up with directories that refer to 
>> inodes
>> that were not replayed and we read existing data off disk.  That existing
>> data is usually previous instances of inodes.  We had cases of regular 
>> files
>> turning into directories and inode version mismatches.
>>
>> Lachlan
>>
> In 2.6.23-rc8?
> 
The bad change went into rc7 and was still there in rc8.
It was backed out for rc9.

--Tim

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Justin Piszcz]

On Wed, 3 Oct 2007, Timothy Shimmin wrote:

> Hi Justin,
>
> Justin Piszcz wrote:
>> 
>> 
>> On Tue, 2 Oct 2007, Lachlan McIlroy wrote:
>> 
>>> Yeah that about sums it up.  In an attempt to prevent log replay of inodes
>>> in cases when we shouldn't replay we also prevented log replay of inodes 
>>> in
>>> cases when we should replay.  We end up with directories that refer to 
>>> inodes
>>> that were not replayed and we read existing data off disk.  That existing
>>> data is usually previous instances of inodes.  We had cases of regular 
>>> files
>>> turning into directories and inode version mismatches.
>>> 
>>> Lachlan
>>> 
>> In 2.6.23-rc8?
>> 
> The bad change went into rc7 and was still there in rc8.
> It was backed out for rc9.
>
> --Tim
>

If one with was running 2.6.23-rc8 with XFS, does that mean we should run 
xfs_repair on our filesystems after upgrading to -rc9?

Justin.

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[David Chinner]

On Wed, Oct 03, 2007 at 04:11:50AM -0400, Justin Piszcz wrote:
> If one with was running 2.6.23-rc8 with XFS, does that mean we should run 
> xfs_repair on our filesystems after upgrading to -rc9?

Only if you had unclean shutdowns while running 2.6.23-rc8.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [GIT PULL] XFS update for 2.6.23 - revert a commit

[Justin Piszcz]

On Thu, 4 Oct 2007, David Chinner wrote:

> On Wed, Oct 03, 2007 at 04:11:50AM -0400, Justin Piszcz wrote:
>> If one with was running 2.6.23-rc8 with XFS, does that mean we should run
>> xfs_repair on our filesystems after upgrading to -rc9?
>
> Only if you had unclean shutdowns while running 2.6.23-rc8.
>
> Cheers,
>
> Dave.
> -- 
> Dave Chinner
> Principal Engineer
> SGI Australian Software Group
>
>

K, thanks.

Justin.