| Message ID | 20240226224628.710547-1-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qnx4: convert qnx4 to use the new mount api | expand |
On Mon, 26 Feb 2024 16:46:28 -0600, Bill O'Donnell wrote: > Convert the qnx4 filesystem to use the new mount API. > > Tested mount, umount, and remount using a qnx4 boot image. > > Thank you! --- Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] qnx4: convert qnx4 to use the new mount api https://git.kernel.org/vfs/vfs/c/bc07c283ea2a
On Mon, Feb 26, 2024 at 04:46:28PM -0600, Bill O'Donnell wrote: > Convert the qnx4 filesystem to use the new mount API. > > Tested mount, umount, and remount using a qnx4 boot image. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- > fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c > index 6eb9bb369b57..c36fbe45a0e9 100644 > --- a/fs/qnx4/inode.c > +++ b/fs/qnx4/inode.c > @@ -21,6 +21,7 @@ > #include <linux/buffer_head.h> > #include <linux/writeback.h> > #include <linux/statfs.h> > +#include <linux/fs_context.h> > #include "qnx4.h" > > #define QNX4_VERSION 4 > @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; > > static struct inode *qnx4_alloc_inode(struct super_block *sb); > static void qnx4_free_inode(struct inode *inode); > -static int qnx4_remount(struct super_block *sb, int *flags, char *data); > static int qnx4_statfs(struct dentry *, struct kstatfs *); > +static int qnx4_get_tree(struct fs_context *fc); > > static const struct super_operations qnx4_sops = > { > .alloc_inode = qnx4_alloc_inode, > .free_inode = qnx4_free_inode, > .statfs = qnx4_statfs, > - .remount_fs = qnx4_remount, > }; > > -static int qnx4_remount(struct super_block *sb, int *flags, char *data) > +static int qnx4_reconfigure(struct fs_context *fc) > { > - struct qnx4_sb_info *qs; > + struct super_block *sb = fc->root->d_sb; > + struct qnx4_sb_info *qs = sb->s_fs_info; > > sync_filesystem(sb); > qs = qnx4_sb(sb); > qs->Version = QNX4_VERSION; > - *flags |= SB_RDONLY; > + fc->sb_flags |= SB_RDONLY; This confused me to no end because setting SB_RDONLY here unconditionally would be wrong if it's not requested from userspace during a remount. Because in that case the vfs wouldn't know that an actual read-only remount request had been made which means that we don't take the necessary protection steps to transition from read-write to read-only. But qnx{4,6} are read-only so this is actually correct even though it seems pretty weird.
On 2024-02-26 23:46 Bill O'Donnell wrote: > Convert the qnx4 filesystem to use the new mount API. > > Tested mount, umount, and remount using a qnx4 boot image. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> Acked-by: Anders Larsen <al@alarsen.net> > --- > fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c > index 6eb9bb369b57..c36fbe45a0e9 100644 > --- a/fs/qnx4/inode.c > +++ b/fs/qnx4/inode.c > @@ -21,6 +21,7 @@ > #include <linux/buffer_head.h> > #include <linux/writeback.h> > #include <linux/statfs.h> > +#include <linux/fs_context.h> > #include "qnx4.h" > > #define QNX4_VERSION 4 > @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; > > static struct inode *qnx4_alloc_inode(struct super_block *sb); > static void qnx4_free_inode(struct inode *inode); > -static int qnx4_remount(struct super_block *sb, int *flags, char *data); > static int qnx4_statfs(struct dentry *, struct kstatfs *); > +static int qnx4_get_tree(struct fs_context *fc); > > static const struct super_operations qnx4_sops = > { > .alloc_inode = qnx4_alloc_inode, > .free_inode = qnx4_free_inode, > .statfs = qnx4_statfs, > - .remount_fs = qnx4_remount, > }; > > -static int qnx4_remount(struct super_block *sb, int *flags, char *data) > +static int qnx4_reconfigure(struct fs_context *fc) > { > - struct qnx4_sb_info *qs; > + struct super_block *sb = fc->root->d_sb; > + struct qnx4_sb_info *qs = sb->s_fs_info; > > sync_filesystem(sb); > qs = qnx4_sb(sb); > qs->Version = QNX4_VERSION; > - *flags |= SB_RDONLY; > + fc->sb_flags |= SB_RDONLY; > return 0; > } > > +static const struct fs_context_operations qnx4_context_opts = { > + .get_tree = qnx4_get_tree, > + .reconfigure = qnx4_reconfigure, > +}; > + > static int qnx4_get_block( struct inode *inode, sector_t iblock, struct > buffer_head *bh, int create ) { > unsigned long phys; > @@ -183,12 +189,13 @@ static const char *qnx4_checkroot(struct super_block > *sb, return "bitmap file not found."; > } > > -static int qnx4_fill_super(struct super_block *s, void *data, int silent) > +static int qnx4_fill_super(struct super_block *s, struct fs_context *fc) > { > struct buffer_head *bh; > struct inode *root; > const char *errmsg; > struct qnx4_sb_info *qs; > + int silent = fc->sb_flags & SB_SILENT; > > qs = kzalloc(sizeof(struct qnx4_sb_info), GFP_KERNEL); > if (!qs) > @@ -216,7 +223,7 @@ static int qnx4_fill_super(struct super_block *s, void > *data, int silent) errmsg = qnx4_checkroot(s, (struct qnx4_super_block *) > bh->b_data); brelse(bh); > if (errmsg != NULL) { > - if (!silent) > + if (!silent) > printk(KERN_ERR "qnx4: %s\n", errmsg); > return -EINVAL; > } > @@ -235,6 +242,18 @@ static int qnx4_fill_super(struct super_block *s, void > *data, int silent) return 0; > } > > +static int qnx4_get_tree(struct fs_context *fc) > +{ > + return get_tree_bdev(fc, qnx4_fill_super); > +} > + > +static int qnx4_init_fs_context(struct fs_context *fc) > +{ > + fc->ops = &qnx4_context_opts; > + > + return 0; > +} > + > static void qnx4_kill_sb(struct super_block *sb) > { > struct qnx4_sb_info *qs = qnx4_sb(sb); > @@ -376,18 +395,12 @@ static void destroy_inodecache(void) > kmem_cache_destroy(qnx4_inode_cachep); > } > > -static struct dentry *qnx4_mount(struct file_system_type *fs_type, > - int flags, const char *dev_name, void *data) > -{ > - return mount_bdev(fs_type, flags, dev_name, data, qnx4_fill_super); > -} > - > static struct file_system_type qnx4_fs_type = { > - .owner = THIS_MODULE, > - .name = "qnx4", > - .mount = qnx4_mount, > - .kill_sb = qnx4_kill_sb, > - .fs_flags = FS_REQUIRES_DEV, > + .owner = THIS_MODULE, > + .name = "qnx4", > + .kill_sb = qnx4_kill_sb, > + .fs_flags = FS_REQUIRES_DEV, > + .init_fs_context = qnx4_init_fs_context, > }; > MODULE_ALIAS_FS("qnx4");
On 2/26/24 4:46 PM, Bill O'Donnell wrote: > Convert the qnx4 filesystem to use the new mount API. > > Tested mount, umount, and remount using a qnx4 boot image. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- > fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 18 deletions(-) > > diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c > index 6eb9bb369b57..c36fbe45a0e9 100644 > --- a/fs/qnx4/inode.c > +++ b/fs/qnx4/inode.c > @@ -21,6 +21,7 @@ > #include <linux/buffer_head.h> > #include <linux/writeback.h> > #include <linux/statfs.h> > +#include <linux/fs_context.h> > #include "qnx4.h" > > #define QNX4_VERSION 4 > @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; > > static struct inode *qnx4_alloc_inode(struct super_block *sb); > static void qnx4_free_inode(struct inode *inode); > -static int qnx4_remount(struct super_block *sb, int *flags, char *data); > static int qnx4_statfs(struct dentry *, struct kstatfs *); > +static int qnx4_get_tree(struct fs_context *fc); > > static const struct super_operations qnx4_sops = > { > .alloc_inode = qnx4_alloc_inode, > .free_inode = qnx4_free_inode, > .statfs = qnx4_statfs, > - .remount_fs = qnx4_remount, > }; > > -static int qnx4_remount(struct super_block *sb, int *flags, char *data) > +static int qnx4_reconfigure(struct fs_context *fc) > { > - struct qnx4_sb_info *qs; > + struct super_block *sb = fc->root->d_sb; > + struct qnx4_sb_info *qs = sb->s_fs_info; You assign *qs here at declaration > sync_filesystem(sb); > qs = qnx4_sb(sb); and then reassign it here (qnx4_sb() just gets sb->s_fs_info as well) Don't need both, I'd stick with just the uninitialized *qs as was originally in qnx4_remount(). The rest looks fine to me, -Eric
On 2/27/24 6:26 AM, Christian Brauner wrote: > On Mon, Feb 26, 2024 at 04:46:28PM -0600, Bill O'Donnell wrote: >> Convert the qnx4 filesystem to use the new mount API. >> >> Tested mount, umount, and remount using a qnx4 boot image. >> >> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> >> --- >> fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ >> 1 file changed, 31 insertions(+), 18 deletions(-) >> >> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c >> index 6eb9bb369b57..c36fbe45a0e9 100644 >> --- a/fs/qnx4/inode.c >> +++ b/fs/qnx4/inode.c >> @@ -21,6 +21,7 @@ >> #include <linux/buffer_head.h> >> #include <linux/writeback.h> >> #include <linux/statfs.h> >> +#include <linux/fs_context.h> >> #include "qnx4.h" >> >> #define QNX4_VERSION 4 >> @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; >> >> static struct inode *qnx4_alloc_inode(struct super_block *sb); >> static void qnx4_free_inode(struct inode *inode); >> -static int qnx4_remount(struct super_block *sb, int *flags, char *data); >> static int qnx4_statfs(struct dentry *, struct kstatfs *); >> +static int qnx4_get_tree(struct fs_context *fc); >> >> static const struct super_operations qnx4_sops = >> { >> .alloc_inode = qnx4_alloc_inode, >> .free_inode = qnx4_free_inode, >> .statfs = qnx4_statfs, >> - .remount_fs = qnx4_remount, >> }; >> >> -static int qnx4_remount(struct super_block *sb, int *flags, char *data) >> +static int qnx4_reconfigure(struct fs_context *fc) >> { >> - struct qnx4_sb_info *qs; >> + struct super_block *sb = fc->root->d_sb; >> + struct qnx4_sb_info *qs = sb->s_fs_info; >> >> sync_filesystem(sb); >> qs = qnx4_sb(sb); >> qs->Version = QNX4_VERSION; >> - *flags |= SB_RDONLY; >> + fc->sb_flags |= SB_RDONLY; > > This confused me to no end because setting SB_RDONLY here > unconditionally would be wrong if it's not requested from userspace > during a remount. Because in that case the vfs wouldn't know that an > actual read-only remount request had been made which means that we don't > take the necessary protection steps to transition from read-write to > read-only. But qnx{4,6} are read-only so this is actually correct even > though it seems pretty weird. This seems to be how every only-read-only filesystem does it, see erofs, romfs, squashfs, cramfs ... -Eric
On Tue, Feb 27, 2024 at 04:49:04PM -0600, Eric Sandeen wrote: > On 2/26/24 4:46 PM, Bill O'Donnell wrote: > > Convert the qnx4 filesystem to use the new mount API. > > > > Tested mount, umount, and remount using a qnx4 boot image. > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > > --- > > fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ > > 1 file changed, 31 insertions(+), 18 deletions(-) > > > > diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c > > index 6eb9bb369b57..c36fbe45a0e9 100644 > > --- a/fs/qnx4/inode.c > > +++ b/fs/qnx4/inode.c > > @@ -21,6 +21,7 @@ > > #include <linux/buffer_head.h> > > #include <linux/writeback.h> > > #include <linux/statfs.h> > > +#include <linux/fs_context.h> > > #include "qnx4.h" > > > > #define QNX4_VERSION 4 > > @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; > > > > static struct inode *qnx4_alloc_inode(struct super_block *sb); > > static void qnx4_free_inode(struct inode *inode); > > -static int qnx4_remount(struct super_block *sb, int *flags, char *data); > > static int qnx4_statfs(struct dentry *, struct kstatfs *); > > +static int qnx4_get_tree(struct fs_context *fc); > > > > static const struct super_operations qnx4_sops = > > { > > .alloc_inode = qnx4_alloc_inode, > > .free_inode = qnx4_free_inode, > > .statfs = qnx4_statfs, > > - .remount_fs = qnx4_remount, > > }; > > > > -static int qnx4_remount(struct super_block *sb, int *flags, char *data) > > +static int qnx4_reconfigure(struct fs_context *fc) > > { > > - struct qnx4_sb_info *qs; > > + struct super_block *sb = fc->root->d_sb; > > + struct qnx4_sb_info *qs = sb->s_fs_info; > > You assign *qs here at declaration > > > sync_filesystem(sb); > > qs = qnx4_sb(sb); > > and then reassign it here (qnx4_sb() just gets sb->s_fs_info as well) > > Don't need both, I'd stick with just the uninitialized *qs as was > originally in qnx4_remount(). I did wonder about that. I'll submit a v2. Thanks for your review. -Bill > > The rest looks fine to me, > -Eric > >
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c index 6eb9bb369b57..c36fbe45a0e9 100644 --- a/fs/qnx4/inode.c +++ b/fs/qnx4/inode.c @@ -21,6 +21,7 @@ #include <linux/buffer_head.h> #include <linux/writeback.h> #include <linux/statfs.h> +#include <linux/fs_context.h> #include "qnx4.h" #define QNX4_VERSION 4 @@ -30,28 +31,33 @@ static const struct super_operations qnx4_sops; static struct inode *qnx4_alloc_inode(struct super_block *sb); static void qnx4_free_inode(struct inode *inode); -static int qnx4_remount(struct super_block *sb, int *flags, char *data); static int qnx4_statfs(struct dentry *, struct kstatfs *); +static int qnx4_get_tree(struct fs_context *fc); static const struct super_operations qnx4_sops = { .alloc_inode = qnx4_alloc_inode, .free_inode = qnx4_free_inode, .statfs = qnx4_statfs, - .remount_fs = qnx4_remount, }; -static int qnx4_remount(struct super_block *sb, int *flags, char *data) +static int qnx4_reconfigure(struct fs_context *fc) { - struct qnx4_sb_info *qs; + struct super_block *sb = fc->root->d_sb; + struct qnx4_sb_info *qs = sb->s_fs_info; sync_filesystem(sb); qs = qnx4_sb(sb); qs->Version = QNX4_VERSION; - *flags |= SB_RDONLY; + fc->sb_flags |= SB_RDONLY; return 0; } +static const struct fs_context_operations qnx4_context_opts = { + .get_tree = qnx4_get_tree, + .reconfigure = qnx4_reconfigure, +}; + static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_head *bh, int create ) { unsigned long phys; @@ -183,12 +189,13 @@ static const char *qnx4_checkroot(struct super_block *sb, return "bitmap file not found."; } -static int qnx4_fill_super(struct super_block *s, void *data, int silent) +static int qnx4_fill_super(struct super_block *s, struct fs_context *fc) { struct buffer_head *bh; struct inode *root; const char *errmsg; struct qnx4_sb_info *qs; + int silent = fc->sb_flags & SB_SILENT; qs = kzalloc(sizeof(struct qnx4_sb_info), GFP_KERNEL); if (!qs) @@ -216,7 +223,7 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent) errmsg = qnx4_checkroot(s, (struct qnx4_super_block *) bh->b_data); brelse(bh); if (errmsg != NULL) { - if (!silent) + if (!silent) printk(KERN_ERR "qnx4: %s\n", errmsg); return -EINVAL; } @@ -235,6 +242,18 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent) return 0; } +static int qnx4_get_tree(struct fs_context *fc) +{ + return get_tree_bdev(fc, qnx4_fill_super); +} + +static int qnx4_init_fs_context(struct fs_context *fc) +{ + fc->ops = &qnx4_context_opts; + + return 0; +} + static void qnx4_kill_sb(struct super_block *sb) { struct qnx4_sb_info *qs = qnx4_sb(sb); @@ -376,18 +395,12 @@ static void destroy_inodecache(void) kmem_cache_destroy(qnx4_inode_cachep); } -static struct dentry *qnx4_mount(struct file_system_type *fs_type, - int flags, const char *dev_name, void *data) -{ - return mount_bdev(fs_type, flags, dev_name, data, qnx4_fill_super); -} - static struct file_system_type qnx4_fs_type = { - .owner = THIS_MODULE, - .name = "qnx4", - .mount = qnx4_mount, - .kill_sb = qnx4_kill_sb, - .fs_flags = FS_REQUIRES_DEV, + .owner = THIS_MODULE, + .name = "qnx4", + .kill_sb = qnx4_kill_sb, + .fs_flags = FS_REQUIRES_DEV, + .init_fs_context = qnx4_init_fs_context, }; MODULE_ALIAS_FS("qnx4");
Convert the qnx4 filesystem to use the new mount API. Tested mount, umount, and remount using a qnx4 boot image. Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> --- fs/qnx4/inode.c | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-)