| Message ID | 20231002023125.GE3389589@ZenIV (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] rcu pathwalk: prevent bogus hard errors from may_lookup() | expand |
Instead of all this duplicatio in the file system, can we please just add a struct nls_table *s_nls; to struct super_block and RCU free it in common code and drop all the code in the file systems?
On Mon, Oct 02, 2023 at 08:49:12AM +0200, Christoph Hellwig wrote: > Instead of all this duplicatio in the file system, can we please just > add a > > struct nls_table *s_nls; > > to struct super_block and RCU free it in common code and drop all the > code in the file systems? It makes no sense for most of the filesystems, for one thing (note that any use in ->lookup() does not warrant rcu delays). What's more, how do you formulate the rules for what goes in that field when filesystem uses more than one nls_table?
On Mon, Oct 02, 2023 at 08:14:01AM +0100, Al Viro wrote: > On Mon, Oct 02, 2023 at 08:49:12AM +0200, Christoph Hellwig wrote: > > Instead of all this duplicatio in the file system, can we please just > > add a > > > > struct nls_table *s_nls; > > > > to struct super_block and RCU free it in common code and drop all the > > code in the file systems? > > It makes no sense for most of the filesystems, for one thing (note that > any use in ->lookup() does not warrant rcu delays). What's more, > how do you formulate the rules for what goes in that field when filesystem > uses more than one nls_table? Consider e.g. HFS; two separate nls_table (nls_disk, nls_io), neither needs RCU delays of any sort. On VFAT, for that matter - again, two tables, one needs RCU delay, another doesn't (both get dropped from the same helper, so both get it).
On Mon, Oct 02, 2023 at 08:21:43AM +0100, Al Viro wrote: > On Mon, Oct 02, 2023 at 08:14:01AM +0100, Al Viro wrote: > > On Mon, Oct 02, 2023 at 08:49:12AM +0200, Christoph Hellwig wrote: > > > Instead of all this duplicatio in the file system, can we please just > > > add a > > > > > > struct nls_table *s_nls; > > > > > > to struct super_block and RCU free it in common code and drop all the > > > code in the file systems? > > > > It makes no sense for most of the filesystems, for one thing (note that > > any use in ->lookup() does not warrant rcu delays). What's more, > > how do you formulate the rules for what goes in that field when filesystem > > uses more than one nls_table? > > Consider e.g. HFS; two separate nls_table (nls_disk, nls_io), neither needs > RCU delays of any sort. On VFAT, for that matter - again, two tables, > one needs RCU delay, another doesn't (both get dropped from the same > helper, so both get it). BTW, is there any reason not to have synchronize_rcu() in delete_module(2), just before calling ->exit()? It's not a hot path, unless something really weird is going on, and it would get rid of the need to delay unload_nls() calls...
On Mon, 2 Oct 2023 at 11:09, Al Viro <viro@zeniv.linux.org.uk> wrote: > > BTW, is there any reason not to have synchronize_rcu() in delete_module(2), > just before calling ->exit()? > > It's not a hot path, unless something really weird is going on, and it > would get rid of the need to delay unload_nls() calls... We already have one - it's hidden in free_module(). It's done after the kallsyms list removal. Is that too late? Note that module *loading* actually has a few other synchronize_rcu() calls in the failure path. In fact, load_module() has *two* "synchronize_rcu()" calls: - after ftrace_release_mod(mod) - before releasing module_mutex for the failure path, but there's only one for module unloading. Adding one to after ftrace_release_mod() - where we have that async_synchronize_full() - would make module unloading match the loading error path. But the synchronize_rcu calls do seem to be a bit randomly sprinkled about. Maybe the one in free_module() is already sufficient for nls? Linus
On Wed, 4 Oct 2023 at 12:04, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Adding one to after ftrace_release_mod() - where we have that > async_synchronize_full() - would make module unloading match the > loading error path. > > But the synchronize_rcu calls do seem to be a bit randomly sprinkled > about. Maybe the one in free_module() is already sufficient for nls? Never mind. You want it before the ->exit(). That makes sense, and there is no matching code path for the module load failure case (since that implies no - or failed - init()). Linus
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index 7ededcb720c1..012a3d003fbe 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -190,6 +190,7 @@ struct hfsplus_sb_info { int work_queued; /* non-zero delayed work is queued */ struct delayed_work sync_work; /* FS sync delayed work */ spinlock_t work_lock; /* protects sync_work and work_queued */ + struct rcu_head rcu; }; #define HFSPLUS_SB_WRITEBACKUP 0 diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 1986b4f18a90..97920202790f 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -277,6 +277,14 @@ void hfsplus_mark_mdb_dirty(struct super_block *sb) spin_unlock(&sbi->work_lock); } +static void delayed_free(struct rcu_head *p) +{ + struct hfsplus_sb_info *sbi = container_of(p, struct hfsplus_sb_info, rcu); + + unload_nls(sbi->nls); + kfree(sbi); +} + static void hfsplus_put_super(struct super_block *sb) { struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb); @@ -302,9 +310,7 @@ static void hfsplus_put_super(struct super_block *sb) hfs_btree_close(sbi->ext_tree); kfree(sbi->s_vhdr_buf); kfree(sbi->s_backup_vhdr_buf); - unload_nls(sbi->nls); - kfree(sb->s_fs_info); - sb->s_fs_info = NULL; + call_rcu(&sbi->rcu, delayed_free); } static int hfsplus_statfs(struct dentry *dentry, struct kstatfs *buf)
->d_hash() and ->d_compare() use those, so we need to delay freeing them. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/hfsplus/hfsplus_fs.h | 1 + fs/hfsplus/super.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-)