Skip to contentSkip to conversation

r/o bind mounts: add vfsmount and superblock writer counts

PATCH In review Dave Hansen 2 participants 3 messages Jan 8, 2007 – Jan 10, 2007 VFS

lore
Share & tools
lore

Dave Hansen

author
#0 lore raw

PATCH 1/1 r/o bind mounts: add vfsmount and superblock writer counts

Hi Al,

I was having Christoph review some of the r/o bind mount patches, and he wasn't a very big fan of the list of mounts hanging off of the superblock.

Removing it had a few side-effects. First of all, it made me move all of the operations on the counts of writers to underneath the spinlock that was already there. I guess this could be a cause for potential contention because there _are_ locks in the common code paths now. But, I do agree with Christoph that it would be awfully hard to get it contended.

The other side-effect is that we can't have the bit in mnt_flags to be a shortcut to the superblock's writeable state since we don't have a way to go find the mounts and that bit when a fs changes writeable state. This causes a potential cache miss when we have to check the superblock directly during the relatively common __mnt_is_readonly() function.

It does simplify the code quite a bit removing the logic which avoids looking at the superblock. Anyway, I thought I'd have you take a look over this to see if you prefer it over the previous approach. Here's a copy of the old approach, just in case you want to take a look:

http://sr71.net/~dave/robind/03-24-add-vfsmount-writer-count.patch

---

 lxc-dave/fs/inode.c            |    5 ++++
 lxc-dave/fs/namespace.c        |   47 ++++++++++++++++++++++++++++++++++++++---
 lxc-dave/fs/super.c            |   31 ++++++++++++++++++++++-----
 lxc-dave/include/linux/fs.h    |    5 ++++
 lxc-dave/include/linux/mount.h |   21 +++++++++++-------
 5 files changed, 93 insertions(+), 16 deletions(-)
fs/namespace.c renamed from fs/namespace.c~03-24-add-vfsmount-writer-count +44 -3
diff -puN fs/namespace.c~03-24-add-vfsmount-writer-count fs/namespace.c
@@ -56,6 +56,7 @@ struct vfsmount *alloc_vfsmnt(const char
 	struct vfsmount *mnt = kmem_cache_zalloc(mnt_cache, GFP_KERNEL);
 	if (mnt) {
 		atomic_set(&mnt->mnt_count, 1);
+		mnt->mnt_writers = 0;
 		INIT_LIST_HEAD(&mnt->mnt_hash);
 		INIT_LIST_HEAD(&mnt->mnt_child);
 		INIT_LIST_HEAD(&mnt->mnt_mounts);

@@ -877,15 +878,52 @@ out_unlock:
 	return err;
 }
 
+int mnt_make_readonly(struct vfsmount *mnt)
+{
+	int ret = 0;
+
+	WARN_ON(__mnt_is_readonly(mnt));
+
+	/*
+	 * This flag set is actually redundant with what
+	 * happens in do_remount(), but since we do this
+	 * under the lock, anyone attempting to get a write
+	 * on it after this will fail.
+	 */
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	if (!mnt->mnt_writers)
+		mnt->mnt_flags |= MNT_READONLY;
+	else
+		ret = -EBUSY;
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
+	return ret;
+}
+
 int mnt_want_write(struct vfsmount *mnt)
 {
-	if (__mnt_is_readonly(mnt))
-		return -EROFS;
-	return 0;
+	int ret = 0;
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	if (mnt->mnt_writers)
+		goto out;
+
+	if (__mnt_is_readonly(mnt)) {
+		ret = -EROFS;
+		goto out;
+	}
+	mnt->mnt_sb->s_writers++;
+	mnt->mnt_writers++;
+out:
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(mnt_want_write);
 
 void mnt_drop_write(struct vfsmount *mnt)
 {
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	mnt->mnt_sb->s_writers--;
+	mnt->mnt_writers--;
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
 }
 
 /*

@@ -986,6 +1024,7 @@ static int do_remount(struct nameidata *
 		security_sb_post_remount(nd->mnt, flags, data);
 	return err;
 }
+EXPORT_SYMBOL_GPL(mnt_drop_write);
 
 static inline int tree_contains_unbindable(struct vfsmount *mnt)
 {

@@ -1410,6 +1449,8 @@ long do_mount(char *dev_name, char *dir_
 		((char *)data_page)[PAGE_SIZE - 1] = 0;
 
 	/* Separate the per-mountpoint flags */
+	if (flags & MS_RDONLY)
+		mnt_flags |= MNT_READONLY;
 	if (flags & MS_NOSUID)
 		mnt_flags |= MNT_NOSUID;
 	if (flags & MS_NODEV)
fs/super.c renamed from fs/super.c~03-24-add-vfsmount-writer-count +26 -5
diff -puN fs/super.c~03-24-add-vfsmount-writer-count fs/super.c
@@ -93,6 +93,8 @@ static struct super_block *alloc_super(s
 		s->s_qcop = sb_quotactl_ops;
 		s->s_op = &default_op;
 		s->s_time_gran = 1000000000;
+		s->s_writers = 0;
+		spin_lock_init(&s->s_mnt_writers_lock);
 	}
 out:
 	return s;

@@ -576,6 +578,20 @@ static void mark_files_ro(struct super_b
 	file_list_unlock();
 }
 
+static int sb_remount_ro(struct super_block *sb)
+{
+	return fs_may_remount_ro(sb);
+}
+
+static void sb_remount_rw(struct super_block *sb)
+{
+	/*
+	 * No action, yet.  This will soon
+	 * notify the mounts that the sb
+	 * has become writeable.
+	 */
+}
+
 /**
  *	do_remount_sb - asks filesystem to change mount options.
  *	@sb:	superblock in question

@@ -587,7 +603,8 @@ static void mark_files_ro(struct super_b
  */
 int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
 {
-	int retval;
+	int retval = 0;
+	int sb_started_ro = (sb->s_flags & MS_RDONLY);
 	
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))

@@ -600,13 +617,14 @@ int do_remount_sb(struct super_block *sb
 
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
-	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
+	if ((flags & MS_RDONLY) && !sb_started_ro) {
 		if (force)
 			mark_files_ro(sb);
-		else if (!fs_may_remount_ro(sb))
-			return -EBUSY;
+		else
+			retval = sb_remount_ro(sb);
+		if (retval)
+			return retval;
 	}
-
 	if (sb->s_op->remount_fs) {
 		lock_super(sb);
 		retval = sb->s_op->remount_fs(sb, &flags, data);

@@ -614,6 +632,9 @@ int do_remount_sb(struct super_block *sb
 		if (retval)
 			return retval;
 	}
+	if (!(flags & MS_RDONLY) && sb_started_ro)
+		sb_remount_rw(sb);
+
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	return 0;
 }
include/linux/fs.h renamed from include/linux/fs.h~03-24-add-vfsmount-writer-count +5 -0
diff -puN include/linux/fs.h~03-24-add-vfsmount-writer-count include/linux/fs.h
@@ -969,6 +969,8 @@ struct super_block {
 	struct list_head	s_io;		/* parked for writeback */
 	struct hlist_head	s_anon;		/* anonymous dentries for (nfs) exporting */
 	struct list_head	s_files;
+	int			s_writers;	/* number of files open for write */
+	spinlock_t		s_mnt_writers_lock; /* taken when mounts change rw state */
 
 	struct block_device	*s_bdev;
 	struct list_head	s_instances;

@@ -1277,6 +1279,9 @@ static inline void check_nlink(struct in
 		return;
 
 	inode->i_state |= I_AWAITING_FINAL_IPUT;
+	spin_lock(&inode->i_sb->s_mnt_writers_lock);
+	inode->i_sb->s_writers--;
+	spin_unlock(&inode->i_sb->s_mnt_writers_lock);
 }
 
 /**
include/linux/mount.h renamed from include/linux/mount.h~03-24-add-vfsmount-writer-count +13 -8
diff -puN include/linux/mount.h~03-24-add-vfsmount-writer-count include/linux/mount.h
@@ -28,6 +28,7 @@ struct mnt_namespace;
 #define MNT_NOATIME	0x08
 #define MNT_NODIRATIME	0x10
 #define MNT_RELATIME	0x20
+#define MNT_READONLY	0x40 /* does the user want this to be r/o? */
 
 #define MNT_SHRINKABLE	0x100
 

@@ -53,7 +54,7 @@ struct vfsmount {
 	struct list_head mnt_slave;	/* slave list entry */
 	struct vfsmount *mnt_master;	/* slave is on master->mnt_slave_list */
 	struct mnt_namespace *mnt_ns;	/* containing namespace */
-	atomic_t mnt_writers;		/* nr files open for write */
+	int mnt_writers;		/* nr files open for write */
 	/*
 	 * We put mnt_count & mnt_expiry_mark at the end of struct vfsmount
 	 * to let these frequently modified fields in a separate cache line

@@ -71,17 +72,21 @@ static inline struct vfsmount *mntget(st
 	return mnt;
 }
 
-/*
- * This is temporary for now.  We also don't want to check
- * the SB in because it is already checked in other
- * code paths.  We'll have a better way to do this in
- * the end of this series
- */
 static inline int __mnt_is_readonly(struct vfsmount *mnt)
 {
-	return 0;
+	return (mnt->mnt_flags & MNT_READONLY) ||
+	       (mnt->mnt_sb->s_flags & MS_RDONLY);
 }
 
+static inline void __mnt_unmake_readonly(struct vfsmount *mnt)
+{
+	WARN_ON(!__mnt_is_readonly(mnt));
+	mnt->mnt_flags &= ~MNT_READONLY;
+}
+
+extern int mnt_make_readonly(struct vfsmount *mnt);
+extern int mnt_want_write(struct vfsmount *mnt);
+extern void mnt_drop_write(struct vfsmount *mnt);
 extern void mntput_no_expire(struct vfsmount *mnt);
 extern void mnt_pin(struct vfsmount *mnt);
 extern void mnt_unpin(struct vfsmount *mnt);

diff -L lxc-dave/include/linux/fs.h -puN /dev/null /dev/null

fs/inode.c renamed from fs/inode.c~03-24-add-vfsmount-writer-count +5 -0
diff -puN fs/inode.c~03-24-add-vfsmount-writer-count fs/inode.c
@@ -1101,6 +1101,11 @@ static inline void iput_final(struct ino
 	if (op && op->drop_inode)
 		drop = op->drop_inode;
 	drop(inode);
+	if (must_drop_sb_write) {
+		spin_lock(&sb->s_mnt_writers_lock);
+		sb->s_writers--;
+		spin_unlock(&sb->s_mnt_writers_lock);
+	}
 }
 
 /**

_

#1 lore raw
Quoted 7 lines · Dave Hansen

Removing it had a few side-effects. First of all, it made me move all of the operations on the counts of writers to underneath the spinlock that was already there. I guess this could be a cause for potential contention because there _are_ locks in the common code paths now. But, I do agree with Christoph that it would be awfully hard to get it contended.

Quoted 6 lines

The other side-effect is that we can't have the bit in mnt_flags to be a shortcut to the superblock's writeable state since we don't have a way to go find the mounts and that bit when a fs changes writeable state. This causes a potential cache miss when we have to check the superblock directly during the relatively common __mnt_is_readonly() function.

Why? We _only_ need to check the vfsmount flag. vfsmount can become r/w if the superblock is marked r/o which means the underlying (block/network/etc) device is fundamentally not writeable.

author
#2 lore raw
Quoted 11 lines · Christoph Hellwig
Quoted 6 lines · Dave Hansen

The other side-effect is that we can't have the bit in mnt_flags to be a shortcut to the superblock's writeable state since we don't have a way to go find the mounts and that bit when a fs changes writeable state. This causes a potential cache miss when we have to check the superblock directly during the relatively common __mnt_is_readonly() function.

Why? We _only_ need to check the vfsmount flag. vfsmount can become r/w if the superblock is marked r/o which means the underlying (block/network/etc) device is fundamentally not writeable.

It does seem a bit silly to go all the way down to the permissions checks to finally find out that the sb is r/o when it is pretty easy to catch here. But, I guess it is a common-case hit for a relatively uncommon occurrence.

Here is an updated patch?

After this, do you think it is ready for -mm?

---

 lxc-dave/fs/inode.c            |    5 ++++
 lxc-dave/fs/namespace.c        |   47 ++++++++++++++++++++++++++++++++++++++---
 lxc-dave/fs/super.c            |   31 ++++++++++++++++++++++-----
 lxc-dave/include/linux/fs.h    |    5 ++++
 lxc-dave/include/linux/mount.h |   20 ++++++++++-------
 5 files changed, 92 insertions(+), 16 deletions(-)
fs/namespace.c renamed from fs/namespace.c~03-24-add-vfsmount-writer-count +44 -3
diff -puN fs/namespace.c~03-24-add-vfsmount-writer-count fs/namespace.c
@@ -56,6 +56,7 @@ struct vfsmount *alloc_vfsmnt(const char
 	struct vfsmount *mnt = kmem_cache_zalloc(mnt_cache, GFP_KERNEL);
 	if (mnt) {
 		atomic_set(&mnt->mnt_count, 1);
+		mnt->mnt_writers = 0;
 		INIT_LIST_HEAD(&mnt->mnt_hash);
 		INIT_LIST_HEAD(&mnt->mnt_child);
 		INIT_LIST_HEAD(&mnt->mnt_mounts);

@@ -877,15 +878,52 @@ out_unlock:
 	return err;
 }
 
+int mnt_make_readonly(struct vfsmount *mnt)
+{
+	int ret = 0;
+
+	WARN_ON(__mnt_is_readonly(mnt));
+
+	/*
+	 * This flag set is actually redundant with what
+	 * happens in do_remount(), but since we do this
+	 * under the lock, anyone attempting to get a write
+	 * on it after this will fail.
+	 */
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	if (!mnt->mnt_writers)
+		mnt->mnt_flags |= MNT_READONLY;
+	else
+		ret = -EBUSY;
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
+	return ret;
+}
+
 int mnt_want_write(struct vfsmount *mnt)
 {
-	if (__mnt_is_readonly(mnt))
-		return -EROFS;
-	return 0;
+	int ret = 0;
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	if (mnt->mnt_writers)
+		goto out;
+
+	if (__mnt_is_readonly(mnt)) {
+		ret = -EROFS;
+		goto out;
+	}
+	mnt->mnt_sb->s_writers++;
+	mnt->mnt_writers++;
+out:
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(mnt_want_write);
 
 void mnt_drop_write(struct vfsmount *mnt)
 {
+	spin_lock(&mnt->mnt_sb->s_mnt_writers_lock);
+	mnt->mnt_sb->s_writers--;
+	mnt->mnt_writers--;
+	spin_unlock(&mnt->mnt_sb->s_mnt_writers_lock);
 }
 
 /*

@@ -986,6 +1024,7 @@ static int do_remount(struct nameidata *
 		security_sb_post_remount(nd->mnt, flags, data);
 	return err;
 }
+EXPORT_SYMBOL_GPL(mnt_drop_write);
 
 static inline int tree_contains_unbindable(struct vfsmount *mnt)
 {

@@ -1410,6 +1449,8 @@ long do_mount(char *dev_name, char *dir_
 		((char *)data_page)[PAGE_SIZE - 1] = 0;
 
 	/* Separate the per-mountpoint flags */
+	if (flags & MS_RDONLY)
+		mnt_flags |= MNT_READONLY;
 	if (flags & MS_NOSUID)
 		mnt_flags |= MNT_NOSUID;
 	if (flags & MS_NODEV)
fs/super.c renamed from fs/super.c~03-24-add-vfsmount-writer-count +26 -5
diff -puN fs/super.c~03-24-add-vfsmount-writer-count fs/super.c
@@ -93,6 +93,8 @@ static struct super_block *alloc_super(s
 		s->s_qcop = sb_quotactl_ops;
 		s->s_op = &default_op;
 		s->s_time_gran = 1000000000;
+		s->s_writers = 0;
+		spin_lock_init(&s->s_mnt_writers_lock);
 	}
 out:
 	return s;

@@ -576,6 +578,20 @@ static void mark_files_ro(struct super_b
 	file_list_unlock();
 }
 
+static int sb_remount_ro(struct super_block *sb)
+{
+	return fs_may_remount_ro(sb);
+}
+
+static void sb_remount_rw(struct super_block *sb)
+{
+	/*
+	 * No action, yet.  This will soon
+	 * notify the mounts that the sb
+	 * has become writeable.
+	 */
+}
+
 /**
  *	do_remount_sb - asks filesystem to change mount options.
  *	@sb:	superblock in question

@@ -587,7 +603,8 @@ static void mark_files_ro(struct super_b
  */
 int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
 {
-	int retval;
+	int retval = 0;
+	int sb_started_ro = (sb->s_flags & MS_RDONLY);
 	
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))

@@ -600,13 +617,14 @@ int do_remount_sb(struct super_block *sb
 
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
-	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
+	if ((flags & MS_RDONLY) && !sb_started_ro) {
 		if (force)
 			mark_files_ro(sb);
-		else if (!fs_may_remount_ro(sb))
-			return -EBUSY;
+		else
+			retval = sb_remount_ro(sb);
+		if (retval)
+			return retval;
 	}
-
 	if (sb->s_op->remount_fs) {
 		lock_super(sb);
 		retval = sb->s_op->remount_fs(sb, &flags, data);

@@ -614,6 +632,9 @@ int do_remount_sb(struct super_block *sb
 		if (retval)
 			return retval;
 	}
+	if (!(flags & MS_RDONLY) && sb_started_ro)
+		sb_remount_rw(sb);
+
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	return 0;
 }
include/linux/fs.h renamed from include/linux/fs.h~03-24-add-vfsmount-writer-count +5 -0
diff -puN include/linux/fs.h~03-24-add-vfsmount-writer-count include/linux/fs.h
@@ -969,6 +969,8 @@ struct super_block {
 	struct list_head	s_io;		/* parked for writeback */
 	struct hlist_head	s_anon;		/* anonymous dentries for (nfs) exporting */
 	struct list_head	s_files;
+	int			s_writers;	/* number of files open for write */
+	spinlock_t		s_mnt_writers_lock; /* taken when mounts change rw state */
 
 	struct block_device	*s_bdev;
 	struct list_head	s_instances;

@@ -1277,6 +1279,9 @@ static inline void check_nlink(struct in
 		return;
 
 	inode->i_state |= I_AWAITING_FINAL_IPUT;
+	spin_lock(&inode->i_sb->s_mnt_writers_lock);
+	inode->i_sb->s_writers--;
+	spin_unlock(&inode->i_sb->s_mnt_writers_lock);
 }
 
 /**
include/linux/mount.h renamed from include/linux/mount.h~03-24-add-vfsmount-writer-count +12 -8
diff -puN include/linux/mount.h~03-24-add-vfsmount-writer-count include/linux/mount.h
@@ -28,6 +28,7 @@ struct mnt_namespace;
 #define MNT_NOATIME	0x08
 #define MNT_NODIRATIME	0x10
 #define MNT_RELATIME	0x20
+#define MNT_READONLY	0x40 /* does the user want this to be r/o? */
 
 #define MNT_SHRINKABLE	0x100
 

@@ -53,7 +54,7 @@ struct vfsmount {
 	struct list_head mnt_slave;	/* slave list entry */
 	struct vfsmount *mnt_master;	/* slave is on master->mnt_slave_list */
 	struct mnt_namespace *mnt_ns;	/* containing namespace */
-	atomic_t mnt_writers;		/* nr files open for write */
+	int mnt_writers;		/* nr files open for write */
 	/*
 	 * We put mnt_count & mnt_expiry_mark at the end of struct vfsmount
 	 * to let these frequently modified fields in a separate cache line

@@ -71,17 +72,20 @@ static inline struct vfsmount *mntget(st
 	return mnt;
 }
 
-/*
- * This is temporary for now.  We also don't want to check
- * the SB in because it is already checked in other
- * code paths.  We'll have a better way to do this in
- * the end of this series
- */
 static inline int __mnt_is_readonly(struct vfsmount *mnt)
 {
-	return 0;
+	return mnt->mnt_flags & MNT_READONLY;
 }
 
+static inline void __mnt_unmake_readonly(struct vfsmount *mnt)
+{
+	WARN_ON(!__mnt_is_readonly(mnt));
+	mnt->mnt_flags &= ~MNT_READONLY;
+}
+
+extern int mnt_make_readonly(struct vfsmount *mnt);
+extern int mnt_want_write(struct vfsmount *mnt);
+extern void mnt_drop_write(struct vfsmount *mnt);
 extern void mntput_no_expire(struct vfsmount *mnt);
 extern void mnt_pin(struct vfsmount *mnt);
 extern void mnt_unpin(struct vfsmount *mnt);

diff -L lxc-dave/include/linux/fs.h -puN /dev/null /dev/null

fs/inode.c renamed from fs/inode.c~03-24-add-vfsmount-writer-count +5 -0
diff -puN fs/inode.c~03-24-add-vfsmount-writer-count fs/inode.c
@@ -1101,6 +1101,11 @@ static inline void iput_final(struct ino
 	if (op && op->drop_inode)
 		drop = op->drop_inode;
 	drop(inode);
+	if (must_drop_sb_write) {
+		spin_lock(&sb->s_mnt_writers_lock);
+		sb->s_writers--;
+		spin_unlock(&sb->s_mnt_writers_lock);
+	}
 }
 
 /**

_

-- Dave