Re: [PATCH 1/9] vfs: add __iterate_supers() and helpers around it

[Jan Kara <jack@suse.cz>]

On Tue 25-09-12 18:42:16, Fernando Luis Vazquez Cao wrote:
> 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@fusionio.com>
> >>Cc: Eric Sandeen <sandeen@redhat.com>
> >>Cc: Christoph Hellwig <hch@infradead.org>
> >>Cc: Jan Kara <jack@suse.cz>
> >>Cc: Dave Chinner <dchinner@redhat.com>
> >>Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> >>---
> >...
> >>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).
  Well, except that iterate_supers() actually takes a passive reference
(s_count) of the superblock. Thus deactivate_locked_super() will never
really destroy it. So what I propose should be safe.

> 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.
  I know but really the assymetry in the locking has to be observed by the
callback function. And if you have a callback function which doesn't want
to deactivate superblock, it is far from obvious it should drop the
s_umount semaphore... And I'm happy how you documented stuff but people
read documentation only after they spot problems so it is better to have
interfaces which are hard to get wrong.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html