Re: [RFC 6/9] fsfreeze: move emergency thaw code to fs/super.c

[Eric Sandeen <sandeen@redhat.com>]

On 9/13/12 2:00 PM, Josef Bacik wrote:
> On Thu, Sep 13, 2012 at 05:08:19AM -0600, Fernando Luis Vázquez Cao wrote:
>> It makes no sense having the emergency thaw code in fs/buffer.c when all of
>> it's operations are one superblocks and the code it executes is all in
>> fs/super.c. Move the code there and clean it up.
>>
>> Cc: Josef Bacik <jbacik@fusionio.com>
>> Cc: Eric Sandeen <sandeen@redhat.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: 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/buffer.c linux-3.6-rc5/fs/buffer.c
>> --- linux-3.6-rc5-orig/fs/buffer.c	2012-09-12 20:44:13.226112590 +0900
>> +++ linux-3.6-rc5/fs/buffer.c	2012-09-12 20:50:25.406058417 +0900
>> @@ -511,52 +511,6 @@ repeat:
>>  	return err;
>>  }
>>  
>> -static int thaw_super_emergency(struct super_block *sb)
>> -{
>> -	int res;
>> -	/* We were called from __iterate_supers with superblock lock taken
>> -	 * so we do not need to do it here. */
>> -	res = __thaw_super(sb);
>> -	if (!res)
>> -		deactivate_locked_super(sb);
>> -	else
>> -		up_write(&sb->s_umount);
>> -	return res;
>> -}
>> -
>> -static void do_thaw_one(struct super_block *sb, void *unused)
>> -{
>> -	if (sb->s_bdev) {
>> -		char b[BDEVNAME_SIZE];
>> -		printk(KERN_WARNING "Emergency Thaw on %s.\n",
>> -		       bdevname(sb->s_bdev, b));
>> -	}
>> -	while (!thaw_super_emergency(sb));
>> -}
>> -
>> -static void do_thaw_all(struct work_struct *work)
>> -{
>> -	__iterate_supers(do_thaw_one, NULL, true);
>> -	kfree(work);
>> -	printk(KERN_WARNING "Emergency Thaw complete\n");
>> -}
>> -
>> -/**
>> - * emergency_thaw_all -- forcibly thaw every frozen filesystem
>> - *
>> - * Used for emergency unfreeze of all filesystems via SysRq
>> - */
>> -void emergency_thaw_all(void)
>> -{
>> -	struct work_struct *work;
>> -
>> -	work = kmalloc(sizeof(*work), GFP_ATOMIC);
>> -	if (work) {
>> -		INIT_WORK(work, do_thaw_all);
>> -		schedule_work(work);
>> -	}
>> -}
>> -
>>  /**
>>   * sync_mapping_buffers - write out & wait upon a mapping's "associated" buffers
>>   * @mapping: the mapping which wants those buffers written
>> 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-12 20:24:10.474041390 +0900
>> +++ linux-3.6-rc5/fs/super.c	2012-09-12 20:50:42.546044906 +0900
>> @@ -1475,3 +1475,49 @@ int thaw_super(struct super_block *sb)
>>  	return res;
>>  }
>>  EXPORT_SYMBOL(thaw_super);
>> +
>> +static int thaw_super_emergency(struct super_block *sb)
>> +{
>> +	int res;
>> +	/* We were called from __iterate_supers with superblock lock taken
>> +	 * so we do not need to do it here. */
>> +	res = __thaw_super(sb);
>> +	if (!res)
>> +		deactivate_locked_super(sb);
>> +	else
>> +		up_write(&sb->s_umount);
>> +	return res;
>> +}
> 	
> So unless I'm missing something this is wrong.  We do __iterate_supers() which
> does down_write(sb) and then call into this.  Lets imagine a perfect world where
> the sb was only frozen once.  So we go into __thaw_super() and return 0 because
> we were successfull and do deactivate_locked_super() which does
> up_write(s_umount), and then we loop because we want to get an -EINVAL to know
> we completely unfroze, so we call into __thaw_super(sb) without s_umount held
> and then we get our error and do up_write(s_umount) _again_.  So this needs to
> be reworked to be correct ;).  Thanks,

The stupid emergency sysrq thing was my fault (at someone else's suggestion) ;)

It's caused a lot of woe, and hasn't worked for two years.  Should we keep it?

-Eric

> Josef
> 

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