From: Jan Kara <jack@suse.cz>
To: Dmitry Monakhov <dmonakhov@openvz.org>
Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org,
Dmitry Monakhov <dmonakhov@gmail.com>
Subject: Re: [PATCH 11/11] quota: relax dq_data_lock dq_lock locking consistency
Date: Wed, 6 Oct 2010 13:56:43 +0200 [thread overview]
Message-ID: <20101006115643.GK3676@quack.suse.cz> (raw)
In-Reply-To: <1286302827-31043-12-git-send-email-dmonakhov@gmail.com>
On Tue 05-10-10 22:20:27, Dmitry Monakhov wrote:
> In fact consistency between mem_info and dq_dqb is weak because we
> just copy data from dqi_{bi}grace to dqb_{bi}time.
> So we protect dqb_{bi}time from races with quota_ctl call.
> Nothing actually happens if we relax this consistency requirement.
> Since dqi_{bi}grace is int it is possible read it atomically without lock.
OK, but comments like "It is ok to read dqi_bgrace without lock here"
do not explain us why it is ok. Instead of commenting at each place where
we read those values, I'd add more detailed comment before declaration of
mem_dqinfo that dqi_bgrace and dqi_igrace can be read without the lock and
that it's safe because the reads are atomic.
Honza
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@gmail.com>
> ---
> fs/quota/dquot.c | 28 ++++++++--------------------
> 1 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index fc3b63a..0c9cf67 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1254,7 +1254,7 @@ static int ignore_hardlimit(struct dquot *dquot)
> !(info->dqi_flags & V1_DQF_RSQUASH));
> }
>
> -/* needs dq_data_lock, ->dq_lock */
> +/* needs ->dq_lock */
> static int check_idq(struct dquot *dquot, qsize_t inodes, char *warntype)
> {
> qsize_t newinodes = dquot->dq_dqb.dqb_curinodes + inodes;
> @@ -1284,6 +1284,7 @@ static int check_idq(struct dquot *dquot, qsize_t inodes, char *warntype)
> newinodes > dquot->dq_dqb.dqb_isoftlimit &&
> dquot->dq_dqb.dqb_itime == 0) {
> *warntype = QUOTA_NL_ISOFTWARN;
> + /* It is ok to read dqi_bgrace without lock here */
> dquot->dq_dqb.dqb_itime = get_seconds() +
> dq_opt(dquot)->info[dquot->dq_type].dqi_igrace;
> }
> @@ -1291,7 +1292,7 @@ static int check_idq(struct dquot *dquot, qsize_t inodes, char *warntype)
> return 0;
> }
>
> -/* needs dq_data_lock, ->dq_lock */
> +/* needs ->dq_lock */
> static int check_bdq(struct dquot *dquot, qsize_t space, int prealloc, char *warntype)
> {
> qsize_t tspace;
> @@ -1328,6 +1329,7 @@ static int check_bdq(struct dquot *dquot, qsize_t space, int prealloc, char *war
> dquot->dq_dqb.dqb_btime == 0) {
> if (!prealloc) {
> *warntype = QUOTA_NL_BSOFTWARN;
> + /* It is ok to read dqi_bgrace without lock here */
> dquot->dq_dqb.dqb_btime = get_seconds() +
> sb_dqopt(sb)->info[dquot->dq_type].dqi_bgrace;
> }
> @@ -1596,7 +1598,6 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags)
> for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> warntype[cnt] = QUOTA_NL_NOWARN;
>
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, dquot);
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> if (!dquot[cnt])
> @@ -1604,7 +1605,6 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags)
> ret = check_bdq(dquot[cnt], number, !warn, warntype+cnt);
> if (ret && !nofail) {
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> goto out_flush_warn;
> }
> }
> @@ -1618,7 +1618,6 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, int flags)
> }
> inode_incr_space(inode, number, reserve);
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
>
> if (reserve)
> goto out_flush_warn;
> @@ -1647,7 +1646,6 @@ int dquot_alloc_inode(const struct inode *inode)
> for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> warntype[cnt] = QUOTA_NL_NOWARN;
> down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, dquot);
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> if (!dquot[cnt])
> @@ -1665,7 +1663,6 @@ int dquot_alloc_inode(const struct inode *inode)
>
> warn_put_all:
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> if (ret == 0)
> mark_all_dquot_dirty(dquot);
> flush_warnings(dquot, warntype);
> @@ -1688,7 +1685,6 @@ int dquot_claim_space_nodirty(struct inode *inode, qsize_t number)
> }
>
> down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, dquot);
> /* Claim reserved quotas to allocated quotas */
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> @@ -1698,7 +1694,6 @@ int dquot_claim_space_nodirty(struct inode *inode, qsize_t number)
> /* Update inode bytes */
> inode_claim_rsv_space(inode, number);
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> mark_all_dquot_dirty(dquot);
> up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> return 0;
> @@ -1723,7 +1718,6 @@ void __dquot_free_space(struct inode *inode, qsize_t number, int flags)
> }
>
> down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, dquot);
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> if (!dquot[cnt])
> @@ -1736,7 +1730,6 @@ void __dquot_free_space(struct inode *inode, qsize_t number, int flags)
> }
> inode_decr_space(inode, number, reserve);
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
>
> if (reserve)
> goto out_unlock;
> @@ -1762,7 +1755,6 @@ void dquot_free_inode(const struct inode *inode)
> return;
>
> down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, dquot);
> for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> if (!dquot[cnt])
> @@ -1771,7 +1763,6 @@ void dquot_free_inode(const struct inode *inode)
> dquot_decr_inodes(dquot[cnt], 1);
> }
> dquot_unlock_all(dquot);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> mark_all_dquot_dirty(dquot);
> flush_warnings(dquot, warntype);
> up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> @@ -1809,7 +1800,6 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
> up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
> return 0;
> }
> - spin_lock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> inode_dquot_lock(inode, transfer_from);
> cur_space = inode_get_bytes(inode);
> rsv_space = inode_get_rsv_space(inode);
> @@ -1858,7 +1848,6 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
> }
> dquot_unlock_all(transfer_to);
> dquot_unlock_all(transfer_from);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
>
> mark_all_dquot_dirty(transfer_from);
> @@ -1874,7 +1863,6 @@ warn:
> over_quota:
> dquot_unlock_all(transfer_to);
> dquot_unlock_all(transfer_from);
> - spin_unlock(&sb_dqopt(inode->i_sb)->dq_data_lock);
> up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
> goto warn;
> }
> @@ -2468,7 +2456,6 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> (di->d_ino_hardlimit > dqi->dqi_maxilimit)))
> return -ERANGE;
>
> - spin_lock(&dq_opt(dquot)->dq_data_lock);
> spin_lock(&dquot->dq_lock);
> if (di->d_fieldmask & FS_DQ_BCOUNT) {
> dm->dqb_curspace = di->d_bcount - dm->dqb_rsvspace;
> @@ -2518,7 +2505,8 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> dm->dqb_btime = 0;
> clear_bit(DQ_BLKS_B, &dquot->dq_flags);
> } else if (!(di->d_fieldmask & FS_DQ_BTIMER))
> - /* Set grace only if user hasn't provided his own... */
> + /* Set grace only if user hasn't provided his own...
> + Read dqi_bgrace without lock */
> dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
> }
> if (check_ilim) {
> @@ -2527,7 +2515,8 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> dm->dqb_itime = 0;
> clear_bit(DQ_INODES_B, &dquot->dq_flags);
> } else if (!(di->d_fieldmask & FS_DQ_ITIMER))
> - /* Set grace only if user hasn't provided his own... */
> + /* Set grace only if user hasn't provided his own...
> + Read dqi_bgrace without lock */
> dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
> }
> if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit ||
> @@ -2536,7 +2525,6 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> else
> set_bit(DQ_FAKE_B, &dquot->dq_flags);
> spin_unlock(&dquot->dq_lock);
> - spin_unlock(&dq_opt(dquot)->dq_data_lock);
> mark_dquot_dirty(dquot);
>
> return 0;
> --
> 1.6.6.1
>
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
next prev parent reply other threads:[~2010-10-06 11:57 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-05 18:20 (unknown), Dmitry Monakhov
2010-10-05 18:20 ` [PATCH 01/11] quota: add wrapper function Dmitry Monakhov
2010-10-06 8:56 ` Christoph Hellwig
2010-10-06 10:01 ` Jan Kara
2010-10-05 18:20 ` [PATCH 02/11] quota: Convert dq_state_lock to per-sb dq_state_lock Dmitry Monakhov
2010-10-06 10:04 ` Jan Kara
2010-10-05 18:20 ` [PATCH 03/11] quota: add quota format lock Dmitry Monakhov
2010-10-06 10:05 ` Jan Kara
2010-10-05 18:20 ` [PATCH 04/11] quota: make dquot lists per-sb Dmitry Monakhov
2010-10-06 8:57 ` Christoph Hellwig
2010-10-06 9:39 ` Dmitry
2010-10-06 10:22 ` Jan Kara
2010-10-06 10:40 ` Dmitry
2010-10-06 10:54 ` Jan Kara
2010-10-05 18:20 ` [PATCH 05/11] quota: make per-sb hash array Dmitry Monakhov
2010-10-06 10:38 ` Jan Kara
2010-10-05 18:20 ` [PATCH 06/11] quota: remove global dq_list_lock Dmitry Monakhov
2010-10-05 18:20 ` [PATCH 07/11] quota: rename dq_lock Dmitry Monakhov
2010-10-05 18:20 ` [PATCH 08/11] quota: make per-sb dq_data_lock Dmitry Monakhov
2010-10-06 11:01 ` Jan Kara
2010-10-05 18:20 ` [PATCH 09/11] quota: protect dquot mem info with objects's lock Dmitry Monakhov
2010-10-06 12:37 ` Jan Kara
2010-10-06 13:17 ` Dmitry
2010-10-06 13:41 ` Jan Kara
2010-10-06 14:19 ` Dmitry
2010-10-06 13:30 ` Jan Kara
2010-10-06 13:41 ` Dmitry
2010-10-05 18:20 ` [PATCH 10/11] quota: drop dq_data_lock where possible Dmitry Monakhov
2010-10-05 18:20 ` [PATCH 11/11] quota: relax dq_data_lock dq_lock locking consistency Dmitry Monakhov
2010-10-06 11:56 ` Jan Kara [this message]
2010-10-06 7:08 ` [PATCH 0/11] RFC quota scalability V1 Dmitry
2010-10-06 9:44 ` Jan Kara
2010-10-06 10:15 ` Dmitry
2010-10-06 10:47 ` Jan Kara
2010-10-10 3:50 ` Brad Boyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101006115643.GK3676@quack.suse.cz \
--to=jack@suse.cz \
--cc=dmonakhov@gmail.com \
--cc=dmonakhov@openvz.org \
--cc=hch@infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).