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: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).

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html