Add extended attributes to ext2/3

PATCH In review [email protected] 6 participants 18 messages Oct 15, 2002

Subsystems (2)

VFS ext2 file system

lore

Share & tools

lore

[email protected]

author

2002-10-15 03:27 UTC #0 lore raw

PATCH 1/4 Add extended attributes to ext2/3

Linus,

Enclosed please find patches to add extended attribute support
to the ext2 and ext3 filesystems. It is a port of Andreas Gruenbacher's patches, which have been quite well tested at this point. Full support for extended attributes have been in e2fsprogs for quite some time. In addition, if CONFIG_EXT[23]_FS_ATTR is not enabled, the code path changes are quite minimal, and hence this should be a very low-risk patch. Please apply.

These patches are a prerequisite for the port of Andreas Gruenbacher's ACL patches, which will follow shortly.

This first patch creates a generic interface for registering caches with the VM subsystem so that they can react appropriately to memory pressure.

- Ted

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
#

# cache_def.h               |   15 +++++++++++++++
# include/linux/cache_def.h |   15 +++++++++++++++
# kernel/ksyms.c            |    3 +++
# mm/vmscan.c               |   35 +++++++++++++++++++++++++++++++++++
# 4 files changed, 68 insertions(+)
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/10/14	[email protected]	1.783
# Port of (updated) 0.8.50 cache-def patch.  (Fixed to take the BKL where necessary)
# 
# --------------------------------------------
#
diff -Nru a/cache_def.h b/cache_def.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/cache_def.h	Mon Oct 14 23:20:54 2002
@@ -0,0 +1,15 @@
+/*
+ * linux/cache_def.h
+ * Handling of caches defined in drivers, filesystems, ...
+ *
+ * Copyright (C) 2002 by Andreas Gruenbacher, <[email protected]>
+ */
+
+struct cache_definition {
+	const char *name;
+	void (*shrink)(int, unsigned int);
+	struct list_head link;
+};
+
+extern void register_cache(struct cache_definition *);
+extern void unregister_cache(struct cache_definition *);
diff -Nru a/include/linux/cache_def.h b/include/linux/cache_def.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/linux/cache_def.h	Mon Oct 14 23:20:54 2002
@@ -0,0 +1,15 @@
+/*
+ * linux/cache_def.h
+ * Handling of caches defined in drivers, filesystems, ...
+ *
+ * Copyright (C) 2002 by Andreas Gruenbacher, <[email protected]>
+ */
+
+struct cache_definition {
+	const char *name;
+	void (*shrink)(int, unsigned int);
+	struct list_head link;
+};
+
+extern void register_cache(struct cache_definition *);
+extern void unregister_cache(struct cache_definition *);
diff -Nru a/kernel/ksyms.c b/kernel/ksyms.c
--- a/kernel/ksyms.c	Mon Oct 14 23:20:54 2002
+++ b/kernel/ksyms.c	Mon Oct 14 23:20:54 2002
@@ -31,6 +31,7 @@
 #include <linux/genhd.h>
 #include <linux/blkpg.h>
 #include <linux/swap.h>
+#include <linux/cache_def.h>
 #include <linux/ctype.h>
 #include <linux/file.h>
 #include <linux/iobuf.h>
@@ -103,6 +104,8 @@
 EXPORT_SYMBOL(kmem_cache_alloc);
 EXPORT_SYMBOL(kmem_cache_free);
 EXPORT_SYMBOL(kmem_cache_size);
+EXPORT_SYMBOL(register_cache);
+EXPORT_SYMBOL(unregister_cache);
 EXPORT_SYMBOL(kmalloc);
 EXPORT_SYMBOL(kfree);
 EXPORT_SYMBOL(vfree);
diff -Nru a/mm/vmscan.c b/mm/vmscan.c
--- a/mm/vmscan.c	Mon Oct 14 23:20:54 2002
+++ b/mm/vmscan.c	Mon Oct 14 23:20:54 2002
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/kernel_stat.h>
 #include <linux/swap.h>
+#include <linux/cache_def.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/highmem.h>
@@ -76,6 +77,39 @@
 #define shrink_dqcache_memory(ratio, gfp_mask) do { } while (0)
 #endif

+static DECLARE_MUTEX(other_caches_sem);
+static LIST_HEAD(cache_definitions);
+
+void register_cache(struct cache_definition *cache) +{
+ down(&other_caches_sem);
+ list_add(&cache->link, &cache_definitions);
+ up(&other_caches_sem);
+}
+
+void unregister_cache(struct cache_definition *cache) +{
+ down(&other_caches_sem);
+ list_del(&cache->link);
+ up(&other_caches_sem);
+}
+
+static void shrink_other_caches(int ratio, int gfp_mask) +{
+ struct list_head *p;
+
+ down(&other_caches_sem);
+ p = cache_definitions.prev;
+ while (p != &cache_definitions) {
+ struct cache_definition *cache =
+ list_entry(p, struct cache_definition, link);
+
+ cache->shrink(ratio, gfp_mask);
+ p = p->prev;
+ }
+ up(&other_caches_sem);
+}
+
/* Must be called with page's pte_chain_lock held. */ static inline int page_mapping_inuse(struct page * page) {
@@ -594,6 +628,7 @@
shrink_dcache_memory(shrink_ratio, gfp_mask); shrink_icache_memory(shrink_ratio, gfp_mask); shrink_dqcache_memory(shrink_ratio, gfp_mask);
+ shrink_other_caches(shrink_ratio, gfp_mask);
}

Andrew Morton

2002-10-15 04:40 UTC #1 lore raw

Quoted 6 lines · [email protected] wrote:

...
This first patch creates a generic interface for registering caches with the VM subsystem so that they can react appropriately to memory pressure.

Seems our patches passed in the night - Linus already has one of those APIs.

I've converted xattr to use the set_shrinker/remove_shrinker API. I'd appreciate it if you could pass an eye over that and give it a test. I'll roll it into 2.5.42-mm3 if that makes it easier.

btw, gcc-2.91.66 is saying:

fs/ext2/xattr.c: In function `ext2_xattr_set':
fs/ext2/xattr.c:612: warning: `block' might be used uninitialized in this function

But the code is:

                int block = EXT2_I(inode)->i_file_acl;

which is rather bizarre. Never seen it do that before.

The patching order becomes:

xattr 2/4
xattr-shrinker
xattr 3/4
xattr 4/4

 fs/mbcache.c |   57 ++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

fs/mbcache.c renamed from fs/mbcache.c~xattr-shrinker +32 ~~-25~~

@@ -29,9 +29,9 @@
 #include <linux/module.h>
 
 #include <linux/fs.h>
+#include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <linux/cache_def.h>
 #include <linux/version.h>
 #include <linux/init.h>
 #include <linux/mbcache.h>

@@ -85,6 +85,7 @@ EXPORT_SYMBOL(mb_cache_entry_find_next);
 static LIST_HEAD(mb_cache_list);
 static LIST_HEAD(mb_cache_lru_list);
 static spinlock_t mb_cache_spinlock = SPIN_LOCK_UNLOCKED;
+static struct shrinker *mb_shrinker;
 
 static inline void
 mb_cache_lock(void)

@@ -112,14 +113,7 @@ mb_cache_indexes(struct mb_cache *cache)
  * What the mbcache registers as to get shrunk dynamically.
  */
 
-static void
-mb_cache_memory_pressure(int priority, unsigned int gfp_mask);
-
-static struct cache_definition mb_cache_definition = {
-	"mb_cache",
-	mb_cache_memory_pressure
-};
-
+static int mb_cache_shrink_fn(int nr_to_scan, unsigned int gfp_mask);
 
 static inline void
 __mb_cache_entry_takeout_lru(struct mb_cache_entry *ce)

@@ -226,16 +220,18 @@ __mb_cache_entry_release_unlock(struct m
 
 
 /*
- * mb_cache_memory_pressure()  memory pressure callback
+ * mb_cache_shrink_fn()  memory pressure callback
  *
  * This function is called by the kernel memory management when memory
  * gets low.
  *
- * @priority: Amount by which to shrink the cache (0 = highes priority)
+ * @nr_to_scan: Number of objects to scan
  * @gfp_mask: (ignored)
+ *
+ * Returns the number of objects which are present in the cache.
  */
-static void
-mb_cache_memory_pressure(int priority, unsigned int gfp_mask)
+static int
+mb_cache_shrink_fn(int nr_to_scan, unsigned int gfp_mask)
 {
 	LIST_HEAD(free_list);
 	struct list_head *l;

@@ -249,11 +245,12 @@ mb_cache_memory_pressure(int priority, u
 			  atomic_read(&cache->c_entry_count));
 		count += atomic_read(&cache->c_entry_count);
 	}
-	mb_debug("trying to free %d of %d entries",
-		  count / (priority ? priority : 1), count);
-	if (priority)
-		count /= priority;
-	while (count && !list_empty(&mb_cache_lru_list)) {
+	mb_debug("trying to free %d entries", nr_to_scan);
+	if (nr_to_scan == 0) {
+		mb_cache_unlock();
+		goto out;
+	}
+	while (nr_to_scan && !list_empty(&mb_cache_lru_list)) {
 		struct mb_cache_entry *ce =
 			list_entry(mb_cache_lru_list.prev,
 				   struct mb_cache_entry, e_lru_list);

@@ -261,7 +258,7 @@ mb_cache_memory_pressure(int priority, u
 		list_add(&ce->e_lru_list, &free_list);
 		if (__mb_cache_entry_is_linked(ce))
 			__mb_cache_entry_unlink(ce);
-		count--;
+		nr_to_scan--;
 	}
 	mb_cache_unlock();
 	l = free_list.prev;

@@ -270,9 +267,11 @@ mb_cache_memory_pressure(int priority, u
 			struct mb_cache_entry, e_lru_list);
 		l = l->prev;
 		__mb_cache_entry_forget(ce);
+		count--;
 	}
-	if (count)
-		mb_debug("%d fewer entries freed", count);
+out:
+	mb_debug("%d remaining entries ", count);
+	return count;
 }

@@ -342,8 +341,14 @@ mb_cache_create(const char *name, struct
 		goto fail;
 
 	mb_cache_lock();
-	if (list_empty(&mb_cache_list))
-		register_cache(&mb_cache_definition);
+	if (list_empty(&mb_cache_list)) {
+		if (mb_shrinker) {
+			printk(KERN_ERR "%s: already have a shrinker!\n",
+					__FUNCTION__);
+			remove_shrinker(mb_shrinker);
+		}
+		mb_shrinker = set_shrinker(DEFAULT_SEEKS, mb_cache_shrink_fn);
+	}
 	list_add(&cache->c_cache_list, &mb_cache_list);
 	mb_cache_unlock();
 	return cache;

@@ -429,8 +434,10 @@ mb_cache_destroy(struct mb_cache *cache)
 		}
 	}
 	list_del(&cache->c_cache_list);
-	if (list_empty(&mb_cache_list))
-		unregister_cache(&mb_cache_definition);
+	if (list_empty(&mb_cache_list) && mb_shrinker) {
+		remove_shrinker(mb_shrinker);
+		mb_shrinker = 0;
+	}
 	mb_cache_unlock();
 
 	l = free_list.prev;

Theodore Ts'o

author

2002-10-15 04:58 UTC #2 lore raw

Quoted 8 lines · Andrew Morton

Quoted 6 lines · [email protected] wrote:
...
This first patch creates a generic interface for registering caches with the VM subsystem so that they can react appropriately to memory pressure.

Quoted 4 lines

Seems our patches passed in the night - Linus already has one of those APIs.
I've converted xattr to use the set_shrinker/remove_shrinker API.

Great! I checked bk://linux.bkbits.net/linux-2.5 before I started assembling this patch set, but I missed the fact that the shrinker changes had made it in.

Quoted 2 lines

I'd appreciate it if you could pass an eye over that and give it a test.

Will do....

- Ted

Andreas Dilger

2002-10-15 05:27 UTC #3 lore raw

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

btw, gcc-2.91.66 is saying:
fs/ext2/xattr.c: In function `ext2_xattr_set':
fs/ext2/xattr.c:612: warning: `block' might be used uninitialized in this function
But the code is:
                int block = EXT2_I(inode)->i_file_acl;
which is rather bizarre. Never seen it do that before.

There's a "goto bad_block" in that function, and it enters _after_ block is set (block is a local variable), so it is impossible for it to have a valid value.

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Theodore Ts'o

author

2002-10-15 12:59 UTC #4 lore raw

Quoted 3 lines · Andreas Dilger

There's a "goto bad_block" in that function, and it enters _after_ block is set (block is a local variable), so it is impossible for it to have a valid value.

Thanks, I'll fix that the xattr-ext2 and xattr-ext3 patches.

- Ted

Olaf Dietsche

2002-10-15 10:20 UTC #5 lore raw

In reply to [email protected]

Quoted 7 lines · [email protected] writes:

Enclosed please find patches to add extended attribute support
to the ext2 and ext3 filesystems. It is a port of Andreas Gruenbacher's patches, which have been quite well tested at this point. Full support for extended attributes have been in e2fsprogs for quite some time. In addition, if CONFIG_EXT[23]_FS_ATTR is not enabled, the code path changes are quite minimal, and hence this should be a very low-risk patch. Please apply.

Maybe this is a stupid question. Does this mean, fs capabilities will come too?

Regards, Olaf.

Christoph Hellwig

2002-10-15 11:52 UTC #6 lore raw

In reply to [email protected]

Quoted 10 lines · On Mon, Oct 14, 2002 at 11:33:04PM -0400, [email protected] wrote:

Linus,
Enclosed please find patches to add extended attribute support
to the ext2 and ext3 filesystems. It is a port of Andreas Gruenbacher's patches, which have been quite well tested at this point. Full support for extended attributes have been in e2fsprogs for quite some time. In addition, if CONFIG_EXT[23]_FS_ATTR is not enabled, the code path changes are quite minimal, and hence this should be a very low-risk patch. Please apply.

Quoted 6 lines

These patches are a prerequisite for the port of Andreas Gruenbacher's ACL patches, which will follow shortly.
This first patch creates a generic interface for registering caches with the VM subsystem so that they can react appropriately to memory pressure.

It doesn't look like you addressed any comments raised, i.e. my complaints or sct's superblock fields. I"ll start feeding some updates to akpm to address a few issues, but I don't really have time to care of all that.

Theodore Ts'o

author

2002-10-15 13:09 UTC #7 lore raw

Quoted 4 lines · Christoph Hellwig

It doesn't look like you addressed any comments raised, i.e. my complaints or sct's superblock fields. I"ll start feeding some updates to akpm to address a few issues, but I don't really have time to care of all that.

Actually, I did, for those comments that made sense. The fs/Config.in logic has been cleaned up, as well as removing excess header files, stray LINUX_VERSION_CODE #ifdef's that I had missed the first time around, etc.

fs/mbcache.c is still there because it applies to both ext2 and ext3 filesystems, and so your suggestion of moving it into the ext2 and ext3 directories would cause code duplication and maintenance headaches. It also *can* be used by other filesystems, as it is written in a generic way. The fs/Config.in only compiles it in if necessary (i.e., if ext2/3 extended attribute is enabled) so it won't cause code bloat for other filesystems if it is not needed.

The superblock fields are more of an issue with the posix acl changes than for the extended attribute patches. I had wanted to get the extended attribute changes in first, since they stand alone, and so I have fewer patches to juggle.

- Ted

Christoph Hellwig

2002-10-15 15:19 UTC #8 lore raw

Quoted 13 lines · Theodore Ts'o

Actually, I did, for those comments that made sense. The fs/Config.in logic has been cleaned up, as well as removing excess header files, stray LINUX_VERSION_CODE #ifdef's that I had missed the first time around, etc.
fs/mbcache.c is still there because it applies to both ext2 and ext3 filesystems, and so your suggestion of moving it into the ext2 and ext3 directories would cause code duplication and maintenance headaches. It also *can* be used by other filesystems, as it is written in a generic way. The fs/Config.in only compiles it in if necessary (i.e., if ext2/3 extended attribute is enabled) so it won't cause code bloat for other filesystems if it is not needed.

Quoted 4 lines

The superblock fields are more of an issue with the posix acl changes than for the extended attribute patches. I had wanted to get the extended attribute changes in first, since they stand alone, and so I have fewer patches to juggle.

Patch 1: Config.in (all against 2.5.42-mm3):

The xattr options must be dep_mbool to actually work, ext2 xattrs should depends on ext2, not ext3

fs/Config.in +2 -2

@@ -27,7 +27,7 @@
 dep_tristate 'BFS file system support (EXPERIMENTAL)' CONFIG_BFS_FS $CONFIG_EXPERIMENTAL
 
 tristate 'Ext3 journalling file system support' CONFIG_EXT3_FS
-dep_bool '  Ext3 extended attributes' CONFIG_EXT3_FS_XATTR $CONFIG_EXT3_FS
+dep_mbool '  Ext3 extended attributes' CONFIG_EXT3_FS_XATTR $CONFIG_EXT3_FS
 # CONFIG_JBD could be its own option (even modular), but until there are
 # other users than ext3, we will simply make it be the same as CONFIG_EXT3_FS
 # dep_tristate '  Journal Block Device support (JBD for ext3)' CONFIG_JBD $CONFIG_EXT3_FS

@@ -97,7 +97,7 @@
 tristate 'ROM file system support' CONFIG_ROMFS_FS
 
 tristate 'Second extended fs support' CONFIG_EXT2_FS
-dep_bool '  Ext2 extended attributes' CONFIG_EXT2_FS_XATTR $CONFIG_EXT3_FS
+dep_mbool '  Ext2 extended attributes' CONFIG_EXT2_FS_XATTR $CONFIG_EXT2_FS
 
 tristate 'System V/Xenix/V7/Coherent file system support' CONFIG_SYSV_FS

Christoph Hellwig

2002-10-15 15:25 UTC #9 lore raw

In reply to Theodore Ts'o

Quoted 13 lines · Theodore Ts'o

Actually, I did, for those comments that made sense. The fs/Config.in logic has been cleaned up, as well as removing excess header files, stray LINUX_VERSION_CODE #ifdef's that I had missed the first time around, etc.
fs/mbcache.c is still there because it applies to both ext2 and ext3 filesystems, and so your suggestion of moving it into the ext2 and ext3 directories would cause code duplication and maintenance headaches. It also *can* be used by other filesystems, as it is written in a generic way. The fs/Config.in only compiles it in if necessary (i.e., if ext2/3 extended attribute is enabled) so it won't cause code bloat for other filesystems if it is not needed.

Quoted 4 lines

The superblock fields are more of an issue with the posix acl changes than for the extended attribute patches. I had wanted to get the extended attribute changes in first, since they stand alone, and so I have fewer patches to juggle.

Patch 2: mbcache (all against 2.5.42-mm3):

o remove LINUX_VERSION_CODE mess
o cleanup mb_cache_lock/mb_cache_unlock mess
o use list_move
o use list_del_init/list_empty instead of messing with .prev directly
o remove useless MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT

fs/mbcache.c +42 ~~-70~~

@@ -27,12 +27,10 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
-
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <linux/version.h>
 #include <linux/init.h>
 #include <linux/mbcache.h>

@@ -56,9 +54,7 @@
 		
 MODULE_AUTHOR("Andreas Gruenbacher <[email protected]>");
 MODULE_DESCRIPTION("Meta block cache (for extended attributes)");
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
 MODULE_LICENSE("GPL");
-#endif
 
 EXPORT_SYMBOL(mb_cache_create);
 EXPORT_SYMBOL(mb_cache_shrink);

@@ -87,18 +83,6 @@
 static spinlock_t mb_cache_spinlock = SPIN_LOCK_UNLOCKED;
 static struct shrinker *mb_shrinker;
 
-static inline void
-mb_cache_lock(void)
-{
-	spin_lock(&mb_cache_spinlock);
-}
-
-static inline void
-mb_cache_unlock(void)
-{
-	spin_unlock(&mb_cache_spinlock);
-}
-
 static inline int
 mb_cache_indexes(struct mb_cache *cache)
 {

@@ -118,10 +102,8 @@
 static inline void
 __mb_cache_entry_takeout_lru(struct mb_cache_entry *ce)
 {
-	if (ce->e_lru_list.prev) {
-		list_del(&ce->e_lru_list);
-		ce->e_lru_list.prev = NULL;
-	}
+	if (!list_empty(&ce->e_lru_list))
+		list_del_init(&ce->e_lru_list);
 }

@@ -135,7 +117,7 @@
 static inline int
 __mb_cache_entry_in_lru(struct mb_cache_entry *ce)
 {
-	return (ce->e_lru_list.prev != NULL);
+	return (!list_empty(&ce->e_lru_list));
 }

@@ -167,9 +149,8 @@
 {
 	int n;
 
-	list_del(&ce->e_block_list);
-	ce->e_block_list.prev = NULL;
-	for (n=0; n<mb_cache_indexes(ce->e_cache); n++)
+	list_del_init(&ce->e_block_list);
+	for (n = 0; n < mb_cache_indexes(ce->e_cache); n++)
 		list_del(&ce->e_indexes[n].o_list);
 }

@@ -177,7 +158,7 @@
 static inline int
 __mb_cache_entry_is_linked(struct mb_cache_entry *ce)
 {
-	return (ce->e_block_list.prev != NULL);
+	return (!list_empty(&ce->e_block_list));
 }

@@ -207,15 +188,15 @@
 __mb_cache_entry_release_unlock(struct mb_cache_entry *ce)
 {
 	if (atomic_dec_and_test(&ce->e_used)) {
-		if (__mb_cache_entry_is_linked(ce))
-			__mb_cache_entry_into_lru(ce);
-		else {
-			mb_cache_unlock();
-			__mb_cache_entry_forget(ce);
-			return;
-		}
+		if (!__mb_cache_entry_is_linked(ce))
+			goto forget;
 	}
-	mb_cache_unlock();
+
+	spin_unlock(&mb_cache_spinlock);
+	return;
+ forget:
+	spin_unlock(&mb_cache_spinlock);
+	__mb_cache_entry_forget(ce);
 }

@@ -237,7 +218,7 @@
 	struct list_head *l;
 	int count = 0;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	list_for_each_prev(l, &mb_cache_list) {
 		struct mb_cache *cache =
 			list_entry(l, struct mb_cache, c_cache_list);

@@ -247,20 +228,20 @@
 	}
 	mb_debug("trying to free %d entries", nr_to_scan);
 	if (nr_to_scan == 0) {
-		mb_cache_unlock();
+		spin_unlock(&mb_cache_spinlock);
 		goto out;
 	}
 	while (nr_to_scan && !list_empty(&mb_cache_lru_list)) {
 		struct mb_cache_entry *ce =
 			list_entry(mb_cache_lru_list.prev,
 				   struct mb_cache_entry, e_lru_list);
-		list_del(&ce->e_lru_list);
-		list_add(&ce->e_lru_list, &free_list);
+		list_move(&ce->e_lru_list, &free_list);
 		if (__mb_cache_entry_is_linked(ce))
 			__mb_cache_entry_unlink(ce);
 		nr_to_scan--;
 	}
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
+
 	l = free_list.prev;
 	while (l != &free_list) {
 		struct mb_cache_entry *ce = list_entry(l,

@@ -303,7 +284,6 @@
 	   indexes_count * sizeof(struct mb_cache_entry_index))
 		return NULL;
 
-	MOD_INC_USE_COUNT;
 	cache = kmalloc(sizeof(struct mb_cache) +
 	                indexes_count * sizeof(struct list_head), GFP_KERNEL);
 	if (!cache)

@@ -340,7 +320,7 @@
 	if (!cache->c_entry_cache)
 		goto fail;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	if (list_empty(&mb_cache_list)) {
 		if (mb_shrinker) {
 			printk(KERN_ERR "%s: already have a shrinker!\n",

@@ -350,7 +330,7 @@
 		mb_shrinker = set_shrinker(DEFAULT_SEEKS, mb_cache_shrink_fn);
 	}
 	list_add(&cache->c_cache_list, &mb_cache_list);
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 	return cache;
 
 fail:

@@ -361,7 +341,6 @@
 			kfree(cache->c_block_hash);
 		kfree(cache);
 	}
-	MOD_DEC_USE_COUNT;
 	return NULL;
 }

@@ -382,20 +361,19 @@
 	LIST_HEAD(free_list);
 	struct list_head *l;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);
 		l = l->prev;
 		if (ce->e_dev == dev) {
-			list_del(&ce->e_lru_list);
-			list_add(&ce->e_lru_list, &free_list);
+			list_move(&ce->e_lru_list, &free_list);
 			if (__mb_cache_entry_is_linked(ce))
 				__mb_cache_entry_unlink(ce);
 		}
 	}
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 	l = free_list.prev;
 	while (l != &free_list) {
 		struct mb_cache_entry *ce =

@@ -420,15 +398,14 @@
 	struct list_head *l;
 	int n;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);
 		l = l->prev;
 		if (ce->e_cache == cache) {
-			list_del(&ce->e_lru_list);
-			list_add(&ce->e_lru_list, &free_list);
+			list_move(&ce->e_lru_list, &free_list);
 			if (__mb_cache_entry_is_linked(ce))
 				__mb_cache_entry_unlink(ce);
 		}

@@ -438,7 +415,7 @@
 		remove_shrinker(mb_shrinker);
 		mb_shrinker = 0;
 	}
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 
 	l = free_list.prev;
 	while (l != &free_list) {

@@ -454,19 +431,12 @@
 			  atomic_read(&cache->c_entry_count));
 	}
 
-#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,3,0))
-	/* We don't have kmem_cache_destroy() in 2.2.x */
-	kmem_cache_shrink(cache->c_entry_cache);
-#else
 	kmem_cache_destroy(cache->c_entry_cache);
-#endif
 	for (n=0; n < mb_cache_indexes(cache); n++)
 		kfree(cache->c_indexes_hash[n]);
 	kfree(cache->c_block_hash);
 
 	kfree(cache);
-
-	MOD_DEC_USE_COUNT;
 }

@@ -486,8 +456,8 @@
 	atomic_inc(&cache->c_entry_count);
 	ce = kmem_cache_alloc(cache->c_entry_cache, GFP_KERNEL);
 	if (ce) {
-		ce->e_lru_list.prev = NULL;
-		ce->e_block_list.prev = NULL;
+		INIT_LIST_HEAD(&ce->e_lru_list);
+		INIT_LIST_HEAD(&ce->e_block_list);
 		ce->e_cache = cache;
 		atomic_set(&ce->e_used, 1);
 	}

@@ -519,7 +489,7 @@
 	struct list_head *l;
 	int error = -EBUSY, n;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	list_for_each_prev(l, &cache->c_block_hash[bucket]) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_block_list);

@@ -533,7 +503,7 @@
 		ce->e_indexes[n].o_key = keys[n];
 	__mb_cache_entry_link(ce);
 out:
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 	return error;
 }

@@ -548,7 +518,7 @@
 void
 mb_cache_entry_release(struct mb_cache_entry *ce)
 {
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	__mb_cache_entry_release_unlock(ce);
 }

@@ -563,11 +533,11 @@
 void
 mb_cache_entry_takeout(struct mb_cache_entry *ce)
 {
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	mb_assert(!__mb_cache_entry_in_lru(ce));
 	if (__mb_cache_entry_is_linked(ce))
 		__mb_cache_entry_unlink(ce);
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 }

@@ -580,7 +550,7 @@
 void
 mb_cache_entry_free(struct mb_cache_entry *ce)
 {
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	mb_assert(!__mb_cache_entry_in_lru(ce));
 	if (__mb_cache_entry_is_linked(ce))
 		__mb_cache_entry_unlink(ce);

@@ -616,7 +586,7 @@
 	struct list_head *l;
 	struct mb_cache_entry *ce;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	list_for_each(l, &cache->c_block_hash[bucket]) {
 		ce = list_entry(l, struct mb_cache_entry, e_block_list);
 		if (ce->e_dev == dev && ce->e_block == block) {

@@ -627,7 +597,7 @@
 	ce = NULL;
 
 cleanup:
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 	return ce;
 }

@@ -674,11 +644,12 @@
 	struct mb_cache_entry *ce;
 
 	mb_assert(index < mb_cache_indexes(cache));
-	mb_cache_lock();
+
+	spin_lock(&mb_cache_spinlock);
 	l = cache->c_indexes_hash[index][bucket].next;
 	ce = __mb_cache_entry_find(l, &cache->c_indexes_hash[index][bucket],
 	                           index, dev, key);
-	mb_cache_unlock();
+	spin_unlock(&mb_cache_spinlock);
 	return ce;
 }

@@ -712,7 +683,8 @@
 	struct mb_cache_entry *ce;
 
 	mb_assert(index < mb_cache_indexes(cache));
-	mb_cache_lock();
+
+	spin_lock(&mb_cache_spinlock);
 	l = prev->e_indexes[index].o_list.next;
 	ce = __mb_cache_entry_find(l, &cache->c_indexes_hash[index][bucket],
 	                           index, dev, key);

Andreas Dilger

2002-10-15 16:01 UTC #10 lore raw

Quoted 21 lines · Christoph Hellwig

Patch 2: mbcache (all against 2.5.42-mm3):

fs/mbcache.c +1 -1

@@ -247,20 +228,20 @@
 	}
 	while (nr_to_scan && !list_empty(&mb_cache_lru_list)) {
 		struct mb_cache_entry *ce =
 			list_entry(mb_cache_lru_list.prev,
 				   struct mb_cache_entry, e_lru_list);

@@ -382,20 +361,19 @@
 	LIST_HEAD(free_list);
 	struct list_head *l;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);

fs/mbcache.c line 405

Quoted 9 lines

fs/mbcache.c +1 -1

@@ -420,15 +398,14 @@
 	struct list_head *l;
 	int n;
 
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);

Couldn't these all be "list_for_each{_safe}"?

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Christoph Hellwig

2002-10-15 16:10 UTC #11 lore raw

Quoted 8 lines · Andreas Dilger

fs/mbcache.c line 368

Quoted 6 lines

fs/mbcache.c +1 -1

-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);

Couldn't these all be "list_for_each{_safe}"?

They'd have to be list_for_each_safe_prev, which is not currently in list.h. The EVMS folks have a patch to add it, though..

Andreas Dilger

2002-10-15 16:18 UTC #12 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Andreas Dilger
fs/mbcache.c line 368
Quoted 6 lines
fs/mbcache.c +1 -1
-	mb_cache_lock();
+	spin_lock(&mb_cache_spinlock);
 	l = mb_cache_lru_list.prev;
 	while (l != &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);
Couldn't these all be "list_for_each{_safe}"?

Quoted 2 lines

They'd have to be list_for_each_safe_prev, which is not currently in list.h. The EVMS folks have a patch to add it, though..

Is there a reason why the code can't just add items into the list in the reverse order (i.e. list_add_tail()) and then walk in the normal direction via list_for_each_safe()?

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Andreas Gruenbacher

2002-10-15 16:37 UTC #13 lore raw

Quoted 3 lines · Andreas Dilger

Is there a reason why the code can't just add items into the list in the reverse order (i.e. list_add_tail()) and then walk in the normal direction via list_for_each_safe()?

No it could well be reversed.

--Andreas.

Theodore Ts'o

author

2002-10-15 16:35 UTC #14 lore raw

In reply to Christoph Hellwig

Quoted 4 lines · Christoph Hellwig

Patch 2: mbcache (all against 2.5.42-mm3):
o remove LINUX_VERSION_CODE mess

Note that this was fixed in the version I sent out; I don't think Andrew grabbed my latest patches. Anyway, I'll merge in the various suggested changes, and send Andrew an updated patch.

- Ted

Christoph Hellwig

2002-10-15 16:53 UTC #15 lore raw

Quoted 9 lines · Theodore Ts'o

Quoted 4 lines · Christoph Hellwig
Patch 2: mbcache (all against 2.5.42-mm3):
o remove LINUX_VERSION_CODE mess
Note that this was fixed in the version I sent out; I don't think Andrew grabbed my latest patches. Anyway, I'll merge in the various suggested changes, and send Andrew an updated patch.

Well, it'ß still in the patch series I that started this thread. I haven't seen newer patches yet.

Christoph Hellwig

2002-10-15 15:28 UTC #16 lore raw

In reply to Theodore Ts'o

Quoted 13 lines · Theodore Ts'o

Actually, I did, for those comments that made sense. The fs/Config.in logic has been cleaned up, as well as removing excess header files, stray LINUX_VERSION_CODE #ifdef's that I had missed the first time around, etc.
fs/mbcache.c is still there because it applies to both ext2 and ext3 filesystems, and so your suggestion of moving it into the ext2 and ext3 directories would cause code duplication and maintenance headaches. It also *can* be used by other filesystems, as it is written in a generic way. The fs/Config.in only compiles it in if necessary (i.e., if ext2/3 extended attribute is enabled) so it won't cause code bloat for other filesystems if it is not needed.

Quoted 4 lines

The superblock fields are more of an issue with the posix acl changes than for the extended attribute patches. I had wanted to get the extended attribute changes in first, since they stand alone, and so I have fewer patches to juggle.

Patch 3: first round of ext2 cleanups (all against 2.5.42-mm3):

o move include/linux/ext2_xattr.h to fs/ext2/xattr.h
o include xattr.h in the files that actually need it
o remove old compat code
o remove dead quota code
o remove ext2_xattr_lock/ext2_xattr_unlock wrappers (only two callers..)

More cleanups will follow.

fs/ext2/ext2.h +0 -1

@@ -1,6 +1,5 @@
 #include <linux/fs.h>
 #include <linux/ext2_fs.h>
-#include <linux/ext2_xattr.h>
 
 /*
  * second extended file system inode data in memory

fs/ext2/file.c +2 -1

@@ -18,8 +18,9 @@
  * 	([email protected])
  */
 
-#include "ext2.h"
 #include <linux/time.h>
+#include "ext2.h"
+#include "xattr.h"
 
 /*
  * Called when an inode is released. Note that this is different

fs/ext2/ialloc.c +1 -0

@@ -14,6 +14,7 @@
 
 #include <linux/config.h>
 #include "ext2.h"
+#include "xattr.h"
 #include <linux/quotaops.h>
 #include <linux/sched.h>
 #include <linux/backing-dev.h>

fs/ext2/namei.c +3 -1

@@ -29,8 +29,10 @@
  *        David S. Miller ([email protected]), 1995
  */
 
-#include "ext2.h"
 #include <linux/pagemap.h>
+#include "ext2.h"
+#include "xattr.h"
+
 
 /*
  * Couple of helper functions - make the code slightly cleaner.

fs/ext2/super.c +1 -0

@@ -27,6 +27,7 @@
 #include <linux/buffer_head.h>
 #include <linux/smp_lock.h>
 #include <asm/uaccess.h>
+#include "xattr.h"
 
 
 static void ext2_sync_super(struct super_block *sb,

fs/ext2/symlink.c +1 -0

@@ -18,6 +18,7 @@
  */
 
 #include "ext2.h"
+#include "xattr.h"
 
 static int ext2_readlink(struct dentry *dentry, char *buffer, int buflen)
 {

fs/ext2/xattr.c +22 ~~-111~~

232 lines not on this page · raw message

fs/ext2/xattr.h +134 -0

134 lines not on this page · raw message

fs/ext2/xattr_user.c +3 -1

@@ -5,9 +5,11 @@
  * Copyright (C) 2001 by Andreas Gruenbacher, <[email protected]>
  */
 
-#include "ext2.h"
+#include <linux/init.h>
 #include <linux/module.h>
 #include <linux/string.h>
+#include "ext2.h"
+#include "xattr.h"
 
 #ifdef CONFIG_EXT2_FS_POSIX_ACL
 # include <linux/ext2_acl.h>

include/linux/ext2_xattr.h +0 ~~-135~~

135 lines not on this page · raw message

Christoph Hellwig

2002-10-15 15:30 UTC #17 lore raw

In reply to Theodore Ts'o

Quoted 13 lines · Theodore Ts'o

Actually, I did, for those comments that made sense. The fs/Config.in logic has been cleaned up, as well as removing excess header files, stray LINUX_VERSION_CODE #ifdef's that I had missed the first time around, etc.
fs/mbcache.c is still there because it applies to both ext2 and ext3 filesystems, and so your suggestion of moving it into the ext2 and ext3 directories would cause code duplication and maintenance headaches. It also *can* be used by other filesystems, as it is written in a generic way. The fs/Config.in only compiles it in if necessary (i.e., if ext2/3 extended attribute is enabled) so it won't cause code bloat for other filesystems if it is not needed.

Quoted 4 lines

The superblock fields are more of an issue with the posix acl changes than for the extended attribute patches. I had wanted to get the extended attribute changes in first, since they stand alone, and so I have fewer patches to juggle.

Patch 4: first round of ext3 cleanups (all against 2.5.42-mm3):

o same for ext3

fs/ext3/file.c +1 -2

@@ -22,9 +22,8 @@
 #include <linux/fs.h>
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
-#include <linux/ext3_xattr.h>
 #include <linux/ext3_jbd.h>
-#include <linux/smp_lock.h>
+#include "xattr.h"
 
 /*
  * Called when an inode is released. Note that this is different

fs/ext3/ialloc.c +2 -1

@@ -17,7 +17,6 @@
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
 #include <linux/ext3_jbd.h>
-#include <linux/ext3_xattr.h>
 #include <linux/stat.h>
 #include <linux/string.h>
 #include <linux/quotaops.h>

@@ -25,6 +24,8 @@
 
 #include <asm/bitops.h>
 #include <asm/byteorder.h>
+
+#include "xattr.h"
 
 /*
  * ialloc.c contains the inodes allocation and deallocation routines

fs/ext3/namei.c +1 -1

@@ -30,13 +30,13 @@
 #include <linux/time.h>
 #include <linux/ext3_fs.h>
 #include <linux/ext3_jbd.h>
-#include <linux/ext3_xattr.h>
 #include <linux/fcntl.h>
 #include <linux/stat.h>
 #include <linux/string.h>
 #include <linux/quotaops.h>
 #include <linux/buffer_head.h>
 #include <linux/smp_lock.h>
+#include "xattr.h"
 
 
 /*

fs/ext3/super.c +1 -1

@@ -24,13 +24,13 @@
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
 #include <linux/ext3_jbd.h>
-#include <linux/ext3_xattr.h>
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <asm/uaccess.h>
+#include "xattr.h"
 
 #ifdef CONFIG_JBD_DEBUG
 static int ext3_ro_after; /* Make fs read-only after this many jiffies */

fs/ext3/symlink.c +1 -1

@@ -20,7 +20,7 @@
 #include <linux/fs.h>
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
-#include <linux/ext3_xattr.h>
+#include "xattr.h"
 
 static int ext3_readlink(struct dentry *dentry, char *buffer, int buflen)
 {

fs/ext3/xattr.c +19 ~~-85~~

190 lines not on this page · raw message

fs/ext3/xattr.h +136 -0

136 lines not on this page · raw message

fs/ext3/xattr_user.c +1 -1

@@ -11,7 +11,7 @@
 #include <linux/smp_lock.h>
 #include <linux/ext3_jbd.h>
 #include <linux/ext3_fs.h>
-#include <linux/ext3_xattr.h>
+#include "xattr.h"
 
 #ifdef CONFIG_EXT3_FS_POSIX_ACL
 # include <linux/ext3_acl.h>

include/linux/ext3_xattr.h +0 ~~-137~~

137 lines not on this page · raw message