[PATCH] quota: handle io errors in dquot_transfer

[PATCH] quota: handle io errors in dquot_transfer

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 153 bytes --]

Currently quota is able to return real error code to caller.
Now it is possible to fix long standing bug with silent quota
corruption in dquot_transfer.

[-- Attachment #2: 0001-quota-handle-io-errors-in-dquot_transfer.patch --]
[-- Type: text/plain, Size: 2910 bytes --]

>From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Mon, 5 Apr 2010 13:02:18 +0400
Subject: [PATCH] quota: handle io errors in dquot_transfer

Currently if one of dquot structures absent due to some io errors
dquot_transfer will ignore corresponding quotatype. Which is very
bad because result in silent quota inconsistency.

Sane implementation must return corresponding error to caller.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/quota/dquot.c |   35 +++++++++++++++++++----------------
 1 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 88a8ef2..324a0fb 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1721,15 +1721,22 @@ static int __dquot_transfer(struct inode *inode, qid_t *chid, unsigned long mask
 	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 (!(mask & (1 << cnt)))
 			continue;
 		transfer_from[cnt] = inode->i_dquot[cnt];
+		/*
+		 * Due to IO error we might not have one of dquot structures.
+		 * If so, cancel whole procedure */
+		if (!transfer_from[cnt] || !transfer_to[cnt]) {
+			ret = -EIO;
+			goto bad_quota;
+		}
 		ret = check_idq(transfer_to[cnt], 1, warntype_to + cnt);
 		if (ret)
-			goto over_quota;
+			goto bad_quota;
 		ret = check_bdq(transfer_to[cnt], space, 0, warntype_to + cnt);
 		if (ret)
-			goto over_quota;
+			goto bad_quota;
 	}
 
 	/*
@@ -1739,20 +1746,16 @@ static int __dquot_transfer(struct inode *inode, qid_t *chid, unsigned long mask
 		/*
 		 * Skip changes for same uid or gid or for turned off quota-type.
 		 */
-		if (!transfer_to[cnt])
+		if (!(mask & (1 << cnt)))
 			continue;
+		warntype_from_inodes[cnt] =
+			info_idq_free(transfer_from[cnt], 1);
+		warntype_from_space[cnt] =
+			info_bdq_free(transfer_from[cnt], space);
 
-		/* 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);
-			warntype_from_space[cnt] =
-				info_bdq_free(transfer_from[cnt], space);
-			dquot_decr_inodes(transfer_from[cnt], 1);
-			dquot_decr_space(transfer_from[cnt], cur_space);
-			dquot_free_reserved_space(transfer_from[cnt],
-						  rsv_space);
-		}
+		dquot_decr_inodes(transfer_from[cnt], 1);
+		dquot_decr_space(transfer_from[cnt], cur_space);
+		dquot_free_reserved_space(transfer_from[cnt], rsv_space);
 
 		dquot_incr_inodes(transfer_to[cnt], 1);
 		dquot_incr_space(transfer_to[cnt], cur_space);
@@ -1776,7 +1779,7 @@ put_all:
 	dqput_all(transfer_from);
 	dqput_all(transfer_to);
 	return ret;
-over_quota:
+bad_quota:
 	spin_unlock(&dq_data_lock);
 	up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
 	/* Clear dquot pointers we don't want to dqput() */
-- 
1.6.6.1

Re: [PATCH] quota: handle io errors in dquot_transfer

[Jan Kara]

On Mon 05-04-10 13:44:54, Dmitry Monakhov wrote:
> Currently quota is able to return real error code to caller.
> Now it is possible to fix long standing bug with silent quota
> corruption in dquot_transfer.

> From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001
> From: Dmitry Monakhov <dmonakhov@openvz.org>
> Date: Mon, 5 Apr 2010 13:02:18 +0400
> Subject: [PATCH] quota: handle io errors in dquot_transfer
> 
> Currently if one of dquot structures absent due to some io errors
> dquot_transfer will ignore corresponding quotatype. Which is very
> bad because result in silent quota inconsistency.
  But because we were unable to read some quota structure, quota already
*is* inconsistent. So it's not like this particular operation would
introduce the inconsistency.

> Sane implementation must return corresponding error to caller.
  But sure I agree we should return the fact that we stumbled on quota
inconsistency the same way we do it for dquot_alloc_space or
dquot_free_space.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: handle io errors in dquot_transfer

[Dmitry Monakhov]

Jan Kara <jack@suse.cz> writes:

> On Mon 05-04-10 13:44:54, Dmitry Monakhov wrote:
>> Currently quota is able to return real error code to caller.
>> Now it is possible to fix long standing bug with silent quota
>> corruption in dquot_transfer.
>
>> From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001
>> From: Dmitry Monakhov <dmonakhov@openvz.org>
>> Date: Mon, 5 Apr 2010 13:02:18 +0400
>> Subject: [PATCH] quota: handle io errors in dquot_transfer
>> 
>> Currently if one of dquot structures absent due to some io errors
>> dquot_transfer will ignore corresponding quotatype. Which is very
>> bad because result in silent quota inconsistency.
>   But because we were unable to read some quota structure, quota already
> *is* inconsistent. So it's not like this particular operation would
> introduce the inconsistency.
Ohh... This is another type of error which is not handled at all.
dquot_initialize() may fail to read dquot from a disk and live
corresponding inode's structures semi-initialized. Before  Christoph's
cleanup it was almost impossible to solve this issue because init()
was called from semi_random places. I'm tried to make it one but
give up this idea due to lack of perspective.
The good things is what since fs itself is now responsible for dquot
init it is possible to solve this issue.
>> Sane implementation must return corresponding error to caller.
>   But sure I agree we should return the fact that we stumbled on quota
> inconsistency the same way we do it for dquot_alloc_space or
> dquot_free_space.
I'll combine init and transfer patches to one patchset.
The way i see it now:
1) dquot_initialize() return eio to caller
2) dquot_transfer() return eio to caller
3-5) dquot_(alloc/free/claim/...) must check that corresponding
     ->i_dquot[type] was initialized.
5) fs: handle error from dquot_initialize (this one will be huge)

Re: [PATCH] quota: handle io errors in dquot_transfer

[Jan Kara]

On Wed 07-04-10 11:22:00, Dmitry Monakhov wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > On Mon 05-04-10 13:44:54, Dmitry Monakhov wrote:
> >> Currently quota is able to return real error code to caller.
> >> Now it is possible to fix long standing bug with silent quota
> >> corruption in dquot_transfer.
> >
> >> From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001
> >> From: Dmitry Monakhov <dmonakhov@openvz.org>
> >> Date: Mon, 5 Apr 2010 13:02:18 +0400
> >> Subject: [PATCH] quota: handle io errors in dquot_transfer
> >> 
> >> Currently if one of dquot structures absent due to some io errors
> >> dquot_transfer will ignore corresponding quotatype. Which is very
> >> bad because result in silent quota inconsistency.
> >   But because we were unable to read some quota structure, quota already
> > *is* inconsistent. So it's not like this particular operation would
> > introduce the inconsistency.
> Ohh... This is another type of error which is not handled at all.
> dquot_initialize() may fail to read dquot from a disk and live
> corresponding inode's structures semi-initialized. Before  Christoph's
> cleanup it was almost impossible to solve this issue because init()
> was called from semi_random places. I'm tried to make it one but
> give up this idea due to lack of perspective.
> The good things is what since fs itself is now responsible for dquot
> init it is possible to solve this issue.
  Yes, it shouldn't be too hard to handle errors from dquot_init now.

> >> Sane implementation must return corresponding error to caller.
> >   But sure I agree we should return the fact that we stumbled on quota
> > inconsistency the same way we do it for dquot_alloc_space or
> > dquot_free_space.
> I'll combine init and transfer patches to one patchset.
> The way i see it now:
> 1) dquot_initialize() return eio to caller
> 2) dquot_transfer() return eio to caller
> 3-5) dquot_(alloc/free/claim/...) must check that corresponding
>      ->i_dquot[type] was initialized.
> 5) fs: handle error from dquot_initialize (this one will be huge)
  Yes, this looks like a good plan.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR