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...
>
> > @@ -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.
I've copied i_dquot to local with the purpose.
Later i've plans to get rid of dqptr_sem by rearranging code like follows.
But may be i've introduced copy logic too soon.
Variant (1)
1) Protect i_dquot dereference/modification with i_lock
So charge/free function will looks like follows
do_alloc_space (struct inode* inode, qsize_t num)
{
spin_lock(&inode->i_lock);
if (!check_bdq(inode->i_dquot, num))
return EQDUOT;
inode_add_bytes_unlocked(inode, num);
dquot_add_bytes(inode->i_dquot, num);
/* We can sleep while we call quota dirty, to protect quota from
* being deleted we have to get ref for the quota.
*/
atomic_inc(dquot->dq_count);
dquot = inode->i_dquot;
spin_unlock(&inode->i_lock);
/* i_lock was dropped, do not touch i_dquot any more */
mark_dquot_dirty(dquot);
dqput(dquot);
}
This is beter than before, but we still have massive anomic operations
So we can try second variant.
Variant (2)
do_alloc_space(struct inode* inode, qsize_t num)
{
idx = srcu_read_lock(&dqptr)
spin_lock(&inode->i_lock);
if (!check_bdq(inode->i_dquot, num))
return EQDUOT;
inode_add_bytes_unlocked(inode, num);
dquot_add_bytes(inode->i_dquot, num);
/* We can sleep while we call quota dirty,
* But this is ok since quota deletion is protected by SRCU
* The only thing we have do is to save old i_dquot array
*/
dquot = inode->i_dquot;
spin_unlock(&inode->i_lock);
/* i_lock was dropped, do not touch i_dquot any more */
mark_dquot_dirty(dquot);
srcu_read_unlock(&dqptr, idx);
}
/* Destroy method may looks like whis, it will be called from
background work.
*/
do_destroy_quotas_loop() {
/* Wait for all quotas in free list */
synchronize_srcu(dqptr&)
/*
Ok now we can safely destroy all quotas in the list,
Some of them may be dirty, so we have to write it first.
*/
list_for_each_entry(dquot, &free_list, dq_free)
dquot_release(dquot);
}
>
> 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
