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@xxxxxxxxx>
> ---
> 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@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
