From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Monakhov Subject: Re: [PATCH 5/5] quota: handle IO errors in dquot_transfer() Date: Tue, 15 Dec 2009 01:43:18 +0300 Message-ID: <87skbdyy61.fsf@openvz.org> References: <1260793276-8511-1-git-send-email-dmonakhov@openvz.org> <1260793276-8511-2-git-send-email-dmonakhov@openvz.org> <1260793276-8511-3-git-send-email-dmonakhov@openvz.org> <1260793276-8511-4-git-send-email-dmonakhov@openvz.org> <1260793276-8511-5-git-send-email-dmonakhov@openvz.org> <20091214184127.GH4731@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT Cc: linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from mail.2ka.mipt.ru ([194.85.80.4]:49641 "EHLO mail.2ka.mipt.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756684AbZLNWnY (ORCPT ); Mon, 14 Dec 2009 17:43:24 -0500 Received: from dmon-lp ([unknown] [10.55.93.124]) by mail.2ka.mipt.ru (Sun Java(tm) System Messaging Server 7u2-7.02 64bit (built Apr 16 2009)) with ESMTPA id <0KUN004JJZCCOG60@mail.2ka.mipt.ru> for linux-fsdevel@vger.kernel.org; Tue, 15 Dec 2009 01:48:15 +0300 (MSK) In-reply-to: <20091214184127.GH4731@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Jan Kara writes: > On Mon 14-12-09 15:21:16, Dmitry Monakhov wrote: >> transfer_to[cnt] = dqget() may returns NULL due to IO error. >> But NULL value in transfer_to[cnt] means a dquot transfer >> optimization. So after operation succeed inode will have new >> i_uid or i_gid but accounted to old dquot. This behaviour >> is differ from dquot_initialize(). Let's handle IO error from >> dqget() equally in all functions. >> >> In appliance to dquot_transfer() this means that we have to finish >> operation regardless to IO errors from dqget(). > In principle, the patch is fine (see just about a bug below). But even > better would be if you converted dquot_transfer() to actually return real > return codes (0, -EDQUOT, -EIO...) and make it return EIO in case of IO Actually we have following set of errors (0, -EDQUOT, -EIO, -ENOMEM, -ENOSPC) And ENOSPC is more realistic than EIO or ENOMEM. But from other point of view quota is some sort of fs meta-data, so we can not let it just *silently* fail because of some error as it done at the moment. > error. Then a filesystem can properly fail chown if quota transfer cannot > be performed. This is a larger project since filesystems using > dquot_transfer need to be changed and also other dquot_... functions need > to be converted to this calling convention for the sake of consistency. But > it would be a good cleanup. I'll handle it. > > ... >> for (cnt = 0; cnt < MAXQUOTAS; cnt++) { >> - transfer_from[cnt] = NULL; >> - transfer_to[cnt] = NULL; >> warntype_to[cnt] = QUOTA_NL_NOWARN; >> + if (!valid[cnt]) >> + continue; >> + transfer_to[cnt] = dqget(inode->i_sb, iattr->ia_uid, cnt); > This is wrong! You have to take ia_gid in case of GRPQUOTA. > >> @@ -1767,9 +1769,11 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr) >> space = cur_space + rsv_space; >> /* Build the transfer_from list and check the limits */ >> for (cnt = 0; cnt < MAXQUOTAS; cnt++) { >> - if (!transfer_to[cnt]) >> + if (!valid[cnt]) >> continue; >> transfer_from[cnt] = inode->i_dquot[cnt]; >> + if (!transfer_to[cnt]) >> + continue; >> if (check_idq(transfer_to[cnt], 1, warntype_to + cnt) == >> NO_QUOTA || check_bdq(transfer_to[cnt], space, 0, >> warntype_to + cnt) == NO_QUOTA) >> @@ -1783,10 +1787,15 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr) >> /* >> * Skip changes for same uid or gid or for turned off quota-type. >> */ >> - if (!transfer_to[cnt]) >> + if (!valid[cnt]) >> continue; >> + /* >> + * Due to IO error we might not have transfer_to[] or >> + * transfer_from[] structure. Nor than less the operation must > ^^ I suppose this should be > "Nonetheless" >> + * being done regardless quota io errors. >> + */ >> + inode->i_dquot[cnt] = transfer_to[cnt]; >> >> - /* Due to IO error we might not have transfer_from[] structure */ >> if (transfer_from[cnt]) { >> warntype_from_inodes[cnt] = >> info_idq_free(transfer_from[cnt], 1); >> @@ -1797,12 +1806,11 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr) >> dquot_free_reserved_space(transfer_from[cnt], >> rsv_space); >> } >> - >> - dquot_incr_inodes(transfer_to[cnt], 1); >> - dquot_incr_space(transfer_to[cnt], cur_space); >> - dquot_resv_space(transfer_to[cnt], rsv_space); >> - >> - inode->i_dquot[cnt] = transfer_to[cnt]; >> + if (transfer_to[cnt]) { >> + dquot_incr_inodes(transfer_to[cnt], 1); >> + dquot_incr_space(transfer_to[cnt], cur_space); >> + dquot_resv_space(transfer_to[cnt], rsv_space); >> + } >> } >> spin_unlock(&dq_data_lock); >> up_write(&sb_dqopt(inode->i_sb)->dqptr_sem); >> -- >> 1.6.0.4 >>