From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 09/11] quota: protect dquot mem info with objects's lock Date: Wed, 6 Oct 2010 14:37:27 +0200 Message-ID: <20101006123726.GL3676@quack.suse.cz> References: <1286302827-31043-1-git-send-email-dmonakhov@gmail.com> <1286302827-31043-10-git-send-email-dmonakhov@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org, Dmitry Monakhov To: Dmitry Monakhov Return-path: Received: from cantor2.suse.de ([195.135.220.15]:51651 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755799Ab0JFMiX (ORCPT ); Wed, 6 Oct 2010 08:38:23 -0400 Content-Disposition: inline In-Reply-To: <1286302827-31043-10-git-send-email-dmonakhov@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 SUSE Labs, CR