From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Date: Fri, 7 May 2010 18:48:10 +0200 Message-ID: <20100507164810.GG3700@quack.suse.cz> References: <1270749865-25441-1-git-send-email-dmonakhov@openvz.org> <1270749865-25441-2-git-send-email-dmonakhov@openvz.org> <1270749865-25441-3-git-send-email-dmonakhov@openvz.org> <1270749865-25441-4-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, sandeen@redhat.com To: Dmitry Monakhov Return-path: Received: from cantor.suse.de ([195.135.220.2]:54277 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751877Ab0EGQsO (ORCPT ); Fri, 7 May 2010 12:48:14 -0400 Content-Disposition: inline In-Reply-To: <1270749865-25441-4-git-send-email-dmonakhov@openvz.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 08-04-10 22:04:22, 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. > > Signed-off-by: Dmitry Monakhov > --- > fs/quota/dquot.c | 39 +++++++++++++++++++++++++++++++++------ > 1 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 3f4541e..7480e03 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1529,13 +1529,27 @@ 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 */ > + if (!(active = sb_any_quota_active(inode->i_sb)) || IS_NOQUOTA(inode)) { Please don't use assignment in conditions... It's against kernel's coding style. > + 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; Umm, I don't like this. I think we should just silently skip the quota modification. The error has already been passed to the filesystem during dquot_initialize and this would effectively disallow any allocation on the filesystem if quota structure isn't available which might be a bit impractical. Honza -- Jan Kara SUSE Labs, CR