| Message ID | 20201029003252.2128653-2-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: idmapped mounts | expand |
> index cebaa3e81794..20ee291a7af4 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -463,7 +463,6 @@ static int mnt_make_readonly(struct mount *mnt) > { > int ret = 0; > > - lock_mount_hash(); What about adding a lockdep_assert_lock_held in all the functions that used to take the lock to document the assumptions? > static int __mnt_unmake_readonly(struct mount *mnt) > { > - lock_mount_hash(); > mnt->mnt.mnt_flags &= ~MNT_READONLY; > - unlock_mount_hash(); > return 0; This helper is rather pointless now. > static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags) > { > - lock_mount_hash(); > mnt_flags |= mnt->mnt.mnt_flags & ~MNT_USER_SETTABLE_MASK; > mnt->mnt.mnt_flags = mnt_flags; > touch_mnt_namespace(mnt->mnt_ns); > - unlock_mount_hash(); In linux-next there is an additional notify_mount after the unlock here. Also while you touch this lock_mount_hash/unlock_mount_hash could be moved to namespace.c and maked static now.
On Sun, Nov 01, 2020 at 02:41:08PM +0000, Christoph Hellwig wrote: > > index cebaa3e81794..20ee291a7af4 100644 > > --- a/fs/namespace.c > > +++ b/fs/namespace.c > > @@ -463,7 +463,6 @@ static int mnt_make_readonly(struct mount *mnt) > > { > > int ret = 0; > > > > - lock_mount_hash(); > > What about adding a lockdep_assert_lock_held in all the functions > that used to take the lock to document the assumptions? Good idea and will do. I wanted to do this but then didn't because I haven't seen widespread use of lockdep assert in fs/namespace.c. > > > static int __mnt_unmake_readonly(struct mount *mnt) > > { > > - lock_mount_hash(); > > mnt->mnt.mnt_flags &= ~MNT_READONLY; > > - unlock_mount_hash(); > > return 0; > > This helper is rather pointless now. Ok, will remove. > > > static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags) > > { > > - lock_mount_hash(); > > mnt_flags |= mnt->mnt.mnt_flags & ~MNT_USER_SETTABLE_MASK; > > mnt->mnt.mnt_flags = mnt_flags; > > touch_mnt_namespace(mnt->mnt_ns); > > - unlock_mount_hash(); > > In linux-next there is an additional notify_mount after the unlock here. Thanks! I can try rebasing on -next. > > Also while you touch this lock_mount_hash/unlock_mount_hash could be > moved to namespace.c and maked static now. Ok, will try to do that.
diff --git a/fs/namespace.c b/fs/namespace.c index cebaa3e81794..20ee291a7af4 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -463,7 +463,6 @@ static int mnt_make_readonly(struct mount *mnt) { int ret = 0; - lock_mount_hash(); mnt->mnt.mnt_flags |= MNT_WRITE_HOLD; /* * After storing MNT_WRITE_HOLD, we'll read the counters. This store @@ -497,15 +496,12 @@ static int mnt_make_readonly(struct mount *mnt) */ smp_wmb(); mnt->mnt.mnt_flags &= ~MNT_WRITE_HOLD; - unlock_mount_hash(); return ret; } static int __mnt_unmake_readonly(struct mount *mnt) { - lock_mount_hash(); mnt->mnt.mnt_flags &= ~MNT_READONLY; - unlock_mount_hash(); return 0; } @@ -2517,11 +2513,9 @@ static int change_mount_ro_state(struct mount *mnt, unsigned int mnt_flags) */ static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags) { - lock_mount_hash(); mnt_flags |= mnt->mnt.mnt_flags & ~MNT_USER_SETTABLE_MASK; mnt->mnt.mnt_flags = mnt_flags; touch_mnt_namespace(mnt->mnt_ns); - unlock_mount_hash(); } static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount *mnt) @@ -2567,9 +2561,11 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags) return -EPERM; down_write(&sb->s_umount); + lock_mount_hash(); ret = change_mount_ro_state(mnt, mnt_flags); if (ret == 0) set_mount_attributes(mnt, mnt_flags); + unlock_mount_hash(); up_write(&sb->s_umount); mnt_warn_timestamp_expiry(path, &mnt->mnt); @@ -2610,8 +2606,11 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags, err = -EPERM; if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) { err = reconfigure_super(fc); - if (!err) + if (!err) { + lock_mount_hash(); set_mount_attributes(mnt, mnt_flags); + unlock_mount_hash(); + } } up_write(&sb->s_umount); }
Changing mount options always ends up taking lock_mount_hash() but when MNT_READONLY is requested and neither the mount nor the superblock are not already MNT_READONLY we end up taking the lock, dropping it, and retaking it to change the other mount attributes. Instead of this, acquire the lock once when changing mount properties. This simplifies the locking in these codepath, makes them easier to reason about and avoids having to reacquire the lock right after dropping it. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: David Howells <dhowells@redhat.com> Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/namespace.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)