linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
@ 2001-03-07 20:35 Andreas Dilger
  2001-03-23  1:39 ` Stephen C. Tweedie
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2001-03-07 20:35 UTC (permalink / raw)
  To: Linux kernel development list, Linux FS development list

Bill Clark wrote (to the moderated linux-lvm@sistina.com list):
> Not sure if this is a LVM problem or a ext2fs problem. It is happening
> with the 2.4.2 kernel and the 0.9 release of the LVM user tools.  
> 
> kernel: Kernel panic: EXT2-fs panic (device lvm(58,0)):
> load_block_bitmap: block_group >= groups_count - block_group = 131071,
> groups_count = 24
> 
> There is a 1gig+ file on the filesystem, and most operations on it seem
> to bring about the error.  

Basically, the error you are getting is impossible.  In all calls to
load_block_bitmap(), the "group number" is either checked directly, or
"block" (which is what is used to find "group number") is checked to be <
s_blocks_count, so by inference should return a valid group number.

The only remote possibility is in ext2_free_blocks() if block+count
overflows a 32-bit unsigned value.  Only 2 places call ext2_free_blocks()
with a count != 1, and ext2_free_data() looks to be OK.  The other
possibility is that i_prealloc_count is bogus - that is it!  Nowhere
is i_prealloc_count initialized to zero AFAICS.

In most cases, this would return a "freeing blocks not in datazone" error,
but depending on the values, it may just pass the test.  This may also
be the cause of the errors people have previously reported where it
looks like they are freeing block numbers which look like ASCII data.
This would happen in ext2_discard_prealloc() when we have a value for
i_prealloc_count != 0 (easy) and i_prealloc_block points to some valid
block number (less likely, but moreso on a large filesystem).

Cheers, Andreas
==========================================================================
diff -ru linux/fs/ext2/ialloc.c.orig linux/fs/ext2/ialloc.c
--- linux/fs/ext2/ialloc.c.orig	Fri Dec  8 18:35:54 2000
+++ linux/fs/ext2/ialloc.c	Wed Mar  7 12:22:11 2001
@@ -432,6 +444,8 @@
 	inode->u.ext2_i.i_file_acl = 0;
 	inode->u.ext2_i.i_dir_acl = 0;
 	inode->u.ext2_i.i_dtime = 0;
+	inode->u.ext2_i.i_prealloc_count = 0;
 	inode->u.ext2_i.i_block_group = i;
 	if (inode->u.ext2_i.i_flags & EXT2_SYNC_FL)
 		inode->i_flags |= S_SYNC;
diff -ru linux/fs/ext2/inode.c.orig linux/fs/ext2/inode.c
--- linux/fs/ext2/inode.c.orig	Tue Jan 16 01:29:29 2001
+++ linux/fs/ext2/inode.c	Wed Mar  7 12:05:47 2001
@@ -1048,6 +1038,8 @@
 			(((__u64)le32_to_cpu(raw_inode->i_size_high)) << 32);
	}
 	inode->i_generation = le32_to_cpu(raw_inode->i_generation);
+	inode->u.ext2_i.i_prealloc_count = 0;
 	inode->u.ext2_i.i_block_group = block_group;
 
 	/*
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
  2001-03-07 20:35 [linux-lvm] EXT2-fs panic (device lvm(58,0)): Andreas Dilger
@ 2001-03-23  1:39 ` Stephen C. Tweedie
  2001-03-23  2:04   ` Alexander Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen C. Tweedie @ 2001-03-23  1:39 UTC (permalink / raw)
  To: Andreas Dilger
  Cc: Linux kernel development list, Linux FS development list,
	Alexander Viro, Alan Cox, Linus Torvalds

Hi,

On Wed, Mar 07, 2001 at 01:35:05PM -0700, Andreas Dilger wrote:

> The only remote possibility is in ext2_free_blocks() if block+count
> overflows a 32-bit unsigned value.  Only 2 places call ext2_free_blocks()
> with a count != 1, and ext2_free_data() looks to be OK.  The other
> possibility is that i_prealloc_count is bogus - that is it!  Nowhere
> is i_prealloc_count initialized to zero AFAICS.
> 
Did you ever push this to Alan and/or Linus?  This looks pretty
important!

Cheers,
 Stephen

> ==========================================================================
> diff -ru linux/fs/ext2/ialloc.c.orig linux/fs/ext2/ialloc.c
> --- linux/fs/ext2/ialloc.c.orig	Fri Dec  8 18:35:54 2000
> +++ linux/fs/ext2/ialloc.c	Wed Mar  7 12:22:11 2001
> @@ -432,6 +444,8 @@
>  	inode->u.ext2_i.i_file_acl = 0;
>  	inode->u.ext2_i.i_dir_acl = 0;
>  	inode->u.ext2_i.i_dtime = 0;
> +	inode->u.ext2_i.i_prealloc_count = 0;
>  	inode->u.ext2_i.i_block_group = i;
>  	if (inode->u.ext2_i.i_flags & EXT2_SYNC_FL)
>  		inode->i_flags |= S_SYNC;
> diff -ru linux/fs/ext2/inode.c.orig linux/fs/ext2/inode.c
> --- linux/fs/ext2/inode.c.orig	Tue Jan 16 01:29:29 2001
> +++ linux/fs/ext2/inode.c	Wed Mar  7 12:05:47 2001
> @@ -1048,6 +1038,8 @@
>  			(((__u64)le32_to_cpu(raw_inode->i_size_high)) << 32);
> 	}
>  	inode->i_generation = le32_to_cpu(raw_inode->i_generation);
> +	inode->u.ext2_i.i_prealloc_count = 0;
>  	inode->u.ext2_i.i_block_group = block_group;
>  
>  	/*

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
  2001-03-23  1:39 ` Stephen C. Tweedie
@ 2001-03-23  2:04   ` Alexander Viro
  2001-03-23  5:48     ` Andreas Dilger
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Viro @ 2001-03-23  2:04 UTC (permalink / raw)
  To: Stephen C. Tweedie
  Cc: Andreas Dilger, Linux kernel development list,
	Linux FS development list, Alexander Viro, Alan Cox,
	Linus Torvalds

On Fri, 23 Mar 2001, Stephen C. Tweedie wrote:

> Hi,
>
> On Wed, Mar 07, 2001 at 01:35:05PM -0700, Andreas Dilger wrote:
>
> > The only remote possibility is in ext2_free_blocks() if block+count
> > overflows a 32-bit unsigned value.  Only 2 places call ext2_free_blocks()
> > with a count != 1, and ext2_free_data() looks to be OK.  The other
> > possibility is that i_prealloc_count is bogus - that is it!  Nowhere
> > is i_prealloc_count initialized to zero AFAICS.
> >
> Did you ever push this to Alan and/or Linus?  This looks pretty
> important!

It isn't. Check fs/inode.c::clean_inode(). Specifically,
        memset(&inode->u, 0, sizeof(inode->u));
The thing is called both by get_empty_inode() and by get_new_inode() (the
former - just before returning, the latter - just before calling
->read_inode()).
								Cheers,
									Al


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
  2001-03-23  2:04   ` Alexander Viro
@ 2001-03-23  5:48     ` Andreas Dilger
  2001-03-23 11:44       ` Alexander Viro
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2001-03-23  5:48 UTC (permalink / raw)
  To: Alexander Viro
  Cc: Stephen C. Tweedie, linux-fsdevel, Linux kernel development list

Al Viro writes:
> On Fri, 23 Mar 2001, Stephen C. Tweedie wrote:
> > On Wed, Mar 07, 2001 at 01:35:05PM -0700, Andreas Dilger wrote:
> > > The only remote possibility is in ext2_free_blocks() if block+count
> > > overflows a 32-bit unsigned value.  Only 2 places call ext2_free_blocks()
> > > with a count != 1, and ext2_free_data() looks to be OK.  The other
> > > possibility is that i_prealloc_count is bogus - that is it!  Nowhere
> > > is i_prealloc_count initialized to zero AFAICS.
> > >
> > Did you ever push this to Alan and/or Linus?  This looks pretty
> > important!
> 
> It isn't. Check fs/inode.c::clean_inode(). Specifically,
>         memset(&inode->u, 0, sizeof(inode->u));
> The thing is called both by get_empty_inode() and by get_new_inode() (the
> former - just before returning, the latter - just before calling
> ->read_inode()).

If this is the case, then all of the other zero initializations can be
removed as well.  I figured that if most of the fields needed to be
zeroed, then ones _not_ being zeroed would lead to this problem.

FYI Stephen, the original poster followed up that the problem was with
an IBM SCSI RAID card...

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
  2001-03-23  5:48     ` Andreas Dilger
@ 2001-03-23 11:44       ` Alexander Viro
  2001-03-23 19:11         ` Andreas Dilger
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Viro @ 2001-03-23 11:44 UTC (permalink / raw)
  To: Andreas Dilger
  Cc: Alexander Viro, Stephen C. Tweedie, linux-fsdevel,
	Linux kernel development list



On Thu, 22 Mar 2001, Andreas Dilger wrote:

> If this is the case, then all of the other zero initializations can be
> removed as well.  I figured that if most of the fields needed to be
> zeroed, then ones _not_ being zeroed would lead to this problem.

	Other zero initializations in inode->u certainly can be
removed, but whether it's worth doing or not depends is a matter of
taste (recall the flamefest around Tigran's crusade against global
zero initializers several months ago ;-)
	The rule is that inode->u is zeroed before fs gets to see
the inode, be it in ->read_inode() or after get_empty_inode().
The rest is private business of that fs. That's what ->u is about,
after all...
						Cheers,
							Al


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [linux-lvm] EXT2-fs panic (device lvm(58,0)):
  2001-03-23 11:44       ` Alexander Viro
@ 2001-03-23 19:11         ` Andreas Dilger
  0 siblings, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2001-03-23 19:11 UTC (permalink / raw)
  To: Alexander Viro
  Cc: Linux FS development list, Linux kernel development list,
	HPFS maintainer, nfs, samba, linux_udf, UFS maintainer,
	Reiserfs mailing list

Al writes:
> On Thu, 22 Mar 2001, Andreas Dilger wrote:
> > If this is the case, then all of the other zero initializations can be
> > removed as well.  I figured that if most of the fields were being
> > zeroed, then ones _not_ being zeroed would lead to this problem.
> 
> 	Other zero initializations in inode->u certainly can be
> removed, but whether it's worth doing or not depends is a matter of
> taste (recall the flamefest around Tigran's crusade against global
> zero initializers several months ago ;-)

Yes, but now it is common practise to remove any global zero inits.

> 	The rule is that inode->u is zeroed before fs gets to see
> the inode, be it in ->read_inode() or after get_empty_inode().
> The rest is private business of that fs. That's what ->u is about,
> after all...

I did a quick check through the 2.4 tree looking for zero initializations
on the ->u data in *_read_inode() and *_new_inode().  Some set individual
fields to zero and others to non-zero values, some only set non-zero
values, and some did memset(inode->u.*_i, 0, sizeof(inode->u.*_i)) before
filling in their private data.

Patch attached, and filesystem maintainers (as applicable) CC'd.  UDF
needs a bit of looking into, because I'm not 100% sure of the code.

Cheers, Andreas
=========================================================================
diff -ru linux-2.4.3p6/fs/affs/inode.c linux-2.4.3p6-aed/fs/affs/inode.c
--- linux-2.4.3p6/fs/affs/inode.c	Wed Feb 21 19:09:45 2001
+++ linux-2.4.3p6-aed/fs/affs/inode.c	Fri Mar 23 01:11:52 2001
@@ -324,14 +324,8 @@
 	inode->i_ino     = block;
 	inode->i_mtime   = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 
-	inode->u.affs_i.i_original  = 0;
+	/* The inode->u struct is zeroed for us by new_inode() */
 	inode->u.affs_i.i_parent    = dir->i_ino;
-	inode->u.affs_i.i_zone      = 0;
-	inode->u.affs_i.i_hlink     = 0;
-	inode->u.affs_i.i_pa_cnt    = 0;
-	inode->u.affs_i.i_pa_next   = 0;
-	inode->u.affs_i.i_pa_last   = 0;
-	inode->u.affs_i.i_ec        = NULL;
 	inode->u.affs_i.i_lastblock = -1;
 
 	insert_inode_hash(inode);
diff -ru linux-2.4.3p6/fs/coda/inode.c linux-2.4.3p6-aed/fs/coda/inode.c
--- linux-2.4.3p6/fs/coda/inode.c	Fri Mar 23 10:54:50 2001
+++ linux-2.4.3p6-aed/fs/coda/inode.c	Fri Mar 23 10:57:05 2001
@@ -209,7 +209,7 @@
             return;
         }
 
-	memset(cii, 0, sizeof(struct coda_inode_info));
+	/* The inode->u struct is zeroed for us by clear_inode() */
 	list_add(&cii->c_cilist, &sbi->sbi_cihead);
         cii->c_magic = CODA_CNODE_MAGIC;
 }
diff -ru linux-2.4.3p6/fs/ext2/ialloc.c linux-2.4.3p6-aed/fs/ext2/ialloc.c
--- linux-2.4.3p6/fs/ext2/ialloc.c	Fri Dec  8 18:35:54 2000
+++ linux-2.4.3p6-aed/fs/ext2/ialloc.c	Fri Mar 23 00:47:46 2001
@@ -432,16 +416,13 @@
 	inode->i_blksize = PAGE_SIZE;	/* This is the optimal IO size (for stat), not the fs block size */
 	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
+	/* The inode->u struct is zeroed for us by new_inode() */
 	inode->u.ext2_i.i_new_inode = 1;
 	inode->u.ext2_i.i_flags = dir->u.ext2_i.i_flags;
 	if (S_ISLNK(mode))
 		inode->u.ext2_i.i_flags &= ~(EXT2_IMMUTABLE_FL | EXT2_APPEND_FL);
-	inode->u.ext2_i.i_faddr = 0;
-	inode->u.ext2_i.i_frag_no = 0;
-	inode->u.ext2_i.i_frag_size = 0;
-	inode->u.ext2_i.i_file_acl = 0;
-	inode->u.ext2_i.i_dir_acl = 0;
-	inode->u.ext2_i.i_dtime = 0;
 	inode->u.ext2_i.i_block_group = i;
 	if (inode->u.ext2_i.i_flags & EXT2_SYNC_FL)
 		inode->i_flags |= S_SYNC;
diff -ru linux-2.4.3p6/fs/hpfs/inode.c linux-2.4.3p6-aed/fs/hpfs/inode.c
--- linux-2.4.3p6/fs/hpfs/inode.c	Mon Jan 22 11:40:47 2001
+++ linux-2.4.3p6-aed/fs/hpfs/inode.c	Fri Mar 23 01:02:36 2001
@@ -68,21 +68,9 @@
 	i->i_blksize = 512;
 	i->i_size = -1;
 	i->i_blocks = -1;
-	
-	i->i_hpfs_dno = 0;
-	i->i_hpfs_n_secs = 0;
-	i->i_hpfs_file_sec = 0;
-	i->i_hpfs_disk_sec = 0;
-	i->i_hpfs_dpos = 0;
-	i->i_hpfs_dsubdno = 0;
-	i->i_hpfs_ea_mode = 0;
-	i->i_hpfs_ea_uid = 0;
-	i->i_hpfs_ea_gid = 0;
-	i->i_hpfs_ea_size = 0;
-	i->i_version = ++event;
 
-	i->i_hpfs_rddir_off = NULL;
-	i->i_hpfs_dirty = 0;
+	/* The inode->u struct is zeroed for us by clear_inode() */
+	i->i_version = ++event;
 
 	i->i_atime = 0;
 	i->i_mtime = 0;
diff -ru linux-2.4.3p6/fs/nfs/inode.c linux-2.4.3p6-aed/fs/nfs/inode.c
--- linux-2.4.3p6/fs/nfs/inode.c	Fri Mar 23 10:54:52 2001
+++ linux-2.4.3p6-aed/fs/nfs/inode.c	Fri Mar 23 10:57:06 2001
@@ -98,17 +98,11 @@
 	inode->i_blksize = inode->i_sb->s_blocksize;
 	inode->i_mode = 0;
 	inode->i_rdev = 0;
+	/* The inode->u struct is zeroed for us by clear_inode() */
-	NFS_FILEID(inode) = 0;
-	NFS_FSID(inode) = 0;
-	NFS_FLAGS(inode) = 0;
 	INIT_LIST_HEAD(&inode->u.nfs_i.read);
 	INIT_LIST_HEAD(&inode->u.nfs_i.dirty);
 	INIT_LIST_HEAD(&inode->u.nfs_i.commit);
 	INIT_LIST_HEAD(&inode->u.nfs_i.writeback);
-	inode->u.nfs_i.nread = 0;
-	inode->u.nfs_i.ndirty = 0;
-	inode->u.nfs_i.ncommit = 0;
-	inode->u.nfs_i.npages = 0;
 	NFS_CACHEINV(inode);
 	NFS_ATTRTIMEO(inode) = NFS_MINATTRTIMEO(inode);
 	NFS_ATTRTIMEO_UPDATE(inode) = jiffies;
diff -ru linux-2.4.3p6/fs/reiserfs/inode.c linux-2.4.3p6-aed/fs/reiserfs/inode.c
--- linux-2.4.3p6/fs/reiserfs/inode.c	Fri Mar 23 10:54:53 2001
+++ linux-2.4.3p6-aed/fs/reiserfs/inode.c	Fri Mar 23 10:57:06 2001
@@ -946,8 +946,7 @@
 	rdev = le32_to_cpu (sd->u.sd_rdev);
     }
 
-    /* nopack = 0, by default */
-    inode->u.reiserfs_i.nopack = 0;
+    /* The inode->u struct is zeroed for us by get_empty_inode() */
 
     pathrelse (path);
     if (S_ISREG (inode->i_mode)) {
diff -ru linux-2.4.3p6/fs/smbfs/inode.c linux-2.4.3p6-aed/fs/smbfs/inode.c
--- linux-2.4.3p6/fs/smbfs/inode.c	Fri Mar 23 10:54:53 2001
+++ linux-2.4.3p6-aed/fs/smbfs/inode.c	Fri Mar 23 10:57:06 2001
@@ -65,7 +65,7 @@
 	if (!result)
 		return result;
 	result->i_ino = fattr->f_ino;
-	memset(&(result->u.smbfs_i), 0, sizeof(result->u.smbfs_i));
+	/* The inode->u struct is zeroed for us by new_inode() */
 	smb_set_inode_attr(result, fattr);
 	if (S_ISREG(result->i_mode)) {
 		result->i_op = &smb_file_inode_operations;
diff -ru linux-2.4.3p6/fs/udf/ialloc.c linux-2.4.3p6-aed/fs/udf/ialloc.c
--- linux-2.4.3p6/fs/udf/ialloc.c	Fri Nov 17 12:35:27 2000
+++ linux-2.4.3p6-aed/fs/udf/ialloc.c	Fri Mar 23 00:49:54 2001
@@ -127,15 +127,12 @@
 	inode->i_ino = udf_get_lb_pblock(sb, UDF_I_LOCATION(inode), 0);
 	inode->i_blksize = PAGE_SIZE;
 	inode->i_blocks = 0;
-	UDF_I_LENEATTR(inode) = 0;
-	UDF_I_LENALLOC(inode) = 0;
+	/* The inode->u struct is zeroed for us by new_inode() */
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_EXTENDED_FE))
 	{
 		UDF_I_EXTENDED_FE(inode) = 1;
 		UDF_UPDATE_UDFREV(inode->i_sb, UDF_VERS_USE_EXTENDED_FE);
 	}
-	else
-		UDF_I_EXTENDED_FE(inode) = 0;
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_AD_IN_ICB))
 		UDF_I_ALLOCTYPE(inode) = ICB_FLAG_AD_IN_ICB;
 	else if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_SHORT_AD))
diff -ru linux-2.4.3p6/fs/ufs/ialloc.c linux-2.4.3p6-aed/fs/ufs/ialloc.c
--- linux-2.4.3p6/fs/ufs/ialloc.c	Thu Nov 16 14:18:26 2000
+++ linux-2.4.3p6-aed/fs/ufs/ialloc.c	Fri Mar 23 00:51:16 2001
@@ -271,7 +271,7 @@
 	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->u.ufs_i.i_flags = dir->u.ufs_i.i_flags;
-	inode->u.ufs_i.i_lastfrag = 0;
+	/* The inode->u struct is zeroed for us by new_inode() */
 
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2001-03-23 19:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-03-07 20:35 [linux-lvm] EXT2-fs panic (device lvm(58,0)): Andreas Dilger
2001-03-23  1:39 ` Stephen C. Tweedie
2001-03-23  2:04   ` Alexander Viro
2001-03-23  5:48     ` Andreas Dilger
2001-03-23 11:44       ` Alexander Viro
2001-03-23 19:11         ` Andreas Dilger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).