From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Monakhov Subject: Re: [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Date: Mon, 17 May 2010 10:22:54 +0400 Message-ID: <87vdan59oh.fsf@openvz.org> 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> <20100507164810.GG3700@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, hch@infradead.org, sandeen@redhat.com To: Jan Kara Return-path: Received: from mail-fx0-f46.google.com ([209.85.161.46]:51392 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753119Ab0EQGW6 (ORCPT ); Mon, 17 May 2010 02:22:58 -0400 Received: by fxm6 with SMTP id 6so3401250fxm.19 for ; Sun, 16 May 2010 23:22:57 -0700 (PDT) In-Reply-To: <20100507164810.GG3700@quack.suse.cz> (Jan Kara's message of "Fri, 7 May 2010 18:48:10 +0200") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Jan Kara writes: > 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. In fact in many places ret code from dquot_initialize() is simply ignored, because before this patch-set init was void, so it takes significant amount of time to fix all callers. And charge callers are usually do care about error code, so IMHO we have to signal what quota is broken if it is actually broken. Another example: If filesystem becomes RO (due to journal abort, and etc) we do care about this, and check that condition in almost all places and return appropriate error, instead of simply skip write() syscall. User is able to disable quota if it becomes broken, and continue with hope that this is just an quota issue, but not disk failure :)