Re: [PATCH 1/9] vfs: add __iterate_supers() helper

[Eric Sandeen <sandeen@redhat.com>]

On 9/13/12 6:00 AM, Fernando Luis Vázquez Cao wrote:
> iterate_supers() calls a function provided by the caller with the s_umount
> semaphore taken in read mode. However, there may be cases where write mode
> is preferable, so we add a new helper, __iterate_supers(), which lets one
> specify the mode of the lock.
> 
> This will be used to fix the emergency thaw (filesystem unfreeze) code, which
> iterates over the list of superblocks but needs to hold the s_umount semaphore
> in _write_ mode bebore carrying out the actual thaw operation.

I'll go ahead & comment on this even though all of this is to support the
emergency thaw misfeature which I'm liking less and less this evening.

I think this split is a little odd, usually __foo() provides some fundamental
action, and foo_*() functions call it after possibly defining or altering aspects of
that action, or preparing for that action.

So having iterate_supers() do read locks, and __iterate_supers do either read
or write, seems asymmetric, and isn't very well self-documenting.

Why not have i.e. iterate_supers_read() and iterate_supers_write(), each
of which can call __iterate_supers(blah, blah, locktype).

Anyway, it'd mean changing 5 callers but it's a little clearer and more symmetric
to me.  What do you think?

The logic seems fine, just a question about the style.

Thanks,
-Eric

> 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>
> ---
> 
> 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 18:45:13.818046999 +0900
> +++ linux-3.6-rc5/fs/super.c	2012-09-12 19:08:58.214034467 +0900
> @@ -537,14 +537,22 @@ void drop_super(struct super_block *sb)
>  EXPORT_SYMBOL(drop_super);
>  
>  /**
> - *	iterate_supers - call function for all active superblocks
> + *	__iterate_supers - call function for all active superblocks
>   *	@f: function to call
>   *	@arg: argument to pass to it
> + *	@wlock: mode of superblock lock (false->read lock, true->write lock)
>   *
>   *	Scans the superblock list and calls given function, passing it
>   *	locked superblock and given argument.
> + *
> + *	When the caller asks for the superblock lock (s_umount semaphore) to be
> + *	taken in write mode, the lock is taken but not released because the
> + *	function provided by the caller may deactivate the superblock itself.
> + *	It is that function's job to unlock the superblock as needed in such a
> + *	case.
>   */
> -void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> +void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
> +		      bool wlock)
>  {
>  	struct super_block *sb, *p = NULL;
>  
> @@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup
>  		sb->s_count++;
>  		spin_unlock(&sb_lock);
>  
> -		down_read(&sb->s_umount);
> +		if (wlock)
> +			down_write(&sb->s_umount);
> +		else
> +			down_read(&sb->s_umount);
> +
>  		if (sb->s_root && (sb->s_flags & MS_BORN))
>  			f(sb, arg);
> -		up_read(&sb->s_umount);
> +
> +		/* When the semaphore was taken in write mode the function
> +		 * provided by the caller takes care of unlocking it as
> +		 * needed. See explanation above for details. */
> +		if (!wlock)
> +			up_read(&sb->s_umount);
>  
>  		spin_lock(&sb_lock);
>  		if (p)
> @@ -571,6 +588,19 @@ void iterate_supers(void (*f)(struct sup
>  }
>  
>  /**
> + *	iterate_supers - call function for all active superblocks
> + *	@f: function to call
> + *	@arg: argument to pass to it
> + *
> + *	Scans the superblock list and calls given function, passing it
> + *	a superblock locked in _read_ mode and given argument.
> + */
> +void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
> +{
> +	__iterate_supers(f, arg , false);
> +}
> +
> +/**
>   *	iterate_supers_type - call function for superblocks of given type
>   *	@type: fs type
>   *	@f: function to call
> 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-12 18:45:14.002047001 +0900
> +++ linux-3.6-rc5/include/linux/fs.h	2012-09-12 18:46:39.466082276 +0900
> @@ -2684,6 +2684,8 @@ extern struct super_block *get_super(str
>  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 (*f)(struct super_block *, void *), void *arg,
> +		      bool wlock);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);
>  extern void iterate_supers_type(struct file_system_type *,
>  			        void (*)(struct super_block *, void *), void *);
> 
> 

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