* [PATCH 0/2] Refactor file_systems to use the kernel's list
@ 2019-05-04 9:45 Carmeli Tamir
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
2019-05-04 9:45 ` [PATCH 2/2] Changed unsigned param type to unsigned int Carmeli Tamir
0 siblings, 2 replies; 9+ messages in thread
From: Carmeli Tamir @ 2019-05-04 9:45 UTC (permalink / raw)
To: carmeli.tamir, viro, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 9:45 [PATCH 0/2] Refactor file_systems to use the kernel's list Carmeli Tamir
@ 2019-05-04 9:45 ` Carmeli Tamir
2019-05-04 13:20 ` Al Viro
` (3 more replies)
2019-05-04 9:45 ` [PATCH 2/2] Changed unsigned param type to unsigned int Carmeli Tamir
1 sibling, 4 replies; 9+ messages in thread
From: Carmeli Tamir @ 2019-05-04 9:45 UTC (permalink / raw)
To: carmeli.tamir, viro, linux-fsdevel, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] Changed unsigned param type to unsigned int
2019-05-04 9:45 [PATCH 0/2] Refactor file_systems to use the kernel's list Carmeli Tamir
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
@ 2019-05-04 9:45 ` Carmeli Tamir
1 sibling, 0 replies; 9+ messages in thread
From: Carmeli Tamir @ 2019-05-04 9:45 UTC (permalink / raw)
To: carmeli.tamir, viro, linux-fsdevel, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
@ 2019-05-04 13:20 ` Al Viro
2019-05-04 13:45 ` Matthew Wilcox
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Al Viro @ 2019-05-04 13:20 UTC (permalink / raw)
To: Carmeli Tamir; +Cc: linux-fsdevel, linux-kernel
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
2019-05-04 13:20 ` Al Viro
@ 2019-05-04 13:45 ` Matthew Wilcox
2019-05-05 18:25 ` Tamir Carmeli
2019-05-06 15:16 ` kbuild test robot
2019-05-06 17:33 ` kbuild test robot
3 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2019-05-04 13:45 UTC (permalink / raw)
To: Carmeli Tamir; +Cc: viro, linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 13:45 ` Matthew Wilcox
@ 2019-05-05 18:25 ` Tamir Carmeli
2019-05-06 2:49 ` Matthew Wilcox
0 siblings, 1 reply; 9+ messages in thread
From: Tamir Carmeli @ 2019-05-05 18:25 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: viro, linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-05 18:25 ` Tamir Carmeli
@ 2019-05-06 2:49 ` Matthew Wilcox
0 siblings, 0 replies; 9+ messages in thread
From: Matthew Wilcox @ 2019-05-06 2:49 UTC (permalink / raw)
To: Tamir Carmeli; +Cc: viro, linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
2019-05-04 13:20 ` Al Viro
2019-05-04 13:45 ` Matthew Wilcox
@ 2019-05-06 15:16 ` kbuild test robot
2019-05-06 17:33 ` kbuild test robot
3 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2019-05-06 15:16 UTC (permalink / raw)
To: Carmeli Tamir
Cc: kbuild-all, carmeli.tamir, viro, linux-fsdevel, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Use list.h instead of file_system_type next
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
` (2 preceding siblings ...)
2019-05-06 15:16 ` kbuild test robot
@ 2019-05-06 17:33 ` kbuild test robot
3 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2019-05-06 17:33 UTC (permalink / raw)
To: Carmeli Tamir
Cc: kbuild-all, carmeli.tamir, viro, linux-fsdevel, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-06 17:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-04 9:45 [PATCH 0/2] Refactor file_systems to use the kernel's list Carmeli Tamir
2019-05-04 9:45 ` [PATCH 1/2] Use list.h instead of file_system_type next Carmeli Tamir
2019-05-04 13:20 ` Al Viro
2019-05-04 13:45 ` Matthew Wilcox
2019-05-05 18:25 ` Tamir Carmeli
2019-05-06 2:49 ` Matthew Wilcox
2019-05-06 15:16 ` kbuild test robot
2019-05-06 17:33 ` kbuild test robot
2019-05-04 9:45 ` [PATCH 2/2] Changed unsigned param type to unsigned int Carmeli Tamir
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).