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

[Josef Bacik <jbacik@fusionio.com>]

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,

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