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

[Fernando Luis Vazquez Cao <fernando_b1@lab.ntt.co.jp>]

On 2012/09/25 18:52, Jan Kara wrote:
> 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.

Good point. I missed the fact that we are taking a passive
reference there, which should simplifying locking. I will
take that into account for the next revision.

Thanks,
Fernando
--
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