* [PATCH - regression] devtmpfs: reconfigure on each mount @ 2021-12-13 1:12 NeilBrown 2021-12-13 12:59 ` Christian Brauner 0 siblings, 1 reply; 11+ messages in thread From: NeilBrown @ 2021-12-13 1:12 UTC (permalink / raw) To: Al Viro, David Howells, Greg Kroah-Hartman; +Cc: LKML, linux-fsdevel Prior to Linux v5.4 devtmpfs used mount_single() which treats the given mount options as "remount" options, updating the configuration of the single super_block on each mount. Since that was changed, the mount options used for devtmpfs are ignored. This is a regression which affects systemd - which mounts devtmpfs with "-o mode=755,size=4m,nr_inodes=1m". This patch restores the "remount" effect by calling reconfigure_single() Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") Signed-off-by: NeilBrown <neilb@suse.de> --- drivers/base/devtmpfs.c | 7 +++++++ fs/super.c | 4 ++-- include/linux/fs_context.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c index 8be352ab4ddb..fa13ad49d211 100644 --- a/drivers/base/devtmpfs.c +++ b/drivers/base/devtmpfs.c @@ -59,8 +59,15 @@ static struct dentry *public_dev_mount(struct file_system_type *fs_type, int fla const char *dev_name, void *data) { struct super_block *s = mnt->mnt_sb; + int err; + atomic_inc(&s->s_active); down_write(&s->s_umount); + err = reconfigure_single(s, flags, data); + if (err < 0) { + deactivate_locked_super(s); + return ERR_PTR(err); + } return dget(s->s_root); } diff --git a/fs/super.c b/fs/super.c index 3bfc0f8fbd5b..a6405d44d4ca 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1423,8 +1423,8 @@ struct dentry *mount_nodev(struct file_system_type *fs_type, } EXPORT_SYMBOL(mount_nodev); -static int reconfigure_single(struct super_block *s, - int flags, void *data) +int reconfigure_single(struct super_block *s, + int flags, void *data) { struct fs_context *fc; int ret; diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 6b54982fc5f3..13fa6f3df8e4 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -142,6 +142,8 @@ extern void put_fs_context(struct fs_context *fc); extern int vfs_parse_fs_param_source(struct fs_context *fc, struct fs_parameter *param); extern void fc_drop_locked(struct fs_context *fc); +int reconfigure_single(struct super_block *s, + int flags, void *data); /* * sget() wrappers to be called from the ->get_tree() op. -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH - regression] devtmpfs: reconfigure on each mount 2021-12-13 1:12 [PATCH - regression] devtmpfs: reconfigure on each mount NeilBrown @ 2021-12-13 12:59 ` Christian Brauner 2021-12-13 20:46 ` Anthony Iliopoulos 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2021-12-13 12:59 UTC (permalink / raw) To: NeilBrown; +Cc: Al Viro, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Mon, Dec 13, 2021 at 12:12:26PM +1100, NeilBrown wrote: > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > mount options as "remount" options, updating the configuration of the > single super_block on each mount. > Since that was changed, the mount options used for devtmpfs are ignored. > This is a regression which affects systemd - which mounts devtmpfs > with "-o mode=755,size=4m,nr_inodes=1m". > > This patch restores the "remount" effect by calling reconfigure_single() > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > Signed-off-by: NeilBrown <neilb@suse.de> > --- Hey Neil, So far this hasn't been an issue for us in systemd upstream. Is there a specific use-case where this is causing issues? I'm mostly asking because this change is fairly old. What I actually find more odd is that there's no .reconfigure for devtmpfs for non-vfs generic mount options it supports. So it's possible to change vfs generic stuff like mount -o remount,ro,nosuid /dev but none of the other mount options it supports and there's no word lost anywhere about whether or not that's on purpose. It feels odd because it uses the fs parameters from shmem/ramfs const struct fs_parameter_spec shmem_fs_parameters[] = { fsparam_u32 ("gid", Opt_gid), fsparam_enum ("huge", Opt_huge, shmem_param_enums_huge), fsparam_u32oct("mode", Opt_mode), fsparam_string("mpol", Opt_mpol), fsparam_string("nr_blocks", Opt_nr_blocks), fsparam_string("nr_inodes", Opt_nr_inodes), fsparam_string("size", Opt_size), fsparam_u32 ("uid", Opt_uid), fsparam_flag ("inode32", Opt_inode32), fsparam_flag ("inode64", Opt_inode64), {} } but doesn't allow to actually change them neither with your fix or with the old way of doing things. But afaict, all of them could be set via the "devtmpfs.mount" kernel command line option. So I could set gid=, uid=, and mpol= for devtmpfs via devtmpfs.mount but wouldn't be able to change it through remount or - in your case - with a mount with new parameters? Just wondering whether that's on purpose or an oversight. > drivers/base/devtmpfs.c | 7 +++++++ > fs/super.c | 4 ++-- > include/linux/fs_context.h | 2 ++ > 3 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c > index 8be352ab4ddb..fa13ad49d211 100644 > --- a/drivers/base/devtmpfs.c > +++ b/drivers/base/devtmpfs.c > @@ -59,8 +59,15 @@ static struct dentry *public_dev_mount(struct file_system_type *fs_type, int fla > const char *dev_name, void *data) > { > struct super_block *s = mnt->mnt_sb; > + int err; > + > atomic_inc(&s->s_active); > down_write(&s->s_umount); > + err = reconfigure_single(s, flags, data); > + if (err < 0) { > + deactivate_locked_super(s); > + return ERR_PTR(err); > + } > return dget(s->s_root); > } > > diff --git a/fs/super.c b/fs/super.c > index 3bfc0f8fbd5b..a6405d44d4ca 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -1423,8 +1423,8 @@ struct dentry *mount_nodev(struct file_system_type *fs_type, > } > EXPORT_SYMBOL(mount_nodev); > > -static int reconfigure_single(struct super_block *s, > - int flags, void *data) > +int reconfigure_single(struct super_block *s, > + int flags, void *data) > { > struct fs_context *fc; > int ret; > diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h > index 6b54982fc5f3..13fa6f3df8e4 100644 > --- a/include/linux/fs_context.h > +++ b/include/linux/fs_context.h > @@ -142,6 +142,8 @@ extern void put_fs_context(struct fs_context *fc); > extern int vfs_parse_fs_param_source(struct fs_context *fc, > struct fs_parameter *param); > extern void fc_drop_locked(struct fs_context *fc); > +int reconfigure_single(struct super_block *s, > + int flags, void *data); > > /* > * sget() wrappers to be called from the ->get_tree() op. > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - regression] devtmpfs: reconfigure on each mount 2021-12-13 12:59 ` Christian Brauner @ 2021-12-13 20:46 ` Anthony Iliopoulos 2021-12-14 10:12 ` Christian Brauner 0 siblings, 1 reply; 11+ messages in thread From: Anthony Iliopoulos @ 2021-12-13 20:46 UTC (permalink / raw) To: Christian Brauner Cc: NeilBrown, Al Viro, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Mon, Dec 13, 2021 at 01:59:06PM +0100, Christian Brauner wrote: > On Mon, Dec 13, 2021 at 12:12:26PM +1100, NeilBrown wrote: > > > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > > mount options as "remount" options, updating the configuration of the > > single super_block on each mount. > > Since that was changed, the mount options used for devtmpfs are ignored. > > This is a regression which affects systemd - which mounts devtmpfs > > with "-o mode=755,size=4m,nr_inodes=1m". > > > > This patch restores the "remount" effect by calling reconfigure_single() > > > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > Hey Neil, > > So far this hasn't been an issue for us in systemd upstream. Is there a > specific use-case where this is causing issues? I'm mostly asking > because this change is fairly old. This is standard init with systemd for SLE, where the systemd-provided mount params for devtmpfs are being effectively ignored due to this regression, so nr_inodes and size params are falling back to kernel defaults. It is also not specific to systemd, and can be easily reproduced by e.g. booting with devtmpfs.mount=0 and doing mount -t devtmpfs none /dev -o nr_inodes=1024. > What I actually find more odd is that there's no .reconfigure for > devtmpfs for non-vfs generic mount options it supports. There is a .reconfigure for devtmpfs, e.g. shmem_init_fs_context sets fc->ops to shmem_fs_context_ops, so everything goes through shmem_reconfigure. > So it's possible to change vfs generic stuff like > > mount -o remount,ro,nosuid /dev > > but none of the other mount options it supports and there's no word lost > anywhere about whether or not that's on purpose. That's not the case: even after d401727ea0d7 a remount can change any shmem-specific mount params. > It feels odd because it uses the fs parameters from shmem/ramfs > > const struct fs_parameter_spec shmem_fs_parameters[] = { > fsparam_u32 ("gid", Opt_gid), > fsparam_enum ("huge", Opt_huge, shmem_param_enums_huge), > fsparam_u32oct("mode", Opt_mode), > fsparam_string("mpol", Opt_mpol), > fsparam_string("nr_blocks", Opt_nr_blocks), > fsparam_string("nr_inodes", Opt_nr_inodes), > fsparam_string("size", Opt_size), > fsparam_u32 ("uid", Opt_uid), > fsparam_flag ("inode32", Opt_inode32), > fsparam_flag ("inode64", Opt_inode64), > {} > } > > but doesn't allow to actually change them neither with your fix or with > the old way of doing things. But afaict, all of them could be set via As per above, all those mount params are changeable via remount irrespective of the regression. What d401727ea0d7 regressed is that all those params are being ignored on new mounts only (and thus any init that mounts devtmpfs with params would be affected). > the "devtmpfs.mount" kernel command line option. So I could set gid=, > uid=, and mpol= for devtmpfs via devtmpfs.mount but wouldn't be able to > change it through remount or - in your case - with a mount with new > parameters? The devtmpfs.mount kernel boot param only controls if devtmpfs will be automatically mounted by the kernel during boot, and has nothing to do with the actual tmpfs mount params. Regards, Anthony ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - regression] devtmpfs: reconfigure on each mount 2021-12-13 20:46 ` Anthony Iliopoulos @ 2021-12-14 10:12 ` Christian Brauner 2021-12-14 14:06 ` Anthony Iliopoulos 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2021-12-14 10:12 UTC (permalink / raw) To: Anthony Iliopoulos Cc: NeilBrown, Al Viro, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Mon, Dec 13, 2021 at 09:46:53PM +0100, Anthony Iliopoulos wrote: > On Mon, Dec 13, 2021 at 01:59:06PM +0100, Christian Brauner wrote: > > On Mon, Dec 13, 2021 at 12:12:26PM +1100, NeilBrown wrote: > > > > > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > > > mount options as "remount" options, updating the configuration of the > > > single super_block on each mount. > > > Since that was changed, the mount options used for devtmpfs are ignored. > > > This is a regression which affects systemd - which mounts devtmpfs > > > with "-o mode=755,size=4m,nr_inodes=1m". > > > > > > This patch restores the "remount" effect by calling reconfigure_single() > > > > > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > --- > > > > Hey Neil, > > > > So far this hasn't been an issue for us in systemd upstream. Is there a > > specific use-case where this is causing issues? I'm mostly asking > > because this change is fairly old. > > This is standard init with systemd for SLE, where the systemd-provided > mount params for devtmpfs are being effectively ignored due to this > regression, so nr_inodes and size params are falling back to kernel > defaults. It is also not specific to systemd, and can be easily > reproduced by e.g. booting with devtmpfs.mount=0 and doing mount -t > devtmpfs none /dev -o nr_inodes=1024. > > > What I actually find more odd is that there's no .reconfigure for > > devtmpfs for non-vfs generic mount options it supports. > > There is a .reconfigure for devtmpfs, e.g. shmem_init_fs_context sets > fc->ops to shmem_fs_context_ops, so everything goes through > shmem_reconfigure. > > > So it's possible to change vfs generic stuff like > > > > mount -o remount,ro,nosuid /dev > > > > but none of the other mount options it supports and there's no word lost > > anywhere about whether or not that's on purpose. > > That's not the case: even after d401727ea0d7 a remount can change any > shmem-specific mount params. > > > It feels odd because it uses the fs parameters from shmem/ramfs > > > > const struct fs_parameter_spec shmem_fs_parameters[] = { > > fsparam_u32 ("gid", Opt_gid), > > fsparam_enum ("huge", Opt_huge, shmem_param_enums_huge), > > fsparam_u32oct("mode", Opt_mode), > > fsparam_string("mpol", Opt_mpol), > > fsparam_string("nr_blocks", Opt_nr_blocks), > > fsparam_string("nr_inodes", Opt_nr_inodes), > > fsparam_string("size", Opt_size), > > fsparam_u32 ("uid", Opt_uid), > > fsparam_flag ("inode32", Opt_inode32), > > fsparam_flag ("inode64", Opt_inode64), > > {} > > } > > > > but doesn't allow to actually change them neither with your fix or with > > the old way of doing things. But afaict, all of them could be set via > > As per above, all those mount params are changeable via remount > irrespective of the regression. What d401727ea0d7 regressed is that all Ah, I missed that. So shmem_reconfigure simple ignores some options for remount instead of returning an error. That's annoying: root@f2-vm:~# findmnt | grep devtmpfs ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 root@f2-vm:~# mount -o remount,gid=1000 /dev/ root@f2-vm:~# findmnt | grep devtmpfs ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 root@f2-vm:~# mount -o remount,mode=600 /dev root@f2-vm:~# findmnt | grep devtmpfs ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > those params are being ignored on new mounts only (and thus any init > that mounts devtmpfs with params would be affected). > > > the "devtmpfs.mount" kernel command line option. So I could set gid=, > > uid=, and mpol= for devtmpfs via devtmpfs.mount but wouldn't be able to > > change it through remount or - in your case - with a mount with new > > parameters? > > The devtmpfs.mount kernel boot param only controls if devtmpfs will be > automatically mounted by the kernel during boot, and has nothing to do > with the actual tmpfs mount params. Thanks! I'm not a fan of a proper mount changing mount options tbh but if it is a regression for users then we should fix it. Though I'm surprised it took that such a long time to even realize that there was a regression. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - regression] devtmpfs: reconfigure on each mount 2021-12-14 10:12 ` Christian Brauner @ 2021-12-14 14:06 ` Anthony Iliopoulos 2021-12-14 14:18 ` Christian Brauner 0 siblings, 1 reply; 11+ messages in thread From: Anthony Iliopoulos @ 2021-12-14 14:06 UTC (permalink / raw) To: Christian Brauner Cc: NeilBrown, Al Viro, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Tue, Dec 14, 2021 at 11:12:07AM +0100, Christian Brauner wrote: > On Mon, Dec 13, 2021 at 09:46:53PM +0100, Anthony Iliopoulos wrote: > > On Mon, Dec 13, 2021 at 01:59:06PM +0100, Christian Brauner wrote: > > > On Mon, Dec 13, 2021 at 12:12:26PM +1100, NeilBrown wrote: > > > > > > > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > > > > mount options as "remount" options, updating the configuration of the > > > > single super_block on each mount. > > > > Since that was changed, the mount options used for devtmpfs are ignored. > > > > This is a regression which affects systemd - which mounts devtmpfs > > > > with "-o mode=755,size=4m,nr_inodes=1m". > > > > > > > > This patch restores the "remount" effect by calling reconfigure_single() > > > > > > > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > --- > > > > > > Hey Neil, > > > > > > So far this hasn't been an issue for us in systemd upstream. Is there a > > > specific use-case where this is causing issues? I'm mostly asking > > > because this change is fairly old. > > > > This is standard init with systemd for SLE, where the systemd-provided > > mount params for devtmpfs are being effectively ignored due to this > > regression, so nr_inodes and size params are falling back to kernel > > defaults. It is also not specific to systemd, and can be easily > > reproduced by e.g. booting with devtmpfs.mount=0 and doing mount -t > > devtmpfs none /dev -o nr_inodes=1024. > > > > > What I actually find more odd is that there's no .reconfigure for > > > devtmpfs for non-vfs generic mount options it supports. > > > > There is a .reconfigure for devtmpfs, e.g. shmem_init_fs_context sets > > fc->ops to shmem_fs_context_ops, so everything goes through > > shmem_reconfigure. > > > > > So it's possible to change vfs generic stuff like > > > > > > mount -o remount,ro,nosuid /dev > > > > > > but none of the other mount options it supports and there's no word lost > > > anywhere about whether or not that's on purpose. > > > > That's not the case: even after d401727ea0d7 a remount can change any > > shmem-specific mount params. > > > > > It feels odd because it uses the fs parameters from shmem/ramfs > > > > > > const struct fs_parameter_spec shmem_fs_parameters[] = { > > > fsparam_u32 ("gid", Opt_gid), > > > fsparam_enum ("huge", Opt_huge, shmem_param_enums_huge), > > > fsparam_u32oct("mode", Opt_mode), > > > fsparam_string("mpol", Opt_mpol), > > > fsparam_string("nr_blocks", Opt_nr_blocks), > > > fsparam_string("nr_inodes", Opt_nr_inodes), > > > fsparam_string("size", Opt_size), > > > fsparam_u32 ("uid", Opt_uid), > > > fsparam_flag ("inode32", Opt_inode32), > > > fsparam_flag ("inode64", Opt_inode64), > > > {} > > > } > > > > > > but doesn't allow to actually change them neither with your fix or with > > > the old way of doing things. But afaict, all of them could be set via > > > > As per above, all those mount params are changeable via remount > > irrespective of the regression. What d401727ea0d7 regressed is that all > > Ah, I missed that. So shmem_reconfigure simple ignores some options for > remount instead of returning an error. That's annoying: > > root@f2-vm:~# findmnt | grep devtmpfs > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > > root@f2-vm:~# mount -o remount,gid=1000 /dev/ > root@f2-vm:~# findmnt | grep devtmpfs > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > > root@f2-vm:~# mount -o remount,mode=600 /dev > root@f2-vm:~# findmnt | grep devtmpfs > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 This is a slightly different issue: shmem_reconfigure intentionally and specifically does not reconfigure any of the uid/gid/mode options that you picked in the above examples, and those can only be set on initial mounts (and only on tmpfs, not devtmpfs). This was the case since devtmps inception given that there was always an internal kernel mount with hardcoded mount options (mode=0755), and any subsequent public mounts from userspace are simply remounts (thus for devtmpfs specifying uid/gid/mode was never possible). But any other shmem-specific mount option that can be reconfigured via remounts is working irrespective of this regression. What has really regressed is the ability to set the rest of the shmem_fs_parameters on devtmpfs on initial mounts (remounts work just fine). > > those params are being ignored on new mounts only (and thus any init > > that mounts devtmpfs with params would be affected). > > > > > the "devtmpfs.mount" kernel command line option. So I could set gid=, > > > uid=, and mpol= for devtmpfs via devtmpfs.mount but wouldn't be able to > > > change it through remount or - in your case - with a mount with new > > > parameters? > > > > The devtmpfs.mount kernel boot param only controls if devtmpfs will be > > automatically mounted by the kernel during boot, and has nothing to do > > with the actual tmpfs mount params. > > Thanks! > I'm not a fan of a proper mount changing mount options tbh but if it is > a regression for users then we should fix it. > Though I'm surprised it took that such a long time to even realize that > there was a regression. I think this is due to the devtmpfs shmem options falling back to kernel defaults, which are apparently good enough for most use-cases. The only reason we observed the regression is due to a customer case where the avail inodes in /dev where exhausted and thus userspace was getting -ENOSPC. Subsequent attempts to raise the nr_inodes during boot were failing due to the regression. Regards, Anthony ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - regression] devtmpfs: reconfigure on each mount 2021-12-14 14:06 ` Anthony Iliopoulos @ 2021-12-14 14:18 ` Christian Brauner 2022-01-16 22:07 ` [PATCH - resend] devtmpfs regression fix: " NeilBrown 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2021-12-14 14:18 UTC (permalink / raw) To: Anthony Iliopoulos Cc: NeilBrown, Al Viro, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Tue, Dec 14, 2021 at 03:06:29PM +0100, Anthony Iliopoulos wrote: > On Tue, Dec 14, 2021 at 11:12:07AM +0100, Christian Brauner wrote: > > On Mon, Dec 13, 2021 at 09:46:53PM +0100, Anthony Iliopoulos wrote: > > > On Mon, Dec 13, 2021 at 01:59:06PM +0100, Christian Brauner wrote: > > > > On Mon, Dec 13, 2021 at 12:12:26PM +1100, NeilBrown wrote: > > > > > > > > > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > > > > > mount options as "remount" options, updating the configuration of the > > > > > single super_block on each mount. > > > > > Since that was changed, the mount options used for devtmpfs are ignored. > > > > > This is a regression which affects systemd - which mounts devtmpfs > > > > > with "-o mode=755,size=4m,nr_inodes=1m". > > > > > > > > > > This patch restores the "remount" effect by calling reconfigure_single() > > > > > > > > > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > > --- > > > > > > > > Hey Neil, > > > > > > > > So far this hasn't been an issue for us in systemd upstream. Is there a > > > > specific use-case where this is causing issues? I'm mostly asking > > > > because this change is fairly old. > > > > > > This is standard init with systemd for SLE, where the systemd-provided > > > mount params for devtmpfs are being effectively ignored due to this > > > regression, so nr_inodes and size params are falling back to kernel > > > defaults. It is also not specific to systemd, and can be easily > > > reproduced by e.g. booting with devtmpfs.mount=0 and doing mount -t > > > devtmpfs none /dev -o nr_inodes=1024. > > > > > > > What I actually find more odd is that there's no .reconfigure for > > > > devtmpfs for non-vfs generic mount options it supports. > > > > > > There is a .reconfigure for devtmpfs, e.g. shmem_init_fs_context sets > > > fc->ops to shmem_fs_context_ops, so everything goes through > > > shmem_reconfigure. > > > > > > > So it's possible to change vfs generic stuff like > > > > > > > > mount -o remount,ro,nosuid /dev > > > > > > > > but none of the other mount options it supports and there's no word lost > > > > anywhere about whether or not that's on purpose. > > > > > > That's not the case: even after d401727ea0d7 a remount can change any > > > shmem-specific mount params. > > > > > > > It feels odd because it uses the fs parameters from shmem/ramfs > > > > > > > > const struct fs_parameter_spec shmem_fs_parameters[] = { > > > > fsparam_u32 ("gid", Opt_gid), > > > > fsparam_enum ("huge", Opt_huge, shmem_param_enums_huge), > > > > fsparam_u32oct("mode", Opt_mode), > > > > fsparam_string("mpol", Opt_mpol), > > > > fsparam_string("nr_blocks", Opt_nr_blocks), > > > > fsparam_string("nr_inodes", Opt_nr_inodes), > > > > fsparam_string("size", Opt_size), > > > > fsparam_u32 ("uid", Opt_uid), > > > > fsparam_flag ("inode32", Opt_inode32), > > > > fsparam_flag ("inode64", Opt_inode64), > > > > {} > > > > } > > > > > > > > but doesn't allow to actually change them neither with your fix or with > > > > the old way of doing things. But afaict, all of them could be set via > > > > > > As per above, all those mount params are changeable via remount > > > irrespective of the regression. What d401727ea0d7 regressed is that all > > > > Ah, I missed that. So shmem_reconfigure simple ignores some options for > > remount instead of returning an error. That's annoying: > > > > root@f2-vm:~# findmnt | grep devtmpfs > > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > > > > root@f2-vm:~# mount -o remount,gid=1000 /dev/ > > root@f2-vm:~# findmnt | grep devtmpfs > > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > > > > root@f2-vm:~# mount -o remount,mode=600 /dev > > root@f2-vm:~# findmnt | grep devtmpfs > > ├─/dev udev devtmpfs rw,nosuid,noexec,relatime,size=1842984k,nr_inodes=460746,mode=755,inode64 > > This is a slightly different issue: shmem_reconfigure intentionally and > specifically does not reconfigure any of the uid/gid/mode options that > you picked in the above examples, and those can only be set on initial > mounts (and only on tmpfs, not devtmpfs). > > This was the case since devtmps inception given that there was always an > internal kernel mount with hardcoded mount options (mode=0755), and any > subsequent public mounts from userspace are simply remounts (thus for > devtmpfs specifying uid/gid/mode was never possible). > > But any other shmem-specific mount option that can be reconfigured via > remounts is working irrespective of this regression. What has really Right, I understood all that. Just confusing from todays perspective that mount options that can't be changed on (superblock) remount are silently skipped instead of causing an error. But for historical reasons it obviously makes sense. > regressed is the ability to set the rest of the shmem_fs_parameters on > devtmpfs on initial mounts (remounts work just fine). > > > > those params are being ignored on new mounts only (and thus any init > > > that mounts devtmpfs with params would be affected). > > > > > > > the "devtmpfs.mount" kernel command line option. So I could set gid=, > > > > uid=, and mpol= for devtmpfs via devtmpfs.mount but wouldn't be able to > > > > change it through remount or - in your case - with a mount with new > > > > parameters? > > > > > > The devtmpfs.mount kernel boot param only controls if devtmpfs will be > > > automatically mounted by the kernel during boot, and has nothing to do > > > with the actual tmpfs mount params. > > > > Thanks! > > I'm not a fan of a proper mount changing mount options tbh but if it is > > a regression for users then we should fix it. > > Though I'm surprised it took that such a long time to even realize that > > there was a regression. > > I think this is due to the devtmpfs shmem options falling back to kernel > defaults, which are apparently good enough for most use-cases. The only > reason we observed the regression is due to a customer case where the > avail inodes in /dev where exhausted and thus userspace was getting > -ENOSPC. Subsequent attempts to raise the nr_inodes during boot were > failing due to the regression. Ah, that sucks. Acked-by: Christian Brauner <christian.brauner@ubuntu.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH - resend] devtmpfs regression fix: reconfigure on each mount 2021-12-14 14:18 ` Christian Brauner @ 2022-01-16 22:07 ` NeilBrown 2022-01-17 7:47 ` Linus Torvalds 2022-01-17 8:03 ` Greg Kroah-Hartman 0 siblings, 2 replies; 11+ messages in thread From: NeilBrown @ 2022-01-16 22:07 UTC (permalink / raw) To: Al Viro, Andrew Morton, torvalds Cc: Christian Brauner, Anthony Iliopoulos, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel Prior to Linux v5.4 devtmpfs used mount_single() which treats the given mount options as "remount" options, so it updates the configuration of the single super_block on each mount. Since that was changed, the mount options used for devtmpfs are ignored. This is a regression which affect systemd - which mounts devtmpfs with "-o mode=755,size=4m,nr_inodes=1m". This patch restores the "remount" effect by calling reconfigure_single() Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") Acked-by: Christian Brauner <christian.brauner@ubuntu.com> Signed-off-by: NeilBrown <neilb@suse.de> --- drivers/base/devtmpfs.c | 7 +++++++ fs/super.c | 4 ++-- include/linux/fs_context.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c index 1e2c2d3882e2..f41063ac1aee 100644 --- a/drivers/base/devtmpfs.c +++ b/drivers/base/devtmpfs.c @@ -65,8 +65,15 @@ static struct dentry *public_dev_mount(struct file_system_type *fs_type, int fla const char *dev_name, void *data) { struct super_block *s = mnt->mnt_sb; + int err; + atomic_inc(&s->s_active); down_write(&s->s_umount); + err = reconfigure_single(s, flags, data); + if (err < 0) { + deactivate_locked_super(s); + return ERR_PTR(err); + } return dget(s->s_root); } diff --git a/fs/super.c b/fs/super.c index 3bfc0f8fbd5b..a6405d44d4ca 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1423,8 +1423,8 @@ struct dentry *mount_nodev(struct file_system_type *fs_type, } EXPORT_SYMBOL(mount_nodev); -static int reconfigure_single(struct super_block *s, - int flags, void *data) +int reconfigure_single(struct super_block *s, + int flags, void *data) { struct fs_context *fc; int ret; diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 6b54982fc5f3..13fa6f3df8e4 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -142,6 +142,8 @@ extern void put_fs_context(struct fs_context *fc); extern int vfs_parse_fs_param_source(struct fs_context *fc, struct fs_parameter *param); extern void fc_drop_locked(struct fs_context *fc); +int reconfigure_single(struct super_block *s, + int flags, void *data); /* * sget() wrappers to be called from the ->get_tree() op. -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH - resend] devtmpfs regression fix: reconfigure on each mount 2022-01-16 22:07 ` [PATCH - resend] devtmpfs regression fix: " NeilBrown @ 2022-01-17 7:47 ` Linus Torvalds 2022-01-17 22:57 ` NeilBrown 2022-01-17 8:03 ` Greg Kroah-Hartman 1 sibling, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2022-01-17 7:47 UTC (permalink / raw) To: NeilBrown Cc: Al Viro, Andrew Morton, Christian Brauner, Anthony Iliopoulos, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Mon, Jan 17, 2022 at 12:07 AM NeilBrown <neilb@suse.de> wrote: > > Since that was changed, the mount options used for devtmpfs are ignored. > This is a regression which affect systemd - which mounts devtmpfs > with "-o mode=755,size=4m,nr_inodes=1m". Hmm, I've applied this, but I'd have loved to see what the actual symptoms of the problem were. Particularly since it's apparently been broken for 18 months with this being the first I hear of it. Yes, yes, I could (and did) search for this on the mailing lists, and found the discussion and more information, but I think that info should have been in the commit message rather than me having to go look for it just to see the clarifications.. Linus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - resend] devtmpfs regression fix: reconfigure on each mount 2022-01-17 7:47 ` Linus Torvalds @ 2022-01-17 22:57 ` NeilBrown 0 siblings, 0 replies; 11+ messages in thread From: NeilBrown @ 2022-01-17 22:57 UTC (permalink / raw) To: Linus Torvalds Cc: Al Viro, Andrew Morton, Christian Brauner, Anthony Iliopoulos, David Howells, Greg Kroah-Hartman, LKML, linux-fsdevel On Mon, 17 Jan 2022, Linus Torvalds wrote: > On Mon, Jan 17, 2022 at 12:07 AM NeilBrown <neilb@suse.de> wrote: > > > > Since that was changed, the mount options used for devtmpfs are ignored. > > This is a regression which affect systemd - which mounts devtmpfs > > with "-o mode=755,size=4m,nr_inodes=1m". > > Hmm, I've applied this, but I'd have loved to see what the actual > symptoms of the problem were. Particularly since it's apparently been > broken for 18 months with this being the first I hear of it. > > Yes, yes, I could (and did) search for this on the mailing lists, and > found the discussion and more information, but I think that info > should have been in the commit message rather than me having to go > look for it just to see the clarifications.. Sorry about that. The trail was a bit convoluted and I hadn't bothered to straighten it out as the behavioural change was so easy to demonstrate. I've had a better look now. A customer with a 5.3 based kernel reported that udev was having problems creating all the symlinks for lots of LUNs for some storage array (With dm devices over the LUNs etc). It ran out of inodes because systemd always mounts devtmpfs with size=4m,nr_inodes=64k This was bumped to 128k and then to 1m in systemd-v250. We provided our customer with a systemd which used a larger limit, but when we tested this fix on Tumbleweed (with a newer kernel), we noticed that it had no effect. Now admittedly the default provided by the kernel is a lot bigger than even the current 1m setting from systemd so maybe this doesn't matter. Had the commit which changed behaviour said "systemd sets nr_inodes to a stupidly low number, let's just ignore the mount options", then I probably would have accept it. But it looked like behaviour change without justification and that suggests a regression. So I patched it. Thanks, NeilBrown ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - resend] devtmpfs regression fix: reconfigure on each mount 2022-01-16 22:07 ` [PATCH - resend] devtmpfs regression fix: " NeilBrown 2022-01-17 7:47 ` Linus Torvalds @ 2022-01-17 8:03 ` Greg Kroah-Hartman 2022-01-17 8:04 ` Greg Kroah-Hartman 1 sibling, 1 reply; 11+ messages in thread From: Greg Kroah-Hartman @ 2022-01-17 8:03 UTC (permalink / raw) To: NeilBrown Cc: Al Viro, Andrew Morton, torvalds, Christian Brauner, Anthony Iliopoulos, David Howells, LKML, linux-fsdevel On Mon, Jan 17, 2022 at 09:07:26AM +1100, NeilBrown wrote: > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > mount options as "remount" options, so it updates the configuration of the > single super_block on each mount. > Since that was changed, the mount options used for devtmpfs are ignored. > This is a regression which affect systemd - which mounts devtmpfs > with "-o mode=755,size=4m,nr_inodes=1m". > > This patch restores the "remount" effect by calling reconfigure_single() > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > Signed-off-by: NeilBrown <neilb@suse.de> > --- > drivers/base/devtmpfs.c | 7 +++++++ > fs/super.c | 4 ++-- > include/linux/fs_context.h | 2 ++ > 3 files changed, 11 insertions(+), 2 deletions(-) Sorry, I thought Al was going to take this as the regression came from his tree, I should have picked it up earlier. I'll queue it up after 5.17-rc1 is out. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH - resend] devtmpfs regression fix: reconfigure on each mount 2022-01-17 8:03 ` Greg Kroah-Hartman @ 2022-01-17 8:04 ` Greg Kroah-Hartman 0 siblings, 0 replies; 11+ messages in thread From: Greg Kroah-Hartman @ 2022-01-17 8:04 UTC (permalink / raw) To: NeilBrown Cc: Al Viro, Andrew Morton, torvalds, Christian Brauner, Anthony Iliopoulos, David Howells, LKML, linux-fsdevel On Mon, Jan 17, 2022 at 09:03:58AM +0100, Greg Kroah-Hartman wrote: > On Mon, Jan 17, 2022 at 09:07:26AM +1100, NeilBrown wrote: > > > > Prior to Linux v5.4 devtmpfs used mount_single() which treats the given > > mount options as "remount" options, so it updates the configuration of the > > single super_block on each mount. > > Since that was changed, the mount options used for devtmpfs are ignored. > > This is a regression which affect systemd - which mounts devtmpfs > > with "-o mode=755,size=4m,nr_inodes=1m". > > > > This patch restores the "remount" effect by calling reconfigure_single() > > > > Fixes: d401727ea0d7 ("devtmpfs: don't mix {ramfs,shmem}_fill_super() with mount_single()") > > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > drivers/base/devtmpfs.c | 7 +++++++ > > fs/super.c | 4 ++-- > > include/linux/fs_context.h | 2 ++ > > 3 files changed, 11 insertions(+), 2 deletions(-) > > Sorry, I thought Al was going to take this as the regression came from > his tree, I should have picked it up earlier. I'll queue it up after > 5.17-rc1 is out. Ah, nevermind, Linus just took it. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-01-17 22:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-13 1:12 [PATCH - regression] devtmpfs: reconfigure on each mount NeilBrown 2021-12-13 12:59 ` Christian Brauner 2021-12-13 20:46 ` Anthony Iliopoulos 2021-12-14 10:12 ` Christian Brauner 2021-12-14 14:06 ` Anthony Iliopoulos 2021-12-14 14:18 ` Christian Brauner 2022-01-16 22:07 ` [PATCH - resend] devtmpfs regression fix: " NeilBrown 2022-01-17 7:47 ` Linus Torvalds 2022-01-17 22:57 ` NeilBrown 2022-01-17 8:03 ` Greg Kroah-Hartman 2022-01-17 8:04 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.