On 2012年09月25日 18:11, Jan Kara wrote:
On Fri 14-09-12 15:45:04, Fernando Luis Vázquez Cao wrote:
iterate_supers() calls a function provided by the caller with the s_umount
semaphore taken in read mode. However, there may be cases where write mode
is preferable, so we add __iterate_supers(), which lets one
specify the mode of the lock, and replace iterate_supers with two helpers
around __iterate_supers(), iterate_supers_read() and iterate_supers_write().
This will be used to fix the emergency thaw (filesystem unfreeze) code, which
iterates over the list of superblocks but needs to hold the s_umount semaphore
in _write_ mode bebore carrying out the actual thaw operation.
This patch introduces no semantic changes since iterate_supers() users become
iterate_supers_read() which is equivalent.
Cc: Josef Bacik <jbacik@xxxxxxxxxxxx>
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Cc: Jan Kara <jack@xxxxxxx>
Cc: Dave Chinner <dchinner@xxxxxxxxxx>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
---
...
diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c
--- linux-3.6-rc5-orig/fs/super.c 2012-09-14 11:53:43.416703312 +0900
+++ linux-3.6-rc5/fs/super.c 2012-09-14 12:30:52.188833193 +0900
@@ -537,14 +537,22 @@ void drop_super(struct super_block *sb)
EXPORT_SYMBOL(drop_super);
/**
- * iterate_supers - call function for all active superblocks
+ * __iterate_supers - call function for all active superblocks
* @f: function to call
* @arg: argument to pass to it
+ * @wlock: mode of superblock lock (false->read lock, true->write lock)
*
* Scans the superblock list and calls given function, passing it
* locked superblock and given argument.
+ *
+ * When the caller asks for the superblock lock (s_umount semaphore) to be
+ * taken in write mode, the lock is taken but not released because the
+ * function provided by the caller may deactivate the superblock itself.
+ * It is that function's job to unlock the superblock as needed in such a
+ * case.
*/
-void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
+static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
+ bool wlock)
{
struct super_block *sb, *p = NULL;
@@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup
sb->s_count++;
spin_unlock(&sb_lock);
- down_read(&sb->s_umount);
+ if (wlock)
+ down_write(&sb->s_umount);
+ else
+ down_read(&sb->s_umount);
+
if (sb->s_root && (sb->s_flags & MS_BORN))
f(sb, arg);
- up_read(&sb->s_umount);
+
+ /* When the semaphore was taken in write mode the function
+ * provided by the caller takes care of unlocking it as
+ * needed. See explanation above for details. */
+ if (!wlock)
+ up_read(&sb->s_umount);
spin_lock(&sb_lock);
if (p)
These locking rules are ugly and counterintuitive. People will easily
get them wrong and create bugs. I'd rather see emergency thaw retake the
s_umount semaphore so that iterate_supers() can drop it...
I guess you are referring to treating the write lock differently
and not dropping the lock inside __iterate_supers(). The
problem is that f() may release the last reference to the
superblock which in turn will go away, so letting
__iterate_supers() drop the lock is not safe (I added a
comment about this issue in the function itself).
Regarding the ugliness, please notice that __iterate_supers
is static and is not supposed to be used directly; I added two
wrappers around it (a read variant that is semantically identical
to what we have now and a write variant) and documented them
as thoroughly as I could.
Thanks,
Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
