[PATCH 0/2] Refactor file_systems to use the kernel's list

[PATCH 0/2] Refactor file_systems to use the kernel's list

[Carmeli Tamir]

struct file_system_type defines a next field which is used to link the file_system_type structs registered to the kernel. This patch set replaces the propietry linked list implementation with the kernel's list.h.

This change makes clearer interfaces and code (e.g. the change in register_filesystem and find_filesystem), and eliminates unnecessary usage of * and & operators.

Tested by comparing the lists in /proc/filesystems.

Carmeli Tamir (2):
  Use list.h instead of file_system_type next
  Changed unsigned param type to unsigned int

 fs/filesystems.c   | 69 ++++++++++++++++++++++++----------------------
 include/linux/fs.h |  2 +-
 2 files changed, 37 insertions(+), 34 deletions(-)

-- 
2.19.1

[PATCH 1/2] Use list.h instead of file_system_type next

[Carmeli Tamir]

From: Tamir <carmeli.tamir@gmail.com>

Changed file_system_type next field to list_head and refactored
the code to use list.h functions.

Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/filesystems.c   | 68 ++++++++++++++++++++++++----------------------
 include/linux/fs.h |  2 +-
 2 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 9135646e41ac..f12b98f2f079 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -31,7 +31,7 @@
  *	Once the reference is obtained we can drop the spinlock.
  */
 
-static struct file_system_type *file_systems;
+static LIST_HEAD(file_systems);
 static DEFINE_RWLOCK(file_systems_lock);
 
 /* WARNING: This can be used only if we _already_ own a reference */
@@ -46,14 +46,16 @@ void put_filesystem(struct file_system_type *fs)
 	module_put(fs->owner);
 }
 
-static struct file_system_type **find_filesystem(const char *name, unsigned len)
+static struct file_system_type *find_filesystem(const char *name, unsigned len)
 {
-	struct file_system_type **p;
-	for (p = &file_systems; *p; p = &(*p)->next)
-		if (strncmp((*p)->name, name, len) == 0 &&
-		    !(*p)->name[len])
-			break;
-	return p;
+	struct file_system_type *p;
+
+	list_for_each_entry(p, &file_systems, fs_types) {
+		if (strncmp(p->name, name, len) == 0 &&
+		    !p->name[len])
+			return p;
+	}
+	return NULL;
 }
 
 /**
@@ -72,20 +74,21 @@ static struct file_system_type **find_filesystem(const char *name, unsigned len)
 int register_filesystem(struct file_system_type * fs)
 {
 	int res = 0;
-	struct file_system_type ** p;
+	struct file_system_type *p;
 
 	if (fs->parameters && !fs_validate_description(fs->parameters))
 		return -EINVAL;
 
 	BUG_ON(strchr(fs->name, '.'));
-	if (fs->next)
-		return -EBUSY;
+
+	INIT_LIST_HEAD(&fs->fs_types);
+
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name, strlen(fs->name));
-	if (*p)
+	if (p)
 		res = -EBUSY;
 	else
-		*p = fs;
+		list_add_tail(&fs->fs_types, &file_systems);
 	write_unlock(&file_systems_lock);
 	return res;
 }
@@ -106,19 +109,16 @@ EXPORT_SYMBOL(register_filesystem);
  
 int unregister_filesystem(struct file_system_type * fs)
 {
-	struct file_system_type ** tmp;
+	struct file_system_type *tmp;
 
 	write_lock(&file_systems_lock);
-	tmp = &file_systems;
-	while (*tmp) {
-		if (fs == *tmp) {
-			*tmp = fs->next;
-			fs->next = NULL;
+	list_for_each_entry(tmp, &file_systems, fs_types) {
+		if (fs == tmp) {
+			list_del(&tmp->fs_types);
 			write_unlock(&file_systems_lock);
 			synchronize_rcu();
 			return 0;
 		}
-		tmp = &(*tmp)->next;
 	}
 	write_unlock(&file_systems_lock);
 
@@ -141,7 +141,8 @@ static int fs_index(const char __user * __name)
 
 	err = -EINVAL;
 	read_lock(&file_systems_lock);
-	for (tmp=file_systems, index=0 ; tmp ; tmp=tmp->next, index++) {
+	list_for_each_entry(tmp, &file_systems, fs_types) {
+		index++;
 		if (strcmp(tmp->name, name->name) == 0) {
 			err = index;
 			break;
@@ -158,9 +159,11 @@ static int fs_name(unsigned int index, char __user * buf)
 	int len, res;
 
 	read_lock(&file_systems_lock);
-	for (tmp = file_systems; tmp; tmp = tmp->next, index--)
+	list_for_each_entry(tmp, &file_systems, fs_types) {
+		index--;
 		if (index <= 0 && try_module_get(tmp->owner))
 			break;
+	}
 	read_unlock(&file_systems_lock);
 	if (!tmp)
 		return -EINVAL;
@@ -174,12 +177,13 @@ static int fs_name(unsigned int index, char __user * buf)
 
 static int fs_maxindex(void)
 {
-	struct file_system_type * tmp;
-	int index;
+	struct list_head *pos;
+	int index = 0;
 
 	read_lock(&file_systems_lock);
-	for (tmp = file_systems, index = 0 ; tmp ; tmp = tmp->next, index++)
-		;
+	list_for_each(pos, &file_systems) {
+		index++;
+	}
 	read_unlock(&file_systems_lock);
 	return index;
 }
@@ -214,12 +218,12 @@ int __init get_filesystem_list(char *buf)
 	struct file_system_type * tmp;
 
 	read_lock(&file_systems_lock);
-	tmp = file_systems;
-	while (tmp && len < PAGE_SIZE - 80) {
+	list_for_each_entry(tmp, &file_systems, fs_types) {
+		if (len >= PAGE_SIZE - 80)
+			break;
 		len += sprintf(buf+len, "%s\t%s\n",
 			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
 			tmp->name);
-		tmp = tmp->next;
 	}
 	read_unlock(&file_systems_lock);
 	return len;
@@ -231,12 +235,10 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
 	struct file_system_type * tmp;
 
 	read_lock(&file_systems_lock);
-	tmp = file_systems;
-	while (tmp) {
+	list_for_each_entry(tmp, &file_systems, fs_types) {
 		seq_printf(m, "%s\t%s\n",
 			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
 			tmp->name);
-		tmp = tmp->next;
 	}
 	read_unlock(&file_systems_lock);
 	return 0;
@@ -255,7 +257,7 @@ static struct file_system_type *__get_fs_type(const char *name, int len)
 	struct file_system_type *fs;
 
 	read_lock(&file_systems_lock);
-	fs = *(find_filesystem(name, len));
+	fs = find_filesystem(name, len);
 	if (fs && !try_module_get(fs->owner))
 		fs = NULL;
 	read_unlock(&file_systems_lock);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index dd28e7679089..833ada15bc8a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2182,7 +2182,7 @@ struct file_system_type {
 		       const char *, void *);
 	void (*kill_sb) (struct super_block *);
 	struct module *owner;
-	struct file_system_type * next;
+	struct list_head fs_types; /* All registered file_system_type structs */
 	struct hlist_head fs_supers;
 
 	struct lock_class_key s_lock_key;
-- 
2.19.1

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[Al Viro]

On Sat, May 04, 2019 at 05:45:48AM -0400, Carmeli Tamir wrote:
> From: Tamir <carmeli.tamir@gmail.com>
> 
> Changed file_system_type next field to list_head and refactored
> the code to use list.h functions.

... except that list_head is not a good match here.  For one thing,
we never walk that thing backwards.  For another, filesystem
can be used without ever going through register_filesystem(),
making your data structure quite a mess - e.g. use of list_empty()
(a perfectly normal list.h primitive) on it might oops on some
instances.

IOW, what you are making is not quite list_head and pretending it
to be list_head is asking for serious headache down the road.

Frankly, what's the point?  Reusing an existing data type, to
avoid DIY is generally a good advice, but then you'd better
make sure that existing type *does* fit your needs and that
your creation is playing by that type's rules.

This patch does neither...

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[Matthew Wilcox]

On Sat, May 04, 2019 at 05:45:48AM -0400, Carmeli Tamir wrote:
> Changed file_system_type next field to list_head and refactored
> the code to use list.h functions.

What might be interesting is getting rid of this list and using an XArray
instead.  This would be a more in-depth change; getting rid of the rwlock
in favour of using RCU accesses for the read-side and the xa_lock for
write accesses to the filesystem list.

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[Tamir Carmeli]

I just found it weird that there is a proprietary implementation of a
linked list while surely the kernel already offers well established
data structures.
IMO, the current code is a bit hard to understand, especially the
addition of a new struct to the list in the line "*p = fs" after
find_filesystem returned the last member.
Correct, I'm not familiar with all the use cases of the code.

I'm not sure that XArray is a good choice since there is no notion of
an index attached to the pointer, it's really just a linked list of
pointers.
Tell me if you think there is any way to improve my suggestion.
Thanks for you attention.


On Sat, May 4, 2019 at 4:45 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sat, May 04, 2019 at 05:45:48AM -0400, Carmeli Tamir wrote:
> > Changed file_system_type next field to list_head and refactored
> > the code to use list.h functions.
>
> What might be interesting is getting rid of this list and using an XArray
> instead.  This would be a more in-depth change; getting rid of the rwlock
> in favour of using RCU accesses for the read-side and the xa_lock for
> write accesses to the filesystem list.

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[Matthew Wilcox]

On Sun, May 05, 2019 at 09:25:21PM +0300, Tamir Carmeli wrote:
> I just found it weird that there is a proprietary implementation of a
> linked list while surely the kernel already offers well established
> data structures.

It's a singly linked list rather than a doubly linked list.

> IMO, the current code is a bit hard to understand, especially the
> addition of a new struct to the list in the line "*p = fs" after
> find_filesystem returned the last member.
> Correct, I'm not familiar with all the use cases of the code.

It looks like a fairly standard implementation of a singly-linked 
list in C to me.

> I'm not sure that XArray is a good choice since there is no notion of
> an index attached to the pointer, it's really just a linked list of
> pointers.

You don't need to attach an index to the pointer; you can just use
xa_alloc() to store it at the first available index.

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[kbuild test robot]

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

Hi Carmeli,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.1 next-20190506]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Carmeli-Tamir/Use-list-h-instead-of-file_system_type-next/20190506-214032
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/ocfs2/super.c:1250:3: error: 'struct file_system_type' has no member named 'next'
     .next           = NULL
      ^~~~

vim +1250 fs/ocfs2/super.c

ccd979bdb Mark Fasheh       2005-12-15  1243  
ccd979bdb Mark Fasheh       2005-12-15  1244  static struct file_system_type ocfs2_fs_type = {
ccd979bdb Mark Fasheh       2005-12-15  1245  	.owner          = THIS_MODULE,
ccd979bdb Mark Fasheh       2005-12-15  1246  	.name           = "ocfs2",
152a08366 Al Viro           2010-07-25  1247  	.mount          = ocfs2_mount,
8ed6b2370 Goldwyn Rodrigues 2014-04-03  1248  	.kill_sb        = kill_block_super,
1ba9da2ff Mark Fasheh       2006-09-08  1249  	.fs_flags       = FS_REQUIRES_DEV|FS_RENAME_DOES_D_MOVE,
ccd979bdb Mark Fasheh       2005-12-15 @1250  	.next           = NULL
ccd979bdb Mark Fasheh       2005-12-15  1251  };
914177054 Eric W. Biederman 2013-03-07  1252  MODULE_ALIAS_FS("ocfs2");
ccd979bdb Mark Fasheh       2005-12-15  1253  

:::::: The code at line 1250 was first introduced by commit
:::::: ccd979bdbce9fba8412beb3f1de68a9d0171b12c [PATCH] OCFS2: The Second Oracle Cluster Filesystem

:::::: TO: Mark Fasheh <mark.fasheh@oracle.com>
:::::: CC: Joel Becker <joel.becker@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56244 bytes --]

Re: [PATCH 1/2] Use list.h instead of file_system_type next

[kbuild test robot]

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

Hi Carmeli,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.1 next-20190506]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Carmeli-Tamir/Use-list-h-instead-of-file_system_type-next/20190506-214032
config: i386-randconfig-i2-201918 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> fs/ocfs2/super.c:1250:2: error: unknown field 'next' specified in initializer
     .next           = NULL
     ^

vim +/next +1250 fs/ocfs2/super.c

ccd979bdb Mark Fasheh       2005-12-15  1243  
ccd979bdb Mark Fasheh       2005-12-15  1244  static struct file_system_type ocfs2_fs_type = {
ccd979bdb Mark Fasheh       2005-12-15  1245  	.owner          = THIS_MODULE,
ccd979bdb Mark Fasheh       2005-12-15  1246  	.name           = "ocfs2",
152a08366 Al Viro           2010-07-25  1247  	.mount          = ocfs2_mount,
8ed6b2370 Goldwyn Rodrigues 2014-04-03  1248  	.kill_sb        = kill_block_super,
1ba9da2ff Mark Fasheh       2006-09-08  1249  	.fs_flags       = FS_REQUIRES_DEV|FS_RENAME_DOES_D_MOVE,
ccd979bdb Mark Fasheh       2005-12-15 @1250  	.next           = NULL
ccd979bdb Mark Fasheh       2005-12-15  1251  };
914177054 Eric W. Biederman 2013-03-07  1252  MODULE_ALIAS_FS("ocfs2");
ccd979bdb Mark Fasheh       2005-12-15  1253  

:::::: The code at line 1250 was first introduced by commit
:::::: ccd979bdbce9fba8412beb3f1de68a9d0171b12c [PATCH] OCFS2: The Second Oracle Cluster Filesystem

:::::: TO: Mark Fasheh <mark.fasheh@oracle.com>
:::::: CC: Joel Becker <joel.becker@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31111 bytes --]

[PATCH 2/2] Changed unsigned param type to unsigned int

[Carmeli Tamir]

From: Tamir <carmeli.tamir@gmail.com>

Fixed a checkpatch warning for usage of unsigned type.
Submitted as different patch in the series since it's not related
to the change, just wanted to fix checkpatch warnings from it.

Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/filesystems.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index f12b98f2f079..561fd7787822 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -46,7 +46,8 @@ void put_filesystem(struct file_system_type *fs)
 	module_put(fs->owner);
 }
 
-static struct file_system_type *find_filesystem(const char *name, unsigned len)
+static struct file_system_type *find_filesystem(const char *name,
+		unsigned int len)
 {
 	struct file_system_type *p;
 
-- 
2.19.1

[PATCH 0/2] Refactor file_systems to use the kernel's list

[Carmeli Tamir]

struct file_system_type defines a next field which is used to link the file_system_type structs registered to the kernel. This patch set replaces the propietry linked list implementation with the kernel's list.h.

This change makes clearer interfaces and code (e.g. the change in register_filesystem and find_filesystem), and eliminates unnecessary usage of * and & operators.

Tested by comparing the lists in /proc/filesystems.

Carmeli Tamir (2):
  Use list.h instead of file_system_type next
  Changed unsigned param type to unsigned int

 fs/filesystems.c   | 69 ++++++++++++++++++++++++----------------------
 include/linux/fs.h |  2 +-
 2 files changed, 37 insertions(+), 34 deletions(-)

-- 
2.19.1