Re: getting rid of filp search in fs_may_remount_ro()

[Dave Hansen <haveblue@us.ibm.com>]

On Mon, 2007-12-31 at 11:54 -0800, Dave Hansen wrote:
> On Wed, 2007-12-26 at 15:12 +0100, Christoph Hellwig wrote:
> > Btw, I just noticed in current -mm fs_may_remount_ro() is still around
> > and not replaced by ther per-sb writers count.  That surely sounds like
> > some kind of mismerge..
> 
> I was actually leaving that for later.  Getting rid of the filp search
> is a great benefit of the r/o bind patches, but it isn't strictly
> necessary and it doesn't really hurt anything to keep it.
> 
> The reason that it was contentious was that we need some way to be able
> to do an sb-to-mount mapping.  When remounting the sb, we need to
> determine whether *any* of the mounts of that sb have any writers.
> 
> We don't currently have any mechanisms to do direct lookups from sb to
> mount.  The only alternative I can see right now is to walk over all
> tasks, then walk over all vfs namespaces, and walk each mount tree to
> see if any mounts are of the sb we're looking for.  This needs to be
> done while already holding the mnt_writers[] locks so that no new mnt
> writers can come in.
> 
> *THAT* is going to be a heavyweight operation.  I need to go look in
> detail at how the mount trees are kept, and we'll need some kind of
> mechanism to keep track of which vfs namespaces we've looked at during
> the search so we don't search them twice. 
> 
> Can you think of a simpler way to do it?

Here's one blatantly untested idea I have.  The idea is to keep track if
anyone might be writing to a mnt.  We keep track on a flag in the mnt.
When we set the flag, we increment a counter in the sb and decrement
when the flag is cleared.

We can't simply look at mnt->__mnt_writers because there might be
"checked-out" writers in the mnt_writers[] array.  We also have to keep
new writers from coming in while we do this, so we use the spinlocks in
the mnt_writers[] array for exclusion.  This is a pretty heavyweight
lock, but it only gets used at rw->ro transitions.

-- Dave

--- ./fs/file_table.c.orig	2007-12-31 11:14:59.000000000 -0800
+++ ./fs/file_table.c	2007-12-31 14:38:19.000000000 -0800
@@ -376,26 +376,12 @@
 
 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;
-
-		/* 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))
-			goto too_bad;
-	}
-	file_list_unlock();
-	return 1; /* Tis' cool bro. */
-too_bad:
-	file_list_unlock();
-	return 0;
+	int ret = 1;
+	lock_mnt_writers();
+	if (atomic_read(&sb->__s_mnt_writers))
+		ret = 0;
+	unlock_mnt_writers();
+	return ret;
 }
 
 void __init files_init(unsigned long mempages)
--- ./fs/namespace.c.orig	2007-12-31 13:52:36.000000000 -0800
+++ ./fs/namespace.c	2007-12-31 14:53:56.000000000 -0800
@@ -138,7 +138,7 @@
 }
 fs_initcall(init_mnt_writers);
 
-static void mnt_unlock_cpus(void)
+void mnt_unlock_cpus(void)
 {
 	int cpu;
 	struct mnt_writer *cpu_writer;
@@ -149,6 +149,22 @@
 	}
 }
 
+static void mark_mnt_has_writer(struct vfsmount *mnt)
+{
+	if (!test_and_set_bit(ilog2(MNT_MAY_HAVE_WRITERS), &mnt->mnt_flags))
+		atomic_inc(&mnt->mnt_sb->s_possible_mnt_writers);
+}
+
+static void check_mnt_for_writers(struct vfsmount *mnt)
+{
+	int bitnr = ilog2(MNT_MAY_HAVE_WRITERS);
+
+	if (atomic_read(&mnt->__mnt_writers))
+		mark_mnt_has_writer(mnt);
+	else if (test_and_clear_bit(bitnr, &mnt->mnt_flags))
+		atomic_dec(&mnt->mnt_sb->s_possible_mnt_writers);
+}
+
 static inline void __clear_mnt_count(struct mnt_writer *cpu_writer)
 {
 	if (!cpu_writer->mnt)
@@ -199,6 +215,7 @@
 	}
 	use_cpu_writer_for_mount(cpu_writer, mnt);
 	cpu_writer->count++;
+	mark_mnt_has_writer(mnt);
 out:
 	spin_unlock(&cpu_writer->lock);
 	put_cpu_var(mnt_writers);
@@ -215,11 +232,33 @@
 		cpu_writer = &per_cpu(mnt_writers, cpu);
 		spin_lock(&cpu_writer->lock);
 		__clear_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.
+		 */
+		check_mnt_has_writers(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
--- ./include/linux/fs.h.orig	2007-12-31 14:08:20.000000000 -0800
+++ ./include/linux/fs.h	2007-12-31 14:31:12.000000000 -0800
@@ -1005,6 +1005,9 @@
 #ifdef CONFIG_SECURITY
 	void                    *s_security;
 #endif
+	atomic_t		__s_mnt_writers;/* how many mounts of this sb might
+						 * have writers.  Only stable with
+						 * all mnt_writers[] locks held */
 	struct xattr_handler	**s_xattr;
 
 	struct list_head	s_inodes;	/* all inodes */
--- ./include/linux/mount.h.orig	2007-12-31 14:00:13.000000000 -0800
+++ ./include/linux/mount.h	2007-12-31 14:33:44.000000000 -0800
@@ -33,6 +33,7 @@
 
 #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 @@
 
 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);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/