ext3: Handle ext[34]_journal_stop() failure

RFC PATCH In review Dmitriy Monakhov 2 participants 2 messages Feb 28, 2007 – Mar 2, 2007 ext4 file system

Share & tools

Dmitriy Monakhov

author

2007-02-28 16:45 UTC #0 lore raw

RFC PATCH 1/1 ext3: Handle ext[34]_journal_stop() failure

Where are many places where xxxx_journal_stop() return code wasn't checked. Off cause xxxx_journal_stop() failed very rarely (and usually with fatal consequences), but this does'n meen it should not be checked. For example most retry loops looks like follows:

	ext3_journal_stop(handle);
	if (err == -ENOSPC && ext3_should_retry_alloc(dir->i_sb, &retries))
		goto retry;

It is realy insane do "retry" if ext3_journal_stop() has failed, because this may increase damage in case of real problem.

Signed-off-by Monakhov Dmitriy <[email protected]>

---
 fs/ext3/acl.c   |   12 ++++++++----
 fs/ext3/inode.c |   22 +++++++++++++++++-----
 fs/ext3/ioctl.c |   12 ++++++++----
 fs/ext3/namei.c |   48 ++++++++++++++++++++++++++++++++----------------
 fs/ext3/xattr.c |    4 ++--
 fs/ext4/acl.c   |   12 ++++++++----
 fs/ext4/inode.c |   22 +++++++++++++++++-----
 fs/ext4/ioctl.c |   12 ++++++++----
 fs/ext4/namei.c |   48 ++++++++++++++++++++++++++++++++----------------
 fs/ext4/xattr.c |    4 ++--
 10 files changed, 134 insertions(+), 62 deletions(-)

fs/ext3/acl.c +8 -4

index 1e5038d..bbf4d8a 100644
@@ -375,7 +375,7 @@ int
 ext3_acl_chmod(struct inode *inode)
 {
 	struct posix_acl *acl, *clone;
-        int error;
+	int error, error2;
 
 	if (S_ISLNK(inode->i_mode))
 		return -EOPNOTSUPP;

@@ -402,7 +402,9 @@ ext3_acl_chmod(struct inode *inode)
 			goto out;
 		}
 		error = ext3_set_acl(handle, inode, ACL_TYPE_ACCESS, clone);
-		ext3_journal_stop(handle);
+		error2 = ext3_journal_stop(handle);
+		if (error2 && (!error || error == -ENOSPC))
+			error = error2;
 		if (error == -ENOSPC &&
 		    ext3_should_retry_alloc(inode->i_sb, &retries))
 			goto retry;

@@ -485,7 +487,7 @@ ext3_xattr_set_acl(struct inode *inode, int type, const void *value,
 {
 	handle_t *handle;
 	struct posix_acl *acl;
-	int error, retries = 0;
+	int error, error2, retries = 0;
 
 	if (!test_opt(inode->i_sb, POSIX_ACL))
 		return -EOPNOTSUPP;

@@ -509,7 +511,9 @@ retry:
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 	error = ext3_set_acl(handle, inode, type, acl);
-	ext3_journal_stop(handle);
+	error2 = ext3_journal_stop(handle);
+	if (error2 && (!error || error == -ENOSPC))
+		error = error2;
 	if (error == -ENOSPC && ext3_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;

fs/ext3/inode.c +17 -5

index 816f0c5..68d1c71 100644
@@ -962,7 +962,9 @@ static int ext3_get_block(struct inode *inode, sector_t iblock,
 		 * Huge direct-io writes can hold off commits for long
 		 * periods of time.  Let this commit run.
 		 */
-		ext3_journal_stop(handle);
+		ret = ext3_journal_stop(handle);
+		if (ret)
+			goto get_block;
 		handle = ext3_journal_start(inode, DIO_CREDITS);
 		if (IS_ERR(handle))
 			ret = PTR_ERR(handle);

@@ -2998,7 +3000,13 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 		if (attr->ia_valid & ATTR_GID)
 			inode->i_gid = attr->ia_gid;
 		error = ext3_mark_inode_dirty(handle, inode);
-		ext3_journal_stop(handle);
+		if (error) {
+			ext3_journal_stop(handle);
+			goto err_out;
+		}
+		error = ext3_journal_stop(handle);
+		if (error)
+			goto err_out;
 	}
 
 	if (S_ISREG(inode->i_mode) &&

@@ -3016,7 +3024,9 @@ int ext3_setattr(struct dentry *dentry, struct iattr *attr)
 		rc = ext3_mark_inode_dirty(handle, inode);
 		if (!error)
 			error = rc;
-		ext3_journal_stop(handle);
+		rc = ext3_journal_stop(handle);
+		if (!error)
+			error = rc;
 	}
 
 	rc = inode_setattr(inode, attr);

@@ -3231,7 +3241,7 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
 {
 	journal_t *journal;
 	handle_t *handle;
-	int err;
+	int err, err2;
 
 	/*
 	 * We have to be very careful here: changing a data block's

@@ -3274,7 +3284,9 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
 
 	err = ext3_mark_inode_dirty(handle, inode);
 	handle->h_sync = 1;
-	ext3_journal_stop(handle);
+	err2 = ext3_journal_stop(handle);
+	if (!err)
+		err = err2;
 	ext3_std_error(inode->i_sb, err);
 
 	return err;

fs/ext3/ioctl.c +8 -4

index 9b8090d..c90999f 100644
@@ -32,7 +32,7 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 		return put_user(flags, (int __user *) arg);
 	case EXT3_IOC_SETFLAGS: {
 		handle_t *handle = NULL;
-		int err;
+		int err, err2;
 		struct ext3_iloc iloc;
 		unsigned int oldflags;
 		unsigned int jflag;

@@ -100,7 +100,9 @@ int ext3_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 
 		err = ext3_mark_iloc_dirty(handle, inode, &iloc);
 flags_err:
-		ext3_journal_stop(handle);
+		err2 = ext3_journal_stop(handle);
+		if (!err)
+			err = err2;
 		if (err) {
 			mutex_unlock(&inode->i_mutex);
 			return err;

@@ -119,7 +121,7 @@ flags_err:
 		handle_t *handle;
 		struct ext3_iloc iloc;
 		__u32 generation;
-		int err;
+		int err, err2;
 
 		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
 			return -EPERM;

@@ -137,7 +139,9 @@ flags_err:
 			inode->i_generation = generation;
 			err = ext3_mark_iloc_dirty(handle, inode, &iloc);
 		}
-		ext3_journal_stop(handle);
+		err2 = ext3_journal_stop(handle);
+		if (!err)
+			err = err2;
 		return err;
 	}
 #ifdef CONFIG_JBD_DEBUG

fs/ext3/namei.c +32 ~~-16~~

144 lines not on this page · raw message

fs/ext3/xattr.c +2 -2

index 99857a4..18b74df 100644
@@ -1047,11 +1047,11 @@ retry:
 		error = ext3_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
 		error2 = ext3_journal_stop(handle);
+		if (error2 && (!error || error == -ENOSPC))
+			error = error2;
 		if (error == -ENOSPC &&
 		    ext3_should_retry_alloc(inode->i_sb, &retries))
 			goto retry;
-		if (error == 0)
-			error = error2;
 	}
 
 	return error;

fs/ext4/acl.c +8 -4

index 9e88254..10e02c1 100644
@@ -375,7 +375,7 @@ int
 ext4_acl_chmod(struct inode *inode)
 {
 	struct posix_acl *acl, *clone;
-	int error;
+	int error, error2;
 
 	if (S_ISLNK(inode->i_mode))
 		return -EOPNOTSUPP;

@@ -402,7 +402,9 @@ ext4_acl_chmod(struct inode *inode)
 			goto out;
 		}
 		error = ext4_set_acl(handle, inode, ACL_TYPE_ACCESS, clone);
-		ext4_journal_stop(handle);
+		error2 = ext4_journal_stop(handle);
+		if (error2 && (!error || error == -ENOSPC))
+			error = error2;
 		if (error == -ENOSPC &&
 		    ext4_should_retry_alloc(inode->i_sb, &retries))
 			goto retry;

@@ -485,7 +487,7 @@ ext4_xattr_set_acl(struct inode *inode, int type, const void *value,
 {
 	handle_t *handle;
 	struct posix_acl *acl;
-	int error, retries = 0;
+	int error, error2, retries = 0;
 
 	if (!test_opt(inode->i_sb, POSIX_ACL))
 		return -EOPNOTSUPP;

@@ -509,7 +511,9 @@ retry:
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 	error = ext4_set_acl(handle, inode, type, acl);
-	ext4_journal_stop(handle);
+	error2 = ext4_journal_stop(handle);
+	if (error2 && (!error || error == -ENOSPC))
+		error = error2;
 	if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;

fs/ext4/inode.c +17 -5

index 02735b1..937d625 100644
@@ -961,7 +961,9 @@ static int ext4_get_block(struct inode *inode, sector_t iblock,
 		 * Huge direct-io writes can hold off commits for long
 		 * periods of time.  Let this commit run.
 		 */
-		ext4_journal_stop(handle);
+		ret = ext4_journal_stop(handle);
+		if (ret)
+			goto get_block;
 		handle = ext4_journal_start(inode, DIO_CREDITS);
 		if (IS_ERR(handle))
 			ret = PTR_ERR(handle);

@@ -3009,7 +3011,13 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 		if (attr->ia_valid & ATTR_GID)
 			inode->i_gid = attr->ia_gid;
 		error = ext4_mark_inode_dirty(handle, inode);
-		ext4_journal_stop(handle);
+		if (error) {
+			ext4_journal_stop(handle);
+			goto err_out;
+		}
+		error = ext4_journal_stop(handle);
+		if (error)
+			goto err_out;
 	}
 
 	if (S_ISREG(inode->i_mode) &&

@@ -3027,7 +3035,9 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 		rc = ext4_mark_inode_dirty(handle, inode);
 		if (!error)
 			error = rc;
-		ext4_journal_stop(handle);
+		rc = ext4_journal_stop(handle);
+		if (!error)
+			error = rc;
 	}
 
 	rc = inode_setattr(inode, attr);

@@ -3245,7 +3255,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 {
 	journal_t *journal;
 	handle_t *handle;
-	int err;
+	int err, err2;
 
 	/*
 	 * We have to be very careful here: changing a data block's

@@ -3288,7 +3298,9 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 
 	err = ext4_mark_inode_dirty(handle, inode);
 	handle->h_sync = 1;
-	ext4_journal_stop(handle);
+	err2 = ext4_journal_stop(handle);
+	if (!err)
+		err = err2;
 	ext4_std_error(inode->i_sb, err);
 
 	return err;

fs/ext4/ioctl.c +8 -4

index 500567d..9a62c9a 100644
@@ -32,7 +32,7 @@ int ext4_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 		return put_user(flags, (int __user *) arg);
 	case EXT4_IOC_SETFLAGS: {
 		handle_t *handle = NULL;
-		int err;
+		int err, err2;
 		struct ext4_iloc iloc;
 		unsigned int oldflags;
 		unsigned int jflag;

@@ -100,7 +100,9 @@ int ext4_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
 
 		err = ext4_mark_iloc_dirty(handle, inode, &iloc);
 flags_err:
-		ext4_journal_stop(handle);
+		err2 = ext4_journal_stop(handle);
+		if (!err)
+			err = err2;
 		if (err) {
 			mutex_unlock(&inode->i_mutex);
 			return err;

@@ -119,7 +121,7 @@ flags_err:
 		handle_t *handle;
 		struct ext4_iloc iloc;
 		__u32 generation;
-		int err;
+		int err, err2;
 
 		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
 			return -EPERM;

@@ -137,7 +139,9 @@ flags_err:
 			inode->i_generation = generation;
 			err = ext4_mark_iloc_dirty(handle, inode, &iloc);
 		}
-		ext4_journal_stop(handle);
+		err2 = ext4_journal_stop(handle);
+		if (!err)
+			err = err2;
 		return err;
 	}
 #ifdef CONFIG_JBD_DEBUG

fs/ext4/namei.c +32 ~~-16~~

144 lines not on this page · raw message

fs/ext4/xattr.c +2 -2

index dc969c3..47d9ce5 100644
@@ -1047,11 +1047,11 @@ retry:
 		error = ext4_xattr_set_handle(handle, inode, name_index, name,
 					      value, value_len, flags);
 		error2 = ext4_journal_stop(handle);
+		if (error2 && (!error || error == -ENOSPC))
+			error = error2;
 		if (error == -ENOSPC &&
 		    ext4_should_retry_alloc(inode->i_sb, &retries))
 			goto retry;
-		if (error == 0)
-			error = error2;
 	}
 
 	return error;

Signature · 1 line

1.5.0.1

Andreas Dilger

2007-03-02 09:06 UTC #1 lore raw

Quoted 3 lines · Dmitriy Monakhov

Where are many places where xxxx_journal_stop() return code wasn't checked. Off cause xxxx_journal_stop() failed very rarely (and usually with fatal consequences), but this does'n meen it should not be checked.

I fully agree with this. Otherwise an application will believe that the operation has succeeded when in fact the filesystem has failed. It would notice this on the next filesystem operation, but that might be too late for e.g. an MTA that just confirmed acceptance of an email.

There are also several places in the code where we don't check error returns from ext3_journal_get_write_access(). I have a patch that I was starting to work on but never managed to finish/test. I'd be happy if you had time to finish it off.

================= ext3-check-jbd-errors-2.6.20.patch =================

fs/ext3/namei.c renamed from fs/ext3/namei.c.orig +29 -9

@@ -59,9 +59,14 @@ static struct buffer_head *ext3_append(h
 	*block = inode->i_size >> inode->i_sb->s_blocksize_bits;
 
 	if ((bh = ext3_bread(handle, inode, *block, 1, err))) {
-		inode->i_size += inode->i_sb->s_blocksize;
-		EXT3_I(inode)->i_disksize = inode->i_size;
-		ext3_journal_get_write_access(handle,bh);
+		*err = ext3_journal_get_write_access(handle,bh);
+		if (err) {
+			brelse(bh);
+			bh = NULL;
+		} else {
+			inode->i_size += inode->i_sb->s_blocksize;
+			EXT3_I(inode)->i_disksize = inode->i_size;
+		}
 	}
 	return bh;
 }

@@ -1597,8 +1602,12 @@ static int ext3_delete_entry (handle_t *
 		if (!ext3_check_dir_entry("ext3_delete_entry", dir, de, bh, i))
 			return -EIO;
 		if (de == de_del)  {
+			int err;
+
 			BUFFER_TRACE(bh, "get_write_access");
-			ext3_journal_get_write_access(handle, bh);
+			err = ext3_journal_get_write_access(handle, bh);
+			if (err)
+				return err;
 			if (pde)
 				pde->rec_len =
 					cpu_to_le16(le16_to_cpu(pde->rec_len) +

@@ -1758,7 +1767,13 @@ retry:
 		goto out_stop;
 	}
 	BUFFER_TRACE(dir_block, "get_write_access");
-	ext3_journal_get_write_access(handle, dir_block);
+	err = ext3_journal_get_write_access(handle, dir_block);
+	if (err) {
+		drop_nlink(inode); /* is this nlink == 0? */
+		ext3_mark_inode_dirty(handle, inode);
+		iput (inode);
+		goto out_stop;
+	}
 	de = (struct ext3_dir_entry_2 *) dir_block->b_data;
 	de->inode = cpu_to_le32(inode->i_ino);
 	de->name_len = 1;

@@ -2262,6 +2277,11 @@ static int ext3_rename (struct inode * o
 		if (!new_inode) {
 			brelse (new_bh);
 			new_bh = NULL;
+		} else {
+			BUFFER_TRACE(new_bh, "get write access");
+			retval = ext3_journal_get_write_access(handle, new_bh);
+			if (retval)
+				goto end_rename;
 		}
 	}
 	if (S_ISDIR(old_inode->i_mode)) {

@@ -2280,14 +2300,16 @@ static int ext3_rename (struct inode * o
 		if (!new_inode && new_dir!=old_dir &&
 				new_dir->i_nlink >= EXT3_LINK_MAX)
 			goto end_rename;
+		BUFFER_TRACE(dir_bh, "get_write_access");
+		retval = ext3_journal_get_write_access(handle, dir_bh);
+		if (retval)
+			goto end_rename;
 	}
 	if (!new_bh) {
 		retval = ext3_add_entry (handle, new_dentry, old_inode);
 		if (retval)
 			goto end_rename;
 	} else {
-		BUFFER_TRACE(new_bh, "get write access");
-		ext3_journal_get_write_access(handle, new_bh);
 		new_de->inode = cpu_to_le32(old_inode->i_ino);
 		if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
 					      EXT3_FEATURE_INCOMPAT_FILETYPE))

@@ -2341,8 +2363,6 @@ static int ext3_rename (struct inode * o
 	old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
 	ext3_update_dx_flag(old_dir);
 	if (dir_bh) {
-		BUFFER_TRACE(dir_bh, "get_write_access");
-		ext3_journal_get_write_access(handle, dir_bh);
 		PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
 		BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
 		ext3_journal_dirty_metadata(handle, dir_bh);

==============================================================================

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.