| Message ID | 20171014003151.GK21978@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 14 Oct 2017, Al Viro wrote: > On Fri, Oct 13, 2017 at 04:09:23PM -0400, Nicolas Pitre wrote: > > On Fri, 13 Oct 2017, Al Viro wrote: > > > > > OK... I wonder if it should simply define stubs for kill_mtd_super(), > > > mtd_unpoint() and kill_block_super() in !CONFIG_MTD and !CONFIG_BLOCK > > > cases. mount_mtd() and mount_bdev() as well - e.g. mount_bdev() > > > returning ERR_PTR(-ENODEV) and kill_block_super() being simply BUG() > > > in !CONFIG_BLOCK case. Then cramfs_kill_sb() would be > > > if (sb->s_mtd) { > > > if (sbi->mtd_point_size) > > > mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size); > > > kill_mtd_super(sb); > > > } else { > > > kill_block_super(sb); > > > } > > > kfree(sbi); > > > > Well... Stubs have to be named differently or they conflict with > > existing declarations. At that point that makes for more lines of code > > compared to the current patch and the naming indirection makes it less > > obvious when reading the code. Alternatively I could add those stubs in > > the corresponding header files and #ifdef the existing declarations > > away. That might look somewhat less cluttered in the main code but it > > also hides what is actually going on and left me unconvinced. And I'm > > not sure this is worth it in the end given this is not a common > > occurrence in the kernel either. > > What I mean is this (completely untested) for CONFIG_BLOCK side of things, > with something similar for CONFIG_MTD one: > > Provide definitions of mount_bdev/kill_block_super() in case !CONFIG_BLOCK Yes, that's what I thought you meant, which corresponds to the second part of my comment above. And as I said I'm not convinced this hiding of kernel config effects is better for understanding what is actually going on locally, and my own preference is how things are right now. But if you confirm you really want things that other way then I'll oblige and repost. Nicolas
On Fri, 13 Oct 2017, Nicolas Pitre wrote: > On Sat, 14 Oct 2017, Al Viro wrote: > > > On Fri, Oct 13, 2017 at 04:09:23PM -0400, Nicolas Pitre wrote: > > > On Fri, 13 Oct 2017, Al Viro wrote: > > > > > > > OK... I wonder if it should simply define stubs for kill_mtd_super(), > > > > mtd_unpoint() and kill_block_super() in !CONFIG_MTD and !CONFIG_BLOCK > > > > cases. mount_mtd() and mount_bdev() as well - e.g. mount_bdev() > > > > returning ERR_PTR(-ENODEV) and kill_block_super() being simply BUG() > > > > in !CONFIG_BLOCK case. Then cramfs_kill_sb() would be > > > > if (sb->s_mtd) { > > > > if (sbi->mtd_point_size) > > > > mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size); > > > > kill_mtd_super(sb); > > > > } else { > > > > kill_block_super(sb); > > > > } > > > > kfree(sbi); > > > > > > Well... Stubs have to be named differently or they conflict with > > > existing declarations. At that point that makes for more lines of code > > > compared to the current patch and the naming indirection makes it less > > > obvious when reading the code. Alternatively I could add those stubs in > > > the corresponding header files and #ifdef the existing declarations > > > away. That might look somewhat less cluttered in the main code but it > > > also hides what is actually going on and left me unconvinced. And I'm > > > not sure this is worth it in the end given this is not a common > > > occurrence in the kernel either. > > > > What I mean is this (completely untested) for CONFIG_BLOCK side of things, > > with something similar for CONFIG_MTD one: > > > > Provide definitions of mount_bdev/kill_block_super() in case !CONFIG_BLOCK > > Yes, that's what I thought you meant, which corresponds to the second > part of my comment above. And as I said I'm not convinced this hiding of > kernel config effects is better for understanding what is actually going > on locally, and my own preference is how things are right now. Another case that your suggestion doesn't cover well is the ability to still have block device support in the kernel for other filesystems but _without_ block device support in the cramfs case. In other words, having CONFIG_BLOCK=y and CONFIG_CRAMFS_BLOCKDEV=n. This is a common case to have embedded devices with the root filesystem in flash while still needing access to a FAT filesystem on SD cards. Your stubs are conditional on CONFIG_BLOCK but that is not sufficient in this example. Nicolas
diff --git a/include/linux/fs.h b/include/linux/fs.h index 339e73742e73..e773c1c51aad 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2094,9 +2094,18 @@ struct file_system_type { extern struct dentry *mount_ns(struct file_system_type *fs_type, int flags, void *data, void *ns, struct user_namespace *user_ns, int (*fill_super)(struct super_block *, void *, int)); +#ifdef CONFIG_BLOCK extern struct dentry *mount_bdev(struct file_system_type *fs_type, int flags, const char *dev_name, void *data, int (*fill_super)(struct super_block *, void *, int)); +#else +static inline struct dentry *mount_bdev(struct file_system_type *fs_type, + int flags, const char *dev_name, void *data, + int (*fill_super)(struct super_block *, void *, int)) +{ + return ERR_PTR(-ENODEV); +} +#endif extern struct dentry *mount_single(struct file_system_type *fs_type, int flags, void *data, int (*fill_super)(struct super_block *, void *, int)); @@ -2105,7 +2114,14 @@ extern struct dentry *mount_nodev(struct file_system_type *fs_type, int (*fill_super)(struct super_block *, void *, int)); extern struct dentry *mount_subtree(struct vfsmount *mnt, const char *path); void generic_shutdown_super(struct super_block *sb); +#ifdef CONFIG_BLOCK void kill_block_super(struct super_block *sb); +#else +static inline void kill_block_super(struct super_block *sb) +{ + BUG(); +} +#endif void kill_anon_super(struct super_block *sb); void kill_litter_super(struct super_block *sb); void deactivate_super(struct super_block *sb);