track number of mnts writing to superblocks

One of the benefits of the r/o bind mount patches is that they make it explicit when a write to a superblock might occur. We currently search sb->s_files when remounting rw->ro to look for writable files. But, that search is not comprehensive, and it is racy. This replaces that search.

The idea is to keep a bit in each mount saying whether the mount has any writers on it. When the bit is set the first time, we also increment a counter in the superblock. That sb counter is the number of mounts with that bit set and thus, potential writers.

The other problem is that, after we make this check for the number of writable mounts, we need to exclude all new writers on those mounts. We do this by requring that the superblock mnt writer count be incremented under a lock_super() and also holding that lock over the remount operation. Effectively, this keeps us from *adding* to the sb's writable mounts during a remount.

The alternative to doing this is to do a much simpler list of mounts for each superblock. I could also code that up to see what it look like. Shouldn't be too bad.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/fs/file_table.c       |   24 -----
 linux-2.6.git-dave/fs/namespace.c        |  134 +++++++++++++++++++++++++------
 linux-2.6.git-dave/fs/super.c            |   61 +++++++++++---
 linux-2.6.git-dave/include/linux/fs.h    |    5 -
 linux-2.6.git-dave/include/linux/mount.h |    3 
 5 files changed, 163 insertions(+), 64 deletions(-)

diff -puN fs/file_table.c~track_sb_mnt_writers fs/file_table.c

fs/file_table.c renamed from fs/file_table.c~track_sb_mnt_writers +0 -5

@@ -374,30 +374,6 @@ void file_kill(struct file *file)
 	}
 }
 
-int fs_may_remount_ro(struct super_block *sb)
-{
-	struct file *file;
-
-	/* Check that no files are currently opened for writing. */

fs/file_table.c renamed from fs/file_table.c~track_sb_mnt_writers +0 -9

-			goto too_bad;
-	}
-	file_list_unlock();
-	return 1; /* Tis' cool bro. */
-too_bad:
-	file_list_unlock();
-	return 0;
-}
-
 void __init files_init(unsigned long mempages)
 { 
 	int n; 

diff -puN fs/file_table.c.orig~track_sb_mnt_writers fs/file_table.c.orig diff -puN fs/namespace.c~track_sb_mnt_writers fs/namespace.c

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +3 -4

@@ -118,7 +118,7 @@ struct mnt_writer {
 	 * If holding multiple instances of this lock, they
 	 * must be ordered by cpu number.
 	 */
-	spinlock_t lock;
+	struct mutex lock;
 	struct lock_class_key lock_class; /* compiles out with !lockdep */
 	unsigned long count;
 	struct vfsmount *mnt;

@@ -130,7 +130,7 @@ static int __init init_mnt_writers(void)
 	int cpu;
 	for_each_possible_cpu(cpu) {
 		struct mnt_writer *writer = &per_cpu(mnt_writers, cpu);
-		spin_lock_init(&writer->lock);
+		mutex_init(&writer->lock);
 		lockdep_set_class(&writer->lock, &writer->lock_class);
 		writer->count = 0;
 	}

@@ -145,11 +145,52 @@ static void mnt_unlock_cpus(void)
 
 	for_each_possible_cpu(cpu) {
 		cpu_writer = &per_cpu(mnt_writers, cpu);
-		spin_unlock(&cpu_writer->lock);
+		mutex_unlock(&cpu_writer->lock);
 	}
 }
 
-static inline void __clear_mnt_count(struct mnt_writer *cpu_writer)

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +5 -0

+	if (cpu_writer == NULL)
+		return 0;
+
+	/*
+	 * Our goal here is to get exclusion from a superblock

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +67 -21

+}
+
+static inline void __move_cpu_mnt_count(struct mnt_writer *cpu_writer)
 {
 	if (!cpu_writer->mnt)
 		return;

@@ -164,7 +205,7 @@ static inline void use_cpu_writer_for_mo
 {
 	if (cpu_writer->mnt == mnt)
 		return;
-	__clear_mnt_count(cpu_writer);
+	__move_cpu_mnt_count(cpu_writer);
 	cpu_writer->mnt = mnt;
 }
 

@@ -188,20 +229,31 @@ static inline void use_cpu_writer_for_mo
  */
 int mnt_want_write(struct vfsmount *mnt)
 {
-	int ret = 0;
+	int ret;
 	struct mnt_writer *cpu_writer;
 
-	cpu_writer = &get_cpu_var(mnt_writers);
-	spin_lock(&cpu_writer->lock);
+	/*
+	 * It is OK to not disable preemption here.  We
+	 * only use the per-cpu vars to reduce cache
+	 * effects, and not for any real exclusion.
+	 * We use the mutexes for that.
+	 */
+	cpu_writer = &__get_cpu_var(mnt_writers);
+	mutex_lock(&cpu_writer->lock);
+again:
+	ret = 0;
 	if (__mnt_is_readonly(mnt)) {
 		ret = -EROFS;
 		goto out;
 	}
+	ret = mark_mnt_has_writer(mnt, cpu_writer);
+	/* did we hit the slow path and re-do the lock? */
+	if (ret == -EAGAIN)
+		goto again;
 	use_cpu_writer_for_mount(cpu_writer, mnt);
 	cpu_writer->count++;
 out:
-	spin_unlock(&cpu_writer->lock);
-	put_cpu_var(mnt_writers);
+	mutex_unlock(&cpu_writer->lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mnt_want_write);

@@ -213,13 +265,37 @@ static void lock_and_coalesce_cpu_mnt_wr
 
 	for_each_possible_cpu(cpu) {
 		cpu_writer = &per_cpu(mnt_writers, cpu);
-		spin_lock(&cpu_writer->lock);
-		__clear_mnt_count(cpu_writer);
+		mutex_lock(&cpu_writer->lock);
+		__move_cpu_mnt_count(cpu_writer);
+		/*
+		 * __mnt_writers may temporarily hit zero
+		 * (and trigger MNT_MAY_HAVE_WRITERS to get
+		 * cleared), but it will get set again if
+		 * and when another mnt_writer[] has an
+		 * entry for that mnt later in this loop.
+		 * Holding the locks keeps anyone from
+		 * seeing this.
+		 */
+		__mark_sb_if_writable(cpu_writer->mnt);
 		cpu_writer->mnt = NULL;
 	}
 }
 
 /*
+ * This is just an external interface.  I want
+ * to use the long names in here, but leave the
+ * simpler names for external users.
+ */
+void lock_mnt_writers()
+{
+	lock_and_coalesce_cpu_mnt_writer_counts();
+}
+void unlock_mnt_writers()
+{
+	mnt_unlock_cpus();
+}
+
+/*
  * These per-cpu write counts are not guaranteed to have
  * matched increments and decrements on any given cpu.
  * A file open()ed for write on one cpu and close()d on

@@ -262,23 +338,40 @@ static void handle_write_count_underflow
  * performing writes to it.  Must be matched with
  * mnt_want_write() call above.
  */
+#define MNT_WRITERS_UNDERFLOW_LIMIT 1024
 void mnt_drop_write(struct vfsmount *mnt)
 {
 	int must_check_underflow = 0;
 	struct mnt_writer *cpu_writer;
 
-	cpu_writer = &get_cpu_var(mnt_writers);
-	spin_lock(&cpu_writer->lock);
+retry:
+	cpu_writer = &__get_cpu_var(mnt_writers);
+	mutex_lock(&cpu_writer->lock);
 
 	use_cpu_writer_for_mount(cpu_writer, mnt);
 	if (cpu_writer->count > 0) {
 		cpu_writer->count--;
 	} else {
-		must_check_underflow = 1;
+		/*
+		 * Without this check, it is theoretically
+		 * possible for large number of processes
+		 * to atomic_dec(__mnt_writers) and cause
+		 * it to underflow.  Because we are under
+		 * the mutex (and there are NR_CPUS
+		 * mutexes), this limits the number of
+		 * concurrent decrements and underflow
+		 * level to NR_CPUS.
+		 */
+		if (atomic_read(&mnt->__mnt_writers) <
+			MNT_WRITER_UNDERFLOW_LIMIT) {
+			mutex_unlock(&cpu_writer->lock);
+			goto retry;
+		}
 		atomic_dec(&mnt->__mnt_writers);
+		must_check_underflow = 1;
 	}
 
-	spin_unlock(&cpu_writer->lock);
+	mutex_unlock(&cpu_writer->lock);
 	/*
 	 * Logically, we could call this each time,
 	 * but the __mnt_writers cacheline tends to

@@ -286,15 +379,6 @@ void mnt_drop_write(struct vfsmount *mnt
 	 */
 	if (must_check_underflow)
 		handle_write_count_underflow(mnt);
-	/*
-	 * This could be done right after the spinlock
-	 * is taken because the spinlock keeps us on
-	 * the cpu, and disables preemption.  However,
-	 * putting it here bounds the amount that
-	 * __mnt_writers can underflow.  Without it,
-	 * we could theoretically wrap __mnt_writers.
-	 */
-	put_cpu_var(mnt_writers);
 }
 EXPORT_SYMBOL_GPL(mnt_drop_write);
 

diff -puN fs/namespace.c.orig~track_sb_mnt_writers fs/namespace.c.orig diff -puN include/linux/fs.h~track_sb_mnt_writers include/linux/fs.h

include/linux/fs.h renamed from include/linux/fs.h~track_sb_mnt_writers +3 -2

@@ -1005,6 +1005,9 @@ struct super_block {
 #ifdef CONFIG_SECURITY
 	void                    *s_security;
 #endif
+	atomic_t		__s_mnt_writers;/* how many mounts of this sb
+						 * might have writers.  Only
+						 * stable with lock_mnt_writers() */
 	struct xattr_handler	**s_xattr;
 
 	struct list_head	s_inodes;	/* all inodes */

@@ -1650,8 +1653,6 @@ extern const struct file_operations read
 extern const struct file_operations write_fifo_fops;
 extern const struct file_operations rdwr_fifo_fops;
 
-extern int fs_may_remount_ro(struct super_block *);
-
 #ifdef CONFIG_BLOCK
 /*
  * return READ, READA, or WRITE

diff -puN include/linux/fs.h.orig~track_sb_mnt_writers include/linux/fs.h.orig diff -puN include/linux/mount.h~track_sb_mnt_writers include/linux/mount.h

include/linux/mount.h renamed from include/linux/mount.h~track_sb_mnt_writers +3 -0

@@ -33,6 +33,7 @@ struct mnt_namespace;
 
 #define MNT_SHRINKABLE	0x100
 #define MNT_IMBALANCED_WRITE_COUNT	0x200 /* just for debugging */
+#define MNT_MAY_HAVE_WRITERS		0x400 /* did this ever have a writer? */
 
 #define MNT_SHARED	0x1000	/* if the vfsmount is a shared mount */
 #define MNT_UNBINDABLE	0x2000	/* if the vfsmount is a unbindable mount */

@@ -80,6 +81,8 @@ static inline struct vfsmount *mntget(st
 
 extern int mnt_want_write(struct vfsmount *mnt);
 extern void mnt_drop_write(struct vfsmount *mnt);
+extern void lock_mnt_writers(void);
+extern void unlock_mnt_writers(void);
 extern void mntput_no_expire(struct vfsmount *mnt);
 extern void mnt_pin(struct vfsmount *mnt);
 extern void mnt_unpin(struct vfsmount *mnt);

diff -puN include/linux/mount.h.orig~track_sb_mnt_writers include/linux/mount.h.orig diff -puN fs/super.c~track_sb_mnt_writers fs/super.c

fs/super.c renamed from fs/super.c~track_sb_mnt_writers +48 -13

@@ -574,6 +574,41 @@ static void mark_files_ro(struct super_b
 	file_list_unlock();
 }
 
+static inline int fs_may_remount_ro(struct super_block *sb)
+{
+	int ret = 1;
+	lock_mnt_writers();
+	if (atomic_read(&sb->__s_mnt_writers))
+		ret = 0;
+	unlock_mnt_writers();
+	return ret;
+}
+
+/*
+ * This unconditionally acquires the sb lock,
+ * even if it returns an error code, and should
+ * not be used generally.
+ *
+ * This must be done to ensure that no new
+ * writers come in on the mounts after the
+ * check is performed.
+ */
+static int __try_to_make_sb_ro(struct super_block *sb, int force)
+{
+	int retval = 0;
+
+	if (force) {
+		mark_files_ro(sb);
+		lock_super(sb);
+	} else {
+		lock_super(sb);
+		/* Make sure there are no mounts that have writers */
+		if (!fs_may_remount_ro(sb))
+			retval = -EBUSY;
+	}
+	return retval;
+}
+
 /**
  *	do_remount_sb - asks filesystem to change mount options.
  *	@sb:	superblock in question

@@ -585,7 +620,7 @@ 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;
 
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))

@@ -596,23 +631,23 @@ int do_remount_sb(struct super_block *sb
 	shrink_dcache_sb(sb);
 	fsync_super(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 (force)
-			mark_files_ro(sb);
-		else if (!fs_may_remount_ro(sb))
-			return -EBUSY;
-	}
-
-	if (sb->s_op->remount_fs) {
+	/* Are we remounting RDONLY when current sb is read/write? */
+	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY))
+		retval = __try_to_make_sb_ro(sb, force);
+	else
 		lock_super(sb);
+
+	if (retval)
+		goto out;
+	if (sb->s_op->remount_fs)
 		retval = sb->s_op->remount_fs(sb, &flags, data);
+out:
+	if (retval) {
 		unlock_super(sb);
-		if (retval)
-			return retval;
+		return retval;
 	}
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
+	unlock_super(sb);
 	return 0;
 }
 

diff -puN block/cfq-iosched.c~track_sb_mnt_writers block/cfq-iosched.c diff -puN fs/namei.c~track_sb_mnt_writers fs/namei.c
diff -puN fs/buffer.c~track_sb_mnt_writers fs/buffer.c
diff -puN kernel/Makefile~track_sb_mnt_writers kernel/Makefile _

Ok I'm blabbing quite a bit here while trying to figure out the patch, and maybe there are some useful hints for where more comments would be useful. But other than the fact that mark_mnt_has_writer() needs to the atomic_inc() even if cpu_writer was passed in as NULL, the patch seems good.

Quoted 26 lines

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/fs/file_table.c       |   24 -----
 linux-2.6.git-dave/fs/namespace.c        |  134 +++++++++++++++++++++++++------
 linux-2.6.git-dave/fs/super.c            |   61 +++++++++++---
 linux-2.6.git-dave/include/linux/fs.h    |    5 -
 linux-2.6.git-dave/include/linux/mount.h |    3 
 5 files changed, 163 insertions(+), 64 deletions(-)

diff -puN fs/file_table.c~track_sb_mnt_writers fs/file_table.c

fs/file_table.c renamed from fs/file_table.c~track_sb_mnt_writers +0 -9

@@ -374,30 +374,6 @@ void file_kill(struct file *file)
 	}
 }
 
-int fs_may_remount_ro(struct super_block *sb)
-{
-	struct file *file;
-
-	/* Check that no files are currently opened for writing. */
-	file_list_lock();
-	list_for_each_entry(file, &sb->s_files, f_u.fu_list) {
-		struct inode *inode = file->f_path.dentry->d_inode;
-

fs/file_table.c void file_kill(struct file *file) · line 391

Quoted 6 lines

fs/file_table.c renamed from fs/file_table.c~track_sb_mnt_writers +0 -6

-		/* File with pending delete? */
-		if (inode->i_nlink == 0)
-			goto too_bad;
-
-		/* Writeable file? */
-		if (S_ISREG(inode->i_mode) && (file->f_mode & FMODE_WRITE))

(why did this originally skip directories?)

Quoted 31 lines

diff -puN fs/file_table.c.orig~track_sb_mnt_writers fs/file_table.c.orig diff -puN fs/namespace.c~track_sb_mnt_writers fs/namespace.c

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +3 -3

@@ -118,7 +118,7 @@ struct mnt_writer {
 	 * If holding multiple instances of this lock, they
 	 * must be ordered by cpu number.
 	 */
-	spinlock_t lock;
+	struct mutex lock;
 	struct lock_class_key lock_class; /* compiles out with !lockdep */
 	unsigned long count;
 	struct vfsmount *mnt;

@@ -130,7 +130,7 @@ static int __init init_mnt_writers(void)
 	int cpu;
 	for_each_possible_cpu(cpu) {
 		struct mnt_writer *writer = &per_cpu(mnt_writers, cpu);
-		spin_lock_init(&writer->lock);
+		mutex_init(&writer->lock);
 		lockdep_set_class(&writer->lock, &writer->lock_class);
 		writer->count = 0;
 	}

@@ -145,11 +145,52 @@ static void mnt_unlock_cpus(void)
 
 	for_each_possible_cpu(cpu) {
 		cpu_writer = &per_cpu(mnt_writers, cpu);
-		spin_unlock(&cpu_writer->lock);
+		mutex_unlock(&cpu_writer->lock);
 	}
 }
 

fs/namespace.c static void mnt_unlock_cpus(void) · line 161

Quoted 11 lines

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +10 -1

-static inline void __clear_mnt_count(struct mnt_writer *cpu_writer)
+static int mark_mnt_has_writer(struct vfsmount *mnt,
+			       struct mnt_writer *cpu_writer)
+{
+	/*
+	 * Ensure that if there are people racing to set
+	 * the bit that only one of them succeeds and can
+	 * increment the sb count.
+	 */
+	if (test_and_set_bit(ilog2(MNT_MAY_HAVE_WRITERS), &mnt->mnt_flags))
+		return 0;

Comment isn't entirely clear, but you're returning 0 here because someone else has already set the flag and incremented sb->__s_mnt_writers so you don't have to and you're all set to go on with writing?

Quoted 11 lines

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +11 -0

+	if (cpu_writer == NULL)
+		return 0;
+
+	/*
+	 * Our goal here is to get exclusion from a superblock
+	 * remount operation (fs_may_remount_ro()).  This is
+	 * effectively a slow path that we must go through the
+	 * first time we set the bit on each mount, but never
+	 * again unless the writer counts get coalesced.
+	 */
+

fs/namespace.c static void mnt_unlock_cpus(void) · line 176

Quoted 4 lines

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +4 -0

+	mutex_unlock(&cpu_writer->lock);
+	lock_super(mnt->mnt_sb);
+
+	atomic_inc(&mnt->mnt_sb->__s_mnt_writers);

The atomic_inc of course should be done even if cpu_writer was passed in as NULL, you just don't need to do the locking then, and can return 0 in that case?

fs/namespace.c static void mnt_unlock_cpus(void) · line 183

Quoted 7 lines

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +7 -0

+
+	unlock_super(mnt->mnt_sb);
+	mutex_lock(&cpu_writer->lock);
+	return -EAGAIN;
+}
+
+static void __mark_sb_if_writable(struct vfsmount *mnt)

This function is taking the writable state from a mnt and marking it in the sb. So the name should be a shorter verison of something like "commit_mnt_writable_state_to_sb"

fs/namespace.c static void mnt_unlock_cpus(void) · line 190

Quoted 7 lines

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +7 -0

+{
+	int bitnr = ilog2(MNT_MAY_HAVE_WRITERS);
+
+	if (atomic_read(&mnt->__mnt_writers))
+		mark_mnt_has_writer(mnt, NULL);
+	else if (test_and_clear_bit(bitnr, &mnt->mnt_flags))
+		atomic_dec(&mnt->mnt_sb->__s_mnt_writers);

And after staring at this code for awhile it did make sense, but a comment above it would help saying that

	/* If mnt has writers, mark_mnt_has_writer() will make
	   sure that is marked in the sb.  If mnt has no writers,
	   then if mnt->mnt_flags was previously set, that means
	   that mnt->mnt_sb->__s_mnt_writers reflects our having
	   writers, so we decrement it
	 */

Ok, maybe it's clearer to other people and the comment isn't needed...

One of the benefits of the r/o bind mount patches is that they make it explicit when a write to a superblock might occur. We currently search sb->s_files when remounting rw->ro to look for writable files. But, that search is not comprehensive, and it is racy. This replaces that search.

The idea is to keep a bit in each mount saying whether the mount has any writers on it. When the bit is set the first time, we also increment a counter in the superblock. That sb counter is the number of mounts with that bit set and thus, potential writers.

The other problem is that, after we make this check for the number of writable mounts, we need to exclude all new writers on those mounts. We do this by requring that the superblock mnt writer count be incremented under a lock_super() and also holding that lock over the remount operation. Effectively, this keeps us from *adding* to the sb's writable mounts during a remount.

The alternative to doing this is to do a much simpler list of mounts for each superblock. I could also code that up to see what it look like. Shouldn't be too bad.

...

fs/namespace.c renamed from fs/namespace.c~track_sb_mnt_writers +0 -1

-static inline void __clear_mnt_count(struct mnt_writer *cpu_writer)