cleancache: remove limit on the number of cleancache enabled filesystems

PATCH In review Vladimir Davydov 2 participants 12 messages Feb 22, 2015 – Mar 6, 2015

Subsystems (5)

Memory management Documentation VFS OCFS2 +1 more subsystems

lore

Share & tools

lore

Vladimir Davydov

author

2015-02-22 18:32 UTC #0 lore raw

cover cleancache: remove limit on the number of cleancache enabled filesystems

Hi,

Currently, maximal number of cleancache enabled filesystems equals 32, which is insufficient nowadays, because a Linux host can have hundreds of containers on board, each of which might want its own filesystem. This patch set targets at removing this limitation - see patch 4 for more details. Patches 1-3 prepare the code for this change.

Thanks,

Vladimir Davydov (4):
ocfs2: copy fs uuid to superblock
cleancache: zap uuid arg of cleancache_init_shared_fs
cleancache: forbid overriding cleancache_ops
cleancache: remove limit on the number of cleancache enabled
filesystems

 Documentation/vm/cleancache.txt |    4 +-
 drivers/xen/tmem.c              |   16 ++-
 fs/ocfs2/super.c                |    4 +-
 fs/super.c                      |    2 +-
 include/linux/cleancache.h      |   13 +-
 mm/cleancache.c                 |  270 +++++++++++----------------------------
 6 files changed, 94 insertions(+), 215 deletions(-)

Signature · 1 line

1.7.10.4

Vladimir Davydov

author

2015-02-22 18:32 UTC #1 lore raw

PATCH 1/4 ocfs2: copy fs uuid to superblock

This will allow us to remove the uuid argument from cleancache_init_shared_fs.

Signed-off-by Vladimir Davydov <[email protected]>

---
 fs/ocfs2/super.c |    2 ++
 1 file changed, 2 insertions(+)

fs/ocfs2/super.c +2 -0

index 26675185b886..43f5a9e71b35 100644
@@ -2069,6 +2069,8 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	cbits = le32_to_cpu(di->id2.i_super.s_clustersize_bits);
 	bbits = le32_to_cpu(di->id2.i_super.s_blocksize_bits);
 	sb->s_maxbytes = ocfs2_max_file_offset(bbits, cbits);
+	memcpy(sb->s_uuid, di->id2.i_super.s_uuid,
+	       sizeof(di->id2.i_super.s_uuid));
 
 	osb->osb_dx_mask = (1 << (cbits - bbits)) - 1;

Signature · 7 lines

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vladimir Davydov

author

2015-02-22 18:32 UTC #2 lore raw

PATCH 2/4 cleancache: zap uuid arg of cleancache_init_shared_fs

In reply to Vladimir Davydov

Use super_block->s_uuid instead. Every shared filesystem using cleancache must now initialize super_block->s_uuid before calling cleancache_init_shared_fs. The only one on the tree, ocfs2, already meets this requirement.

Signed-off-by Vladimir Davydov <[email protected]>

---
 fs/ocfs2/super.c           |    2 +-
 include/linux/cleancache.h |    6 +++---
 mm/cleancache.c            |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

fs/ocfs2/super.c +1 -1

index 43f5a9e71b35..18f830a9df50 100644
@@ -2335,7 +2335,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
 		mlog_errno(status);
 		goto bail;
 	}
-	cleancache_init_shared_fs((char *)&di->id2.i_super.s_uuid, sb);
+	cleancache_init_shared_fs(sb);
 
 bail:
 	return status;

include/linux/cleancache.h +3 -3

index 4ce9056b31a8..29657d1c83fb 100644
@@ -36,7 +36,7 @@ struct cleancache_ops {
 extern struct cleancache_ops *
 	cleancache_register_ops(struct cleancache_ops *ops);
 extern void __cleancache_init_fs(struct super_block *);
-extern void __cleancache_init_shared_fs(char *, struct super_block *);
+extern void __cleancache_init_shared_fs(struct super_block *);
 extern int  __cleancache_get_page(struct page *);
 extern void __cleancache_put_page(struct page *);
 extern void __cleancache_invalidate_page(struct address_space *, struct page *);

@@ -78,10 +78,10 @@ static inline void cleancache_init_fs(struct super_block *sb)
 		__cleancache_init_fs(sb);
 }
 
-static inline void cleancache_init_shared_fs(char *uuid, struct super_block *sb)
+static inline void cleancache_init_shared_fs(struct super_block *sb)
 {
 	if (cleancache_enabled)
-		__cleancache_init_shared_fs(uuid, sb);
+		__cleancache_init_shared_fs(sb);
 }
 
 static inline int cleancache_get_page(struct page *page)

mm/cleancache.c +3 -3

index 053bcd8f12fb..532495f2e4f4 100644
@@ -155,7 +155,7 @@ void __cleancache_init_fs(struct super_block *sb)
 EXPORT_SYMBOL(__cleancache_init_fs);
 
 /* Called by a cleancache-enabled clustered filesystem at time of mount */
-void __cleancache_init_shared_fs(char *uuid, struct super_block *sb)
+void __cleancache_init_shared_fs(struct super_block *sb)
 {
 	int i;

@@ -163,10 +163,10 @@ void __cleancache_init_shared_fs(char *uuid, struct super_block *sb)
 	for (i = 0; i < MAX_INITIALIZABLE_FS; i++) {
 		if (shared_fs_poolid_map[i] == FS_UNKNOWN) {
 			sb->cleancache_poolid = i + FAKE_SHARED_FS_POOLID_OFFSET;
-			uuids[i] = uuid;
+			uuids[i] = sb->s_uuid;
 			if (cleancache_ops)
 				shared_fs_poolid_map[i] = cleancache_ops->init_shared_fs
-						(uuid, PAGE_SIZE);
+						(sb->s_uuid, PAGE_SIZE);
 			else
 				shared_fs_poolid_map[i] = FS_NO_BACKEND;
 			break;

Signature · 1 line

1.7.10.4

Vladimir Davydov

author

2015-02-22 18:32 UTC #3 lore raw

PATCH 3/4 cleancache: forbid overriding cleancache_ops

In reply to Vladimir Davydov

Currently, cleancache_register_ops returns the previous value of cleancache_ops to allow chaining. However, chaining, as it is implemented now, is extremely dangerous due to possible pool id collisions. Suppose, a new cleancache driver is registered after the previous one assigned an id to a super block. If the new driver assigns the same id to another super block, which is perfectly possible, we will have two different filesystems using the same id. No matter if the new driver implements chaining or not, we are likely to get data corruption with such a configuration eventually.

This patch therefore disables the ability to override cleancache_ops altogether as potentially dangerous. If there is already cleancache driver registered, all further calls to cleancache_register_ops will return EBUSY. Since no user of cleancache implements chaining, we only need to make minor changes to the code outside the cleancache core.

Signed-off-by Vladimir Davydov <[email protected]>

---
 Documentation/vm/cleancache.txt |    4 +---
 drivers/xen/tmem.c              |   16 +++++++++-------
 include/linux/cleancache.h      |    3 +--
 mm/cleancache.c                 |   12 +++++++-----
 4 files changed, 18 insertions(+), 17 deletions(-)

Documentation/vm/cleancache.txt +1 -3

index 01d76282444e..e4b49df7a048 100644
@@ -28,9 +28,7 @@ IMPLEMENTATION OVERVIEW
 A cleancache "backend" that provides transcendent memory registers itself
 to the kernel's cleancache "frontend" by calling cleancache_register_ops,
 passing a pointer to a cleancache_ops structure with funcs set appropriately.
-Note that cleancache_register_ops returns the previous settings so that
-chaining can be performed if desired. The functions provided must conform to
-certain semantics as follows:
+The functions provided must conform to certain semantics as follows:
 
 Most important, cleancache is "ephemeral".  Pages which are copied into
 cleancache have an indefinite lifetime which is completely unknowable

drivers/xen/tmem.c +9 -7

index 8a65423bc696..8529e535459e 100644
@@ -397,13 +397,15 @@ static int __init xen_tmem_init(void)
 #ifdef CONFIG_CLEANCACHE
 	BUG_ON(sizeof(struct cleancache_filekey) != sizeof(struct tmem_oid));
 	if (tmem_enabled && cleancache) {
-		char *s = "";
-		struct cleancache_ops *old_ops =
-			cleancache_register_ops(&tmem_cleancache_ops);
-		if (old_ops)
-			s = " (WARNING: cleancache_ops overridden)";
-		pr_info("cleancache enabled, RAM provided by Xen Transcendent Memory%s\n",
-			s);
+		int err;
+		
+		err = cleancache_register_ops(&tmem_cleancache_ops);
+		if (err)
+			pr_warn("xen-tmem: failed to enable cleancache: %d\n",
+				err);
+		else
+			pr_info("cleancache enabled, RAM provided by "
+				"Xen Transcendent Memory\n");
 	}
 #endif
 #ifdef CONFIG_XEN_SELFBALLOONING

include/linux/cleancache.h +1 -2

index 29657d1c83fb..b23611f43cfb 100644
@@ -33,8 +33,7 @@ struct cleancache_ops {
 	void (*invalidate_fs)(int);
 };
 
-extern struct cleancache_ops *
-	cleancache_register_ops(struct cleancache_ops *ops);
+extern int cleancache_register_ops(struct cleancache_ops *ops);
 extern void __cleancache_init_fs(struct super_block *);
 extern void __cleancache_init_shared_fs(struct super_block *);
 extern int  __cleancache_get_page(struct page *);

mm/cleancache.c +7 -5

index 532495f2e4f4..aa10f9a3bc88 100644
@@ -106,15 +106,17 @@ static DEFINE_MUTEX(poolid_mutex);
  */
 
 /*
- * Register operations for cleancache, returning previous thus allowing
- * detection of multiple backends and possible nesting.
+ * Register operations for cleancache. Returns 0 on success.
  */
-struct cleancache_ops *cleancache_register_ops(struct cleancache_ops *ops)
+int cleancache_register_ops(struct cleancache_ops *ops)
 {
-	struct cleancache_ops *old = cleancache_ops;
 	int i;
 
 	mutex_lock(&poolid_mutex);
+	if (cleancache_ops) {
+		mutex_unlock(&poolid_mutex);
+		return -EBUSY;
+	}
 	for (i = 0; i < MAX_INITIALIZABLE_FS; i++) {
 		if (fs_poolid_map[i] == FS_NO_BACKEND)
 			fs_poolid_map[i] = ops->init_fs(PAGE_SIZE);

@@ -130,7 +132,7 @@ struct cleancache_ops *cleancache_register_ops(struct cleancache_ops *ops)
 	barrier();
 	cleancache_ops = ops;
 	mutex_unlock(&poolid_mutex);
-	return old;
+	return 0;
 }
 EXPORT_SYMBOL(cleancache_register_ops);

Signature · 7 lines

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vladimir Davydov

author

2015-02-22 18:32 UTC #4 lore raw

PATCH 4/4 cleancache: remove limit on the number of cleancache enabled filesystems

In reply to Vladimir Davydov

The limit equals 32 and is imposed by the number of entries in the fs_poolid_map and shared_fs_poolid_map. Nowadays it is insufficient, because with containers on board a Linux host can have hundreds of active fs mounts.

These maps were introduced by commit 49a9ab815acb8 ("mm: cleancache:
lazy initialization to allow tmem backends to build/run as modules") in order to allow compiling cleancache drivers as modules. Real pool ids are stored in these maps while super_block->cleancache_poolid points to an entry in the map, so that on cleancache registration we can walk over all (if there are <= 32 of them, of course) cleancache-enabled super blocks and assign real pool ids.

Actually, there is absolutely no need in these maps, because we can iterate over all super blocks immediately using iterate_supers. This is not racy, because cleancache_init_ops is called from mount_fs with super_block->s_umount held for writing, while iterate_supers takes this semaphore for reading, so if we call iterate_supers after setting cleancache_ops, all super blocks that had been created before cleancache_register_ops was called will be assigned pool ids by the action function of iterate_supers while all newer super blocks will receive it in cleancache_init_fs.

This patch therefore removes the maps and hence the artificial limit on the number of cleancache enabled filesystems.

Signed-off-by Vladimir Davydov <[email protected]>

---
 fs/super.c                 |    2 +-
 include/linux/cleancache.h |    4 +
 mm/cleancache.c            |  260 +++++++++++---------------------------------
 3 files changed, 71 insertions(+), 195 deletions(-)

fs/super.c +1 -1

index 65a53efc1cf4..ed5a9b9c3206 100644
@@ -224,7 +224,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	s->s_maxbytes = MAX_NON_LFS;
 	s->s_op = &default_op;
 	s->s_time_gran = 1000000000;
-	s->cleancache_poolid = -1;
+	s->cleancache_poolid = CLEANCACHE_NO_POOL;
 
 	s->s_shrink.seeks = DEFAULT_SEEKS;
 	s->s_shrink.scan_objects = super_cache_scan;

include/linux/cleancache.h +4 -0

index b23611f43cfb..bda5ec0b4b4d 100644
@@ -5,6 +5,10 @@
 #include <linux/exportfs.h>
 #include <linux/mm.h>
 
+#define CLEANCACHE_NO_POOL		-1
+#define CLEANCACHE_NO_BACKEND		-2
+#define CLEANCACHE_NO_BACKEND_SHARED	-3
+
 #define CLEANCACHE_KEY_MAX 6
 
 /*

mm/cleancache.c +66 ~~-194~~

384 lines not on this page · raw message

Signature · 1 line

1.7.10.4

Vladimir Davydov

author

2015-02-23 10:31 UTC #5 lore raw

Rechecking this patch, I find it rather difficult to review, because it not only rids of fake_pool_id, but also rearranges code of cleancache methods. Here is an updated patch, which attempts to be less intrusive:
---
From: Vladimir Davydov <[email protected]>
Subject: [PATCH v2] cleancache: remove limit on the number of cleancache
enabled filesystems

This patch therefore removes the maps and hence the artificial limit on the number of cleancache enabled filesystems.

Signed-off-by Vladimir Davydov <[email protected]>

---
Changes in v2:
- do not rearrange code in cleancache_{get,put,invalidate}_page
- use cmpxchg instead of spinlock to synchronize concurrent cleancache_ops updates

 fs/super.c                 |    2 +-
 include/linux/cleancache.h |    4 +
 mm/cleancache.c            |  223 ++++++++------------------------------------
 3 files changed, 45 insertions(+), 184 deletions(-)

fs/super.c +1 -1

index 65a53efc1cf4..ed5a9b9c3206 100644
@@ -224,7 +224,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	s->s_maxbytes = MAX_NON_LFS;
 	s->s_op = &default_op;
 	s->s_time_gran = 1000000000;
-	s->cleancache_poolid = -1;
+	s->cleancache_poolid = CLEANCACHE_NO_POOL;
 
 	s->s_shrink.seeks = DEFAULT_SEEKS;
 	s->s_shrink.scan_objects = super_cache_scan;

include/linux/cleancache.h +4 -0

index b23611f43cfb..bda5ec0b4b4d 100644
@@ -5,6 +5,10 @@
 #include <linux/exportfs.h>
 #include <linux/mm.h>
 
+#define CLEANCACHE_NO_POOL		-1
+#define CLEANCACHE_NO_BACKEND		-2
+#define CLEANCACHE_NO_BACKEND_SHARED	-3
+
 #define CLEANCACHE_KEY_MAX 6
 
 /*

mm/cleancache.c +40 ~~-183~~

333 lines not on this page · raw message

Signature · 1 line

1.7.10.4

Konrad Rzeszutek Wilk

2015-02-23 16:13 UTC #6 lore raw

In reply to Vladimir Davydov

Quoted 7 lines · Vladimir Davydov

Hi,
Currently, maximal number of cleancache enabled filesystems equals 32, which is insufficient nowadays, because a Linux host can have hundreds of containers on board, each of which might want its own filesystem. This patch set targets at removing this limitation - see patch 4 for more details. Patches 1-3 prepare the code for this change.

Hey Vladimir,

Thank you for posting these patches. I was wondering if you had run through some of the different combinations that you can load the filesystems/tmem drivers in random order? The #4 patch deleted a nice chunk of documentation that outlines the different combinations.

Thank you!

Quoted 10 lines

Thanks,
Vladimir Davydov (4):
ocfs2: copy fs uuid to superblock
cleancache: zap uuid arg of cleancache_init_shared_fs
cleancache: forbid overriding cleancache_ops
cleancache: remove limit on the number of cleancache enabled
filesystems

Quoted 11 lines

 Documentation/vm/cleancache.txt |    4 +-
 drivers/xen/tmem.c              |   16 ++-
 fs/ocfs2/super.c                |    4 +-
 fs/super.c                      |    2 +-
 include/linux/cleancache.h      |   13 +-
 mm/cleancache.c                 |  270 +++++++++++----------------------------
 6 files changed, 94 insertions(+), 215 deletions(-)

Signature · 1 line

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vladimir Davydov

author

2015-02-24 10:34 UTC #7 lore raw

Quoted 5 lines · Konrad Rzeszutek Wilk

Thank you for posting these patches. I was wondering if you had run through some of the different combinations that you can load the filesystems/tmem drivers in random order? The #4 patch deleted a nice chunk of documentation that outlines the different combinations.

Yeah, I admit the synchronization between cleancache_register_ops and cleancache_init_fs is far not obvious. I should have updated the comment instead of merely dropping it, sorry. What about the following patch proving correctness of register_ops-vs-init_fs synchronization? It is meant to be applied incrementally on top of patch #4.
---

mm/cleancache.c +51 -0

index fbdaf9c77d7a..8fc50811119b 100644
@@ -54,6 +54,57 @@ int cleancache_register_ops(struct cleancache_ops *ops)
 	if (cmpxchg(&cleancache_ops, NULL, ops))
 		return -EBUSY;
 
+	/*
+	 * A cleancache backend can be built as a module and hence loaded after
+	 * a cleancache enabled filesystem has called cleancache_init_fs. To
+	 * handle such a scenario, here we call ->init_fs or ->init_shared_fs
+	 * for each active super block. To differentiate between local and
+	 * shared filesystems, we temporarily initialize sb->cleancache_poolid
+	 * to CLEANCACHE_NO_BACKEND or CLEANCACHE_NO_BACKEND_SHARED
+	 * respectively in case there is no backend registered at the time
+	 * cleancache_init_fs or cleancache_init_shared_fs is called.
+	 *
+	 * Since filesystems can be mounted concurrently with cleancache
+	 * backend registration, we have to be careful to guarantee that all
+	 * cleancache enabled filesystems that has been mounted by the time
+	 * cleancache_register_ops is called has got and all mounted later will
+	 * get cleancache_poolid. This is assured by the following statements
+	 * tied together:
+	 *
+	 * a) iterate_supers skips only those super blocks that has started
+	 *    ->kill_sb
+	 *
+	 * b) if iterate_supers encounters a super block that has not finished
+	 *    ->mount yet, it waits until it is finished
+	 *
+	 * c) cleancache_init_fs is called from ->mount and
+	 *    cleancache_invalidate_fs is called from ->kill_sb
+	 *
+	 * d) we call iterate_supers after cleancache_ops has been set
+	 *
+	 * From a) it follows that if iterate_supers skips a super block, then
+	 * either the super block is already dead, in which case we do not need
+	 * to bother initializing cleancache for it, or it was mounted after we
+	 * initiated iterate_supers. In the latter case, it must have seen
+	 * cleancache_ops set according to d) and initialized cleancache from
+	 * ->mount by itself according to c). This proves that we call
+	 * ->init_fs at least once for each active super block.
+	 *
+	 * From b) and c) it follows that if iterate_supers encounters a super
+	 * block that has already started ->init_fs, it will wait until ->mount
+	 * and hence ->init_fs has finished, then check cleancache_poolid, see
+	 * that it has already been set and therefore do nothing. This proves
+	 * that we call ->init_fs no more than once for each super block.
+	 *
+	 * Combined together, the last two paragraphs prove the function
+	 * correctness.
+	 *
+	 * Note that various cleancache callbacks may proceed before this
+	 * function is called or even concurrently with it, but since
+	 * CLEANCACHE_NO_BACKEND is negative, they will all result in a noop
+	 * until the corresponding ->init_fs has been actually called and
+	 * cleancache_ops has been set.
+	 */
 	iterate_supers(cleancache_register_ops_sb, NULL);
 	return 0;
 }

Konrad Rzeszutek Wilk

2015-03-04 21:23 UTC #8 lore raw

Quoted 7 lines · Vladimir Davydov

Quoted 5 lines · Konrad Rzeszutek Wilk
Thank you for posting these patches. I was wondering if you had run through some of the different combinations that you can load the filesystems/tmem drivers in random order? The #4 patch deleted a nice chunk of documentation that outlines the different combinations.

Quoted 5 lines

Yeah, I admit the synchronization between cleancache_register_ops and cleancache_init_fs is far not obvious. I should have updated the comment instead of merely dropping it, sorry. What about the following patch proving correctness of register_ops-vs-init_fs synchronization? It is meant to be applied incrementally on top of patch #4.

Just fold it in please. But more importantly - I was wondering if you had run throught the different combinations it outlines?

Quoted 53 lines

---

mm/cleancache.c +44 -0

index fbdaf9c77d7a..8fc50811119b 100644
@@ -54,6 +54,57 @@ int cleancache_register_ops(struct cleancache_ops *ops)
 	if (cmpxchg(&cleancache_ops, NULL, ops))
 		return -EBUSY;
 
+	/*
+	 * A cleancache backend can be built as a module and hence loaded after
+	 * a cleancache enabled filesystem has called cleancache_init_fs. To
+	 * handle such a scenario, here we call ->init_fs or ->init_shared_fs
+	 * for each active super block. To differentiate between local and
+	 * shared filesystems, we temporarily initialize sb->cleancache_poolid
+	 * to CLEANCACHE_NO_BACKEND or CLEANCACHE_NO_BACKEND_SHARED
+	 * respectively in case there is no backend registered at the time
+	 * cleancache_init_fs or cleancache_init_shared_fs is called.
+	 *
+	 * Since filesystems can be mounted concurrently with cleancache
+	 * backend registration, we have to be careful to guarantee that all
+	 * cleancache enabled filesystems that has been mounted by the time
+	 * cleancache_register_ops is called has got and all mounted later will
+	 * get cleancache_poolid. This is assured by the following statements
+	 * tied together:
+	 *
+	 * a) iterate_supers skips only those super blocks that has started
+	 *    ->kill_sb
+	 *
+	 * b) if iterate_supers encounters a super block that has not finished
+	 *    ->mount yet, it waits until it is finished
+	 *
+	 * c) cleancache_init_fs is called from ->mount and
+	 *    cleancache_invalidate_fs is called from ->kill_sb
+	 *
+	 * d) we call iterate_supers after cleancache_ops has been set
+	 *
+	 * From a) it follows that if iterate_supers skips a super block, then
+	 * either the super block is already dead, in which case we do not need
+	 * to bother initializing cleancache for it, or it was mounted after we
+	 * initiated iterate_supers. In the latter case, it must have seen
+	 * cleancache_ops set according to d) and initialized cleancache from
+	 * ->mount by itself according to c). This proves that we call
+	 * ->init_fs at least once for each active super block.
+	 *
+	 * From b) and c) it follows that if iterate_supers encounters a super
+	 * block that has already started ->init_fs, it will wait until ->mount
+	 * and hence ->init_fs has finished, then check cleancache_poolid, see
+	 * that it has already been set and therefore do nothing. This proves
+	 * that we call ->init_fs no more than once for each super block.
+	 *
+	 * Combined together, the last two paragraphs prove the function
+	 * correctness.

mm/cleancache.c int cleancache_register_ops(struct cleancache_ops *ops) · line 110

Quoted 10 lines

mm/cleancache.c +7 -0

+	 *
+	 * Note that various cleancache callbacks may proceed before this
+	 * function is called or even concurrently with it, but since
+	 * CLEANCACHE_NO_BACKEND is negative, they will all result in a noop
+	 * until the corresponding ->init_fs has been actually called and
+	 * cleancache_ops has been set.
+	 */
 	iterate_supers(cleancache_register_ops_sb, NULL);
 	return 0;
 }

Vladimir Davydov

author

2015-03-05 16:46 UTC #9 lore raw

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 7 lines · Vladimir Davydov
Quoted 5 lines · Konrad Rzeszutek Wilk
Thank you for posting these patches. I was wondering if you had run through some of the different combinations that you can load the filesystems/tmem drivers in random order? The #4 patch deleted a nice chunk of documentation that outlines the different combinations.

Quoted 8 lines

Quoted 5 lines
Yeah, I admit the synchronization between cleancache_register_ops and cleancache_init_fs is far not obvious. I should have updated the comment instead of merely dropping it, sorry. What about the following patch proving correctness of register_ops-vs-init_fs synchronization? It is meant to be applied incrementally on top of patch #4.
Just fold it in please. But more importantly - I was wondering if you had run throught the different combinations it outlines?

Ah, you mean testing - I misunderstood you at first, sorry.

Of course, I checked that a cleancache backend module works fine no matter if it is loaded before or after a filesystem is mounted. However, I used our own cleancache driver for testing (we are trying to use cleancache for containers).

To be 100% sure that I did not occasionally break anything, today I installed XenServer on my test machine, enabled tmem both in dom0 and domU, and ran through all possible sequences of tmem load vs fs mount/use/unmount described in the old comment.

Thanks,
Vladimir

Konrad Rzeszutek Wilk

2015-03-06 15:15 UTC #10 lore raw

Quoted 25 lines · Vladimir Davydov

Quoted 8 lines · Konrad Rzeszutek Wilk
Quoted 7 lines · Vladimir Davydov
Quoted 5 lines · Konrad Rzeszutek Wilk
Thank you for posting these patches. I was wondering if you had run through some of the different combinations that you can load the filesystems/tmem drivers in random order? The #4 patch deleted a nice chunk of documentation that outlines the different combinations.
Quoted 8 lines
Quoted 5 lines
Yeah, I admit the synchronization between cleancache_register_ops and cleancache_init_fs is far not obvious. I should have updated the comment instead of merely dropping it, sorry. What about the following patch proving correctness of register_ops-vs-init_fs synchronization? It is meant to be applied incrementally on top of patch #4.
Just fold it in please. But more importantly - I was wondering if you had run throught the different combinations it outlines?
Ah, you mean testing - I misunderstood you at first, sorry.
Of course, I checked that a cleancache backend module works fine no matter if it is loaded before or after a filesystem is mounted. However, I used our own cleancache driver for testing (we are trying to use cleancache for containers).

Quoted 4 lines

To be 100% sure that I did not occasionally break anything, today I installed XenServer on my test machine, enabled tmem both in dom0 and domU, and ran through all possible sequences of tmem load vs fs mount/use/unmount described in the old comment.

Wow!

Well then, I think this patchset is ready to go then!

Would you be willing to fold in the description in the patch #4 and repost it?

Andrew - are you OK picking it up or would you prefer me as the maintainer to feed it to Linus? [either option is fine with me]

Quoted 3 lines

Thanks,
Vladimir

Vladimir Davydov

author

2015-03-06 16:01 UTC #11 lore raw

Quoted 4 lines · Konrad Rzeszutek Wilk

Would you be willing to fold in the description in the patch #4 and repost it?
Andrew - are you OK picking it up or would you prefer me as the maintainer to feed it to Linus? [either option is fine with me]

AFAICS Andrew has already picked it up with the description folded in patch #4.

Thanks,
Vladimir