On Wed, 6 Oct 2010 14:37:27 +0200, Jan Kara <jack@xxxxxxx> wrote:
> 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...
Currently it is unlikely to observe any difference at all
because we end up with blocking on dq_mutex, dqio_mutex and i_mutex :(
I'll plan optimization for io mutexes in third series of scalability
patches. But nor than less, i'll try to meashure ext2 over tmpfs.
>
> > @@ -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@xxxxxxx>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
