Re: [PATCH 09/11] quota: protect dquot mem info with objects's lock

[Dmitry <dmonakhov@openvz.org>]

On Wed, 6 Oct 2010 15:30:18 +0200, Jan Kara <jack@suse.cz> wrote:
> On Tue 05-10-10 22:20:25, Dmitry Monakhov wrote:
> > @@ -1762,14 +1810,16 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
> >  		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);
> >  	space = cur_space + rsv_space;
> > +	dquot_lock_all(transfer_to);
>   One more problem I've spotted - here you cannot lock new and old dquot
> structures in an arbitrary order as that would lead to deadlocks when two
> dquot_transfers run in parallel. So you have to establish some ordering and
> follow that while locking...
Absolutely, WOW, i've even written a remiander for that somewhere,
And as usually simply forgot about that :(

Minor issue: I've also forgot to place different quota types to different
lockdep classes. Which result complain from lockdep. Also will fix.
> 
> 								Honza
> 
> >  	/* Build the transfer_from list and check the limits */
> > +
> >  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> >  		if (!transfer_to[cnt])
> >  			continue;
> > -		transfer_from[cnt] = inode->i_dquot[cnt];
> >  		ret = check_idq(transfer_to[cnt], 1, warntype_to + cnt);
> >  		if (ret)
> >  			goto over_quota;
> > @@ -1806,6 +1856,8 @@ int __dquot_transfer(struct inode *inode, struct dquot **transfer_to)
> >  
> >  		inode->i_dquot[cnt] = transfer_to[cnt];
> >  	}
> > +	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);
> >  
> > @@ -1820,6 +1872,8 @@ warn:
> >  	flush_warnings(transfer_from, warntype_from_space);
> >  	return ret;
> >  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;
> > @@ -2363,6 +2417,7 @@ static void do_get_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> >  	di->d_id = dquot->dq_id;
> >  
> >  	spin_lock(&dq_opt(dquot)->dq_data_lock);
> > +	spin_lock(&dquot->dq_lock);
> >  	di->d_blk_hardlimit = stoqb(dm->dqb_bhardlimit);
> >  	di->d_blk_softlimit = stoqb(dm->dqb_bsoftlimit);
> >  	di->d_ino_hardlimit = dm->dqb_ihardlimit;
> > @@ -2371,6 +2426,7 @@ static void do_get_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> >  	di->d_icount = dm->dqb_curinodes;
> >  	di->d_btimer = dm->dqb_btime;
> >  	di->d_itimer = dm->dqb_itime;
> > +	spin_unlock(&dquot->dq_lock);
> >  	spin_unlock(&dq_opt(dquot)->dq_data_lock);
> >  }
> >  
> > @@ -2415,6 +2471,7 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> >  		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;
> >  		check_blim = 1;
> > @@ -2480,6 +2537,7 @@ static int do_set_dqblk(struct dquot *dquot, struct fs_disk_quota *di)
> >  		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
> >  	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);
> >  
> > diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> > index 8b04f24..1643c30 100644
> > --- a/fs/quota/quota_tree.c
> > +++ b/fs/quota/quota_tree.c
> > @@ -376,7 +376,9 @@ int qtree_write_dquot(struct qtree_mem_dqinfo *info, struct dquot *dquot)
> >  		}
> >  	}
> >  	spin_lock(&sb_dqopt(sb)->dq_data_lock);
> > +	spin_lock(&dquot->dq_lock);
> >  	info->dqi_ops->mem2disk_dqblk(ddquot, dquot);
> > +	spin_unlock(&dquot->dq_lock);
> >  	spin_unlock(&sb_dqopt(sb)->dq_data_lock);
> >  	ret = sb->s_op->quota_write(sb, type, ddquot, info->dqi_entry_size,
> >  				    dquot->dq_off);
> > @@ -632,12 +634,14 @@ int qtree_read_dquot(struct qtree_mem_dqinfo *info, struct dquot *dquot)
> >  		goto out;
> >  	}
> >  	spin_lock(&sb_dqopt(sb)->dq_data_lock);
> > +	spin_lock(&dquot->dq_lock);
> >  	info->dqi_ops->disk2mem_dqblk(dquot, ddquot);
> >  	if (!dquot->dq_dqb.dqb_bhardlimit &&
> >  	    !dquot->dq_dqb.dqb_bsoftlimit &&
> >  	    !dquot->dq_dqb.dqb_ihardlimit &&
> >  	    !dquot->dq_dqb.dqb_isoftlimit)
> >  		set_bit(DQ_FAKE_B, &dquot->dq_flags);
> > +	spin_unlock(&dquot->dq_lock);
> >  	spin_unlock(&sb_dqopt(sb)->dq_data_lock);
> >  	kfree(ddquot);
> >  out:
> > diff --git a/include/linux/quota.h b/include/linux/quota.h
> > index 9e7a102..197660f 100644
> > --- a/include/linux/quota.h
> > +++ b/include/linux/quota.h
> > @@ -294,6 +294,7 @@ struct dquot {
> >  	unsigned long dq_flags;		/* See DQ_* */
> >  	short dq_type;			/* Type of quota */
> >  	struct mem_dqblk dq_dqb;	/* Diskquota usage */
> > +	spinlock_t dq_lock;		/* protect in mem_dqblk */
> >  };
> >  
> >  /* Operations which must be implemented by each quota format */
> > -- 
> > 1.6.6.1
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html