Re: [PATCH 06/19] quota: Remove state_lock

[Jan Kara <jack@suse.cz>]

On Thu 11-11-10 15:14:25, Dmitry Monakhov wrote:
> The only reader which use state_lock is dqget(), and is it serialized
> with quota_disable via SRCU. And state_lock doesn't guaranties anything
> for that case. All methods which modify quota flags already protected by
> dqonoff_mutex. Get rid of useless state_lock.
  dq_state_lock has two properties you miss here:
a) It guarantees we read a consistent value from a quota state variable.
b) We read an uptodate value from quota state variable.

b) is kind of achieved by rcu_read_lock() which implies a memory barrier
but still stores can be reordered and it's not completely clear it does not
matter. a) is simply not achieved by your code.

So I think you have to change quota code to manipulate state flags in an
atomic manner and at least add comments why reordering the state flags
store e.g. during quotaon does not matter...

								Honza

> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/quota/dquot.c |   24 ++----------------------
>  1 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 78e48f3..a1efacd 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -86,12 +86,9 @@
>   * dq_data_lock protects data from dq_dqb and also mem_dqinfo structures and
>   * also guards consistency of dquot->dq_dqb with inode->i_blocks, i_bytes.
>   * i_blocks and i_bytes updates itself are guarded by i_lock acquired directly
> - * in inode_add_bytes() and inode_sub_bytes(). dq_state_lock protects
> - * modifications of quota state (on quotaon and quotaoff) and readers who care
> - * about latest values take it as well.
> + * in inode_add_bytes() and inode_sub_bytes().
>   *
> - * The spinlock ordering is hence: dq_data_lock > dq_list_lock > i_lock,
> - *   dq_list_lock > dq_state_lock
> + * The spinlock ordering is hence: dq_data_lock > dq_list_lock > i_lock.
>   *
>   * Note that some things (eg. sb pointer, type, id) doesn't change during
>   * the life of the dquot structure and so needn't to be protected by a lock
> @@ -128,7 +125,6 @@
>   */
>  
>  static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_list_lock);
> -static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_state_lock);
>  __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_data_lock);
>  EXPORT_SYMBOL(dq_data_lock);
>  
> @@ -827,14 +823,10 @@ struct dquot *dqget(struct super_block *sb, unsigned int id, int type)
>  	rcu_read_unlock();
>  we_slept:
>  	spin_lock(&dq_list_lock);
> -	spin_lock(&dq_state_lock);
>  	if (!sb_has_quota_active(sb, type)) {
> -		spin_unlock(&dq_state_lock);
>  		spin_unlock(&dq_list_lock);
>  		goto out;
>  	}
> -	spin_unlock(&dq_state_lock);
> -
>  	dquot = find_dquot(hashent, sb, id, type);
>  	if (!dquot) {
>  		if (!empty) {
> @@ -2022,24 +2014,19 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags)
>  			continue;
>  
>  		if (flags & DQUOT_SUSPENDED) {
> -			spin_lock(&dq_state_lock);
>  			qctl->flags |=
>  				dquot_state_flag(DQUOT_SUSPENDED, cnt);
> -			spin_unlock(&dq_state_lock);
>  		} else {
> -			spin_lock(&dq_state_lock);
>  			qctl->flags &= ~dquot_state_flag(flags, cnt);
>  			/* Turning off suspended quotas? */
>  			if (!sb_has_quota_loaded(sb, cnt) &&
>  			    sb_has_quota_suspended(sb, cnt)) {
>  				qctl->flags &=	~dquot_state_flag(
>  							DQUOT_SUSPENDED, cnt);
> -				spin_unlock(&dq_state_lock);
>  				iput(dqopt->files[cnt]);
>  				dqopt->files[cnt] = NULL;
>  				continue;
>  			}
> -			spin_unlock(&dq_state_lock);
>  		}
>  
>  		/* We still have to keep quota loaded? */
> @@ -2235,10 +2222,7 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
>  		goto out_file_init;
>  	}
>  	mutex_unlock(&dqopt->dqio_mutex);
> -	spin_lock(&dq_state_lock);
>  	dqctl(sb)->flags |= dquot_state_flag(flags, type);
> -	spin_unlock(&dq_state_lock);
> -
>  	add_dquot_ref(sb, type);
>  	mutex_unlock(&dqctl(sb)->dqonoff_mutex);
>  
> @@ -2290,12 +2274,10 @@ int dquot_resume(struct super_block *sb, int type)
>  		}
>  		inode = qctl->dq_opt->files[cnt];
>  		qctl->dq_opt->files[cnt] = NULL;
> -		spin_lock(&dq_state_lock);
>  		flags = qctl->flags & dquot_state_flag(DQUOT_USAGE_ENABLED |
>  							DQUOT_LIMITS_ENABLED,
>  							cnt);
>  		qctl->flags &= ~dquot_state_flag(DQUOT_STATE_FLAGS, cnt);
> -		spin_unlock(&dq_state_lock);
>  		mutex_unlock(&qctl->dqonoff_mutex);
>  
>  		flags = dquot_generic_flag(flags, cnt);
> @@ -2369,9 +2351,7 @@ int dquot_enable(struct inode *inode, int type, int format_id,
>  			ret = -EBUSY;
>  			goto out_lock;
>  		}
> -		spin_lock(&dq_state_lock);
>  		qctl->flags |= dquot_state_flag(flags, type);
> -		spin_unlock(&dq_state_lock);
>  out_lock:
>  		mutex_unlock(&qctl->dqonoff_mutex);
>  		return ret;
> -- 
> 1.6.5.2
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR