Skip to contentSkip to conversation

quota: handle io errors in dquot_transfer

PATCH In review Dmitry Monakhov 2 participants 4 messages Apr 5, 2010 – Apr 7, 2010

Subsystems (2)
VFS Diskquota
lore
Share & tools
lore

Dmitry Monakhov

author
#0 lore raw

PATCH 1/1 quota: handle io errors in dquot_transfer

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.

Quoted 1 line

From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001

From: Dmitry Monakhov <[email protected]>
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 <[email protected]>
---
 fs/quota/dquot.c |   35 +++++++++++++++++++----------------
 1 files changed, 19 insertions(+), 16 deletions(-)
fs/quota/dquot.c +19 -16
index 88a8ef2..324a0fb 100644
@@ -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() */
Signature · 1 line
1.6.6.1
reviewer
#1 lore raw
Quoted 4 lines · Dmitry Monakhov

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.

Quoted 8 lines

From 5e529864261c7bffe32a8b3d45d7b51749b4512f Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <[email protected]>
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.

Quoted 1 line

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

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author
#2 lore raw
Quoted 10 lines · Jan Kara
Quoted 9 lines · Dmitry Monakhov

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 <[email protected]>
Date: Mon, 5 Apr 2010 13:02:18 +0400
Subject: [PATCH] quota: handle io errors in dquot_transfer

Quoted 6 lines
Quoted 3 lines

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.

Quoted 4 lines
Quoted 1 line

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)

reviewer
#3 lore raw
Quoted 18 lines · Dmitry Monakhov
Quoted 10 lines · Jan Kara
Quoted 9 lines · Dmitry Monakhov

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 <[email protected]>
Date: Mon, 5 Apr 2010 13:02:18 +0400
Subject: [PATCH] quota: handle io errors in dquot_transfer

Quoted 6 lines
Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 11 lines
Quoted 4 lines
Quoted 1 line

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

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR