From: Tamir <[email protected]>
Changed file_system_type next field to list_head and refactored
the code to use list.h functions.
Signed-off-by: Carmeli Tamir <[email protected]>
---
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
On Sat, May 04, 2019 at 05:45:48AM -0400, Carmeli Tamir wrote:
> From: Tamir <[email protected]>
>
> 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 <[email protected]>
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.
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 <[email protected]> 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.
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.
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 <[email protected]>
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 <[email protected]>
:::::: CC: Joel Becker <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <[email protected]>
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 <[email protected]>
:::::: CC: Joel Becker <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation