linux likes to kill bad inodes

PATCH In review Pavel Machek 7 participants 13 messages Apr 25, 2001 – Apr 30, 2001

Subsystems (4)

VFS ext2 file system UDF filesystem Hpfs filesystem

lore

Share & tools

lore

Pavel Machek

author

2001-04-25 12:20 UTC #0 lore raw

PATCH 1/1 linux likes to kill bad inodes

Hi!

I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.

Solution is not to write bad inodes back to disk.

[Thanx to Jan Kara]
Pavel

fs/inode.c +1 -1

@@ -179,7 +179,7 @@
 
 static inline void write_inode(struct inode *inode, int sync)
 {
-	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode)
+	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode && !is_bad_inode(inode))
 		inode->i_sb->s_op->write_inode(inode, sync);
 }

Signature · 2 lines

I'm [email protected]. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at [email protected]

Chris Mason

2001-04-25 13:30 UTC #1 lore raw

Quoted 10 lines · On Sunday, April 22, 2001 02:10:42 PM +0200 Pavel Machek <[email protected]> wrote:

Hi!
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.
Solution is not to write bad inodes back to disk.

Wouldn't we rather make it so bad inodes don't get marked dirty at all?

-chris

Pavel Machek

author

2001-04-25 20:05 UTC #2 lore raw

In reply to Pavel Machek

Hi!

Quoted 8 lines

Quoted 8 lines
Hi!
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.

Quoted 4 lines

Quoted 2 lines
Solution is not to write bad inodes back to disk.
Wouldn't we rather make it so bad inodes don't get marked dirty at all?

I guess this is cheaper: we can mark inode dirty at 1000 points, but you only write it at one point.

But I'm no FS expert.
Pavel

Signature · 2 lines

I'm [email protected]. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at [email protected]

Chris Mason

2001-04-25 20:32 UTC #3 lore raw

Quoted 10 lines · On Wednesday, April 25, 2001 10:01:20 PM +0200 Pavel Machek <[email protected]>…

Hi!
Quoted 8 lines
Quoted 8 lines
Hi!
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Solution is not to write bad inodes back to disk.
Wouldn't we rather make it so bad inodes don't get marked dirty at all?
I guess this is cheaper: we can mark inode dirty at 1000 points, but you only write it at one point.

Whoops, I worded that poorly. To me, it seems like a bug to dirty a bad inode. If this patch works, it is because somewhere, somebody did something with a bad inode, and thought the operation worked (otherwise, why dirty it?).

So yes, even if we dirty them in a 1000 different places, we need to find the one place that believes it can do something worthwhile to a bad inode.

-chris

Pavel Machek

author

2001-04-26 09:29 UTC #4 lore raw

In reply to Pavel Machek

Hi!

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 10 lines
Hi!
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.
Solution is not to write bad inodes back to disk.
Wouldn't we rather make it so bad inodes don't get marked dirty at all?

Quoted 7 lines

Quoted 2 lines
I guess this is cheaper: we can mark inode dirty at 1000 points, but you only write it at one point.
Whoops, I worded that poorly. To me, it seems like a bug to dirty a bad inode. If this patch works, it is because somewhere, somebody did something with a bad inode, and thought the operation worked (otherwise, why dirty it?).

Would it make sense to put the check into write_inode_ with BUG() and return, then?

Quoted 3 lines

So yes, even if we dirty them in a 1000 different places, we need to find the one place that believes it can do something worthwhile to a bad inode.

Could not it be something as simple as atime update?

Pavel

Signature · 2 lines

The best software in life is free (not shareware)!		Pavel
GCM d? s-: !g p?:+ au- a--@ w+ v- C++@ UL+++ L++ N++ E++ W--- M- Y- R+

Jan Kara

reviewer

2001-04-26 09:33 UTC #5 lore raw

In reply to Pavel Machek

Quoted 23 lines

Quoted 10 lines · On Wednesday, April 25, 2001 10:01:20 PM +0200 Pavel Machek <[email protected]>…
Hi!
Quoted 8 lines
Quoted 8 lines
Hi!
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Solution is not to write bad inodes back to disk.
Wouldn't we rather make it so bad inodes don't get marked dirty at all?
I guess this is cheaper: we can mark inode dirty at 1000 points, but you only write it at one point.

Quoted 7 lines

Whoops, I worded that poorly. To me, it seems like a bug to dirty a bad inode. If this patch works, it is because somewhere, somebody did something with a bad inode, and thought the operation worked (otherwise, why dirty it?).
So yes, even if we dirty them in a 1000 different places, we need to find the one place that believes it can do something worthwhile to a bad inode.

Yes checking those places where bad inode gets dirty is probably good
idea. But I'd add test to write_inode anyway together with warning that this shouldn't happen.

Honza

--
Jan Kara <[email protected]>
SuSE Labs

Pavel Machek

author

2001-04-27 14:16 UTC #6 lore raw

In reply to Pavel Machek

Hi!

Quoted 14 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I had a temporary disk failure (played with acpi too much). What happened was that disk was not able to do anything for five minutes or so. When disk recovered, linux happily overwrote all inodes it could not read while disk was down with zeros -> massive disk corruption.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Solution is not to write bad inodes back to disk.
Wouldn't we rather make it so bad inodes don't get marked dirty at all?
I guess this is cheaper: we can mark inode dirty at 1000 points, but you only write it at one point.

Quoted 7 lines

Whoops, I worded that poorly. To me, it seems like a bug to dirty a bad inode. If this patch works, it is because somewhere, somebody did something with a bad inode, and thought the operation worked (otherwise, why dirty it?).
So yes, even if we dirty them in a 1000 different places, we need to find the one place that believes it can do something worthwhile to a bad inode.

Okay, so what about following patch, followed by attempt to debug it? [I'd really like to get patch it; killing user's data without good reason seems evil to me, and this did quite a lot of damage to my $HOME.]

Pavel
PS: Only filesystem at use at time of problem was ext2, and it was ext2 iinode that got killed.

fs/inode.c +4 -0

@@ -179,6 +179,10 @@
 
 static inline void write_inode(struct inode *inode, int sync)
 {
+	if (is_bad_inode(inode)) {
+		printk(KERN_CRIT "Cowardly refusing to kill your inode\n");
+		return;
+	}		
 	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode)
 		inode->i_sb->s_op->write_inode(inode, sync);
 }

Signature · 2 lines

I'm [email protected]. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at [email protected]

Chris Mason

2001-04-27 14:37 UTC #7 lore raw

Quoted 4 lines · On Friday, April 27, 2001 12:28:54 AM +0200 Pavel Machek <[email protected]> wrote:

Okay, so what about following patch, followed by attempt to debug it? [I'd really like to get patch it; killing user's data without good reason seems evil to me, and this did quite a lot of damage to my $HOME.]

2.4.4-pre8 does have the patch to keep write_inode from syncing a
bad_inode. In the short term this is the best way to go.

For debugging further, it is probably best to put the warning in when marking the inode dirty, and randomly returning bad_inodes from read_inode. I'll give this a try next week.

My guess is that UPDATE_ATIME is the offending caller, the follow_link path in open_namei is at least one place that should trigger it.

-chris

Andreas Dilger

2001-04-27 22:11 UTC #8 lore raw

In reply to Pavel Machek

Quoted 7 lines · Pavel

[I'd really like to get patch it; killing user's data without good reason seems evil to me, and this did quite a lot of damage to my $HOME.]
Pavel
PS: Only filesystem at use at time of problem was ext2, and it was ext2 iinode that got killed.

However, since make_bad_inode() only changes the file methods and not the superblock, any of the put_inode(), delete_inode(), or write_inode() methods can still be called. While the write_inode() call is safe to block in the VFS, I don't think it is safe to block *_put_inode() and *_delete_inode() in the VFS because the filesystem may have allocated memory or other things that need to be undone even for bad inodes.

The following patch fixes the VFS write_inode() (per Pavel's patch), and ext2_put_inode(), bfs_delete_inode(), and udf_put_inode() to not do anything with bad inodes. I have bailed (with a FIXME) on hpfs_delete_inode() and smb_delete_inode(), because I don't know what (if anything) needs to be done to correctly clean up a bad inode.

I will post a patch separately which handles a couple of cases where *_delete_inode() does not call clear_inode() in all cases.

Cheers, Andreas
=======================================================================

fs/inode.c +5 -0

diff -ru linux-2.4.4p1.orig/fs/inode.c linux/fs/inode.c
@@ -179,6 +181,12 @@
 
 static inline void write_inode(struct inode *inode, int sync)
 {
+	if (is_bad_inode(inode)) {
+		printk(KERN_CRIT "Cowardly refusing to write bad inode %s:%d\n",+		       kdevname(inode->i_dev), inode->i_ino);
+		return;
+	}
+
 	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode)
 		inode->i_sb->s_op->write_inode(inode, sync);
 }

fs/ext2/inode.c +3 -0

diff -ru linux-2.4.4p1.orig/fs/ext2/inode.c linux/fs/ext2/inode.c
@@ -36,6 +36,9 @@
  */
 void ext2_put_inode (struct inode * inode)
 {
+	if (is_bad_inode(inode))
+		return;
+
 	ext2_discard_prealloc (inode);
 }

fs/bfs/inode.c +2 -1

diff -ru linux-2.4.4p1.orig/fs/bfs/inode.c linux/fs/bfs/inode.c
@@ -142,7 +142,8 @@
 
 	dprintf("ino=%08lx\n", inode->i_ino);
 
-	if (inode->i_ino < BFS_ROOT_INO || inode->i_ino > inode->i_sb->su_lasti) {
+	if (is_bad_inode(inode) || inode->i_ino < BFS_ROOT_INO ||
+	    inode->i_ino > inode->i_sb->su_lasti) {
 		printf("invalid ino=%08lx\n", inode->i_ino);
 		return;
 	}

fs/udf/inode.c +1 -1

diff -ru linux-2.4.4p1.orig/fs/udf/inode.c linux/fs/udf/inode.c
@@ -74,7 +74,7 @@
  */
 void udf_put_inode(struct inode * inode)
 {
-	if (!(inode->i_sb->s_flags & MS_RDONLY))
+	if (!(inode->i_sb->s_flags & MS_RDONLY) && !is_bad_inode(inode))
 	{
 		lock_kernel();
 		udf_discard_prealloc(inode);

fs/hpfs/inode.c +1 -0

diff -ru linux-2.4.4p1.orig/fs/hpfs/inode.c linux/fs/hpfs/inode.c
@@ -316,6 +304,7 @@
 
 void hpfs_delete_inode(struct inode *inode)
 {
+	/* FIXME: handle is_bad_inode??? */
 	lock_kernel();
 	hpfs_remove_fnode(inode->i_sb, inode->i_ino);
 	unlock_kernel();

fs/smbfs/inode.c +1 -0

diff -ru linux-2.4.4p1.orig/fs/smbfs/inode.c linux/fs/smbfs/inode.c
@@ -254,6 +254,7 @@
 smb_delete_inode(struct inode *ino)
 {
 	DEBUG1("ino=%ld\n", ino->i_ino);
+	/* FIXME: handle is_bad_inode() case??? */
 	lock_kernel();
 	if (smb_close(ino))
 		PARANOIA("could not close inode %ld\n", ino->i_ino);

Signature · 3 lines

Andreas Dilger                               TurboLabs filesystem development
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Linus Torvalds

2001-04-27 22:15 UTC #9 lore raw

Quoted 3 lines · Andreas Dilger

However, since make_bad_inode() only changes the file methods and not the superblock

Please just make "make_bad_inode()" just do

	inode->i_sb = bad_super_block;

and do everything else too.

It's not acceptable to make low-level filesystems care about these things.

Linus

Alan Cox

2001-04-30 22:25 UTC #10 lore raw

In reply to Pavel Machek

fs/inode.c line 184

Quoted 5 lines

{

fs/inode.c +1 -1

-	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode)
+	if (inode->i_sb && inode->i_sb->s_op && inode->i_sb->s_op->write_inode && !is_bad_inode(inode))
 		inode->i_sb->s_op->write_inode(inode, sync);
 }

Any reason a bad inode can't have its i_sb changed to a bad_inode_fs ?

David S. Miller

2001-04-30 22:34 UTC #11 lore raw

Quoted 1 line · Alan Cox

Any reason a bad inode can't have its i_sb changed to a bad_inode_fs ?

I believe this is what Linus suggested.

Later,
David S. Miller
[email protected]

Linus Torvalds

2001-04-30 23:23 UTC #12 lore raw

In reply to Alan Cox

Quoted 2 lines · Alan Cox

Any reason a bad inode can't have its i_sb changed to a bad_inode_fs ?

That would be my personal preference too, this was just the quick hack version.

Changing superblocks might have other consequences (like getting the superblock inode lists right etc).

Linus