From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 02/12] quota: Check what quota is properly initialized for inode before charge Date: Thu, 20 May 2010 20:13:33 +0200 Message-ID: <20100520181332.GN3395@quack.suse.cz> References: <1274248928-5113-1-git-send-email-dmonakhov@openvz.org> <1274248928-5113-2-git-send-email-dmonakhov@openvz.org> <1274248928-5113-3-git-send-email-dmonakhov@openvz.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, jack@suse.cz, hch@infradead.org To: Dmitry Monakhov Return-path: Received: from cantor2.suse.de ([195.135.220.15]:44781 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754837Ab0ETSNl (ORCPT ); Thu, 20 May 2010 14:13:41 -0400 Content-Disposition: inline In-Reply-To: <1274248928-5113-3-git-send-email-dmonakhov@openvz.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed 19-05-10 10:01:58, Dmitry Monakhov wrote: > Due to previous IO error or other bugs an inode may not has quota > pointer. This definite sign of quota inconsistency/corruption. > In fact this condition must being checked in all charge/uncharge > functions. And if error was detected it is reasonable to fail whole > operation. But uncharge(free_space/claim_space/free_inode) functions > has no fail nature. This is because caller can't handle errors from > such functions. How can you handle error from following call-trace? > write_page()->quota_claim_space() > > So alloc_space() and alloc_inode() are the only functions which we may > reliably abort in case of any errors. > > This patch add corresponding checks only for charge functions. Actually, I've realized that this patch has a problem if dquot_alloc_space is called while add_dquot_ref() is running. Then it would return EIO although it should just silently succeed (and yes, quotas will be inconsistent but that's "life" when quota isn't filesystem metadata...). So I don't think this is a move in the right direction at least for now... Honza > > Signed-off-by: Dmitry Monakhov > --- > fs/quota/dquot.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 95aa445..f0e68b3 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1541,7 +1541,7 @@ static void inode_decr_space(struct inode *inode, qsize_t number, int reserve) > int __dquot_alloc_space(struct inode *inode, qsize_t number, > int warn, int reserve) > { > - int cnt, ret = 0; > + int cnt, active, ret = 0; > char warntype[MAXQUOTAS]; > > /* > @@ -1554,13 +1554,28 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, > } > > down_read(&sb_dqopt(inode->i_sb)->dqptr_sem); > + /* Now recheck reliably when holding dqptr_sem */ > + active = sb_any_quota_active(inode->i_sb); > + if (!active || IS_NOQUOTA(inode)) { > + inode_incr_space(inode, number, reserve); > + goto out; > + } > + > for (cnt = 0; cnt < MAXQUOTAS; cnt++) > warntype[cnt] = QUOTA_NL_NOWARN; > > spin_lock(&dq_data_lock); > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > - if (!inode->i_dquot[cnt]) > + if (!(active & (1 << cnt))) > continue; > + /* > + * Given quota type is active, so quota must being > + * initialized for this inode > + */ > + if (!inode->i_dquot[cnt]) { > + ret = -EIO; > + goto out_flush_warn; > + } > ret = check_bdq(inode->i_dquot[cnt], number, !warn, > warntype+cnt); > if (ret) { > @@ -1569,7 +1584,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number, > } > } > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > - if (!inode->i_dquot[cnt]) > + if (!(active & (1 << cnt))) > continue; > if (reserve) > dquot_resv_space(inode->i_dquot[cnt], number); > @@ -1595,7 +1610,7 @@ EXPORT_SYMBOL(__dquot_alloc_space); > */ > int dquot_alloc_inode(const struct inode *inode) > { > - int cnt, ret = 0; > + int cnt, active, ret = 0; > char warntype[MAXQUOTAS]; > > /* First test before acquiring mutex - solves deadlocks when we > @@ -1605,17 +1620,31 @@ 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); > + /* Now recheck reliably when holding dqptr_sem */ > + active = sb_any_quota_active(inode->i_sb); > + if (!active || IS_NOQUOTA(inode)) { > + return 0; > + } > + > spin_lock(&dq_data_lock); > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > - if (!inode->i_dquot[cnt]) > + if (!(active & (1 < cnt))) > continue; > + /* > + * Given quota type is active, so quota must being > + * initialized for this inode > + */ > + if (!inode->i_dquot[cnt]) { > + ret = -EIO; > + goto warn_put_all; > + } > ret = check_idq(inode->i_dquot[cnt], 1, warntype + cnt); > if (ret) > goto warn_put_all; > } > > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > - if (!inode->i_dquot[cnt]) > + if ((!active & (1 < cnt))) > continue; > dquot_incr_inodes(inode->i_dquot[cnt], 1); > } > -- > 1.6.6.1 > -- Jan Kara SUSE Labs, CR