Re: [PATCH 2/8] fsfreeze: emergency thaw will deadlock on s_umount

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

On 2012/07/13 22:17, Jan Kara wrote:
> On Thu 12-07-12 18:04:09, Fernando Luis Vázquez Cao wrote:
>> diff -urNp vfs-orig/fs/super.c vfs/fs/super.c
>> --- vfs-orig/fs/super.c	2012-07-04 18:57:54.000000000 +0900
>> +++ vfs/fs/super.c	2012-07-12 14:21:09.736566768 +0900
>> @@ -1221,23 +1221,25 @@ int freeze_super(struct super_block *sb)
>>   EXPORT_SYMBOL(freeze_super);
>>   
>>   /**
>> - * thaw_super -- unlock filesystem
>> + * __thaw_super -- unlock filesystem
>>    * @sb: the super to thaw
>> + * @emergency:	emergency thaw
>>    *
>>    * Unlocks the filesystem and marks it writeable again after freeze_super().
>> + * This is the unlocked version of thaw_super and has to be called with the
>> + * sb->s_umount lock held in the non-emergency thaw case.
>>    */
>> -int thaw_super(struct super_block *sb)
>> +static int __thaw_super(struct super_block *sb, int emergency)
>>   {
>> -	int error;
>> +	int error = 0;
>>   
>> -	down_write(&sb->s_umount);
>>   	if (sb->s_frozen == SB_UNFROZEN) {
>> -		up_write(&sb->s_umount);
>> -		return -EINVAL;
>> +		error = -EINVAL;
>> +		goto out;
>>   	}
>>   
>>   	if (sb->s_flags & MS_RDONLY)
>> -		goto out;
>> +		goto out_thaw;
>>   
>>   	if (sb->s_op->unfreeze_fs) {
>>   		error = sb->s_op->unfreeze_fs(sb);
>> @@ -1245,17 +1247,52 @@ int thaw_super(struct super_block *sb)
>>   			printk(KERN_ERR
>>   				"VFS:Filesystem thaw failed\n");
>>   			sb->s_frozen = SB_FREEZE_TRANS;
>> -			up_write(&sb->s_umount);
>> -			return error;
>> +			goto out;
>>   		}
>>   	}
>>   
>> -out:
>> +out_thaw:
>>   	sb->s_frozen = SB_UNFROZEN;
>>   	smp_wmb();
>>   	wake_up(&sb->s_wait_unfrozen);
>> -	deactivate_locked_super(sb);
>>   
>> -	return 0;
>> +	/*
>> +	 * When called from emergency scope, we cannot grab the s_umount lock
>> +	 * so we cannot deactivate the superblock. This may leave unbalanced
>> +	 * superblock references which could prevent unmount, but given this is
>> +	 * an emergency operation....
>> +	 */
>> +	if (!emergency)
>> +		deactivate_locked_super(sb);
>> +out:
>> +	return error;
>> +}
>    This is just wrong. deactivate_locked_super() will unlock the superblock
> for you (and may even free it). So just do this in thaw_super() instead of
> up_write(&sb->s_umount) which is bogus there. That will also allow you to
> get rid of that ugly 'emergency' argument...

Ouch! I am sorry for sending a botched patch. In my local tree
the emergency argument is gone and thaw_super looks like this
(up_write(&sb->s_umount) is needed when __thaw_super() fails
because we do not want to release a reference to the superblock
in that case):

int thaw_super(struct super_block *sb)
{
         int res;
         down_write(&sb->s_umount);
         res = __thaw_super(sb);
         if (!res)
                 deactivate_locked_super(sb);
         else
                 up_write(&sb->s_umount);
         return res;
}


>> +/**
>> + * thaw_super_emergency  -- unlock filesystem
>> + * @sb: the super to thaw
>> + *
>> + * Unlocks the filesystem and marks it writeable again after freeze_super().
>> + * The kernel gets here holding the sb->s_umount lock in read mode so we cannot
>> + * grab it again in write mode.
>> + */
>> +int thaw_super_emergency(struct super_block *sb)
>> +{
>> +	return __thaw_super(sb, 1);
>> +}
>    And this function can be deleted as well. Just call __thaw_super() from
> that one place.

Good point. The code is easier to read that way.

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