From: David Howells <dhowells@redhat.com>
Subject: [RHEL5 PATCH] CacheFiles: Fix object struct recycling
Date: Thu, 04 Jan 2007 17:09:49 +0000
Bugzilla: 215599
Message-Id: <27688.1167930589@redhat.com>
Changelog: CacheFiles: Fix object struct recycling



Fix the recycling of the structures used to represent objects in CacheFiles
[BZ #125599].

The problem was that these structs (cachefiles_object) are being allocated from
their own slab, and are initialised by the slab constructor rather than being
reinitialised after allocation each time.

The structs have two dentry pointers: one for the primary VFS representation of
the cache object, and one for the data storage file.  For index objects, the
latter is unused; for data storage objects and special objects, the two
pointers are the same (and point to a file) unless the object has children (in
which case the former is a directory, and the latter a file in that directory).

What happens is that when an index object is allocated, the "backer" pointer is
not set (on the assumption that it'll be NULL).  cachefiles_put_object(),
however, attempts to release it anyway - which is fine, if it is NULL.

cachefiles_put_object(), though, was only attempting to release the dentry
pointed to by the "backer" pointer if it differed from the representation
dentry (thus only holding one ref on the dentry, not two), and, critically, was
only clearing the "backer" pointer if it released what it pointed to.

This means that if the "backer" pointer was the same as the representation
dentry pointer upon release of a data object, then the "backer" pointer would
not be cleared before the object was handed back to the slab:

	if (object->backer != object->dentry) {
		dput(object->backer);
		object->backer = NULL;
	}

However, the slab object is not then reconstructed by mm/slab.c unless the page
holding it is released and reallocated or CONFIG_DEBUG_SLAB=y.

What happens then is that a data object is destroyed and released back to the
slab and then reallocated.  If it's allocated for use as an index, the "backer"
pointer from the data object is retained and released again when the index
object is released.  This leads to an object being dput()'d multiple times,
which leads to the following BUG occurring when the filesystem with the cache
on it is unmounted:

BUG: Dentry f5eb4888{i=12fb4,n=Ew0wo0000-VNwe02000g0tS_6TkshDZhP7sIo0000-VNw0000} still in use (-1) [unmount of ext3 loop0]
------------[ cut here ]------------
kernel BUG at fs/dcache.c:615!

However, if the dentry gets recycled before it's fully released (cachefilesd
may be using it when the CacheFiles module releases it), it may then affect
other filesystems too.

The solution is simply to always clear the backing pointer before releasing the
object back to the slab.  I've also added in a BUG_ON() to catch the object
being allocated with the "backer" pointer unexpectedly set.


I think the BUG isn't seen much for a few reasons:

 (1) It only happens when an data object is recycled as an index object without
     the slab page being recycled in between.  However, this is very unlikely
     to happen.  FS-Cache has one root index, NFS has one general index and one
     index per server.  All the other cache objects are data objects.

     It's reasonably likely that all the index objects will be allocated up
     front and not released.

 (2) Data objects always change the "backer" pointer, and so don't suffer from
     this bug.

 (3) The problem is avoided by turning on CONFIG_DEBUG_SLAB.


I've tested this by running the cache in a kernel with only the BUG_ON() part
of the patch in there.  Starting up the cache and tarring up a kernel tree
through it, then stopping the cache and unmounting its backing store and then
starting it up again and rerunning the tar showed up the problem immediately:

---------- [cut here ] --------- [please bite here ] ---------
Kernel BUG at fs/cachefiles/cf-interface.c:53

Adding in the rest of the patch caused that to go away under the same
procedure.


Barry Donahue managed to reproduce the bug with reasonable reliability.  With
the patched kernel the bug seems to have gone away for him.

David

--- fscache/fs/cachefiles/cf-interface.c.orig	2007-01-03 18:00:19.000000000 +0000
+++ fscache/fs/cachefiles/cf-interface.c	2007-01-03 18:02:32.000000000 +0000
@@ -50,6 +50,8 @@ static struct fscache_object *cachefiles
 	if (!object)
 		goto nomem_object;
 
+	BUG_ON(object->backer != NULL);
+
 	atomic_set(&object->usage, 1);
 	atomic_set(&object->fscache_usage, 1);
 
@@ -230,10 +232,9 @@ static void cachefiles_put_object(struct
 	}
 
 	/* close the filesystem stuff attached to the object */
-	if (object->backer != object->dentry) {
+	if (object->backer != object->dentry)
 		dput(object->backer);
-		object->backer = NULL;
-	}
+	object->backer = NULL;
 
 	/* note that the object is now inactive */
 	if (test_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags)) {