Re: [PATCH 7/9] fsfreeze: freeze_super and thaw_bdev don't play well together

[Eric Sandeen <sandeen@redhat.com>]

On 9/14/12 2:20 PM, Eric Sandeen wrote:
> On 9/14/12 1:53 AM, Fernando Luis Vázquez Cao wrote:
>> thaw_bdev() has re-entrancy guards to allow freezes to nest
>> together. That is, it ensures that the filesystem is not thawed
>> until the last thaw command is issued. This is needed to prevent the
>> filesystem from being unfrozen while an existing freezer is still
>> operating on the filesystem in a frozen state (e.g. dm-snapshot).
>>
>> Currently, freeze_super() and thaw_super() bypasses these guards,
>> and as a result manual freezing and unfreezing via the ioctl methods
>> do not nest correctly. hence mixing userspace directed freezes with
>> block device level freezes result in inconsistency due to premature
>> thawing of the filesystem.
>>
>> Move the re-enterency guards to the superblock and into freeze_super
>> and thaw_super() so that userspace directed freezes nest correctly
>> again. Caveat: Things work as expected as long as direct calls to
>> thaw_super are always in response to a previous sb level freeze. In
>> other words an unpaired call to thaw_super can still thaw a
>> filesystem frozen using freeze_bdev (this issue could be addressed
>> in a follow-up patch if deemed necessary).
>>
>> This patch retains the bdev level mutex and counter to keep the
>> "feature" that we can freeze a block device that does not have a
>> filesystem mounted yet.
>>
>> IMPORTANT CAVEAT: This patch restores the nesting feature of the fsfreeze ioctl
>> which got removed by commit 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2
>> ("Introduce freeze_super and thaw_super for the fsfreeze ioctl"). It could be
>> argued that it is too late to fix the userland ABI breakage caused by that
>> patch and that the current ABI is the one that should be kept. If this is the
>> respective maintainer(s) opinion this could be modified to not allow fsfreeze
>> ioctl nesting.
> 
> I think what's more important is that a given process or person can expect
> their freeze to last until they issue an unfreeze, but maybe there are
> counter-arguments.
> 
>> Changes since version 2:
>>   - Fix reference count leak in freeze_super when MS_BORN flag is not set in
>>     the superblock.
>>   - Protect freeze counter using s_umount and get rid of superblock level
>>     fsfreeze mutex.
>>
>> 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>
>> ---
> 
> <big snip for now>
> 
>> @@ -1506,6 +1514,7 @@ static void do_thaw_one(struct super_blo
>>  
>>  	/* We were called from __iterate_supers with superblock lock taken
>>  	 * so we do not need to do it here. */
>> +	sb->s_freeze_count = 1; /* avoid multiple calls to __thaw_super */
>>  	res = __thaw_super(sb);
> 
> Hm, freeze_super() did:
> 
> 	atomic_inc(&sb->s_active);
> 
> as well as
> 
> 	if (++sb->s_freeze_count > 1)
> 
> for each successful nested freeze.  
> 
> so won't this leave a bunch of active refs on the superblock?

Sorry, Fernando pointed out that I missed the fact that we only keep 1 active
ref on s_active even for nested freezers.  So this is ok.

-Eric

> 
>>  	if (!res)
>>  		deactivate_locked_super(sb);
>> diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/include/linux/fs.h
>> --- linux-3.6-rc5-orig/include/linux/fs.h	2012-09-14 13:46:38.325179510 +0900
>> +++ linux-3.6-rc5/include/linux/fs.h	2012-09-14 13:51:36.485205815 +0900
>> @@ -1578,6 +1578,8 @@ struct super_block {
>>  
>>  	/* Being remounted read-only */
>>  	int s_readonly_remount;
>> +
>> +	int			s_freeze_count; /* nr of nested freezes */
>>  };
>>  
>>  /* superblock cache pruning functions */
>>
>>
> 

--
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