Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Eric Sandeen <sandeen@sandeen.net>]

On 2/8/12 5:47 PM, Jan Kara wrote:
> On Thu 09-02-12 00:27:05, Jan Kara wrote:
>> On Thu 09-02-12 10:20:49, Dave Chinner wrote:

...

>>> The thawed parameter appears to be unused and the waiting for frozen
>>> filesystems appears to happen for all callers. Is this intentional?
>>   Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
>> up.
>   Attached is a fixed version.
> 
> 								Honza

> From 5dce7adb0c281612a14fa3dd8c8d5ef3f5eb3666 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Tue, 7 Feb 2012 22:59:06 +0100
> Subject: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw
> 
> In quota code we need to find a superblock corresponding to a device and wait
> for superblock to be unfrozen. However this waiting has to happen without
> s_umount semaphore because that is required for superblock to thaw. So provide
> a function in VFS for this to keep dances with s_umount where they belong.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
>  include/linux/fs.h |    1 +
>  2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 6015c02..e15aaa9 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
>  
>  EXPORT_SYMBOL(iterate_supers_type);
>  
> -/**
> - *	get_super - get the superblock of a device
> - *	@bdev: device to get the superblock for
> - *	
> - *	Scans the superblock list and finds the superblock of the file system
> - *	mounted on the device given. %NULL is returned if no match is found.
> - */

> -
> -struct super_block *get_super(struct block_device *bdev)
> +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
>  {
>  	struct super_block *sb;
>  
> @@ -618,9 +610,13 @@ rescan:
>  			spin_unlock(&sb_lock);
>  			down_read(&sb->s_umount);
>  			/* still alive? */
> -			if (sb->s_root && (sb->s_flags & MS_BORN))
> -				return sb;
> -			up_read(&sb->s_umount);
> +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> +				if (!thawed || sb->s_frozen == SB_UNFROZEN)

I guess this has the same race we are worried about elsewhere (sb gets
frozen immediately after we check) but probably can't fix that yet.

would "wait_for_thaw" vs. "thaw" be any clearer?  Nitpicky I guess but the
meaning of "thawed" isn't immediately clear here.  If it's already thawed?  If
we want to wait for it it to be thawed?  You can figure it out from the callers
but maybe a comment or a different name might help.  No big deal.
 
> +					return sb;
> +				up_read(&sb->s_umount);
> +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> +			} else
> +				up_read(&sb->s_umount);
>  			/* nope, got unmounted */
>  			spin_lock(&sb_lock);
>  			__put_super(sb);
> @@ -631,9 +627,36 @@ rescan:
>  	return NULL;
>  }
>  
> +/**
> + *	get_super - get the superblock of a device
> + *	@bdev: device to get the superblock for
> + *
> + *	Scans the superblock list and finds the superblock of the file system
> + *	mounted on the device given. %NULL is returned if no match is found.
> + */

I think it'd be nice to explicitly say in the comment that this may
return a frozen superblock.

> +struct super_block *get_super(struct block_device *bdev)
> +{
> +	return __get_super(bdev, false);
> +}
>  EXPORT_SYMBOL(get_super);
>  
>  /**
> + *	get_super_thawed - get thawed superblock of a device
> + *	@bdev: device to get the superblock for
> + *
> + *	Scans the superblock list and finds the superblock of the file system
> + *	mounted on the device given once the superblock is thawed. %NULL is
> + *	returned if no match is found.
> + */

And to explicitly say that this one will not return until the sb is unfrozen?

Otherwise, this seems fine to me, thanks.

-Eric

> +struct super_block *get_super_thawed(struct block_device *bdev)
> +{
> +	return __get_super(bdev, true);
> +}
> +EXPORT_SYMBOL(get_super_thawed);
> +
> +/**
>   * get_active_super - get an active reference to the superblock of a device
>   * @bdev: device to get the superblock for
>   *
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 386da09..69cd5bb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2496,6 +2496,7 @@ extern void get_filesystem(struct file_system_type *fs);
>  extern void put_filesystem(struct file_system_type *fs);
>  extern struct file_system_type *get_fs_type(const char *name);
>  extern struct super_block *get_super(struct block_device *);
> +extern struct super_block *get_super_thawed(struct block_device *);
>  extern struct super_block *get_active_super(struct block_device *bdev);
>  extern void drop_super(struct super_block *sb);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);