Re: [PATCH 09/11] quota: protect dquot mem info with objects's lock

[Jan Kara <jack@suse.cz>]

On Tue 05-10-10 22:20:25, Dmitry Monakhov wrote:
> currently ->dq_data_lock is responsible for protecting three things
> 
> 1) dquot->dq_dqb info consistency
> 2) synchronization between ->dq_dqb with ->i_bytes
> 3) Protects mem_dqinfo (per-sb data),
>   3b) and consystency between mem_dqinfo and dq_dqb for following data.
               ^^ consistency

>       dqi_bgrace <=> dqb_btime
>       dqi_igrace <=> dqb_itime
> 
> In fact (1) and (2) is conceptually different from (3)
> By introducing per-dquot data lock we later can split (1)(2) from (3)
> This patch simply introduce new lock, without changing ->dq_data_lock.
                    ^^^^^^^^ introduces a

  I'd be interested in how much this split brings. The hold time of
dq_data_lock isn't so big and I wonder if the higher scalability won't
be mitigated by cache ping-pongs of structures themselves...

> @@ -378,6 +380,37 @@ static inline void dqput_all(struct dquot **dquot)
>  		dqput(dquot[cnt]);
>  }
>  
> +static inline void inode_dquot_lock(const struct inode *inode,
> +				struct dquot **dquot)
> +{
> +	unsigned int cnt;
> +
> +	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> +		dquot[cnt] = inode->i_dquot[cnt];
> +		if (dquot[cnt])
> +			spin_lock(&dquot[cnt]->dq_lock);
> +	}
> +}
> +
> +static inline void dquot_lock_all(struct dquot **dquot)
> +{
> +	unsigned int cnt;
> +
> +	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> +		if (dquot[cnt])
> +			spin_lock(&dquot[cnt]->dq_lock);
> +
> +}
> +
> +static inline void dquot_unlock_all(struct dquot **dquot)
> +{
> +	unsigned int cnt;
> +
> +	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> +		if (dquot[cnt])
> +			spin_unlock(&dquot[cnt]->dq_lock);
> +}
  Please, just have two locking functions like:
lock_inode_dquots(inode->i_dquot), unlock_inode_dquots(inode->i_dquot).
Also please avoid this copying of pointers to dquot structures. It just
makes things harder to read and since dquot structures are reference
counted, it's also not obviously correct strictly speaking (although fine
in the end in your case since you hold dqptr_sem). If you wish to save some
typing, just keep inode->i_dquot in a local variable.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR