[ea-in-inode 0/5] Further fixes

[ea-in-inode 0/5] Further fixes

[Andreas Gruenbacher]

Hello,

here is a set of fixes for ext3 in-inode attributes:

patches/ea-xattr-nolock.diff
  No lock needed when freeing inode

  The effect of the additional lock taking is very minor, but
  it's still unnecessary.

patches/ea-xattr-update-sb.diff
  Set the EXT3_FEATURE_COMPAT_EXT_ATTR for in-inode xattrs

  The EXT_ATTR filesystem feature was set for xattr blocks, but not
  for in-inode attributes.

patches/ea-xattr-doc.diff
  Documentation fix

patches/ea-xattr-no-extra_isize.diff
  Fix i_extra_isize check

  Filesystem corruption fix.

patches/ea-xattr-reserved-inodes.diff
  Disallow in-inode attributes for reserved inodes

  Filesystem corruption fix.


Regards,
--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[patch 1/5] No lock needed when freeing inode

[Andreas Gruenbacher]

[-- Attachment #1: ea-xattr-nolock.diff --]
[-- Type: text/plain, Size: 867 bytes --]

ext3_xattr_delete_inode is called from ext3_free_inode which always
has exclusive access to the inode, so there is no need to take the
xattr semaphore.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/xattr.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/xattr.c
+++ linux-2.6.11-latest/fs/ext3/xattr.c
@@ -1066,7 +1066,6 @@ ext3_xattr_delete_inode(handle_t *handle
 {
 	struct buffer_head *bh = NULL;
 
-	down_write(&EXT3_I(inode)->xattr_sem);
 	if (!EXT3_I(inode)->i_file_acl)
 		goto cleanup;
 	bh = sb_bread(inode->i_sb, EXT3_I(inode)->i_file_acl);
@@ -1088,7 +1087,6 @@ ext3_xattr_delete_inode(handle_t *handle
 
 cleanup:
 	brelse(bh);
-	up_write(&EXT3_I(inode)->xattr_sem);
 }
 
 /*

--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[patch 4/5] Fix i_extra_isize check

[Andreas Gruenbacher]

[-- Attachment #1: ea-xattr-no-extra_isize.diff --]
[-- Type: text/plain, Size: 2134 bytes --]

We are checking for (EXT3_SB(inode->i_sb)->s_inode_size <=
EXT3_GOOD_OLD_INODE_SIZE) to find out if we can set in-inode attributes;
the test fails for inodes that have been created before the ea-in-inode
patch. Those inodes have (i_extra_isize == 0), so we end up with the
attributes overlapping the i_extra_isize field. Checking for
(i_extra_isize == 0) instead fixes this case.

The EXT3_STATE_XATTR flag is only set if (i_extra_isize > 0) and the
inodes has in-inode attributes, so that is enough in the first two
tests.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/xattr.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/xattr.c
+++ linux-2.6.11-latest/fs/ext3/xattr.c
@@ -272,8 +272,7 @@ ext3_xattr_ibody_get(struct inode *inode
 	void *end;
 	int error;
 
-	if (EXT3_SB(inode->i_sb)->s_inode_size <= EXT3_GOOD_OLD_INODE_SIZE ||
-	    !(EXT3_I(inode)->i_state & EXT3_STATE_XATTR))
+	if (!(EXT3_I(inode)->i_state & EXT3_STATE_XATTR))
 		return -ENODATA;
 	error = ext3_get_inode_loc(inode, &iloc);
 	if (error)
@@ -399,8 +398,7 @@ ext3_xattr_ibody_list(struct inode *inod
 	void *end;
 	int error;
 
-	if (EXT3_SB(inode->i_sb)->s_inode_size <= EXT3_GOOD_OLD_INODE_SIZE ||
-	    !(EXT3_I(inode)->i_state & EXT3_STATE_XATTR))
+	if (!(EXT3_I(inode)->i_state & EXT3_STATE_XATTR))
 		return 0;
 	error = ext3_get_inode_loc(inode, &iloc);
 	if (error)
@@ -865,7 +863,7 @@ ext3_xattr_ibody_find(struct inode *inod
 	struct ext3_inode *raw_inode;
 	int error;
 
-	if (EXT3_SB(inode->i_sb)->s_inode_size <= EXT3_GOOD_OLD_INODE_SIZE)
+	if (EXT3_I(inode)->i_extra_isize == 0)
 		return 0;
 	raw_inode = ext3_raw_inode(&is->iloc);
 	header = IHDR(inode, raw_inode);
@@ -896,7 +894,7 @@ ext3_xattr_ibody_set(handle_t *handle, s
 	struct ext3_xattr_search *s = &is->s;
 	int error;
 
-	if (EXT3_SB(inode->i_sb)->s_inode_size <= EXT3_GOOD_OLD_INODE_SIZE)
+	if (EXT3_I(inode)->i_extra_isize == 0)
 		return -ENOSPC;
 	error = ext3_xattr_set_entry(i, s);
 	if (error)

--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[patch 3/5] Documentation fix

[Andreas Gruenbacher]

[-- Attachment #1: ea-xattr-doc.diff --]
[-- Type: text/plain, Size: 903 bytes --]

In-inode xattr entry descriptors are unsorted.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/xattr.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/xattr.c
+++ linux-2.6.11-latest/fs/ext3/xattr.c
@@ -37,9 +37,9 @@
  *   | value 2          | |
  *   +------------------+
  *
- * The header is followed by multiple entry descriptors. Descriptors are
- * kept sorted. The attribute values are aligned to the end of the block
- * in no specific order.
+ * The header is followed by multiple entry descriptors. In disk blocks, the
+ * entry descriptors are kept sorted. In inodes, they are unsorted. The
+ * attribute values are aligned to the end of the block in no specific order.
  *
  * Locking strategy
  * ----------------

--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Gruenbacher]

[-- Attachment #1: ea-xattr-reserved-inodes.diff --]
[-- Type: text/plain, Size: 2111 bytes --]

When creating a filesystem with inodes bigger than 128 bytes, mke2fs
fails to clear out bytes beyond EXT3_GOOD_OLD_INODE_SIZE in all inodes
it creates (the journal, the filesystem root, and lost+found). We would
require a zeroed-out i_extra_isize field but we don't get it, so
disallow in-inode attributes for those inodes.

Add an i_extra_isize sanity check.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/inode.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/inode.c
+++ linux-2.6.11-latest/fs/ext3/inode.c
@@ -2493,15 +2493,30 @@ void ext3_read_inode(struct inode * inod
 		ei->i_data[block] = raw_inode->i_block[block];
 	INIT_LIST_HEAD(&ei->i_orphan);
 
-	ei->i_extra_isize =
-		(EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) ?
-		le16_to_cpu(raw_inode->i_extra_isize) : 0;
-	if (ei->i_extra_isize) {
-		__le32 *magic = (void *)raw_inode + EXT3_GOOD_OLD_INODE_SIZE +
-				ei->i_extra_isize;
-		if (le32_to_cpu(*magic))
-			 ei->i_state |= EXT3_STATE_XATTR;
-	}
+	if (inode->i_ino >= EXT3_FIRST_INO(inode->i_sb) + 1 &&
+	    EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) {
+		/*
+		 * When mke2fs creates big inodes it does not zero out
+		 * the unused bytes above EXT3_GOOD_OLD_INODE_SIZE,
+		 * so ignore those first few inodes.
+		 */
+		ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
+		if (EXT3_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
+		    EXT3_INODE_SIZE(inode->i_sb))
+			goto bad_inode;
+		if (ei->i_extra_isize == 0) {
+			/* The extra space is currently unused. Use it. */
+			ei->i_extra_isize = sizeof(struct ext3_inode) -
+					    EXT3_GOOD_OLD_INODE_SIZE;
+		} else {
+			__le32 *magic = (void *)raw_inode +
+					EXT3_GOOD_OLD_INODE_SIZE +
+					ei->i_extra_isize;
+			if (*magic == cpu_to_le32(EXT3_XATTR_MAGIC))
+				 ei->i_state |= EXT3_STATE_XATTR;
+		}
+	} else
+		ei->i_extra_isize = 0;
 
 	if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &ext3_file_inode_operations;

--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

[patch 2/5] Set the EXT3_FEATURE_COMPAT_EXT_ATTR for in-inode xattrs

[Andreas Gruenbacher]

[-- Attachment #1: ea-xattr-update-sb.diff --]
[-- Type: text/plain, Size: 861 bytes --]

The xattr feature was only set when creating an xattr block. Also set it
when creating in-inode xattrs.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.11-latest/fs/ext3/xattr.c
===================================================================
--- linux-2.6.11-latest.orig/fs/ext3/xattr.c
+++ linux-2.6.11-latest/fs/ext3/xattr.c
@@ -823,7 +823,6 @@ getblk_failed:
 			error = ext3_journal_dirty_metadata(handle, new_bh);
 			if (error)
 				goto cleanup;
-			ext3_xattr_update_super_block(handle, sb);
 		}
 	}
 
@@ -1001,6 +1000,7 @@ ext3_xattr_set_handle(handle_t *handle, 
 		}
 	}
 	if (!error) {
+		ext3_xattr_update_super_block(handle, inode->i_sb);
 		inode->i_ctime = CURRENT_TIME_SEC;
 		error = ext3_mark_iloc_dirty(handle, inode, &is.iloc);
 		/*

--
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3922 bytes --]

On Jan 20, 2005  03:01 +0100, Andreas Gruenbacher wrote:
> When creating a filesystem with inodes bigger than 128 bytes, mke2fs
> fails to clear out bytes beyond EXT3_GOOD_OLD_INODE_SIZE in all inodes
> it creates (the journal, the filesystem root, and lost+found). We would
> require a zeroed-out i_extra_isize field but we don't get it, so
> disallow in-inode attributes for those inodes.
> 
> Add an i_extra_isize sanity check.

I'm not sure I agree with this patch.  It definitely resolves a problem
I had with the previous patch (4/5), namely that for inodes that were
created in a large-inode filesystem using a kernel w/o the large-inode
patch we didn't save EAs into the large-inode space at all.  With this
patch we will start using that space.

It is debatable whether we should mark inodes bad if the i_extra_isize
field is bad, or if we should just initialize i_extra_isize in that case.
On one hand there is very little else we can use to validate the on-disk
inodes, but on the other hand the EA data isn't critical to reading the
inode so this would seem to introduce additional failure cases.

I'm not sure whether an old e2fsck will also clear out the space in a
large inode that it writes or not (I know that the patched one we use
validates i_extra_isize).  If it doesn't it may be that there would be a
largish number of inodes that are marked bad because of this, so having
the kernel fix this would be good.  If the old e2fsck zeroes the large
inodes properly it would be prudent to have an ext3_error() here so that
the disk corruption triggers an e2fsck the next time the system starts.

For the root and lost+found inodes it looks like we can never store an
EA in the extra part of the inode regardless of whether i_extra_isize is
good or not.  If a bad value is found we could just initialize it and
start using that space (though not print an ext3_error() in that case,
an ext3_warning() if anything since this is probably the fault of mke2fs).

> Index: linux-2.6.11-latest/fs/ext3/inode.c
> ===================================================================
> --- linux-2.6.11-latest.orig/fs/ext3/inode.c
> +++ linux-2.6.11-latest/fs/ext3/inode.c
> @@ -2493,15 +2493,30 @@ void ext3_read_inode(struct inode * inod
>  		ei->i_data[block] = raw_inode->i_block[block];
>  	INIT_LIST_HEAD(&ei->i_orphan);
>  
> -	ei->i_extra_isize =
> -		(EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) ?
> -		le16_to_cpu(raw_inode->i_extra_isize) : 0;
> -	if (ei->i_extra_isize) {
> -		__le32 *magic = (void *)raw_inode + EXT3_GOOD_OLD_INODE_SIZE +
> -				ei->i_extra_isize;
> -		if (le32_to_cpu(*magic))
> -			 ei->i_state |= EXT3_STATE_XATTR;
> -	}
> +	if (inode->i_ino >= EXT3_FIRST_INO(inode->i_sb) + 1 &&
> +	    EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE) {
> +		/*
> +		 * When mke2fs creates big inodes it does not zero out
> +		 * the unused bytes above EXT3_GOOD_OLD_INODE_SIZE,
> +		 * so ignore those first few inodes.
> +		 */
> +		ei->i_extra_isize = le16_to_cpu(raw_inode->i_extra_isize);
> +		if (EXT3_GOOD_OLD_INODE_SIZE + ei->i_extra_isize >
> +		    EXT3_INODE_SIZE(inode->i_sb))
> +			goto bad_inode;
> +		if (ei->i_extra_isize == 0) {
> +			/* The extra space is currently unused. Use it. */
> +			ei->i_extra_isize = sizeof(struct ext3_inode) -
> +					    EXT3_GOOD_OLD_INODE_SIZE;
> +		} else {
> +			__le32 *magic = (void *)raw_inode +
> +					EXT3_GOOD_OLD_INODE_SIZE +
> +					ei->i_extra_isize;
> +			if (*magic == cpu_to_le32(EXT3_XATTR_MAGIC))
> +				 ei->i_state |= EXT3_STATE_XATTR;
> +		}
> +	} else
> +		ei->i_extra_isize = 0;
>  
>  	if (S_ISREG(inode->i_mode)) {
>  		inode->i_op = &ext3_file_inode_operations;

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Gruenbacher]

On Thursday 20 January 2005 13:16, Andreas Dilger wrote:
> On Jan 20, 2005  03:01 +0100, Andreas Gruenbacher wrote:
> > When creating a filesystem with inodes bigger than 128 bytes, mke2fs
> > fails to clear out bytes beyond EXT3_GOOD_OLD_INODE_SIZE in all inodes
> > it creates (the journal, the filesystem root, and lost+found). We would
> > require a zeroed-out i_extra_isize field but we don't get it, so
> > disallow in-inode attributes for those inodes.
> >
> > Add an i_extra_isize sanity check.
>
> I'm not sure I agree with this patch.  It definitely resolves a problem
> I had with the previous patch (4/5), namely that for inodes that were
> created in a large-inode filesystem using a kernel w/o the large-inode
> patch we didn't save EAs into the large-inode space at all.  With this
> patch we will start using that space.

The ea-in-inode patch totally relies on getting all the available inode space 
cleared out by the kernel (or mke2fs, or e2fsck). If this is not the case for 
any inode we find, then i_extra_isize may contain a random number, and we've 
just lost, period: There is no way of sanitizing a random i_extra_isize; we 
cannot know what the right number would be.

We have two options for going forward from here: (a) depend on the fact that 
all inodes have been cleared out for us, or (b) don't rely on that, fix 
e2fsck before going any further, and have the fixed mke2fs and e2fsck set a 
filesystem feature flag that tells us that all the extra space in all inodes 
is "sane".

For (a) we can make an (ugly) exception because we know which inodes mke2fs 
created. As far as I can tell we can also fully rely on the kernel having 
cleared out the extra space for us. e2fsck may currently fail to move the 
extra space around when moving inodes, but that did not matter without 
ea-in-inode, and if we fix it now, that's probably good enough. We don't 
really care which inode's zeroed extra space we look at without ea-in-inode.

> It is debatable whether we should mark inodes bad if the i_extra_isize
> field is bad, or if we should just initialize i_extra_isize in that case.

IMHO it's not debatable. Taking an i_extra_isize that looks odd and simply 
changing it to something we think is better is a really bad idea. We might as 
well find an unusual but still valid i_extra_isize and use it, then a future 
patch allocates a few more fields in the inode, and suddenly we start to see 
garbage.

> On one hand there is very little else we can use to validate the on-disk
> inodes, but on the other hand the EA data isn't critical to reading the
> inode so this would seem to introduce additional failure cases.

You may have an access acl on the inode. Not being able to read an access acl 
is a clear sign of trouble. The same applies for everything else in the 
system.* and security.* namespaces, at least.

> I'm not sure whether an old e2fsck will also clear out the space in a
> large inode that it writes or not (I know that the patched one we use
> validates i_extra_isize).  If it doesn't it may be that there would be a
> largish number of inodes that are marked bad because of this, so having
> the kernel fix this would be good.

Repairing the filesystem has no place in the kernel code. If what you assume 
is the case (and I sure would have assumed you to check before proposing the 
kernel patch for inclusion), we have to go for the filesystem feature flag 
approach.

> If the old e2fsck zeroes the large 
> inodes properly it would be prudent to have an ext3_error() here so that
> the disk corruption triggers an e2fsck the next time the system starts.

That's even better, yes.

> For the root and lost+found inodes it looks like we can never store an
> EA in the extra part of the inode regardless of whether i_extra_isize is
> good or not.  If a bad value is found we could just initialize it and
> start using that space (though not print an ext3_error() in that case,
> an ext3_warning() if anything since this is probably the fault of mke2fs).

I disagree. We cannot just use the space when we think the inode is corrupted.

Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4260 bytes --]

On Jan 20, 2005  14:29 +0100, Andreas Gruenbacher wrote:
> The ea-in-inode patch totally relies on getting all the available inode space 
> cleared out by the kernel (or mke2fs, or e2fsck). If this is not the case for 
> any inode we find, then i_extra_isize may contain a random number, and we've 
> just lost, period: There is no way of sanitizing a random i_extra_isize; we 
> cannot know what the right number would be.

The large-inode support is designed to allow different amounts of
"fixed" optional data (i.e. what is stored inside i_extra_isize),
so it is valid to set this to 4 (i.e. just enough to hold i_extra_isize
itself) and store the EA data after that.  Any code which reads "fixed"
fields from a large inode (e.g. i_mtime_nsec) needs to validate that
i_extra_isize on that inode is large enough for that data to actually
be in the fixed area in the large inode.

If the kernel is setting i_extra_isize > 4 (i.e. it is storing optional
fields there like i_mtime_msb_and_ns) it should/is-able-to also initialize
those values since it should know what they are or they shouldn't be in
struct ext3_inode.

The whole point of i_extra_isize is that it is possible for inodes
to have different amounts of the optional fixed fields in each large
inode, depending on what the kernel that wrote the inode knew about.
So any value for i_extra_isize is valid as long as those fields
are initialized.  If we arbitrarily set i_extra_isize = 4 instead of
leaving the bad value this is no different than waiting for e2fsck to
do the same.

> > It is debatable whether we should mark inodes bad if the i_extra_isize
> > field is bad, or if we should just initialize i_extra_isize in that case.
> 
> IMHO it's not debatable. Taking an i_extra_isize that looks odd and simply 
> changing it to something we think is better is a really bad idea.
> 
> You may have an access acl on the inode. Not being able to read an access acl 
> is a clear sign of trouble. The same applies for everything else in the 
> system.* and security.* namespaces, at least.

Well, I said it was debatable and we're having a debate ;-).  I don't have
a strong opinion either way.  If we ext3_error() in this case at least we
will check the fs on the next boot (which will just zero i_extra_isize)
instead of never doing anything to resolve the situation.

> > For the root and lost+found inodes it looks like we can never store an
> > EA in the extra part of the inode regardless of whether i_extra_isize is
> > good or not.  If a bad value is found we could just initialize it and
> > start using that space (though not print an ext3_error() in that case,
> > an ext3_warning() if anything since this is probably the fault of mke2fs).
> 
> I disagree. We cannot just use the space when we think the inode is corrupted.

But as your patch stands it doesn't ever check if i_extra_isize is valid
for the root or lost+found inode.  It just always sets i_extra_isize = 0
and never uses it.  Given that the root inode is fairly high-traffic it
makes sense to use the faster EA space if it is available.

If these inodes have a BAD i_extra_isize it is OK to skip it, but I'm
not so keen to have an ext3_error() there.  If the user doesn't have an
e2fsck with ea-in-inode support there isn't anything they can do to fix
it and they will get a full e2fsck on each boot.

Even so, for the effort of setting i_extra_isize = 4 (or larger if we
initialize the fixed fields) we can do the equivalent of what e2fsck will
do when it finds a bogus value.


The good news is that we can still apply your patch as-is and address my
concerns later since this is a transient issue.  Also, given that there
are probably only a handful of filesystems in the world using large inodes
(excluding Lustre filesystems which aren't affected by this) I don't think
it is a pressing issue yet.  I'm going to be away for 2 weeks, so I'll say
accept this patch as is and we can look at it again when I get back, and
maybe Ted and Stephen will have weighed in on this issue also.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 5/5] Disallow in-inode attributes for reserved inodes

[Andreas Gruenbacher]

On Friday 21 January 2005 00:05, Andreas Dilger wrote:
> [...]
> But as your patch stands it doesn't ever check if i_extra_isize is valid
> for the root or lost+found inode.  It just always sets i_extra_isize = 0
	(that's the in-memory i_extra_isize)
> and never uses it.  Given that the root inode is fairly high-traffic it
> makes sense to use the faster EA space if it is available.

It's only a single block we're talking about, not all the overhead you run 
into with huge amounts of attributes in many xattr disk blocks. It sure would 
be much cleaner to use the root inode's in-inode space like with all other 
inodes, but performance wise I don't think it matters.

> If these inodes have a BAD i_extra_isize it is OK to skip it, but I'm
> not so keen to have an ext3_error() there.  If the user doesn't have an
> e2fsck with ea-in-inode support there isn't anything they can do to fix
> it and they will get a full e2fsck on each boot.

Agreed, that would be really bad. We should get e2fsck fixed ASAP.

> Even so, for the effort of setting i_extra_isize = 4 (or larger if we
> initialize the fixed fields) we can do the equivalent of what e2fsck will
> do when it finds a bogus value.

We cannot ask the user, and we don't have the kind of global view that e2fsck 
has. Something different may be messed up, and may have lead to the 
corruption. It's unlikely, but not impossible.

Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [ea-in-inode 0/5] Further fixes

[Stephen C. Tweedie]

Hi Andreas,

On Thu, 2005-01-20 at 02:01, Andreas Gruenbacher wrote:

> here is a set of fixes for ext3 in-inode attributes:

Obvious first question --- have these diffs survived the same
torture-by-tridgell that the previous batch suffered?

Cheers,
 Stephen

Re: [ea-in-inode 0/5] Further fixes

[Andreas Gruenbacher]

On Fri, 2005-01-21 at 23:58, Stephen C. Tweedie wrote:
> Hi Andreas,
> 
> On Thu, 2005-01-20 at 02:01, Andreas Gruenbacher wrote:
> 
> > here is a set of fixes for ext3 in-inode attributes:
> 
> Obvious first question --- have these diffs survived the same
> torture-by-tridgell that the previous batch suffered?

No. The fixes are a lot less intrusive than the full xattr rework
though. I obviously ran tests; this included dbench.

Tridge, can you beat the code some more?

Andrew has the five fixes in 2.6.11-rc1-mm2.

-- Andreas.

Re: [ea-in-inode 0/5] Further fixes

[Andrew Tridgell]

Andreas,

 > Tridge, can you beat the code some more?
 > Andrew has the five fixes in 2.6.11-rc1-mm2.

sorry for the delay. I've started to test 2.6.11-rc1-mm2 tonight. No
problems so far.

Cheers, Tridge

Re: [ea-in-inode 0/5] Further fixes

[Andrew Tridgell]

Andreas,

 > Tridge, can you beat the code some more?
 > 
 > Andrew has the five fixes in 2.6.11-rc1-mm2.

It seemed to pass dbench runs OK, but then I started simultaneously
running dbench and nbench on two different disks (I have a new test
machine with more disks available). I am getting failures like this:

Jan 23 06:54:38 dev4-003 kernel: journal_bmap: journal block not found at offset 1036 on sdc1
Jan 23 06:54:38 dev4-003 kernel: Aborting journal on device sdc1.


Jan 23 13:19:43 dev4-003 kernel: journal_bmap: journal block not found at offset 1036 on sdd1
Jan 23 13:19:43 dev4-003 kernel: Aborting journal on device sdd1.
Jan 23 13:19:43 dev4-003 kernel: EXT3-fs error (device sdd1) in start_transaction: Readonly filesystem


The first failure was on the disk I am using for nbench (sdc). The
second is during a later run on the disk I am using the dbench
(sdd). I rebooted between the runs. It's interesting that its failing
at exactly the same offset both times. Is there anything magic about
offset 1036?

The new test machine is a 4 way PIII, with 4G ram, and 4 36G SCSI
disks. The test machine I have been using previously was a 2 way
(+hyperthreaded) Xeon.

I'll see if I can reproduce the problem with just dbench or just
nbench.

Cheers, Tridge

Re: [ea-in-inode 0/5] Further fixes

[Andreas Gruenbacher]

Hello,

On Sunday 23 January 2005 23:09, Andrew Tridgell wrote:
> Andreas,
>
>  > Tridge, can you beat the code some more?
>  >
>  > Andrew has the five fixes in 2.6.11-rc1-mm2.
>
> It seemed to pass dbench runs OK, but then I started simultaneously
> running dbench and nbench on two different disks (I have a new test
> machine with more disks available). I am getting failures like this:
>
> Jan 23 06:54:38 dev4-003 kernel: journal_bmap: journal block not found at
> offset 1036 on sdc1 Jan 23 06:54:38 dev4-003 kernel: Aborting journal on
> device sdc1.

Are you using data journaling on that filesystem? Does this test pass with the 
patches backed out? With an external journal?

Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [ea-in-inode 0/5] Further fixes

[Andreas Gruenbacher]

On Sunday 23 January 2005 23:58, Andreas Gruenbacher wrote:
> > Jan 23 06:54:38 dev4-003 kernel: journal_bmap: journal block not found at
> > offset 1036 on sdc1 Jan 23 06:54:38 dev4-003 kernel: Aborting journal on
> > device sdc1.
>
> Are you using data journaling on that filesystem? Does this test pass with
> the patches backed out? With an external journal?

There are 12 direct and 1024 indirect blocks on a filesystem with 4k 
blocksize, so block 1036 should be the first double-indirect block. It may be 
that something is messing up the double-indirect link or one of its fields. 
Interesting. Could you maybe try this patch as well?

Index: linux-2.6.11-rc1-mm2/fs/ext3/inode.c
===================================================================
--- linux-2.6.11-rc1-mm2.orig/fs/ext3/inode.c
+++ linux-2.6.11-rc1-mm2/fs/ext3/inode.c
@@ -2653,7 +2653,7 @@ static int ext3_do_update_inode(handle_t
 	} else for (block = 0; block < EXT3_N_BLOCKS; block++)
 		raw_inode->i_block[block] = ei->i_data[block];
 
-	if (EXT3_INODE_SIZE(inode->i_sb) > EXT3_GOOD_OLD_INODE_SIZE)
+	if (ei->i_extra_isize)
 		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
 
 	BUFFER_TRACE(bh, "call ext3_journal_dirty_metadata");

Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [ea-in-inode 0/5] Further fixes

[Andrew Tridgell]

Andreas,

I'm starting to think the bug I saw is hardware error. I just got this
while trying to reproduce it tonight:

Jan 24 02:43:32 dev4-003 kernel: qlogicfc0 : abort failed
Jan 24 02:43:32 dev4-003 kernel: qlogicfc0 : firmware status is 4000 4
Jan 24 02:43:32 dev4-003 kernel: scsi: Device offlined - not ready after error recovery: host 3 
channel 0 id 1 lun 0

I'll see if I can get this confirmed tomorrow.

Cheers, Tridge

Re: [ea-in-inode 0/5] Further fixes

[Christoph Hellwig]

On Mon, Jan 24, 2005 at 10:24:55PM +1100, Andrew Tridgell wrote:
> Andreas,
> 
> I'm starting to think the bug I saw is hardware error. I just got this
> while trying to reproduce it tonight:
> 
> Jan 24 02:43:32 dev4-003 kernel: qlogicfc0 : abort failed
> Jan 24 02:43:32 dev4-003 kernel: qlogicfc0 : firmware status is 4000 4
> Jan 24 02:43:32 dev4-003 kernel: scsi: Device offlined - not ready after error recovery: host 3 
> channel 0 id 1 lun 0
> 
> I'll see if I can get this confirmed tomorrow.

Don't use the qlogicfc driver ever but qla2xxx instead.  It's only still
in the kernel tree because davem's a lazy bastard :)

Re: [ea-in-inode 0/5] Further fixes

[Andreas Gruenbacher]

On Mon, 2005-01-24 at 12:24, Andrew Tridgell wrote:
> Andreas,
> 
> I'm starting to think the bug I saw is hardware error. [...]

The patch in my prevous message (Mon, 24 Jan 2005 00:32:16 +0100) still
makes sense, if only for cleanliness: Without it, the i_extra_isize
field of reserved inodes is zeroed even though we're not allowing
in-inode attributes there. It's probably better to leave the garbage in
these inodes untouched; we could in theory disable in-inode attributes
for arbitrary inodes with this change.

Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH

memory leak in 2.6.11-rc2

[Andrew Tridgell]

I've fixed up the problems I had with raid, and am now testing the
recent xattr changes with dbench and nbench.

The problem I've hit now is a severe memory leak. I have applied the
patch from Linus for the leak in free_pipe_info(), and still I'm
leaking memory at the rate of about 100Mbyte/minute.

I've tested with both 2.6.11-rc2 and with 2.6.11-rc1-mm2, both with
the pipe leak fix. The setup is:

 - 4 way PIII with 4G ram
 - qla2200 adapter with ibm fastt200 disk array 
 - running dbench -x and nbench on separate disks, in a loop

The oom killer kicks in after about 30 minutes. Naturally the oom
killer decided to kill my sshd, which was running vmstat :-) 

Killing the dbench and nbench processes does not recovery the memory.
Here is what it looks like after I kill all processes except a sshd
after about 15 minutes of running:

             total       used       free     shared    buffers     cached
Mem:       3734304    2471608    1262696          0      26820     921440
-/+ buffers/cache:    1523348    2210956
Swap:      4194272          0    4194272

so we've lost nearly 1.5G in that time.

here is /proc/meminfo:

dev4-003:~# cat /proc/meminfo 
MemTotal:      3734304 kB
MemFree:       1266944 kB
Buffers:         27000 kB
Cached:         921464 kB
SwapCached:          0 kB
Active:         292524 kB
Inactive:      2108928 kB
HighTotal:     2850752 kB
HighFree:       472768 kB
LowTotal:       883552 kB
LowFree:        794176 kB
SwapTotal:     4194272 kB
SwapFree:      4194272 kB
Dirty:               4 kB
Writeback:           0 kB
Mapped:           6872 kB
Slab:            54236 kB
CommitLimit:   6061424 kB
Committed_AS:    41660 kB
PageTables:        344 kB
VmallocTotal:   114680 kB
VmallocUsed:      2204 kB
VmallocChunk:   112472 kB

and here is /proc/slabinfo 

slabinfo - version: 2.1
# name            <active_objs> <num_objs> <objsize> <objperslab> <pagesperslab> : tunables <batchcount> <limit> <sharedfactor> : slabdata
 <active_slabs> <num_slabs> <sharedavail>
ip_fib_alias           9    226     16  226    1 : tunables  120   60    8 : slabdata      1      1      0
ip_fib_hash            9    119     32  119    1 : tunables  120   60    8 : slabdata      1      1      0
fib6_nodes             7    119     32  119    1 : tunables  120   60    8 : slabdata      1      1      0
ip6_dst_cache          7     36    224   18    1 : tunables  120   60    8 : slabdata      2      2      0
ndisc_cache            1     25    160   25    1 : tunables  120   60    8 : slabdata      1      1      0
rawv6_sock             6      6    672    6    1 : tunables   54   27    8 : slabdata      1      1      0
udpv6_sock             0      0    640    6    1 : tunables   54   27    8 : slabdata      0      0      0
tcpv6_sock             4      6   1184    6    2 : tunables   24   12    8 : slabdata      1      1      0
unix_sock              2      9    416    9    1 : tunables   54   27    8 : slabdata      1      1      0
ip_mrt_cache           0      0     96   41    1 : tunables  120   60    8 : slabdata      0      0      0
tcp_tw_bucket          0      0    128   31    1 : tunables  120   60    8 : slabdata      0      0      0
tcp_bind_bucket        2    226     16  226    1 : tunables  120   60    8 : slabdata      1      1      0
tcp_open_request       0      0     96   41    1 : tunables  120   60    8 : slabdata      0      0      0
inet_peer_cache        1     61     64   61    1 : tunables  120   60    8 : slabdata      1      1      0
secpath_cache          0      0    128   31    1 : tunables  120   60    8 : slabdata      0      0      0
xfrm_dst_cache         0      0    256   15    1 : tunables  120   60    8 : slabdata      0      0      0
ip_dst_cache           6     15    256   15    1 : tunables  120   60    8 : slabdata      1      1      0
arp_cache              1     25    160   25    1 : tunables  120   60    8 : slabdata      1      1      0
raw_sock               5      7    512    7    1 : tunables   54   27    8 : slabdata      1      1      0
udp_sock               3      8    512    8    1 : tunables   54   27    8 : slabdata      1      1      0
tcp_sock               1      7   1056    7    2 : tunables   24   12    8 : slabdata      1      1      0
flow_cache             0      0     96   41    1 : tunables  120   60    8 : slabdata      0      0      0
qla2xxx_srbs         256    310    128   31    1 : tunables  120   60    8 : slabdata     10     10      0
scsi_cmd_cache        22     22    352   11    1 : tunables   54   27    8 : slabdata      2      2      0
cfq_ioc_pool           0      0     24  156    1 : tunables  120   60    8 : slabdata      0      0      0
cfq_pool               0      0    104   38    1 : tunables  120   60    8 : slabdata      0      0      0
crq_pool               0      0     56   70    1 : tunables  120   60    8 : slabdata      0      0      0
deadline_drq           0      0     52   75    1 : tunables  120   60    8 : slabdata      0      0      0
as_arq                61     61     64   61    1 : tunables  120   60    8 : slabdata      1      1      0
mqueue_inode_cache      1      7    512    7    1 : tunables   54   27    8 : slabdata      1      1      0
devfsd_event           0      0     20  185    1 : tunables  120   60    8 : slabdata      0      0      0
ext2_inode_cache       0      0    456    8    1 : tunables   54   27    8 : slabdata      0      0      0
ext2_xattr             0      0     48   81    1 : tunables  120   60    8 : slabdata      0      0      0
journal_handle         2    185     20  185    1 : tunables  120   60    8 : slabdata      1      1      0
journal_head        3316 155844     48   81    1 : tunables  120   60    8 : slabdata   1924   1924      0
revoke_table          14    290     12  290    1 : tunables  120   60    8 : slabdata      1      1      0
revoke_record          0      0     16  226    1 : tunables  120   60    8 : slabdata      0      0      0
ext3_inode_cache    3626   3745    520    7    1 : tunables   54   27    8 : slabdata    535    535      0
ext3_xattr             0      0     48   81    1 : tunables  120   60    8 : slabdata      0      0      0
dnotify_cache          0      0     20  185    1 : tunables  120   60    8 : slabdata      0      0      0
eventpoll_pwq          0      0     36  107    1 : tunables  120   60    8 : slabdata      0      0      0
eventpoll_epi          0      0     96   41    1 : tunables  120   60    8 : slabdata      0      0      0
kioctx                 0      0    192   20    1 : tunables  120   60    8 : slabdata      0      0      0
kiocb                  0      0    128   31    1 : tunables  120   60    8 : slabdata      0      0      0
fasync_cache           0      0     16  226    1 : tunables  120   60    8 : slabdata      0      0      0
shmem_inode_cache      8     18    416    9    1 : tunables   54   27    8 : slabdata      2      2      0
posix_timers_cache      0      0    104   38    1 : tunables  120   60    8 : slabdata      0      0      0
uid_cache              1     61     64   61    1 : tunables  120   60    8 : slabdata      1      1      0
sgpool-128            32     32   2048    2    1 : tunables   24   12    8 : slabdata     16     16      0
sgpool-64             32     32   1024    4    1 : tunables   54   27    8 : slabdata      8      8      0
sgpool-32             32     32    512    8    1 : tunables   54   27    8 : slabdata      4      4      0
sgpool-16             32     45    256   15    1 : tunables  120   60    8 : slabdata      3      3      0
sgpool-8              50     62    128   31    1 : tunables  120   60    8 : slabdata      2      2      0
blkdev_ioc            40    270     28  135    1 : tunables  120   60    8 : slabdata      2      2      0
blkdev_queue          37     50    380   10    1 : tunables   54   27    8 : slabdata      5      5      0
blkdev_requests       70     81    148   27    1 : tunables  120   60    8 : slabdata      3      3      0
biovec-(256)         256    256   3072    2    2 : tunables   24   12    8 : slabdata    128    128      0
biovec-128           256    260   1536    5    2 : tunables   24   12    8 : slabdata     52     52      0
biovec-64            256    260    768    5    1 : tunables   54   27    8 : slabdata     52     52      0
biovec-16            260    260    192   20    1 : tunables  120   60    8 : slabdata     13     13      0
biovec-4             256    305     64   61    1 : tunables  120   60    8 : slabdata      5      5      0
biovec-1             322    452     16  226    1 : tunables  120   60    8 : slabdata      2      2      0
bio                  303    328     96   41    1 : tunables  120   60    8 : slabdata      8      8      0
file_lock_cache        0      0     92   43    1 : tunables  120   60    8 : slabdata      0      0      0
sock_inode_cache      26     60    384   10    1 : tunables   54   27    8 : slabdata      6      6      0
skbuff_head_cache    137    475    160   25    1 : tunables  120   60    8 : slabdata     19     19      0
sock                   4     11    352   11    1 : tunables   54   27    8 : slabdata      1      1      0
key_jar               90    123     96   41    1 : tunables  120   60    8 : slabdata      3      3      0
proc_inode_cache     265    336    336   12    1 : tunables   54   27    8 : slabdata     28     28      0
sigqueue               1     27    148   27    1 : tunables  120   60    8 : slabdata      1      1      0
radix_tree_node     8235   8750    276   14    1 : tunables   54   27    8 : slabdata    625    625      0
bdev_cache            14     18    448    9    1 : tunables   54   27    8 : slabdata      2      2      0
sysfs_dir_cache     2837   2996     36  107    1 : tunables  120   60    8 : slabdata     28     28      0
mnt_cache             21     41     96   41    1 : tunables  120   60    8 : slabdata      1      1      0
inode_cache          946    948    320   12    1 : tunables   54   27    8 : slabdata     79     79      0
dentry_cache       10903  11032    140   28    1 : tunables  120   60    8 : slabdata    394    394      0
filp                 300    750    160   25    1 : tunables  120   60    8 : slabdata     30     30      0
names_cache            3      3   4096    1    1 : tunables   24   12    8 : slabdata      3      3      0
idr_layer_cache       80     87    136   29    1 : tunables  120   60    8 : slabdata      3      3      0
buffer_head       593099 649500     52   75    1 : tunables  120   60    8 : slabdata   8660   8660      0
mm_struct             54     54    608    6    1 : tunables   54   27    8 : slabdata      9      9      0
vm_area_struct       543   1260     88   45    1 : tunables  120   60    8 : slabdata     28     28      0
fs_cache              82    122     64   61    1 : tunables  120   60    8 : slabdata      2      2      0
files_cache           50     63    416    9    1 : tunables   54   27    8 : slabdata      7      7      0
signal_cache          68    150    256   15    1 : tunables  120   60    8 : slabdata     10     10      0
sighand_cache         76     78   1312    3    1 : tunables   24   12    8 : slabdata     26     26      0
task_struct           78     81   1312    3    1 : tunables   24   12    8 : slabdata     27     27      0
anon_vma             273    870     12  290    1 : tunables  120   60    8 : slabdata      3      3      0
pgd                   24     24   4096    1    1 : tunables   24   12    8 : slabdata     24     24      0
size-131072(DMA)       0      0 131072    1   32 : tunables    8    4    0 : slabdata      0      0      0
size-131072            0      0 131072    1   32 : tunables    8    4    0 : slabdata      0      0      0
size-65536(DMA)        0      0  65536    1   16 : tunables    8    4    0 : slabdata      0      0      0
size-65536             0      0  65536    1   16 : tunables    8    4    0 : slabdata      0      0      0
size-32768(DMA)        0      0  32768    1    8 : tunables    8    4    0 : slabdata      0      0      0
size-32768             0      0  32768    1    8 : tunables    8    4    0 : slabdata      0      0      0
size-16384(DMA)        0      0  16384    1    4 : tunables    8    4    0 : slabdata      0      0      0
size-16384             5      5  16384    1    4 : tunables    8    4    0 : slabdata      5      5      0
size-8192(DMA)         0      0   8192    1    2 : tunables    8    4    0 : slabdata      0      0      0
size-8192             75     75   8192    1    2 : tunables    8    4    0 : slabdata     75     75      0
size-4096(DMA)         0      0   4096    1    1 : tunables   24   12    8 : slabdata      0      0      0
size-4096             29     29   4096    1    1 : tunables   24   12    8 : slabdata     29     29      0
size-2048(DMA)         0      0   2048    2    1 : tunables   24   12    8 : slabdata      0      0      0
size-2048            248    258   2048    2    1 : tunables   24   12    8 : slabdata    129    129      0
size-1024(DMA)         0      0   1024    4    1 : tunables   54   27    8 : slabdata      0      0      0
size-1024            132    132   1024    4    1 : tunables   54   27    8 : slabdata     33     33      0
size-512(DMA)          0      0    512    8    1 : tunables   54   27    8 : slabdata      0      0      0
size-512             403    432    512    8    1 : tunables   54   27    8 : slabdata     54     54      0
size-256(DMA)          0      0    256   15    1 : tunables  120   60    8 : slabdata      0      0      0
size-256             255    255    256   15    1 : tunables  120   60    8 : slabdata     17     17      0
size-128(DMA)          0      0    128   31    1 : tunables  120   60    8 : slabdata      0      0      0
size-128            2403   2542    128   31    1 : tunables  120   60    8 : slabdata     82     82      0
size-64(DMA)           0      0     64   61    1 : tunables  120   60    8 : slabdata      0      0      0
size-64              670    793     64   61    1 : tunables  120   60    8 : slabdata     13     13      0
size-32(DMA)           0      0     32  119    1 : tunables  120   60    8 : slabdata      0      0      0
size-32             1415   3213     32  119    1 : tunables  120   60    8 : slabdata     27     27      0
kmem_cache           175    175    160   25    1 : tunables  120   60    8 : slabdata      7      7      0

Re: memory leak in 2.6.11-rc2

[Randy.Dunlap]

Andrew Tridgell wrote:
> I've fixed up the problems I had with raid, and am now testing the
> recent xattr changes with dbench and nbench.
> 
> The problem I've hit now is a severe memory leak. I have applied the
> patch from Linus for the leak in free_pipe_info(), and still I'm
> leaking memory at the rate of about 100Mbyte/minute.
> 
> I've tested with both 2.6.11-rc2 and with 2.6.11-rc1-mm2, both with
> the pipe leak fix. The setup is:
> 
>  - 4 way PIII with 4G ram
>  - qla2200 adapter with ibm fastt200 disk array 
>  - running dbench -x and nbench on separate disks, in a loop
> 
> The oom killer kicks in after about 30 minutes. Naturally the oom
> killer decided to kill my sshd, which was running vmstat :-) 

Do you have today's memleak patch applied?  (cut-n-paste below).

-- 
~Randy


----
--- 1.40/fs/pipe.c	2005-01-15 12:01:16 -08:00
+++ edited/fs/pipe.c	2005-01-24 14:35:09 -08:00
@@ -630,13 +630,13 @@
  	struct pipe_inode_info *info = inode->i_pipe;

  	inode->i_pipe = NULL;
-	if (info->tmp_page)
-		__free_page(info->tmp_page);
  	for (i = 0; i < PIPE_BUFFERS; i++) {
  		struct pipe_buffer *buf = info->bufs + i;
  		if (buf->ops)
  			buf->ops->release(info, buf);
  	}
+	if (info->tmp_page)
+		__free_page(info->tmp_page);
  	kfree(info);
  }

-

Re: memory leak in 2.6.11-rc2

[Andrew Tridgell]

Randy,

 > I have applied the patch from Linus for the leak in
 > free_pipe_info()
...
 > Do you have today's memleak patch applied?  (cut-n-paste below).

yes :-)

I'm trying the little leak tracking patch from Alexander Nyberg now.

Cheers, Tridge

Re: memory leak in 2.6.11-rc2

[Dave Jones]

On Tue, Jan 25, 2005 at 02:19:24PM +1100, Andrew Tridgell wrote:
 > The problem I've hit now is a severe memory leak. I have applied the
 > patch from Linus for the leak in free_pipe_info(), and still I'm
 > leaking memory at the rate of about 100Mbyte/minute.
 > I've tested with both 2.6.11-rc2 and with 2.6.11-rc1-mm2, both with
 > the pipe leak fix. The setup is:

That's a little more extreme than what I'm seeing, so it may be
something else, but my firewall box needs rebooting every
few days. It leaks around 50MB a day for some reason.
Given it's not got a lot of ram, after 4-5 days or so, it's
completely exhausted its swap too.

It's currently on a 2.6.10-ac kernel, so it's entirely possible that
we're not looking at the same issue, though it could be something
thats been there for a while if your workload makes it appear
quicker than a firewall/ipsec gateway would.
Do you see the same leaks with an earlier kernel ?

post OOM (when there was about 2K free after named got oom-killed)
this is what slabinfo looked like..

dentry_cache        1502   3775    160   25    1 : tunables  120   60    0 : slabdata    151    151      0
vm_area_struct      1599   2021     84   47    1 : tunables  120   60    0 : slabdata     43     43      0
size-128            3431   6262    128   31    1 : tunables  120   60    0 : slabdata    202    202      0
size-64             4352   4575     64   61    1 : tunables  120   60    0 : slabdata     75     75      0
avtab_node          7073   7140     32  119    1 : tunables  120   60    0 : slabdata     60     60      0
size-32             7256   7616     32  119    1 : tunables  120   60    0 : slabdata     64     64      0

		Dave

Re: memory leak in 2.6.11-rc2

[Andrew Tridgell]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2849 bytes --]

 > I'm trying the little leak tracking patch from Alexander Nyberg now.

Here are the results (only backtraces with more than 10k counts
included). The leak was at 1G of memory at the time I ran this, so its
safe to say 10k page allocations ain't enough to explain it :-)

I also attach a hacked version of the pgown sort program that sorts
the output by count, and isn't O(n^2). It took 10 minutes to run the
old version :-)

I'm guessing the leak is in the new xattr code given that is what
dbench and nbench were beating on. Andreas, can you look at the
following and see if you can spot anything?

This was on 2.6.11-rc2 with the pipe leak patch from Linus. The
machine had leaked 1G of ram in 10 minutes, and was idle (only thing
running was sshd).

Cheers, Tridge


175485 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc011b6a9] current_fs_time+77
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc0132e70] generic_file_aio_write+100
[0xc017e586] ext3_file_write+38
[0xc014b7f5] do_sync_write+169
[0xc015f6de] fcntl_setlk64+286
[0xc01295a8] autoremove_wake_function+0

141512 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc0132e70] generic_file_aio_write+100
[0xc017e586] ext3_file_write+38
[0xc014b7f5] do_sync_write+169
[0xc015f6de] fcntl_setlk64+286
[0xc01295a8] autoremove_wake_function+0
[0xc014b8d5] vfs_write+157

67641 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc014dc69] __getblk+29
[0xc011b6a9] current_fs_time+77
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc018d368] ext3_xattr_user_get+108
[0xc0132e70] generic_file_aio_write+100
[0xc017e586] ext3_file_write+38
[0xc014b7f5] do_sync_write+169

52758 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc014dc69] __getblk+29
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc018d368] ext3_xattr_user_get+108
[0xc0132e70] generic_file_aio_write+100
[0xc017e586] ext3_file_write+38
[0xc014b7f5] do_sync_write+169
[0xc0120c0b] kill_proc_info+47

19610 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc011b6a9] current_fs_time+77
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc0132c3a] generic_file_aio_write_nolock+54
[0xc0132def] generic_file_write_nolock+151
[0xc011b7cb] __do_softirq+95
[0xc01295a8] autoremove_wake_function+0
[0xc01295a8] autoremove_wake_function+0

16874 times:
Page allocated via order 0
[0xc0132258] generic_file_buffered_write+280
[0xc011b6a9] current_fs_time+77
[0xc0132a1e] __generic_file_aio_write_nolock+642
[0xc0132c3a] generic_file_aio_write_nolock+54
[0xc0132def] generic_file_write_nolock+151
[0xc01295a8] autoremove_wake_function+0
[0xc01295a8] autoremove_wake_function+0
[0xc0152d4a] blkdev_file_write+38


[-- Attachment #2: pgown-fast.c --]
[-- Type: application/octet-stream, Size: 2427 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>

struct block_list {
	char *txt;
	int len;
	int num;
};


static struct block_list *list;
static int list_size;
static int max_size;

struct block_list *block_head;

int read_block(char *buf, FILE *fin)
{
	int ret = 0;
	int hit = 0;
	char *curr = buf;
	
	for (;;) {
		*curr = getc(fin);
		if (*curr == EOF) return -1;
		
		ret++;
		if (*curr == '\n' && hit == 1)
			return ret - 1;
		else if (*curr == '\n')
			hit = 1;
		else
			hit = 0;
		curr++;
	}
}

static int compare_txt(struct block_list *l1, struct block_list *l2)
{
	return strcmp(l1->txt, l2->txt);
}

static int compare_num(struct block_list *l1, struct block_list *l2)
{
	return l2->num - l1->num;
}

static void add_list(char *buf, int len)
{
	if (list_size != 0 &&
	    len == list[list_size-1].len &&
	    memcmp(buf, list[list_size-1].txt, len) == 0) {
		list[list_size-1].num++;
		return;
	}
	if (list_size == max_size) {
		printf("max_size too small??\n");
		exit(1);
	}
	list[list_size].txt = malloc(len+1);
	list[list_size].len = len;
	list[list_size].num = 1;
	memcpy(list[list_size].txt, buf, len);
	list[list_size].txt[len] = 0;
	list_size++;
	if (list_size % 1000 == 0) {
		printf("loaded %d\r", list_size);
		fflush(stdout);
	}
}

int main(int argc, char **argv)
{
	FILE *fin, *fout;
	char buf[1024];
	int ret, i, count;
	struct block_list *list2;
	struct stat st;
	
	fin = fopen(argv[1], "r");
	fout = fopen(argv[2], "w");
	if (!fin || !fout) {
		printf("Usage: ./program <input> <output>\n");
		perror("open: ");
		exit(2);
	}

	fstat(fileno(fin), &st);
	max_size = st.st_size / 100; /* hack ... */

	list = malloc(max_size * sizeof(*list));
	
	for(;;) {
		ret = read_block(buf, fin);
		if (ret < 0)
			break;
		
		buf[ret] = '\0';
		add_list(buf, ret);
	}

	printf("loaded %d\n", list_size);

	printf("sorting ....\n");

	qsort(list, list_size, sizeof(list[0]), compare_txt);

	list2 = malloc(sizeof(*list) * list_size);

	printf("culling\n");

	for (i=count=0;i<list_size;i++) {
		if (count == 0 || 
		    strcmp(list2[count-1].txt, list[i].txt) != 0) {
			list2[count++] = list[i];
		} else {
			list2[count-1].num += list[i].num;
		}
	}
	
	qsort(list2, count, sizeof(list[0]), compare_num);
	
	for (i=0;i<count;i++) {
		fprintf(fout, "%d times:\n%s\n", list2[i].num, list2[i].txt);
	}
	return 0;
}

Re: memory leak in 2.6.11-rc2

[Andrew Morton]

Andrew Tridgell <tridge@osdl.org> wrote:
>
>  > I'm trying the little leak tracking patch from Alexander Nyberg now.
> 
>  Here are the results (only backtraces with more than 10k counts
>  included). The leak was at 1G of memory at the time I ran this, so its
>  safe to say 10k page allocations ain't enough to explain it :-)
> 
>  I also attach a hacked version of the pgown sort program that sorts
>  the output by count, and isn't O(n^2). It took 10 minutes to run the
>  old version :-)
> 
>  I'm guessing the leak is in the new xattr code given that is what
>  dbench and nbench were beating on. Andreas, can you look at the
>  following and see if you can spot anything?
> 
>  This was on 2.6.11-rc2 with the pipe leak patch from Linus. The
>  machine had leaked 1G of ram in 10 minutes, and was idle (only thing
>  running was sshd).
> 
>  175485 times:
>  Page allocated via order 0
>  [0xc0132258] generic_file_buffered_write+280
>  [0xc011b6a9] current_fs_time+77
>  [0xc0132a1e] __generic_file_aio_write_nolock+642
>  [0xc0132e70] generic_file_aio_write+100
>  [0xc017e586] ext3_file_write+38
>  [0xc014b7f5] do_sync_write+169
>  [0xc015f6de] fcntl_setlk64+286
>  [0xc01295a8] autoremove_wake_function+0

It would be pretty strange for plain old pagecache pages to leak in this
manner.  A few things come to mind.

- The above trace is indistinguishable from the normal situation of
  having a lot of pagecache floating about.  IOW: we don't know if the
  above pages have really leaked or not.

- It's sometimes possible for ext3 pages to remain alive (on the page
  LRU) after a truncate, but with no other references to them.  These pages
  are trivially reclaimable.  So even if you've deleted the files which the
  benchmark created, there _could_ be pages left over.  Although it would
  be unusual.

So what you should do before generating the leak tool output is to put
heavy memory pressure on the machine to try to get it to free up as much of
that pagecache as possible.  bzero(malloc(lots)) will do it - create a real
swapstorm, then do swapoff to kill remaining swapcache as well.

If we still see a lot of pages with the above trace then something really
broke.  Where does one get the appropriate dbench version?  How are you
mkfsing the filesystem?  Was mke2fs patched?  How is the benchmark being
invoked?

Thanks.

Re: memory leak in 2.6.11-rc2

[Andrew Tridgell]

Andrew,

 > So what you should do before generating the leak tool output is to put
 > heavy memory pressure on the machine to try to get it to free up as much of
 > that pagecache as possible.  bzero(malloc(lots)) will do it - create a real
 > swapstorm, then do swapoff to kill remaining swapcache as well.

As you saw when you logged into the machine earlier tonight, when you
suspend the dbench processes and run a memory filler the memory is
reclaimed.

I still think its a bug though, as the oom killer is being triggered
when it shouldn't be. I have 4G of ram in this machine, and I'm only
running a couple of hundred processes that should be using maybe 500M
in total, so for the oom killer to kick in might mean that the memory
isn't being reclaimed under normal memory pressure. Certainly a ps
shows no process using more than a few MB.

The oom killer report is below. This is with 2.6.11-rc2, with the pipe
leak fix, and the pgown monitoring patch. It was running one nbench of
size 50 and one dbench of size 40 at the time.

If this isn't a leak, then it would also be good to fix /usr/bin/free
so the -/+ buffers line becomes meaningful again. With the machine
completely idle (just a sshd running) I see:

Mem:       3701184    3483188     217996          0     130092    1889440
-/+ buffers/cache:    1463656    2237528

which looks like a leak. This persists even after the disks are
unmounted. After filling/freeing memory I see:

Mem:       3701184      28176    3673008          0        520       3764
-/+ buffers/cache:      23892    3677292

so it can recover it, but under normal usage it doesn't before the oom
killer kicks in.

Cheers, Tridge


Jan 25 00:49:14 dev4-003 kernel: Out of Memory: Killed process 18910 (smbd).
Jan 25 00:49:14 dev4-003 kernel: oom-killer: gfp_mask=0xd0
Jan 25 00:49:14 dev4-003 kernel: DMA per-cpu:
Jan 25 00:49:14 dev4-003 kernel: cpu 0 hot: low 2, high 6, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 0 cold: low 0, high 2, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 1 hot: low 2, high 6, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 1 cold: low 0, high 2, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 2 hot: low 2, high 6, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 2 cold: low 0, high 2, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 3 hot: low 2, high 6, batch 1
Jan 25 00:49:14 dev4-003 kernel: cpu 3 cold: low 0, high 2, batch 1
Jan 25 00:49:14 dev4-003 kernel: Normal per-cpu:
Jan 25 00:49:14 dev4-003 kernel: cpu 0 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 0 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 1 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 1 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 2 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 2 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 3 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 3 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: HighMem per-cpu:
Jan 25 00:49:14 dev4-003 kernel: cpu 0 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 0 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 1 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 1 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 2 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 2 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 3 hot: low 32, high 96, batch 16
Jan 25 00:49:14 dev4-003 kernel: cpu 3 cold: low 0, high 32, batch 16
Jan 25 00:49:14 dev4-003 kernel:
Jan 25 00:49:14 dev4-003 kernel: Free pages:        5884kB (640kB HighMem)
Jan 25 00:49:14 dev4-003 kernel: Active:84764 inactive:806173 dirty:338229 writeback:36 unstable:0 free:1471 slab:23814 mapped:31610 pagetables:1616
Jan 25 00:49:14 dev4-003 kernel: DMA free:284kB min:68kB low:84kB high:100kB active:440kB inactive:10048kB present:16384kB pages_scanned:140 all_unreclaimable? no
Jan 25 00:49:14 dev4-003 kernel: protections[]: 0 0 0
Jan 25 00:49:14 dev4-003 kernel: Normal free:4960kB min:3756kB low:4692kB high:5632kB active:72072kB inactive:632096kB present:901120kB pages_scanned:58999 all_unreclaimable? no
Jan 25 00:49:14 dev4-003 kernel: protections[]: 0 0 0
Jan 25 00:49:14 dev4-003 kernel: HighMem free:640kB min:512kB low:640kB high:768kB active:266948kB inactive:2581960kB present:2850752kB pages_scanned:0 all_unreclaimable? no
Jan 25 00:49:14 dev4-003 kernel: protections[]: 0 0 0
Jan 25 00:49:14 dev4-003 kernel: DMA: 53*4kB 3*8kB 1*16kB 1*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 284kB
Jan 25 00:49:14 dev4-003 kernel: Normal: 400*4kB 16*8kB 0*16kB 1*32kB 0*64kB 1*128kB 0*256kB 2*512kB 0*1024kB 1*2048kB 0*4096kB = 4960kB
Jan 25 00:49:14 dev4-003 kernel: HighMem: 6*4kB 11*8kB 1*16kB 0*32kB 0*64kB 2*128kB 1*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 640kB
Jan 25 00:49:14 dev4-003 kernel: Swap cache: add 1584899, delete 1585025, find 294/351, race 0+0
Jan 25 00:49:14 dev4-003 kernel: Out of Memory: Killed process 18914 (smbd).

Re: memory leak in 2.6.11-rc2

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Andrew Tridgell wrote:
> Andrew,
> 
>  > So what you should do before generating the leak tool output is to put
>  > heavy memory pressure on the machine to try to get it to free up as much of
>  > that pagecache as possible.  bzero(malloc(lots)) will do it - create a real
>  > swapstorm, then do swapoff to kill remaining swapcache as well.
> 
> As you saw when you logged into the machine earlier tonight, when you
> suspend the dbench processes and run a memory filler the memory is
> reclaimed.
> 
> I still think its a bug though, as the oom killer is being triggered
> when it shouldn't be. I have 4G of ram in this machine, and I'm only
> running a couple of hundred processes that should be using maybe 500M
> in total, so for the oom killer to kick in might mean that the memory
> isn't being reclaimed under normal memory pressure. Certainly a ps
> shows no process using more than a few MB.
> 
> The oom killer report is below. This is with 2.6.11-rc2, with the pipe
> leak fix, and the pgown monitoring patch. It was running one nbench of
> size 50 and one dbench of size 40 at the time.
> 

There are various OOM killer improvements and fixes that have gone
into Andrew's kernel tree which should be included for 2.6.11.

I don't think the OOM killer was ever perfect in 2.6, but recent
tinkering in mm/ probably aggrivated it. *blush*

Here is another small OOM killer improvement. Previously we needed
to reclaim SWAP_CLUSTER_MAX pages in a single pass. That should be
changed so that we need only reclaim that many pages during the
entire try_to_free_pages run, without going OOM.

Andrea? Andrew? Look OK?

[-- Attachment #2: less-oom.patch --]
[-- Type: text/plain, Size: 800 bytes --]




---

 linux-2.6-npiggin/mm/vmscan.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -puN mm/vmscan.c~oom-helper mm/vmscan.c
--- linux-2.6/mm/vmscan.c~oom-helper	2005-01-25 23:04:28.000000000 +1100
+++ linux-2.6-npiggin/mm/vmscan.c	2005-01-25 23:05:06.000000000 +1100
@@ -914,12 +914,12 @@ int try_to_free_pages(struct zone **zone
 			sc.nr_reclaimed += reclaim_state->reclaimed_slab;
 			reclaim_state->reclaimed_slab = 0;
 		}
-		if (sc.nr_reclaimed >= SWAP_CLUSTER_MAX) {
+		total_scanned += sc.nr_scanned;
+		total_reclaimed += sc.nr_reclaimed;
+		if (total_reclaimed >= SWAP_CLUSTER_MAX) {
 			ret = 1;
 			goto out;
 		}
-		total_scanned += sc.nr_scanned;
-		total_reclaimed += sc.nr_reclaimed;
 
 		/*
 		 * Try to write back as many pages as we just scanned.  This

_

Re: memory leak in 2.6.11-rc2

[Andrea Arcangeli]

On Mon, Jan 24, 2005 at 10:45:47PM -0500, Dave Jones wrote:
> On Tue, Jan 25, 2005 at 02:19:24PM +1100, Andrew Tridgell wrote:
>  > The problem I've hit now is a severe memory leak. I have applied the
>  > patch from Linus for the leak in free_pipe_info(), and still I'm
>  > leaking memory at the rate of about 100Mbyte/minute.
>  > I've tested with both 2.6.11-rc2 and with 2.6.11-rc1-mm2, both with
>  > the pipe leak fix. The setup is:
> 
> That's a little more extreme than what I'm seeing, so it may be
> something else, but my firewall box needs rebooting every
> few days. It leaks around 50MB a day for some reason.
> Given it's not got a lot of ram, after 4-5 days or so, it's
> completely exhausted its swap too.
> 
> It's currently on a 2.6.10-ac kernel, so it's entirely possible that
> we're not looking at the same issue, though it could be something
> thats been there for a while if your workload makes it appear
> quicker than a firewall/ipsec gateway would.
> Do you see the same leaks with an earlier kernel ?
> 
> post OOM (when there was about 2K free after named got oom-killed)
> this is what slabinfo looked like..
> 
> dentry_cache        1502   3775    160   25    1 : tunables  120   60    0 : slabdata    151    151      0
> vm_area_struct      1599   2021     84   47    1 : tunables  120   60    0 : slabdata     43     43      0
> size-128            3431   6262    128   31    1 : tunables  120   60    0 : slabdata    202    202      0
> size-64             4352   4575     64   61    1 : tunables  120   60    0 : slabdata     75     75      0
> avtab_node          7073   7140     32  119    1 : tunables  120   60    0 : slabdata     60     60      0
> size-32             7256   7616     32  119    1 : tunables  120   60    0 : slabdata     64     64      0

What is avtab_node? there's no such thing in my kernel. But the above
can be ok. Can you show meminfo too after oom kill?

Just another datapoint my firewall runs a kernel based on 2.6.11-rc1-bk8 with
all the needed oom fixes and I've no problems on it yet. I run it oom
and this is what I get after the oom:

athlon:/home/andrea # free
             total       used       free     shared    buffers     cached
Mem:        511136      50852     460284          0        572      15764
-/+ buffers/cache:      34516     476620
Swap:      1052248          0    1052248
athlon:/home/andrea # 

The above is sane, 34M is very reasonable for what's loaded there
(there's the X server running, named too, and various other non standard
daemons, one even has a virtual size of >100m so it's not a tiny thing),
so I'm quite sure I'm not hitting a memleak, at least not on the
firewal. No ipsec on it btw, and it's a pure IDE without anything
special, just quite a few nics and USB usermode running all the time.

athlon:/home/andrea # uptime
  1:34pm  up 2 days 12:08,  1 user,  load average: 0.98, 1.13, 0.54
athlon:/home/andrea # iptables -L -v |grep -A2 FORWARD
Chain FORWARD (policy ACCEPT 65 packets, 9264 bytes)
 pkts bytes target     prot opt in     out     source               destination         
3690K 2321M block      all  --  any    any     anywhere             anywhere            
athlon:/home/andrea # 

So if there's a memleak in rc1-bk8, it's probably not in the core of the
kernel, but in some driver or things like ipsec. Either that or it broke
after 2.6.11-rc1-bk8. The kernel I'm running is quite heavily patched
too, but I'm not aware of any memleak fix in the additional patches.

Anyway I'll try again in a few days to verify it goes back down again to
exactly 34M of anonymous/random and 15M of cache.

No apparent problem on my desktop system either, it's running the same
kernel with different config.

If somebody could fix the kernel CVS I could have a look at the
interesting changesets between 2.6.11-rc1-bk8 and 2.6.11-rc2.

Re: memory leak in 2.6.11-rc2

[Andreas Gruenbacher]

On Tue, 2005-01-25 at 13:51, Andrea Arcangeli wrote:
> If somebody could fix the kernel CVS I could have a look at the
> interesting changesets between 2.6.11-rc1-bk8 and 2.6.11-rc2.

What's not okay?

-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX GMBH

Re: memory leak in 2.6.11-rc2

[Andrea Arcangeli]

On Tue, Jan 25, 2005 at 02:31:03PM +0100, Andreas Gruenbacher wrote:
> On Tue, 2005-01-25 at 13:51, Andrea Arcangeli wrote:
> > If somebody could fix the kernel CVS I could have a look at the
> > interesting changesets between 2.6.11-rc1-bk8 and 2.6.11-rc2.
> 
> What's not okay?

I already prepared a separated deatiled bugreport. I'm reproducing one
last time before sending it, after a "su - nobody" to be sure it's not a
local problem in my environment that might have changed, but I'm 99%
sure it'll reproduce just fine in a fresh account too.

Are you using cvsps at all?