All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] RFC quota: Redesign IO error handling interface
@ 2010-04-08 18:04 Dmitry Monakhov
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

This patchset is tended to provide interface for handling IO errors
from internal quota code.
Any error must being returned to fs-caller to signal about possible
quota inconsistency. I've done it in following way:

1) Handle low-level io errors from dqget() and it's callers
2) Handle errors from dquot_initialize
   This path catch most of IO error, but no all.
3) Check what i_dquot was initialized in each low-level function.
   There are two types of such functions
   3A) Charging functions (alloc_{space,inode}): Caller of such
       function may easy handle an error and abort an operation.
   3B) nofail functions (claim_space,free_{space,inode})
       In most cases caller can not abort an operation even if
       inode's quotas was semi-initialized, so I just skip this
       functions for now.
I would like to know you ideas suggestions about this.
Note: Only ext4's part was basically tested for now, others was just
compile tested.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/6] quota: unify quota init condition in setattr
  2010-04-08 18:04 [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
@ 2010-04-08 18:04 ` Dmitry Monakhov
  2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
                     ` (2 more replies)
  2010-05-05  8:45 ` [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
  2010-05-07 16:38 ` Jan Kara
  2 siblings, 3 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

Quota must being initialized if size or uid/git changes requested.
But initialization performed in two different places:
in case of i_size file system is responsible for dquot init
, but in case of uid/gid init will be called internally in
dquot_transfer().
This ambiguity makes code harder to understand.
Let's move this logic to one common helper function.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext2/inode.c          |    2 +-
 fs/ext3/inode.c          |    2 +-
 fs/ext4/inode.c          |    2 +-
 fs/jfs/file.c            |    2 +-
 fs/ocfs2/file.c          |    4 ++--
 fs/quota/dquot.c         |    5 ++---
 fs/reiserfs/inode.c      |    3 ++-
 fs/udf/file.c            |    2 +-
 fs/ufs/truncate.c        |    8 ++++----
 include/linux/quotaops.h |    8 ++++++++
 10 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 45ff49f..445f08a 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1460,7 +1460,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (error)
 		return error;
 
-	if (iattr->ia_valid & ATTR_SIZE)
+	if (is_quota_modification(inode, iattr))
 		dquot_initialize(inode);
 	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
 	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index ffbbc65..0981a27 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -3151,7 +3151,7 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (ia_valid & ATTR_SIZE)
+	if (is_quota_modification(inode, attr))
 		dquot_initialize(inode);
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4a91225..b812655 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5255,7 +5255,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (ia_valid & ATTR_SIZE)
+	if (is_quota_modification(inode, attr))
 		dquot_initialize(inode);
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 14ba982..85d9ec6 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -98,7 +98,7 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (rc)
 		return rc;
 
-	if (iattr->ia_valid & ATTR_SIZE)
+	if (is_quota_modification(inode, iattr))
 		dquot_initialize(inode);
 	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
 	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 3641052..b26df86 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -978,10 +978,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	if (status)
 		return status;
 
+	if (is_quota_modification(inode, attr))
+		dquot_initialize(inode);
 	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
 	if (size_change) {
-		dquot_initialize(inode);
-
 		status = ocfs2_rw_lock(inode, 1);
 		if (status < 0) {
 			mlog_errno(status);
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 8a2f0cb..a168189 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1801,10 +1801,9 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
 		mask |= 1 << GRPQUOTA;
 		chid[GRPQUOTA] = iattr->ia_gid;
 	}
-	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode)) {
-		dquot_initialize(inode);
+	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode))
 		return __dquot_transfer(inode, chid, mask);
-	}
+
 	return 0;
 }
 EXPORT_SYMBOL(dquot_transfer);
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index b8671a5..0bf5324 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3075,9 +3075,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
 	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
 
 	depth = reiserfs_write_lock_once(inode->i_sb);
-	if (attr->ia_valid & ATTR_SIZE) {
+	if (is_quota_modification(inode, attr))
 		dquot_initialize(inode);
 
+	if (attr->ia_valid & ATTR_SIZE) {
 		/* version 2 items will be caught by the s_maxbytes check
 		 ** done for us in vmtruncate
 		 */
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 4b6a46c..6ebc043 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -227,7 +227,7 @@ int udf_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (error)
 		return error;
 
-	if (iattr->ia_valid & ATTR_SIZE)
+	if (is_quota_modification(inode, iattr))
 		dquot_initialize(inode);
 
 	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c
index ee8db3e..f294c44 100644
--- a/fs/ufs/truncate.c
+++ b/fs/ufs/truncate.c
@@ -518,18 +518,18 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
+	if (is_quota_modification(inode, attr))
+		dquot_initialize(inode);
+
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 	    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
 		error = dquot_transfer(inode, attr);
 		if (error)
 			return error;
 	}
-	if (ia_valid & ATTR_SIZE &&
-	    attr->ia_size != i_size_read(inode)) {
+	if (ia_valid & ATTR_SIZE && attr->ia_size != inode->i_size) {
 		loff_t old_i_size = inode->i_size;
 
-		dquot_initialize(inode);
-
 		error = vmtruncate(inode, attr->ia_size);
 		if (error)
 			return error;
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index e6fa7ac..fd8f70b 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -14,6 +14,14 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
 	return &sb->s_dquot;
 }
 
+/* i_mutex must being held */
+static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
+{
+	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
+		(ia->ia_valid & ATTR_UID && ia->ia_uid != inode->i_uid) ||
+		(ia->ia_valid & ATTR_GID && ia->ia_gid != inode->i_gid);
+}
+
 #if defined(CONFIG_QUOTA)
 
 /*
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/6] quota: Add proper error handling on quota initialization.
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
@ 2010-04-08 18:04   ` Dmitry Monakhov
  2010-04-08 18:04     ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Dmitry Monakhov
  2010-05-07 16:44     ` [PATCH 2/6] quota: Add proper error handling on quota initialization Jan Kara
  2010-05-07 16:39   ` [PATCH 1/6] quota: unify quota init condition in setattr Jan Kara
  2010-05-13 16:29   ` Jan Kara
  2 siblings, 2 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

Currently dqget_initialize() is a black-box so IO errors are simply
ignored. In order to pass to the caller real error codes quota
interface must being redesigned in following way.

- return real error code from dqget()
- return real error code from dquot_initialize()

Now filesystem it is filesystem's responsibility to dquot_initialize()
return value.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ocfs2/file.c          |   18 ++++++--
 fs/ocfs2/quota_local.c   |   10 ++--
 fs/quota/dquot.c         |  101 +++++++++++++++++++++++++++++++---------------
 include/linux/quotaops.h |    5 +-
 4 files changed, 90 insertions(+), 44 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index b26df86..3c32f40 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1032,10 +1032,15 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 		    OCFS2_FEATURE_RO_COMPAT_USRQUOTA)) {
 			transfer_to[USRQUOTA] = dqget(sb, attr->ia_uid,
 						      USRQUOTA);
+			if (IS_ERR(transfer_to[USRQUOTA])) {
+				status = PTR_ERR(transfer_to[USRQUOTA]);
+				goto bail_unlock;
+			}
+
 			transfer_from[USRQUOTA] = dqget(sb, inode->i_uid,
 							USRQUOTA);
-			if (!transfer_to[USRQUOTA] || !transfer_from[USRQUOTA]) {
-				status = -ESRCH;
+			if (IS_ERR(transfer_from[USRQUOTA])) {
+				status = PTR_ERR(transfer_from[USRQUOTA]);
 				goto bail_unlock;
 			}
 		}
@@ -1044,10 +1049,15 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 		    OCFS2_FEATURE_RO_COMPAT_GRPQUOTA)) {
 			transfer_to[GRPQUOTA] = dqget(sb, attr->ia_gid,
 						      GRPQUOTA);
+			if (IS_ERR(transfer_to[GRPQUOTA])) {
+				status = PTR_ERR(transfer_to[GRPQUOTA]);
+				goto bail_unlock;
+			}
+
 			transfer_from[GRPQUOTA] = dqget(sb, inode->i_gid,
 							GRPQUOTA);
-			if (!transfer_to[GRPQUOTA] || !transfer_from[GRPQUOTA]) {
-				status = -ESRCH;
+			if (IS_ERR(transfer_from[GRPQUOTA])) {
+				status = PTR_ERR(transfer_from[GRPQUOTA]);
 				goto bail_unlock;
 			}
 		}
diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
index 21f9e71..6827ff6 100644
--- a/fs/ocfs2/quota_local.c
+++ b/fs/ocfs2/quota_local.c
@@ -469,13 +469,13 @@ static int ocfs2_recover_local_quota_file(struct inode *lqinode,
 			dqblk = (struct ocfs2_local_disk_dqblk *)(qbh->b_data +
 				ol_dqblk_block_off(sb, chunk, bit));
 			dquot = dqget(sb, le64_to_cpu(dqblk->dqb_id), type);
-			if (!dquot) {
-				status = -EIO;
+			if (IS_ERR(dquot)) {
+				status = PTR_ERR(dquot);
 				mlog(ML_ERROR, "Failed to get quota structure "
-				     "for id %u, type %d. Cannot finish quota "
-				     "file recovery.\n",
+				     "for id %u, type %d err %d. Cannot finish"
+				     " quota file recovery.\n",
 				     (unsigned)le64_to_cpu(dqblk->dqb_id),
-				     type);
+				     type, status);
 				goto out_put_bh;
 			}
 			status = ocfs2_lock_global_qf(oinfo, 1);
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index a168189..3f4541e 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -230,7 +230,7 @@ struct dqstats dqstats;
 EXPORT_SYMBOL(dqstats);
 
 static qsize_t inode_get_rsv_space(struct inode *inode);
-static void __dquot_initialize(struct inode *inode, int type);
+static int __dquot_initialize(struct inode *inode, int type);
 
 static inline unsigned int
 hashfn(const struct super_block *sb, unsigned int id, int type)
@@ -702,7 +702,7 @@ void dqput(struct dquot *dquot)
 {
 	int ret;
 
-	if (!dquot)
+	if (!dquot || IS_ERR(dquot))
 		return;
 #ifdef __DQUOT_PARANOIA
 	if (!atomic_read(&dquot->dq_count)) {
@@ -800,14 +800,16 @@ static struct dquot *get_empty_dquot(struct super_block *sb, int type)
  * destroying our dquot by:
  *   a) checking for quota flags under dq_list_lock and
  *   b) getting a reference to dquot before we release dq_list_lock
+ * Return: valid pointer on success, ERR_PTR otherwise.
  */
 struct dquot *dqget(struct super_block *sb, unsigned int id, int type)
 {
 	unsigned int hashent = hashfn(sb, id, type);
-	struct dquot *dquot = NULL, *empty = NULL;
+	struct dquot *dquot = ERR_PTR(-ESRCH);
+	struct dquot *empty = NULL;
 
         if (!sb_has_quota_active(sb, type))
-		return NULL;
+		goto out;
 we_slept:
 	spin_lock(&dq_list_lock);
 	spin_lock(&dq_state_lock);
@@ -848,11 +850,13 @@ we_slept:
 	 * already finished or it will be canceled due to dq_count > 1 test */
 	wait_on_dquot(dquot);
 	/* Read the dquot / allocate space in quota file */
-	if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags) &&
-	    sb->dq_op->acquire_dquot(dquot) < 0) {
-		dqput(dquot);
-		dquot = NULL;
-		goto out;
+	if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags)) {
+		int ret = sb->dq_op->acquire_dquot(dquot);
+		if (ret < 0) {
+			dqput(dquot);
+			dquot = ERR_PTR(ret);
+			goto out;
+		}
 	}
 #ifdef __DQUOT_PARANOIA
 	BUG_ON(!dquot->dq_sb);	/* Has somebody invalidated entry under us? */
@@ -1311,18 +1315,19 @@ static int info_bdq_free(struct dquot *dquot, qsize_t space)
  * It is better to call this function outside of any transaction as it
  * might need a lot of space in journal for dquot structure allocation.
  */
-static void __dquot_initialize(struct inode *inode, int type)
+static int __dquot_initialize(struct inode *inode, int type)
 {
 	unsigned int id = 0;
-	int cnt;
+	int cnt, err = 0;
 	struct dquot *got[MAXQUOTAS];
 	struct super_block *sb = inode->i_sb;
 	qsize_t rsv;
 
+repeat:
 	/* First test before acquiring mutex - solves deadlocks when we
          * re-enter the quota code and are already holding the mutex */
 	if (!sb_any_quota_active(inode->i_sb) || IS_NOQUOTA(inode))
-		return;
+		return 0;
 
 	/* First get references to structures we might need. */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
@@ -1341,35 +1346,50 @@ static void __dquot_initialize(struct inode *inode, int type)
 	}
 
 	down_write(&sb_dqopt(sb)->dqptr_sem);
-	if (IS_NOQUOTA(inode))
+	if (IS_NOQUOTA(inode)) {
+		err = 0;
 		goto out_err;
+	}
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (type != -1 && cnt != type)
 			continue;
 		/* Avoid races with quotaoff() */
 		if (!sb_has_quota_active(sb, cnt))
 			continue;
-		if (!inode->i_dquot[cnt]) {
-			inode->i_dquot[cnt] = got[cnt];
-			got[cnt] = NULL;
-			/*
-			 * Make quota reservation system happy if someone
-			 * did a write before quota was turned on
-			 */
-			rsv = inode_get_rsv_space(inode);
-			if (unlikely(rsv))
-				dquot_resv_space(inode->i_dquot[cnt], rsv);
+		if (inode->i_dquot[cnt])
+			continue;
+		if (unlikely(IS_ERR(got[cnt]))) {
+			err = (int)PTR_ERR(got[cnt]);
+			/* If dqget() was raced with quotaon() then we have to
+			 * repeat lookup. */
+			if (err == -ESRCH) {
+				err = 0;
+				up_write(&sb_dqopt(sb)->dqptr_sem);
+				dqput_all(got);
+				goto repeat;
+			}
+			goto out_err;
 		}
+		inode->i_dquot[cnt] = got[cnt];
+		got[cnt] = NULL;
+		/*
+		 * Make quota reservation system happy if someone
+		 * did a write before quota was turned on
+		 */
+		rsv = inode_get_rsv_space(inode);
+		if (unlikely(rsv))
+			dquot_resv_space(inode->i_dquot[cnt], rsv);
 	}
 out_err:
 	up_write(&sb_dqopt(sb)->dqptr_sem);
 	/* Drop unused references */
 	dqput_all(got);
+	return err;
 }
 
-void dquot_initialize(struct inode *inode)
+int dquot_initialize(struct inode *inode)
 {
-	__dquot_initialize(inode, -1);
+	return __dquot_initialize(inode, -1);
 }
 EXPORT_SYMBOL(dquot_initialize);
 
@@ -1696,6 +1716,7 @@ static int __dquot_transfer(struct inode *inode, qid_t *chid, unsigned long mask
 	char warntype_to[MAXQUOTAS];
 	char warntype_from_inodes[MAXQUOTAS], warntype_from_space[MAXQUOTAS];
 
+repeat:
 	/* First test before acquiring mutex - solves deadlocks when we
          * re-enter the quota code and are already holding the mutex */
 	if (IS_NOQUOTA(inode))
@@ -1721,15 +1742,29 @@ 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;
+		if (unlikely(IS_ERR(transfer_to[cnt]))) {
+			ret = (int)PTR_ERR(transfer_to[cnt]);
+			/* If dqget() was raced with quotaon() then we have to
+			 * repeat lookup. */
+			if (ret == -ESRCH) {
+				ret = 0;
+				spin_unlock(&dq_data_lock);
+				up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
+				dqput_all(transfer_to);
+				goto repeat;
+			}
+			goto bad_quota;
+		}
+
 		transfer_from[cnt] = inode->i_dquot[cnt];
 		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;
 	}
 
 	/*
@@ -1776,7 +1811,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() */
@@ -2302,8 +2337,8 @@ int vfs_get_dqblk(struct super_block *sb, int type, qid_t id,
 	struct dquot *dquot;
 
 	dquot = dqget(sb, id, type);
-	if (!dquot)
-		return -ESRCH;
+	if (IS_ERR(dquot))
+		return PTR_ERR(dquot);
 	do_get_dqblk(dquot, di);
 	dqput(dquot);
 
@@ -2396,8 +2431,8 @@ int vfs_set_dqblk(struct super_block *sb, int type, qid_t id,
 	int rc;
 
 	dquot = dqget(sb, id, type);
-	if (!dquot) {
-		rc = -ESRCH;
+	if (IS_ERR(dquot)) {
+		rc = PTR_ERR(dquot);
 		goto out;
 	}
 	rc = do_set_dqblk(dquot, di);
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index fd8f70b..6b50f98 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -31,7 +31,7 @@ void inode_add_rsv_space(struct inode *inode, qsize_t number);
 void inode_claim_rsv_space(struct inode *inode, qsize_t number);
 void inode_sub_rsv_space(struct inode *inode, qsize_t number);
 
-void dquot_initialize(struct inode *inode);
+int dquot_initialize(struct inode *inode);
 void dquot_drop(struct inode *inode);
 struct dquot *dqget(struct super_block *sb, unsigned int id, int type);
 void dqput(struct dquot *dquot);
@@ -206,8 +206,9 @@ static inline int sb_any_quota_active(struct super_block *sb)
 #define sb_dquot_ops				(NULL)
 #define sb_quotactl_ops				(NULL)
 
-static inline void dquot_initialize(struct inode *inode)
+static inline int dquot_initialize(struct inode *inode)
 {
+	return 0;
 }
 
 static inline void dquot_drop(struct inode *inode)
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge
  2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
@ 2010-04-08 18:04     ` Dmitry Monakhov
  2010-04-08 18:04       ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Dmitry Monakhov
  2010-05-07 16:48       ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Jan Kara
  2010-05-07 16:44     ` [PATCH 2/6] quota: Add proper error handling on quota initialization Jan Kara
  1 sibling, 2 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

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 <dmonakhov@openvz.org>
---
 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
@@ -1516,7 +1516,7 @@ static void inode_decr_space(struct inode *inode, qsize_t number, int reserve)
 int __dquot_alloc_space(struct inode *inode, qsize_t number,
 		int warn, int reserve)
 {
-	int cnt, ret = 0;
+	int cnt, active, ret = 0;
 	char warntype[MAXQUOTAS];
 
 	/*
@@ -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)) {
+		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;
+			goto out_flush_warn;
+		}
 		ret = check_bdq(inode->i_dquot[cnt], number, !warn,
 				warntype+cnt);
 		if (ret) {
@@ -1544,7 +1558,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
 		}
 	}
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		if (!inode->i_dquot[cnt])
+		if (!(active & (1 << cnt)))
 			continue;
 		if (reserve)
 			dquot_resv_space(inode->i_dquot[cnt], number);
@@ -1570,7 +1584,7 @@ EXPORT_SYMBOL(__dquot_alloc_space);
  */
 int dquot_alloc_inode(const struct inode *inode)
 {
-	int cnt, ret = 0;
+	int cnt, active, ret = 0;
 	char warntype[MAXQUOTAS];
 
 	/* First test before acquiring mutex - solves deadlocks when we
@@ -1580,17 +1594,30 @@ int dquot_alloc_inode(const struct inode *inode)
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
 		warntype[cnt] = QUOTA_NL_NOWARN;
 	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)) {
+		return 0;
+	}
+
 	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;
+			goto warn_put_all;
+		}
 		ret = check_idq(inode->i_dquot[cnt], 1, warntype + cnt);
 		if (ret)
 			goto warn_put_all;
 	}
 
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
-		if (!inode->i_dquot[cnt])
+		if ((!active & (1 < cnt)))
 			continue;
 		dquot_incr_inodes(inode->i_dquot[cnt], 1);
 	}
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/6] ext3: handle errors in orphan_cleanup
  2010-04-08 18:04     ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Dmitry Monakhov
@ 2010-04-08 18:04       ` Dmitry Monakhov
  2010-04-08 18:04         ` [PATCH 5/6] ext4: " Dmitry Monakhov
  2010-05-07 16:59         ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Jan Kara
  2010-05-07 16:48       ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Jan Kara
  1 sibling, 2 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

Orphan cleanup procedure is complex task and may fail due to number
of reasons. Handle errors from orphan_cleanp according to
predefined per-sb errors behavior flags.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext3/super.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index e844acc..72d979a 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -1422,23 +1422,24 @@ static int ext3_check_descriptors(struct super_block *sb)
  * e2fsck was run on this filesystem, and it must have already done the orphan
  * inode cleanup for us, so we can safely abort without any further action.
  */
-static void ext3_orphan_cleanup (struct super_block * sb,
+static int ext3_orphan_cleanup (struct super_block * sb,
 				 struct ext3_super_block * es)
 {
 	unsigned int s_flags = sb->s_flags;
 	int nr_orphans = 0, nr_truncates = 0;
+	int ret = 0;
 #ifdef CONFIG_QUOTA
 	int i;
 #endif
 	if (!es->s_last_orphan) {
 		jbd_debug(4, "no orphan inodes to clean up\n");
-		return;
+		return ret;
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
 		ext3_msg(sb, KERN_ERR, "error: write access "
 			"unavailable, skipping orphan cleanup.");
-		return;
+		return ret;
 	}
 
 	if (EXT3_SB(sb)->s_mount_state & EXT3_ERROR_FS) {
@@ -1447,7 +1448,7 @@ static void ext3_orphan_cleanup (struct super_block * sb,
 				  "clearing orphan list.\n");
 		es->s_last_orphan = 0;
 		jbd_debug(1, "Skipping orphan recovery on fs with errors.\n");
-		return;
+		return ret;
 	}
 
 	if (s_flags & MS_RDONLY) {
@@ -1460,11 +1461,14 @@ static void ext3_orphan_cleanup (struct super_block * sb,
 	/* Turn on quotas so that they are updated correctly */
 	for (i = 0; i < MAXQUOTAS; i++) {
 		if (EXT3_SB(sb)->s_qf_names[i]) {
-			int ret = ext3_quota_on_mount(sb, i);
-			if (ret < 0)
+			ret = ext3_quota_on_mount(sb, i);
+			if (ret < 0) {
 				ext3_msg(sb, KERN_ERR,
 					"error: cannot turn on journaled "
 					"quota: %d", ret);
+				if (!test_opt (sb, ERRORS_CONT))
+					goto out;
+			}
 		}
 	}
 #endif
@@ -1498,7 +1502,7 @@ static void ext3_orphan_cleanup (struct super_block * sb,
 		}
 		iput(inode);  /* The delete magic happens here! */
 	}
-
+out:
 #define PLURAL(x) (x), ((x)==1) ? "" : "s"
 
 	if (nr_orphans)
@@ -1507,6 +1511,9 @@ static void ext3_orphan_cleanup (struct super_block * sb,
 	if (nr_truncates)
 		ext3_msg(sb, KERN_INFO, "%d truncate%s cleaned up",
 		       PLURAL(nr_truncates));
+	if (ret)
+		ext3_msg(sb, KERN_ERR, "Error %d wile orphan cleanup", ret);
+
 #ifdef CONFIG_QUOTA
 	/* Turn quotas off */
 	for (i = 0; i < MAXQUOTAS; i++) {
@@ -1515,6 +1522,7 @@ static void ext3_orphan_cleanup (struct super_block * sb,
 	}
 #endif
 	sb->s_flags = s_flags; /* Restore MS_RDONLY status */
+	return ret;
 }
 
 /*
@@ -2019,7 +2027,11 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
 	ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY);
 
 	EXT3_SB(sb)->s_mount_state |= EXT3_ORPHAN_FS;
-	ext3_orphan_cleanup(sb, es);
+	ret = ext3_orphan_cleanup(sb, es);
+
+	if (ret)
+		goto failed_mount4;
+
 	EXT3_SB(sb)->s_mount_state &= ~EXT3_ORPHAN_FS;
 	if (needs_recovery)
 		ext3_msg(sb, KERN_INFO, "recovery complete");
@@ -2038,7 +2050,6 @@ cantfind_ext3:
 			"error: can't find ext3 filesystem on dev %s.",
 		       sb->s_id);
 	goto failed_mount;
-
 failed_mount4:
 	journal_destroy(sbi->s_journal);
 failed_mount3:
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/6] ext4: handle errors in orphan_cleanup
  2010-04-08 18:04       ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Dmitry Monakhov
@ 2010-04-08 18:04         ` Dmitry Monakhov
  2010-04-08 18:04           ` [PATCH 6/6] quota: check error code from dquot_initialize Dmitry Monakhov
  2010-05-07 16:59         ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Jan Kara
  1 sibling, 1 reply; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

Orphan cleanup procedure is complex task and may fail due to number
of reasons. Handle errors from orphan_cleanp according to
predefined per-sb errors behavior flags.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/super.c |   30 ++++++++++++++++++++++--------
 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index edcf3b0..230b71c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1873,23 +1873,24 @@ static int ext4_check_descriptors(struct super_block *sb)
  * e2fsck was run on this filesystem, and it must have already done the orphan
  * inode cleanup for us, so we can safely abort without any further action.
  */
-static void ext4_orphan_cleanup(struct super_block *sb,
+static int ext4_orphan_cleanup(struct super_block *sb,
 				struct ext4_super_block *es)
 {
 	unsigned int s_flags = sb->s_flags;
 	int nr_orphans = 0, nr_truncates = 0;
+	int ret = 0;
 #ifdef CONFIG_QUOTA
 	int i;
 #endif
 	if (!es->s_last_orphan) {
 		jbd_debug(4, "no orphan inodes to clean up\n");
-		return;
+		return 0;
 	}
 
 	if (bdev_read_only(sb->s_bdev)) {
 		ext4_msg(sb, KERN_ERR, "write access "
 			"unavailable, skipping orphan cleanup");
-		return;
+		return 0;
 	}
 
 	if (EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS) {
@@ -1898,7 +1899,7 @@ static void ext4_orphan_cleanup(struct super_block *sb,
 				  "clearing orphan list.\n");
 		es->s_last_orphan = 0;
 		jbd_debug(1, "Skipping orphan recovery on fs with errors.\n");
-		return;
+		return 0;
 	}
 
 	if (s_flags & MS_RDONLY) {
@@ -1911,11 +1912,14 @@ static void ext4_orphan_cleanup(struct super_block *sb,
 	/* Turn on quotas so that they are updated correctly */
 	for (i = 0; i < MAXQUOTAS; i++) {
 		if (EXT4_SB(sb)->s_qf_names[i]) {
-			int ret = ext4_quota_on_mount(sb, i);
-			if (ret < 0)
+			ret = ext4_quota_on_mount(sb, i);
+			if (ret < 0) {
 				ext4_msg(sb, KERN_ERR,
 					"Cannot turn on journaled "
 					"quota: error %d", ret);
+				if (!test_opt (sb, ERRORS_CONT))
+					goto out;
+			}
 		}
 	}
 #endif
@@ -1951,13 +1955,16 @@ static void ext4_orphan_cleanup(struct super_block *sb,
 	}
 
 #define PLURAL(x) (x), ((x) == 1) ? "" : "s"
-
+out:
 	if (nr_orphans)
 		ext4_msg(sb, KERN_INFO, "%d orphan inode%s deleted",
 		       PLURAL(nr_orphans));
 	if (nr_truncates)
 		ext4_msg(sb, KERN_INFO, "%d truncate%s cleaned up",
 		       PLURAL(nr_truncates));
+	if (ret)
+		ext4_msg(sb, KERN_ERR, "Error %d wile orphan cleanup", ret);
+
 #ifdef CONFIG_QUOTA
 	/* Turn quotas off */
 	for (i = 0; i < MAXQUOTAS; i++) {
@@ -1966,6 +1973,7 @@ static void ext4_orphan_cleanup(struct super_block *sb,
 	}
 #endif
 	sb->s_flags = s_flags; /* Restore MS_RDONLY status */
+	return ret;
 }
 
 /*
@@ -2915,7 +2923,13 @@ no_journal:
 	};
 
 	EXT4_SB(sb)->s_mount_state |= EXT4_ORPHAN_FS;
-	ext4_orphan_cleanup(sb, es);
+	ret = ext4_orphan_cleanup(sb, es);
+	if (err) {
+		ext4_mb_release(sb);
+		ext4_ext_release(sb);
+		goto failed_mount4;
+	}
+
 	EXT4_SB(sb)->s_mount_state &= ~EXT4_ORPHAN_FS;
 	if (needs_recovery) {
 		ext4_msg(sb, KERN_INFO, "recovery complete");
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/6] quota: check error code from dquot_initialize
  2010-04-08 18:04         ` [PATCH 5/6] ext4: " Dmitry Monakhov
@ 2010-04-08 18:04           ` Dmitry Monakhov
  2010-05-07 17:01             ` Jan Kara
  0 siblings, 1 reply; 16+ messages in thread
From: Dmitry Monakhov @ 2010-04-08 18:04 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack, hch, sandeen, Dmitry Monakhov

In most places quota initialization error may be handled trivially.
But xxx_delete_inode is another story. It can not fail.
So we may just print error and continue.


NOTE: Filesystem which has ondisk orphan list may add such quotaless
inodes to the list to let fsck to handle it later.
But still, if inode hasn't quota initialized at the moment of
->delete_inode() is definite sign of quota corruption so full
fsck, quotacheck is probably required.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext2/ialloc.c        |    5 +++-
 fs/ext2/inode.c         |   11 ++++++-
 fs/ext2/namei.c         |   40 ++++++++++++++++++++++-------
 fs/ext3/ialloc.c        |    6 +++-
 fs/ext3/inode.c         |   19 ++++++++++---
 fs/ext3/namei.c         |   54 +++++++++++++++++++++++++++++----------
 fs/ext3/super.c         |    9 ++++++-
 fs/ext4/ialloc.c        |    5 +++-
 fs/ext4/inode.c         |   18 ++++++++++---
 fs/ext4/namei.c         |   55 ++++++++++++++++++++++++++++++----------
 fs/ext4/super.c         |    9 ++++++-
 fs/jfs/file.c           |    7 +++-
 fs/jfs/jfs_inode.c      |    5 +++-
 fs/jfs/namei.c          |   59 +++++++++++++++++++++++++++++++-----------
 fs/ocfs2/file.c         |   16 ++++++++---
 fs/ocfs2/inode.c        |    5 +++-
 fs/ocfs2/namei.c        |   64 ++++++++++++++++++++++++++++++++++++----------
 fs/ocfs2/refcounttree.c |    5 ++-
 fs/reiserfs/inode.c     |   10 +++++--
 fs/reiserfs/namei.c     |   64 ++++++++++++++++++++++++++++++++++++-----------
 fs/reiserfs/super.c     |    3 +-
 fs/udf/file.c           |    6 +++-
 fs/udf/ialloc.c         |   18 ++++++++-----
 fs/udf/namei.c          |   39 +++++++++++++++++++++-------
 fs/ufs/ialloc.c         |    5 +++-
 fs/ufs/namei.c          |   46 ++++++++++++++++++++++++---------
 fs/ufs/truncate.c       |    6 +++-
 27 files changed, 440 insertions(+), 149 deletions(-)

diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
index ad7d572..e0d1581 100644
--- a/fs/ext2/ialloc.c
+++ b/fs/ext2/ialloc.c
@@ -586,7 +586,10 @@ got:
 		goto fail_drop;
 	}
 
-	dquot_initialize(inode);
+	err = dquot_initialize(inode);
+	if (err)
+		goto fail_drop;
+
 	err = dquot_alloc_inode(inode);
 	if (err)
 		goto fail_drop;
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 445f08a..bf67cbc 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -58,8 +58,15 @@ static inline int ext2_inode_is_fast_symlink(struct inode *inode)
  */
 void ext2_delete_inode (struct inode * inode)
 {
-	if (!is_bad_inode(inode))
-		dquot_initialize(inode);
+	if (!is_bad_inode(inode)) {
+		int ret = dquot_initialize(inode);
+		if (ret)
+			ext2_error(inode->i_sb, __func__, "Fail to init_quota "
+				"for inode %lu, uid %d, gid:%d, error%d, thus "
+				"quota information is probably inconsistent,  "
+				"Please run quotacheck(8)", inode->i_ino,
+				inode->i_uid, inode->i_gid, ret);
+	}
 	truncate_inode_pages(&inode->i_data, 0);
 
 	if (is_bad_inode(inode))
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 71efb0e..d47e549 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -101,8 +101,11 @@ struct dentry *ext2_get_parent(struct dentry *child)
 static int ext2_create (struct inode * dir, struct dentry * dentry, int mode, struct nameidata *nd)
 {
 	struct inode *inode;
+	int err;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode = ext2_new_inode(dir, mode);
 	if (IS_ERR(inode))
@@ -131,7 +134,9 @@ static int ext2_mknod (struct inode * dir, struct dentry *dentry, int mode, dev_
 	if (!new_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode = ext2_new_inode (dir, mode);
 	err = PTR_ERR(inode);
@@ -157,7 +162,9 @@ static int ext2_symlink (struct inode * dir, struct dentry * dentry,
 	if (l > sb->s_blocksize)
 		goto out;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode = ext2_new_inode (dir, S_IFLNK | S_IRWXUGO);
 	err = PTR_ERR(inode);
@@ -202,7 +209,9 @@ static int ext2_link (struct dentry * old_dentry, struct inode * dir,
 	if (inode->i_nlink >= EXT2_LINK_MAX)
 		return -EMLINK;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode->i_ctime = CURRENT_TIME_SEC;
 	inode_inc_link_count(inode);
@@ -226,7 +235,9 @@ static int ext2_mkdir(struct inode * dir, struct dentry * dentry, int mode)
 	if (dir->i_nlink >= EXT2_LINK_MAX)
 		goto out;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode_inc_link_count(dir);
 
@@ -274,7 +285,9 @@ static int ext2_unlink(struct inode * dir, struct dentry *dentry)
 	struct page * page;
 	int err = -ENOENT;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	de = ext2_find_entry (dir, &dentry->d_name, &page);
 	if (!de)
@@ -316,14 +329,21 @@ static int ext2_rename (struct inode * old_dir, struct dentry * old_dentry,
 	struct ext2_dir_entry_2 * dir_de = NULL;
 	struct page * old_page;
 	struct ext2_dir_entry_2 * old_de;
-	int err = -ENOENT;
+	int err;
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	err = dquot_initialize(old_dir);
+	if (err)
+		return err;
+
+	err = dquot_initialize(new_dir);
+	if (err)
+		return err;
 
 	old_de = ext2_find_entry (old_dir, &old_dentry->d_name, &old_page);
-	if (!old_de)
+	if (!old_de) {
+		err = -ENOENT;
 		goto out;
+	}
 
 	if (S_ISDIR(old_inode->i_mode)) {
 		err = -EIO;
diff --git a/fs/ext3/ialloc.c b/fs/ext3/ialloc.c
index ef9008b..15f6ec7 100644
--- a/fs/ext3/ialloc.c
+++ b/fs/ext3/ialloc.c
@@ -588,7 +588,11 @@ got:
 		sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE : 0;
 
 	ret = inode;
-	dquot_initialize(inode);
+
+	err = dquot_initialize(inode);
+	if (err)
+		goto fail_drop;
+
 	err = dquot_alloc_inode(inode);
 	if (err)
 		goto fail_drop;
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 0981a27..ed267ed 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -196,9 +196,15 @@ void ext3_delete_inode (struct inode * inode)
 {
 	handle_t *handle;
 
-	if (!is_bad_inode(inode))
-		dquot_initialize(inode);
-
+	if (!is_bad_inode(inode)) {
+		int ret = dquot_initialize(inode);
+		if (ret)
+			ext3_error(inode->i_sb, __func__, "Fail to init_quota "
+				"for inode %lu, uid %d, gid:%d, error%d, thus "
+				"quota information is probably inconsistent,  "
+				"Please run quotacheck(8)", inode->i_ino,
+				inode->i_uid, inode->i_gid, ret);
+	}
 	truncate_inode_pages(&inode->i_data, 0);
 
 	if (is_bad_inode(inode))
@@ -3151,8 +3157,11 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, attr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, attr)) {
+		error = dquot_initialize(inode);
+		if (error)
+			return error;
+	}
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
 		handle_t *handle;
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index ee18408..4813ebf 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1696,8 +1696,9 @@ static int ext3_create (struct inode * dir, struct dentry * dentry, int mode,
 	struct inode * inode;
 	int err, retries = 0;
 
-	dquot_initialize(dir);
-
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 retry:
 	handle = ext3_journal_start(dir, EXT3_DATA_TRANS_BLOCKS(dir->i_sb) +
 					EXT3_INDEX_EXTRA_TRANS_BLOCKS + 3 +
@@ -1732,7 +1733,9 @@ static int ext3_mknod (struct inode * dir, struct dentry *dentry,
 	if (!new_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext3_journal_start(dir, EXT3_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -1770,7 +1773,9 @@ static int ext3_mkdir(struct inode * dir, struct dentry * dentry, int mode)
 	if (dir->i_nlink >= EXT3_LINK_MAX)
 		return -EMLINK;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext3_journal_start(dir, EXT3_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -2066,8 +2071,13 @@ static int ext3_rmdir (struct inode * dir, struct dentry *dentry)
 
 	/* Initialize quotas before so that eventual writes go in
 	 * separate transaction */
-	dquot_initialize(dir);
-	dquot_initialize(dentry->d_inode);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(dentry->d_inode);
+	if (retval)
+		return retval;
 
 	handle = ext3_journal_start(dir, EXT3_DELETE_TRANS_BLOCKS(dir->i_sb));
 	if (IS_ERR(handle))
@@ -2127,8 +2137,13 @@ static int ext3_unlink(struct inode * dir, struct dentry *dentry)
 
 	/* Initialize quotas before so that eventual writes go
 	 * in separate transaction */
-	dquot_initialize(dir);
-	dquot_initialize(dentry->d_inode);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(dentry->d_inode);
+	if (retval)
+		return retval;
 
 	handle = ext3_journal_start(dir, EXT3_DELETE_TRANS_BLOCKS(dir->i_sb));
 	if (IS_ERR(handle))
@@ -2184,7 +2199,9 @@ static int ext3_symlink (struct inode * dir,
 	if (l > dir->i_sb->s_blocksize)
 		return -ENAMETOOLONG;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext3_journal_start(dir, EXT3_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -2241,7 +2258,9 @@ static int ext3_link (struct dentry * old_dentry,
 	if (inode->i_nlink >= EXT3_LINK_MAX)
 		return -EMLINK;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	/*
 	 * Return -ENOENT if we've raced with unlink and i_nlink is 0.  Doing
@@ -2293,15 +2312,22 @@ static int ext3_rename (struct inode * old_dir, struct dentry *old_dentry,
 	struct ext3_dir_entry_2 * old_de, * new_de;
 	int retval, flush_file = 0;
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	retval = dquot_initialize(old_dir);
+	if (retval)
+		return retval;
+	retval = dquot_initialize(new_dir);
+	if (retval)
+		return retval;
 
 	old_bh = new_bh = dir_bh = NULL;
 
 	/* Initialize quotas before so that eventual writes go
 	 * in separate transaction */
-	if (new_dentry->d_inode)
-		dquot_initialize(new_dentry->d_inode);
+	if (new_dentry->d_inode) {
+		retval = dquot_initialize(new_dentry->d_inode);
+		if (retval)
+			return retval;
+	}
 	handle = ext3_journal_start(old_dir, 2 *
 					EXT3_DATA_TRANS_BLOCKS(old_dir->i_sb) +
 					EXT3_INDEX_EXTRA_TRANS_BLOCKS + 2);
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 72d979a..f25bf61 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -1483,7 +1483,14 @@ static int ext3_orphan_cleanup (struct super_block * sb,
 		}
 
 		list_add(&EXT3_I(inode)->i_orphan, &EXT3_SB(sb)->s_orphan);
-		dquot_initialize(inode);
+		ret = dquot_initialize(inode);
+		if (ret) {
+			ext3_msg(sb, KERN_ERR, "Can not initialize quota for "
+				"inode:%lu error %d",
+				inode->i_ino, ret);
+			if (!test_opt (sb, ERRORS_CONT))
+				goto out;
+		}
 		if (inode->i_nlink) {
 			printk(KERN_DEBUG
 				"%s: truncating inode %lu to %Ld bytes\n",
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 9bb2bb9..139845f 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1034,7 +1034,10 @@ got:
 	ei->i_extra_isize = EXT4_SB(sb)->s_want_extra_isize;
 
 	ret = inode;
-	dquot_initialize(inode);
+	err = dquot_initialize(inode);
+	if(err)
+		goto fail_drop;
+
 	err = dquot_alloc_inode(inode);
 	if (err)
 		goto fail_drop;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b812655..2d04631 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -170,8 +170,15 @@ void ext4_delete_inode(struct inode *inode)
 	handle_t *handle;
 	int err;
 
-	if (!is_bad_inode(inode))
-		dquot_initialize(inode);
+	if (!is_bad_inode(inode)) {
+		int ret = dquot_initialize(inode);
+		if (ret)
+			ext4_error(inode->i_sb, __func__, "Fail to init_quota "
+				"for inode %lu, uid %d, gid:%d, error%d, thus "
+				"quota information is probably inconsistent,  "
+				"Please run quotacheck(8)", inode->i_ino,
+				inode->i_uid, inode->i_gid, ret);
+	}
 
 	if (ext4_should_order_data(inode))
 		ext4_begin_ordered_truncate(inode, 0);
@@ -5255,8 +5262,11 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, attr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, attr)) {
+		error = dquot_initialize(inode);
+		if (error)
+			return error;
+	}
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
 		handle_t *handle;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 7f3d2d7..275822d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1766,8 +1766,9 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, int mode,
 	struct inode *inode;
 	int err, retries = 0;
 
-	dquot_initialize(dir);
-
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 retry:
 	handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
 					EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 +
@@ -1802,7 +1803,9 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry,
 	if (!new_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -1841,7 +1844,9 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	if (EXT4_DIR_LINK_MAX(dir))
 		return -EMLINK;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -2142,8 +2147,13 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
 
 	/* Initialize quotas before so that eventual writes go in
 	 * separate transaction */
-	dquot_initialize(dir);
-	dquot_initialize(dentry->d_inode);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(dentry->d_inode);
+	if (retval)
+		return retval;
 
 	handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
 	if (IS_ERR(handle))
@@ -2203,8 +2213,13 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 
 	/* Initialize quotas before so that eventual writes go
 	 * in separate transaction */
-	dquot_initialize(dir);
-	dquot_initialize(dentry->d_inode);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(dentry->d_inode);
+	if (retval)
+		return retval;
 
 	handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb));
 	if (IS_ERR(handle))
@@ -2260,7 +2275,9 @@ static int ext4_symlink(struct inode *dir,
 	if (l > dir->i_sb->s_blocksize)
 		return -ENAMETOOLONG;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 retry:
 	handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
@@ -2320,7 +2337,9 @@ static int ext4_link(struct dentry *old_dentry,
 	if (inode->i_nlink >= EXT4_LINK_MAX)
 		return -EMLINK;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	/*
 	 * Return -ENOENT if we've raced with unlink and i_nlink is 0.  Doing
@@ -2372,15 +2391,23 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct ext4_dir_entry_2 *old_de, *new_de;
 	int retval, force_da_alloc = 0;
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	retval = dquot_initialize(old_dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(new_dir);
+	if (retval)
+		return retval;
 
 	old_bh = new_bh = dir_bh = NULL;
 
 	/* Initialize quotas before so that eventual writes go
 	 * in separate transaction */
-	if (new_dentry->d_inode)
-		dquot_initialize(new_dentry->d_inode);
+	if (new_dentry->d_inode) {
+		retval = dquot_initialize(new_dentry->d_inode);
+		if (retval)
+			return retval;
+	}
 	handle = ext4_journal_start(old_dir, 2 *
 					EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) +
 					EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 230b71c..c215d26 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1934,7 +1934,14 @@ static int ext4_orphan_cleanup(struct super_block *sb,
 		}
 
 		list_add(&EXT4_I(inode)->i_orphan, &EXT4_SB(sb)->s_orphan);
-		dquot_initialize(inode);
+		ret = dquot_initialize(inode);
+		if (ret) {
+			ext4_msg(sb, KERN_ERR, "Can not initialize quota for "
+				"inode:%lu error %d",
+				inode->i_ino, ret);
+			if (!test_opt (sb, ERRORS_CONT))
+				goto out;
+		}
 		if (inode->i_nlink) {
 			ext4_msg(sb, KERN_DEBUG,
 				"%s: truncating inode %lu to %lld bytes",
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 85d9ec6..cb0da27 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -98,8 +98,11 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (rc)
 		return rc;
 
-	if (is_quota_modification(inode, iattr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, iattr)) {
+		rc = dquot_initialize(inode);
+		if (rc)
+			return rc;
+	}
 	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
 	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
 		rc = dquot_transfer(inode, iattr);
diff --git a/fs/jfs/jfs_inode.c b/fs/jfs/jfs_inode.c
index 829921b..8d18c95 100644
--- a/fs/jfs/jfs_inode.c
+++ b/fs/jfs/jfs_inode.c
@@ -116,7 +116,10 @@ struct inode *ialloc(struct inode *parent, umode_t mode)
 	/*
 	 * Allocate inode to quota.
 	 */
-	dquot_initialize(inode);
+	rc = dquot_initialize(inode);
+	if (rc)
+		goto fail_unlock;
+
 	rc = dquot_alloc_inode(inode);
 	if (rc)
 		goto fail_drop;
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index a9cf8e8..96d40a6 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -85,7 +85,9 @@ static int jfs_create(struct inode *dip, struct dentry *dentry, int mode,
 
 	jfs_info("jfs_create: dip:0x%p name:%s", dip, dentry->d_name.name);
 
-	dquot_initialize(dip);
+	rc = dquot_initialize(dip);
+	if (rc)
+		goto out1;
 
 	/*
 	 * search parent directory for entry/freespace
@@ -217,7 +219,9 @@ static int jfs_mkdir(struct inode *dip, struct dentry *dentry, int mode)
 
 	jfs_info("jfs_mkdir: dip:0x%p name:%s", dip, dentry->d_name.name);
 
-	dquot_initialize(dip);
+	rc = dquot_initialize(dip);
+	if (rc)
+		goto out1;
 
 	/* link count overflow on parent directory ? */
 	if (dip->i_nlink == JFS_LINK_MAX) {
@@ -360,8 +364,12 @@ static int jfs_rmdir(struct inode *dip, struct dentry *dentry)
 	jfs_info("jfs_rmdir: dip:0x%p name:%s", dip, dentry->d_name.name);
 
 	/* Init inode for quota operations. */
-	dquot_initialize(dip);
-	dquot_initialize(ip);
+	rc = dquot_initialize(dip);
+	if (rc)
+		goto out;
+	rc = dquot_initialize(ip);
+	if (rc)
+		goto out;
 
 	/* directory must be empty to be removed */
 	if (!dtEmpty(ip)) {
@@ -488,8 +496,12 @@ static int jfs_unlink(struct inode *dip, struct dentry *dentry)
 	jfs_info("jfs_unlink: dip:0x%p name:%s", dip, dentry->d_name.name);
 
 	/* Init inode for quota operations. */
-	dquot_initialize(dip);
-	dquot_initialize(ip);
+	rc = dquot_initialize(dip);
+	if (rc)
+		goto out;
+	rc = dquot_initialize(ip);
+	if (rc)
+		goto out;
 
 	if ((rc = get_UCSname(&dname, dentry)))
 		goto out;
@@ -811,7 +823,9 @@ static int jfs_link(struct dentry *old_dentry,
 	if (ip->i_nlink == 0)
 		return -ENOENT;
 
-	dquot_initialize(dir);
+	rc = dquot_initialize(dir);
+	if (rc)
+		return rc;
 
 	tid = txBegin(ip->i_sb, 0);
 
@@ -904,7 +918,9 @@ static int jfs_symlink(struct inode *dip, struct dentry *dentry,
 
 	jfs_info("jfs_symlink: dip:0x%p name:%s", dip, name);
 
-	dquot_initialize(dip);
+	rc = dquot_initialize(dip);
+	if(rc)
+		goto out1;
 
 	ssize = strlen(name) + 1;
 
@@ -1097,12 +1113,24 @@ static int jfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	jfs_info("jfs_rename: %s %s", old_dentry->d_name.name,
 		 new_dentry->d_name.name);
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
-
 	old_ip = old_dentry->d_inode;
 	new_ip = new_dentry->d_inode;
 
+	/* Init inode for quota operations. */
+	rc = dquot_initialize(old_dir);
+	if (rc)
+		goto out1;
+
+	rc = dquot_initialize(new_dir);
+	if(rc)
+		goto out1;
+
+	if (new_ip) {
+		rc = dquot_initialize(new_ip);
+		if(rc)
+			goto out1;
+	}
+
 	if ((rc = get_UCSname(&old_dname, old_dentry)))
 		goto out1;
 
@@ -1146,11 +1174,8 @@ static int jfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			rc = -EMLINK;
 			goto out3;
 		}
-	} else if (new_ip) {
+	} else if (new_ip)
 		IWRITE_LOCK(new_ip, RDWRLOCK_NORMAL);
-		/* Init inode for quota operations. */
-		dquot_initialize(new_ip);
-	}
 
 	/*
 	 * The real work starts here
@@ -1373,7 +1398,9 @@ static int jfs_mknod(struct inode *dir, struct dentry *dentry,
 
 	jfs_info("jfs_mknod: %s", dentry->d_name.name);
 
-	dquot_initialize(dir);
+	rc = dquot_initialize(dir);
+	if (rc)
+		goto out;
 
 	if ((rc = get_UCSname(&dname, dentry)))
 		goto out;
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 3c32f40..cafcc59 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -107,9 +107,11 @@ static int ocfs2_file_open(struct inode *inode, struct file *file)
 	mlog_entry("(0x%p, 0x%p, '%.*s')\n", inode, file,
 		   file->f_path.dentry->d_name.len, file->f_path.dentry->d_name.name);
 
-	if (file->f_mode & FMODE_WRITE)
-		dquot_initialize(inode);
-
+	if (file->f_mode & FMODE_WRITE) {
+		status = dquot_initialize(inode);
+		if (status)
+			goto leave;
+	}
 	spin_lock(&oi->ip_lock);
 
 	/* Check that the inode hasn't been wiped from disk by another
@@ -978,8 +980,12 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
 	if (status)
 		return status;
 
-	if (is_quota_modification(inode, attr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, attr)) {
+		status = dquot_initialize(inode);
+		if (status)
+			return status;
+	}
+
 	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
 	if (size_change) {
 		status = ocfs2_rw_lock(inode, 1);
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 278a223..b8c3a14 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -971,7 +971,10 @@ void ocfs2_delete_inode(struct inode *inode)
 		goto bail;
 	}
 
-	dquot_initialize(inode);
+	status = dquot_initialize(inode);
+	/* Even when quota wasn't initialized we can't faile the operation */
+	if (status)
+		mlog_errno(status);
 
 	if (!ocfs2_inode_is_valid_to_delete(inode)) {
 		/* It's probably not necessary to truncate_inode_pages
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index d9cd4e3..a9f0f26 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -190,11 +190,12 @@ bail:
 static struct inode *ocfs2_get_init_inode(struct inode *dir, int mode)
 {
 	struct inode *inode;
+	int status;
 
 	inode = new_inode(dir->i_sb);
 	if (!inode) {
 		mlog(ML_ERROR, "new_inode failed!\n");
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 
 	/* populate as many fields early on as possible - many of
@@ -212,7 +213,14 @@ static struct inode *ocfs2_get_init_inode(struct inode *dir, int mode)
 	} else
 		inode->i_gid = current_fsgid();
 	inode->i_mode = mode;
-	dquot_initialize(inode);
+	status = dquot_initialize(inode);
+	if (status) {
+		clear_nlink(inode);
+		iput(inode);
+		inode = ERR_PTR(status);
+		mlog(ML_ERROR, "new_inode failed!\n");
+	}
+
 	return inode;
 }
 
@@ -244,7 +252,11 @@ static int ocfs2_mknod(struct inode *dir,
 		   (unsigned long)dev, dentry->d_name.len,
 		   dentry->d_name.name);
 
-	dquot_initialize(dir);
+	status = dquot_initialize(dir);
+	if (status) {
+		mlog_errno(status);
+		return status;
+	}
 
 	/* get our super block */
 	osb = OCFS2_SB(dir->i_sb);
@@ -291,9 +303,10 @@ static int ocfs2_mknod(struct inode *dir,
 	}
 
 	inode = ocfs2_get_init_inode(dir, mode);
-	if (!inode) {
-		status = -ENOMEM;
+	if (IS_ERR(inode)) {
+		status = PTR_ERR(inode);
 		mlog_errno(status);
+		inode = NULL;
 		goto leave;
 	}
 
@@ -634,7 +647,11 @@ static int ocfs2_link(struct dentry *old_dentry,
 	if (S_ISDIR(inode->i_mode))
 		return -EPERM;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err) {
+		mlog_errno(err);
+		return err;
+	}
 
 	err = ocfs2_inode_lock_nested(dir, &parent_fe_bh, 1, OI_LS_PARENT);
 	if (err < 0) {
@@ -791,7 +808,11 @@ static int ocfs2_unlink(struct inode *dir,
 	mlog_entry("(0x%p, 0x%p, '%.*s')\n", dir, dentry,
 		   dentry->d_name.len, dentry->d_name.name);
 
-	dquot_initialize(dir);
+	status = dquot_initialize(dir);
+	if (status) {
+		mlog_errno(status);
+		return status;
+	}
 
 	BUG_ON(dentry->d_parent->d_inode != dir);
 
@@ -1053,8 +1074,17 @@ static int ocfs2_rename(struct inode *old_dir,
 		   old_dentry->d_name.len, old_dentry->d_name.name,
 		   new_dentry->d_name.len, new_dentry->d_name.name);
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	status = dquot_initialize(old_dir);
+	if (status) {
+		mlog_errno(status);
+		return status;
+	}
+	status = dquot_initialize(new_dir);
+	if (status) {
+		mlog_errno(status);
+		return status;
+	}
+
 
 	osb = OCFS2_SB(old_dir->i_sb);
 
@@ -1604,7 +1634,11 @@ static int ocfs2_symlink(struct inode *dir,
 	mlog_entry("(0x%p, 0x%p, symname='%s' actual='%.*s')\n", dir,
 		   dentry, symname, dentry->d_name.len, dentry->d_name.name);
 
-	dquot_initialize(dir);
+	status = dquot_initialize(dir);
+	if (status) {
+		mlog_errno(status);
+		return status;
+	}
 
 	sb = dir->i_sb;
 	osb = OCFS2_SB(sb);
@@ -1649,9 +1683,10 @@ static int ocfs2_symlink(struct inode *dir,
 	}
 
 	inode = ocfs2_get_init_inode(dir, S_IFLNK | S_IRWXUGO);
-	if (!inode) {
-		status = -ENOMEM;
+	if (IS_ERR(inode)) {
+		status = PTR_ERR(inode);
 		mlog_errno(status);
+		inode = NULL;
 		goto bail;
 	}
 
@@ -2087,9 +2122,10 @@ int ocfs2_create_inode_in_orphan(struct inode *dir,
 	}
 
 	inode = ocfs2_get_init_inode(dir, mode);
-	if (!inode) {
-		status = -ENOMEM;
+	if (IS_ERR(inode)) {
+		status = PTR_ERR(inode);
 		mlog_errno(status);
+		inode = NULL;
 		goto leave;
 	}
 
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index f3ae10c..7d38db5 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4390,8 +4390,9 @@ static int ocfs2_vfs_reflink(struct dentry *old_dentry, struct inode *dir,
 	}
 
 	mutex_lock(&inode->i_mutex);
-	dquot_initialize(dir);
-	error = ocfs2_reflink(old_dentry, dir, new_dentry, preserve);
+	error = dquot_initialize(dir);
+	if (!error)
+		error = ocfs2_reflink(old_dentry, dir, new_dentry, preserve);
 	mutex_unlock(&inode->i_mutex);
 	if (!error)
 		fsnotify_create(dir, new_dentry);
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 0bf5324..43070d8 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -1768,7 +1768,9 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th,
 
 	BUG_ON(!th->t_trans_id);
 
-	dquot_initialize(inode);
+	err = dquot_initialize(inode);
+	if (err)
+		goto out_end_trans;
 	err = dquot_alloc_inode(inode);
 	if (err)
 		goto out_end_trans;
@@ -3075,8 +3077,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
 	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
 
 	depth = reiserfs_write_lock_once(inode->i_sb);
-	if (is_quota_modification(inode, attr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, attr)) {
+		error = dquot_initialize(inode);
+		goto out;
+	}
 
 	if (attr->ia_valid & ATTR_SIZE) {
 		/* version 2 items will be caught by the s_maxbytes check
diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index 96e4cbb..91474e3 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -577,8 +577,7 @@ static int new_inode_init(struct inode *inode, struct inode *dir, int mode)
 	} else {
 		inode->i_gid = current_fsgid();
 	}
-	dquot_initialize(inode);
-	return 0;
+	return dquot_initialize(inode);
 }
 
 static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
@@ -594,12 +593,18 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
 	struct reiserfs_transaction_handle th;
 	struct reiserfs_security_handle security;
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	if (!(inode = new_inode(dir->i_sb))) {
 		return -ENOMEM;
 	}
-	new_inode_init(inode, dir, mode);
+	retval = new_inode_init(inode, dir, mode);
+	if (retval) {
+		drop_new_inode(inode);
+		return retval;
+	}
 
 	jbegin_count += reiserfs_cache_default_acl(dir);
 	retval = reiserfs_security_init(dir, inode, &security);
@@ -668,12 +673,18 @@ static int reiserfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
 	if (!new_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	if (!(inode = new_inode(dir->i_sb))) {
 		return -ENOMEM;
 	}
-	new_inode_init(inode, dir, mode);
+	retval = new_inode_init(inode, dir, mode);
+	if (retval) {
+		drop_new_inode(inode);
+		return retval;
+	}
 
 	jbegin_count += reiserfs_cache_default_acl(dir);
 	retval = reiserfs_security_init(dir, inode, &security);
@@ -743,7 +754,9 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	    2 * (REISERFS_QUOTA_INIT_BLOCKS(dir->i_sb) +
 		 REISERFS_QUOTA_TRANS_BLOCKS(dir->i_sb));
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 #ifdef DISPLACE_NEW_PACKING_LOCALITIES
 	/* set flag that new packing locality created and new blocks for the content     * of that directory are not displaced yet */
@@ -753,7 +766,11 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	if (!(inode = new_inode(dir->i_sb))) {
 		return -ENOMEM;
 	}
-	new_inode_init(inode, dir, mode);
+	retval = new_inode_init(inode, dir, mode);
+	if (retval) {
+		drop_new_inode(inode);
+		return retval;
+	}
 
 	jbegin_count += reiserfs_cache_default_acl(dir);
 	retval = reiserfs_security_init(dir, inode, &security);
@@ -848,7 +865,9 @@ static int reiserfs_rmdir(struct inode *dir, struct dentry *dentry)
 	    JOURNAL_PER_BALANCE_CNT * 2 + 2 +
 	    4 * REISERFS_QUOTA_TRANS_BLOCKS(dir->i_sb);
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	reiserfs_write_lock(dir->i_sb);
 	retval = journal_begin(&th, dir->i_sb, jbegin_count);
@@ -931,7 +950,9 @@ static int reiserfs_unlink(struct inode *dir, struct dentry *dentry)
 	unsigned long savelink;
 	int depth;
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	inode = dentry->d_inode;
 
@@ -1034,12 +1055,18 @@ static int reiserfs_symlink(struct inode *parent_dir,
 	    2 * (REISERFS_QUOTA_INIT_BLOCKS(parent_dir->i_sb) +
 		 REISERFS_QUOTA_TRANS_BLOCKS(parent_dir->i_sb));
 
-	dquot_initialize(parent_dir);
+	retval = dquot_initialize(parent_dir);
+	if (retval)
+		return retval;
 
 	if (!(inode = new_inode(parent_dir->i_sb))) {
 		return -ENOMEM;
 	}
-	new_inode_init(inode, parent_dir, mode);
+	retval = new_inode_init(inode, parent_dir, mode);
+	if (retval) {
+		drop_new_inode(inode);
+		return retval;
+	}
 
 	retval = reiserfs_security_init(parent_dir, inode, &security);
 	if (retval < 0) {
@@ -1123,7 +1150,9 @@ static int reiserfs_link(struct dentry *old_dentry, struct inode *dir,
 	    JOURNAL_PER_BALANCE_CNT * 3 +
 	    2 * REISERFS_QUOTA_TRANS_BLOCKS(dir->i_sb);
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	reiserfs_write_lock(dir->i_sb);
 	if (inode->i_nlink >= REISERFS_LINK_MAX) {
@@ -1249,8 +1278,13 @@ static int reiserfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	    JOURNAL_PER_BALANCE_CNT * 3 + 5 +
 	    4 * REISERFS_QUOTA_TRANS_BLOCKS(old_dir->i_sb);
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	retval = dquot_initialize(old_dir);
+	if (retval)
+		return retval;
+
+	retval = dquot_initialize(new_dir);
+	if (retval)
+		return retval;
 
 	old_inode = old_dentry->d_inode;
 	new_dentry_inode = new_dentry->d_inode;
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 04bf5d7..afbe4d2 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -246,7 +246,8 @@ static int finish_unfinished(struct super_block *s)
 			retval = remove_save_link_only(s, &save_link_key, 0);
 			continue;
 		}
-		dquot_initialize(inode);
+		retval = dquot_initialize(inode);
+		/* XXX don't know what to do with error here */
 
 		if (truncate && S_ISDIR(inode->i_mode)) {
 			/* We got a truncate request for a dir which is impossible.
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 6ebc043..1ef70d6 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -227,8 +227,10 @@ int udf_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, iattr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, iattr)) {
+		error = dquot_initialize(inode);
+		return error;
+	}
 
 	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
             (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index fb68c9c..dc78998 100644
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -153,17 +153,21 @@ struct inode *udf_new_inode(struct inode *dir, int mode, int *err)
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
 
-	dquot_initialize(inode);
+	ret = dquot_initialize(inode);
+	if (ret)
+		goto out_put;
 	ret = dquot_alloc_inode(inode);
 	if (ret) {
 		dquot_drop(inode);
-		inode->i_flags |= S_NOQUOTA;
-		inode->i_nlink = 0;
-		iput(inode);
-		*err = ret;
-		return NULL;
+		goto out_put;
 	}
-
 	*err = 0;
 	return inode;
+out_put:
+	inode->i_flags |= S_NOQUOTA;
+	clear_nlink(inode);
+	iput(inode);
+	*err = ret;
+	return NULL;
+
 }
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 1fdbee2..21a59a1 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -563,7 +563,9 @@ static int udf_create(struct inode *dir, struct dentry *dentry, int mode,
 	int err;
 	struct udf_inode_info *iinfo;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	lock_kernel();
 	inode = udf_new_inode(dir, mode, &err);
@@ -618,7 +620,9 @@ static int udf_mknod(struct inode *dir, struct dentry *dentry, int mode,
 	if (!old_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	lock_kernel();
 	err = -EIO;
@@ -666,7 +670,9 @@ static int udf_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	struct udf_inode_info *dinfo = UDF_I(dir);
 	struct udf_inode_info *iinfo;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	lock_kernel();
 	err = -EMLINK;
@@ -805,7 +811,9 @@ static int udf_rmdir(struct inode *dir, struct dentry *dentry)
 	struct fileIdentDesc *fi, cfi;
 	struct kernel_lb_addr tloc;
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	retval = -ENOENT;
 	lock_kernel();
@@ -853,7 +861,9 @@ static int udf_unlink(struct inode *dir, struct dentry *dentry)
 	struct fileIdentDesc cfi;
 	struct kernel_lb_addr tloc;
 
-	dquot_initialize(dir);
+	retval = dquot_initialize(dir);
+	if (retval)
+		return retval;
 
 	retval = -ENOENT;
 	lock_kernel();
@@ -909,7 +919,9 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
 	struct buffer_head *bh;
 	struct udf_inode_info *iinfo;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	lock_kernel();
 	inode = udf_new_inode(dir, S_IFLNK, &err);
@@ -1081,7 +1093,9 @@ static int udf_link(struct dentry *old_dentry, struct inode *dir,
 	int err;
 	struct buffer_head *bh;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	lock_kernel();
 	if (inode->i_nlink >= (256 << sizeof(inode->i_nlink)) - 1) {
@@ -1141,13 +1155,18 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct fileIdentDesc *ofi = NULL, *nfi = NULL, *dir_fi = NULL;
 	struct fileIdentDesc ocfi, ncfi;
 	struct buffer_head *dir_bh = NULL;
-	int retval = -ENOENT;
+	int retval;
 	struct kernel_lb_addr tloc;
 	struct udf_inode_info *old_iinfo = UDF_I(old_inode);
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	retval = dquot_initialize(old_dir);
+	if (retval)
+		return retval;
+	retval = dquot_initialize(new_dir);
+	if (retval)
+		return retval;
 
+	retval = -ENOENT;
 	lock_kernel();
 	ofi = udf_find_entry(old_dir, &old_dentry->d_name, &ofibh, &ocfi);
 	if (ofi) {
diff --git a/fs/ufs/ialloc.c b/fs/ufs/ialloc.c
index 230ecf6..33e6fab 100644
--- a/fs/ufs/ialloc.c
+++ b/fs/ufs/ialloc.c
@@ -355,7 +355,10 @@ cg_found:
 
 	unlock_super (sb);
 
-	dquot_initialize(inode);
+	err = dquot_initialize(inode);
+	if (err)
+		goto fail_without_unlock;
+
 	err = dquot_alloc_inode(inode);
 	if (err) {
 		dquot_drop(inode);
diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c
index eabc02e..94edec7 100644
--- a/fs/ufs/namei.c
+++ b/fs/ufs/namei.c
@@ -86,7 +86,9 @@ static int ufs_create (struct inode * dir, struct dentry * dentry, int mode,
 
 	UFSD("BEGIN\n");
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode = ufs_new_inode(dir, mode);
 	err = PTR_ERR(inode);
@@ -112,7 +114,9 @@ static int ufs_mknod (struct inode * dir, struct dentry *dentry, int mode, dev_t
 	if (!old_valid_dev(rdev))
 		return -EINVAL;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		return err;
 
 	inode = ufs_new_inode(dir, mode);
 	err = PTR_ERR(inode);
@@ -138,7 +142,9 @@ static int ufs_symlink (struct inode * dir, struct dentry * dentry,
 	if (l > sb->s_blocksize)
 		goto out_notlocked;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		goto out;
 
 	lock_kernel();
 	inode = ufs_new_inode(dir, S_IFLNK | S_IRWXUGO);
@@ -181,17 +187,20 @@ static int ufs_link (struct dentry * old_dentry, struct inode * dir,
 
 	lock_kernel();
 	if (inode->i_nlink >= UFS_LINK_MAX) {
-		unlock_kernel();
-		return -EMLINK;
+		error = -EMLINK;
+		goto out_unlock;
 	}
 
-	dquot_initialize(dir);
+	error = dquot_initialize(dir);
+	if (error)
+		goto out_unlock;
 
 	inode->i_ctime = CURRENT_TIME_SEC;
 	inode_inc_link_count(inode);
 	atomic_inc(&inode->i_count);
 
 	error = ufs_add_nondir(dentry, inode);
+out_unlock:
 	unlock_kernel();
 	return error;
 }
@@ -204,7 +213,9 @@ static int ufs_mkdir(struct inode * dir, struct dentry * dentry, int mode)
 	if (dir->i_nlink >= UFS_LINK_MAX)
 		goto out;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		goto out;
 
 	lock_kernel();
 	inode_inc_link_count(dir);
@@ -248,10 +259,13 @@ static int ufs_unlink(struct inode *dir, struct dentry *dentry)
 	struct inode * inode = dentry->d_inode;
 	struct ufs_dir_entry *de;
 	struct page *page;
-	int err = -ENOENT;
+	int err;
 
-	dquot_initialize(dir);
+	err = dquot_initialize(dir);
+	if (err)
+		goto out;
 
+	err = -ENOENT;
 	de = ufs_find_entry(dir, &dentry->d_name, &page);
 	if (!de)
 		goto out;
@@ -294,14 +308,20 @@ static int ufs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct ufs_dir_entry * dir_de = NULL;
 	struct page *old_page;
 	struct ufs_dir_entry *old_de;
-	int err = -ENOENT;
+	int err;
 
-	dquot_initialize(old_dir);
-	dquot_initialize(new_dir);
+	err = dquot_initialize(old_dir);
+	if (err)
+		goto out;
+	err = dquot_initialize(new_dir);
+	if (err)
+		goto out;
 
 	old_de = ufs_find_entry(old_dir, &old_dentry->d_name, &old_page);
-	if (!old_de)
+	if (!old_de) {
+		err = -ENOENT;
 		goto out;
+	}
 
 	if (S_ISDIR(old_inode->i_mode)) {
 		err = -EIO;
diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c
index f294c44..2de89ee 100644
--- a/fs/ufs/truncate.c
+++ b/fs/ufs/truncate.c
@@ -518,8 +518,10 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, attr))
-		dquot_initialize(inode);
+	if (is_quota_modification(inode, attr)) {
+		error = dquot_initialize(inode);
+		return error;
+	}
 
 	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 	    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6] RFC quota: Redesign IO error handling interface
  2010-04-08 18:04 [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
@ 2010-05-05  8:45 ` Dmitry Monakhov
  2010-05-07 16:38 ` Jan Kara
  2 siblings, 0 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-05-05  8:45 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: jack


Jan, can you please take a look at following patch set
it starts here: http://marc.info/?l=linux-fsdevel&m=127074989804295&w=2
Do you agree with approach?
Would you accept updated version for recent merge window?
Dmitry Monakhov <dmonakhov@openvz.org> writes:
> This patchset is tended to provide interface for handling IO errors
> from internal quota code.
> Any error must being returned to fs-caller to signal about possible
> quota inconsistency. I've done it in following way:
>
> 1) Handle low-level io errors from dqget() and it's callers
> 2) Handle errors from dquot_initialize
>    This path catch most of IO error, but no all.
> 3) Check what i_dquot was initialized in each low-level function.
>    There are two types of such functions
>    3A) Charging functions (alloc_{space,inode}): Caller of such
>        function may easy handle an error and abort an operation.
>    3B) nofail functions (claim_space,free_{space,inode})
>        In most cases caller can not abort an operation even if
>        inode's quotas was semi-initialized, so I just skip this
>        functions for now.
> I would like to know you ideas suggestions about this.
> Note: Only ext4's part was basically tested for now, others was just
> compile tested.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6] RFC quota: Redesign IO error handling interface
  2010-04-08 18:04 [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
  2010-05-05  8:45 ` [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
@ 2010-05-07 16:38 ` Jan Kara
  2 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-07 16:38 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:19, Dmitry Monakhov wrote:
> This patchset is tended to provide interface for handling IO errors
> from internal quota code.
> Any error must being returned to fs-caller to signal about possible
> quota inconsistency. I've done it in following way:
> 
> 1) Handle low-level io errors from dqget() and it's callers
> 2) Handle errors from dquot_initialize
>    This path catch most of IO error, but no all.
> 3) Check what i_dquot was initialized in each low-level function.
>    There are two types of such functions
>    3A) Charging functions (alloc_{space,inode}): Caller of such
>        function may easy handle an error and abort an operation.
>    3B) nofail functions (claim_space,free_{space,inode})
>        In most cases caller can not abort an operation even if
>        inode's quotas was semi-initialized, so I just skip this
>        functions for now.
> I would like to know you ideas suggestions about this.
> Note: Only ext4's part was basically tested for now, others was just
> compile tested.
  Sorry for not replying earlier. I've glanced over the patch set and I
have no major objections. I'll send you now the problems I've found and
next week I will have a more detailed look.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/6] quota: unify quota init condition in setattr
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
  2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
@ 2010-05-07 16:39   ` Jan Kara
  2010-05-13 16:29   ` Jan Kara
  2 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-07 16:39 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:20, Dmitry Monakhov wrote:
> Quota must being initialized if size or uid/git changes requested.
> But initialization performed in two different places:
> in case of i_size file system is responsible for dquot init
> , but in case of uid/gid init will be called internally in
> dquot_transfer().
> This ambiguity makes code harder to understand.
> Let's move this logic to one common helper function.
  This looks OK.

								Honza
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext2/inode.c          |    2 +-
>  fs/ext3/inode.c          |    2 +-
>  fs/ext4/inode.c          |    2 +-
>  fs/jfs/file.c            |    2 +-
>  fs/ocfs2/file.c          |    4 ++--
>  fs/quota/dquot.c         |    5 ++---
>  fs/reiserfs/inode.c      |    3 ++-
>  fs/udf/file.c            |    2 +-
>  fs/ufs/truncate.c        |    8 ++++----
>  include/linux/quotaops.h |    8 ++++++++
>  10 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 45ff49f..445f08a 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1460,7 +1460,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (error)
>  		return error;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
>  	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index ffbbc65..0981a27 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -3151,7 +3151,7 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> -	if (ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4a91225..b812655 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5255,7 +5255,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> -	if (ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 14ba982..85d9ec6 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -98,7 +98,7 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (rc)
>  		return rc;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
>  	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 3641052..b26df86 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -978,10 +978,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (status)
>  		return status;
>  
> +	if (is_quota_modification(inode, attr))
> +		dquot_initialize(inode);
>  	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
>  	if (size_change) {
> -		dquot_initialize(inode);
> -
>  		status = ocfs2_rw_lock(inode, 1);
>  		if (status < 0) {
>  			mlog_errno(status);
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 8a2f0cb..a168189 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1801,10 +1801,9 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
>  		mask |= 1 << GRPQUOTA;
>  		chid[GRPQUOTA] = iattr->ia_gid;
>  	}
> -	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode)) {
> -		dquot_initialize(inode);
> +	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode))
>  		return __dquot_transfer(inode, chid, mask);
> -	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(dquot_transfer);
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index b8671a5..0bf5324 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -3075,9 +3075,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
>  	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
>  
>  	depth = reiserfs_write_lock_once(inode->i_sb);
> -	if (attr->ia_valid & ATTR_SIZE) {
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  
> +	if (attr->ia_valid & ATTR_SIZE) {
>  		/* version 2 items will be caught by the s_maxbytes check
>  		 ** done for us in vmtruncate
>  		 */
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index 4b6a46c..6ebc043 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -227,7 +227,7 @@ int udf_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (error)
>  		return error;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
> diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c
> index ee8db3e..f294c44 100644
> --- a/fs/ufs/truncate.c
> +++ b/fs/ufs/truncate.c
> @@ -518,18 +518,18 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> +	if (is_quota_modification(inode, attr))
> +		dquot_initialize(inode);
> +
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  	    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
>  		error = dquot_transfer(inode, attr);
>  		if (error)
>  			return error;
>  	}
> -	if (ia_valid & ATTR_SIZE &&
> -	    attr->ia_size != i_size_read(inode)) {
> +	if (ia_valid & ATTR_SIZE && attr->ia_size != inode->i_size) {
>  		loff_t old_i_size = inode->i_size;
>  
> -		dquot_initialize(inode);
> -
>  		error = vmtruncate(inode, attr->ia_size);
>  		if (error)
>  			return error;
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index e6fa7ac..fd8f70b 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -14,6 +14,14 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>  	return &sb->s_dquot;
>  }
>  
> +/* i_mutex must being held */
> +static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> +{
> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> +		(ia->ia_valid & ATTR_UID && ia->ia_uid != inode->i_uid) ||
> +		(ia->ia_valid & ATTR_GID && ia->ia_gid != inode->i_gid);
> +}
> +
>  #if defined(CONFIG_QUOTA)
>  
>  /*
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] quota: Add proper error handling on quota initialization.
  2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
  2010-04-08 18:04     ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Dmitry Monakhov
@ 2010-05-07 16:44     ` Jan Kara
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-07 16:44 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:21, Dmitry Monakhov wrote:
> Currently dqget_initialize() is a black-box so IO errors are simply
> ignored. In order to pass to the caller real error codes quota
> interface must being redesigned in following way.
> 
> - return real error code from dqget()
> - return real error code from dquot_initialize()
> 
> Now filesystem it is filesystem's responsibility to dquot_initialize()
> return value.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ocfs2/file.c          |   18 ++++++--
>  fs/ocfs2/quota_local.c   |   10 ++--
>  fs/quota/dquot.c         |  101 +++++++++++++++++++++++++++++++---------------
>  include/linux/quotaops.h |    5 +-
>  4 files changed, 90 insertions(+), 44 deletions(-)
> 
...
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>  		if (type != -1 && cnt != type)
>  			continue;
>  		/* Avoid races with quotaoff() */
>  		if (!sb_has_quota_active(sb, cnt))
>  			continue;
> -		if (!inode->i_dquot[cnt]) {
> -			inode->i_dquot[cnt] = got[cnt];
> -			got[cnt] = NULL;
> -			/*
> -			 * Make quota reservation system happy if someone
> -			 * did a write before quota was turned on
> -			 */
> -			rsv = inode_get_rsv_space(inode);
> -			if (unlikely(rsv))
> -				dquot_resv_space(inode->i_dquot[cnt], rsv);
> +		if (inode->i_dquot[cnt])
> +			continue;
> +		if (unlikely(IS_ERR(got[cnt]))) {
> +			err = (int)PTR_ERR(got[cnt]);
                              ^^^^ Why is (int) needed?

> @@ -1721,15 +1742,29 @@ 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;
> +		if (unlikely(IS_ERR(transfer_to[cnt]))) {
> +			ret = (int)PTR_ERR(transfer_to[cnt]);
                              ^^^^^ And here as well.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge
  2010-04-08 18:04     ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Dmitry Monakhov
  2010-04-08 18:04       ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Dmitry Monakhov
@ 2010-05-07 16:48       ` Jan Kara
  2010-05-17  6:22         ` Dmitry Monakhov
  1 sibling, 1 reply; 16+ messages in thread
From: Jan Kara @ 2010-05-07 16:48 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

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 <dmonakhov@openvz.org>
> ---
>  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 <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6] ext3: handle errors in orphan_cleanup
  2010-04-08 18:04       ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Dmitry Monakhov
  2010-04-08 18:04         ` [PATCH 5/6] ext4: " Dmitry Monakhov
@ 2010-05-07 16:59         ` Jan Kara
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-07 16:59 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:23, Dmitry Monakhov wrote:
> Orphan cleanup procedure is complex task and may fail due to number
> of reasons. Handle errors from orphan_cleanp according to
> predefined per-sb errors behavior flags.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext3/super.c |   29 ++++++++++++++++++++---------
>  1 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index e844acc..72d979a 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -1460,11 +1461,14 @@ static void ext3_orphan_cleanup (struct super_block * sb,
>  	/* Turn on quotas so that they are updated correctly */
>  	for (i = 0; i < MAXQUOTAS; i++) {
>  		if (EXT3_SB(sb)->s_qf_names[i]) {
> -			int ret = ext3_quota_on_mount(sb, i);
> -			if (ret < 0)
> +			ret = ext3_quota_on_mount(sb, i);
> +			if (ret < 0) {
>  				ext3_msg(sb, KERN_ERR,
>  					"error: cannot turn on journaled "
>  					"quota: %d", ret);
> +				if (!test_opt (sb, ERRORS_CONT))
> +					goto out;
> +			}
  I see two consistent approaches:
a) consider the filesystem is mounted and use ext3_handle_error()
b) consider this a part of mounting procedure and return error and fail the
   mount.

  I would prefer b) but if you have some good arguments for a)...

>  		}
>  	}
>  #endif
...
> @@ -1507,6 +1511,9 @@ static void ext3_orphan_cleanup (struct super_block * sb,
>  	if (nr_truncates)
>  		ext3_msg(sb, KERN_INFO, "%d truncate%s cleaned up",
>  		       PLURAL(nr_truncates));
> +	if (ret)
> +		ext3_msg(sb, KERN_ERR, "Error %d wile orphan cleanup", ret);
                                                 ^^^ while

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6] quota: check error code from dquot_initialize
  2010-04-08 18:04           ` [PATCH 6/6] quota: check error code from dquot_initialize Dmitry Monakhov
@ 2010-05-07 17:01             ` Jan Kara
  0 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-07 17:01 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:25, Dmitry Monakhov wrote:
> In most places quota initialization error may be handled trivially.
> But xxx_delete_inode is another story. It can not fail.
> So we may just print error and continue.
> 
> 
> NOTE: Filesystem which has ondisk orphan list may add such quotaless
> inodes to the list to let fsck to handle it later.
> But still, if inode hasn't quota initialized at the moment of
> ->delete_inode() is definite sign of quota corruption so full
> fsck, quotacheck is probably required.
  Please split this to per-fs patches. They can be merged through
fs-specific trees and can get review from respective filesystem
maintainers.

								Honza

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/6] quota: unify quota init condition in setattr
  2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
  2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
  2010-05-07 16:39   ` [PATCH 1/6] quota: unify quota init condition in setattr Jan Kara
@ 2010-05-13 16:29   ` Jan Kara
  2 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2010-05-13 16:29 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-fsdevel, jack, hch, sandeen

On Thu 08-04-10 22:04:20, Dmitry Monakhov wrote:
> Quota must being initialized if size or uid/git changes requested.
> But initialization performed in two different places:
> in case of i_size file system is responsible for dquot init
> , but in case of uid/gid init will be called internally in
> dquot_transfer().
> This ambiguity makes code harder to understand.
> Let's move this logic to one common helper function.
  Dmitry, I'm fixing some OCFS2 quota bugs and this patch helps it as well
so I took it into my tree.

									Honza
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext2/inode.c          |    2 +-
>  fs/ext3/inode.c          |    2 +-
>  fs/ext4/inode.c          |    2 +-
>  fs/jfs/file.c            |    2 +-
>  fs/ocfs2/file.c          |    4 ++--
>  fs/quota/dquot.c         |    5 ++---
>  fs/reiserfs/inode.c      |    3 ++-
>  fs/udf/file.c            |    2 +-
>  fs/ufs/truncate.c        |    8 ++++----
>  include/linux/quotaops.h |    8 ++++++++
>  10 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 45ff49f..445f08a 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1460,7 +1460,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (error)
>  		return error;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
>  	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index ffbbc65..0981a27 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -3151,7 +3151,7 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> -	if (ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4a91225..b812655 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5255,7 +5255,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> -	if (ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  		(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 14ba982..85d9ec6 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -98,7 +98,7 @@ int jfs_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (rc)
>  		return rc;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
>  	    (iattr->ia_valid & ATTR_GID && iattr->ia_gid != inode->i_gid)) {
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 3641052..b26df86 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -978,10 +978,10 @@ int ocfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (status)
>  		return status;
>  
> +	if (is_quota_modification(inode, attr))
> +		dquot_initialize(inode);
>  	size_change = S_ISREG(inode->i_mode) && attr->ia_valid & ATTR_SIZE;
>  	if (size_change) {
> -		dquot_initialize(inode);
> -
>  		status = ocfs2_rw_lock(inode, 1);
>  		if (status < 0) {
>  			mlog_errno(status);
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 8a2f0cb..a168189 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1801,10 +1801,9 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
>  		mask |= 1 << GRPQUOTA;
>  		chid[GRPQUOTA] = iattr->ia_gid;
>  	}
> -	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode)) {
> -		dquot_initialize(inode);
> +	if (sb_any_quota_active(inode->i_sb) && !IS_NOQUOTA(inode))
>  		return __dquot_transfer(inode, chid, mask);
> -	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(dquot_transfer);
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index b8671a5..0bf5324 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -3075,9 +3075,10 @@ int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
>  	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
>  
>  	depth = reiserfs_write_lock_once(inode->i_sb);
> -	if (attr->ia_valid & ATTR_SIZE) {
> +	if (is_quota_modification(inode, attr))
>  		dquot_initialize(inode);
>  
> +	if (attr->ia_valid & ATTR_SIZE) {
>  		/* version 2 items will be caught by the s_maxbytes check
>  		 ** done for us in vmtruncate
>  		 */
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index 4b6a46c..6ebc043 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -227,7 +227,7 @@ int udf_setattr(struct dentry *dentry, struct iattr *iattr)
>  	if (error)
>  		return error;
>  
> -	if (iattr->ia_valid & ATTR_SIZE)
> +	if (is_quota_modification(inode, iattr))
>  		dquot_initialize(inode);
>  
>  	if ((iattr->ia_valid & ATTR_UID && iattr->ia_uid != inode->i_uid) ||
> diff --git a/fs/ufs/truncate.c b/fs/ufs/truncate.c
> index ee8db3e..f294c44 100644
> --- a/fs/ufs/truncate.c
> +++ b/fs/ufs/truncate.c
> @@ -518,18 +518,18 @@ int ufs_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (error)
>  		return error;
>  
> +	if (is_quota_modification(inode, attr))
> +		dquot_initialize(inode);
> +
>  	if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
>  	    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
>  		error = dquot_transfer(inode, attr);
>  		if (error)
>  			return error;
>  	}
> -	if (ia_valid & ATTR_SIZE &&
> -	    attr->ia_size != i_size_read(inode)) {
> +	if (ia_valid & ATTR_SIZE && attr->ia_size != inode->i_size) {
>  		loff_t old_i_size = inode->i_size;
>  
> -		dquot_initialize(inode);
> -
>  		error = vmtruncate(inode, attr->ia_size);
>  		if (error)
>  			return error;
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index e6fa7ac..fd8f70b 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -14,6 +14,14 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>  	return &sb->s_dquot;
>  }
>  
> +/* i_mutex must being held */
> +static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> +{
> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> +		(ia->ia_valid & ATTR_UID && ia->ia_uid != inode->i_uid) ||
> +		(ia->ia_valid & ATTR_GID && ia->ia_gid != inode->i_gid);
> +}
> +
>  #if defined(CONFIG_QUOTA)
>  
>  /*
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge
  2010-05-07 16:48       ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Jan Kara
@ 2010-05-17  6:22         ` Dmitry Monakhov
  0 siblings, 0 replies; 16+ messages in thread
From: Dmitry Monakhov @ 2010-05-17  6:22 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-fsdevel, hch, sandeen

Jan Kara <jack@suse.cz> 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 <dmonakhov@openvz.org>
>> ---
>>  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 :) 


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2010-05-17  6:22 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-08 18:04 [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
2010-04-08 18:04 ` [PATCH 1/6] quota: unify quota init condition in setattr Dmitry Monakhov
2010-04-08 18:04   ` [PATCH 2/6] quota: Add proper error handling on quota initialization Dmitry Monakhov
2010-04-08 18:04     ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Dmitry Monakhov
2010-04-08 18:04       ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Dmitry Monakhov
2010-04-08 18:04         ` [PATCH 5/6] ext4: " Dmitry Monakhov
2010-04-08 18:04           ` [PATCH 6/6] quota: check error code from dquot_initialize Dmitry Monakhov
2010-05-07 17:01             ` Jan Kara
2010-05-07 16:59         ` [PATCH 4/6] ext3: handle errors in orphan_cleanup Jan Kara
2010-05-07 16:48       ` [PATCH 3/6] quota: Check what quota is properly initialized for inode before charge Jan Kara
2010-05-17  6:22         ` Dmitry Monakhov
2010-05-07 16:44     ` [PATCH 2/6] quota: Add proper error handling on quota initialization Jan Kara
2010-05-07 16:39   ` [PATCH 1/6] quota: unify quota init condition in setattr Jan Kara
2010-05-13 16:29   ` Jan Kara
2010-05-05  8:45 ` [PATCH 0/6] RFC quota: Redesign IO error handling interface Dmitry Monakhov
2010-05-07 16:38 ` Jan Kara

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.