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@xxxxxxxxxxxx>
>> Cc: Eric Sandeen <sandeen@xxxxxxxxxx>
>> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>> Cc: Jan Kara <jack@xxxxxxx>
>> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
>> ---
>
> <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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
