prevent parallel resizers and fix some error handling in resize

Hi,

  This patch series prevents parallel resizer and fixes some error handling in
resize code.  Besides these, some code is simplified so that the code can be
used easily in new resize implementation.

  The patches are tested by resize2fs and e2fsck -fn.

[PATCH 01/12] ext4: prevent parallel resizers by atomic bit ops
[PATCH 02/12] ext4: prevent a fs with errors from being resized
[PATCH 03/12] ext4: prevent a fs without journal from being resized
[PATCH 04/12] ext4: rename ext4_add_groupblocks() to
[PATCH 05/12] ext4: let ext4_group_add_blocks return an error code
[PATCH 06/12] ext4: let ext4_group_add_blocks() handle 0 blocks
[PATCH 07/12] ext4: fix a typo in ext4_group_extend()
[PATCH 08/12] ext4: let setup_new_group_blocks set multi-bits each
[PATCH 09/12] ext4: simplify journal handling in
[PATCH 10/12] ext4: remove lock_buffer in bclean() and
[PATCH 11/12] ext4: simplify parameters of add_new_gdb()
[PATCH 12/12] ext4: simplify parameters of reserve_backup_gdb()

Before this patch, parallel resizers are allowed and protected by a mutex lock,
actually, there is no need to support parallel resizer, so this patch prevents
parallel resizers by atmoic bit ops, like lock_page() and unlock_page() do.

To do this, the patch removed the mutex lock s_resize_lock from struct ext4_sb_info
and added a unsigned long field named s_resize_flags which inidicates if there is
a resizer.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/ext4.h   |    7 +++++-
 fs/ext4/ioctl.c  |   12 +++++++---
 fs/ext4/resize.c |   55 ++++++++++++++++++++---------------------------------
 fs/ext4/super.c  |    2 +-
 4 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 62cee2b..bb0f776 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -1127,7 +1127,8 @@ struct ext4_sb_info {
 	struct journal_s *s_journal;
 	struct list_head s_orphan;
 	struct mutex s_orphan_lock;
-	struct mutex s_resize_lock;
+	unsigned long s_resize_flags;		/* Flags indicating if there
+						   is a resizer */
 	unsigned long s_commit_interval;
 	u32 s_max_batch_time;
 	u32 s_min_batch_time;

@@ -2269,6 +2270,10 @@ static inline void set_bitmap_uptodate(struct buffer_head *bh)
 extern wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ];
 extern struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ];
 
+#define EXT4_RESIZING	0
+extern int ext4_resize_begin(struct super_block *sb);
+extern void ext4_resize_end(struct super_block *sb);
+
 #endif	/* __KERNEL__ */
 
 #endif	/* _EXT4_H */

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 808c554..f18bfe3 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c

@@ -202,8 +202,9 @@ setversion_out:
 		struct super_block *sb = inode->i_sb;
 		int err, err2=0;
 
-		if (!capable(CAP_SYS_RESOURCE))
-			return -EPERM;
+		err = ext4_resize_begin(sb);
+		if (err)
+			return err;
 
 		if (get_user(n_blocks_count, (__u32 __user *)arg))
 			return -EFAULT;

@@ -221,6 +222,7 @@ setversion_out:
 		if (err == 0)
 			err = err2;
 		mnt_drop_write(filp->f_path.mnt);
+		ext4_resize_end(sb);
 
 		return err;
 	}

@@ -271,8 +273,9 @@ mext_out:
 		struct super_block *sb = inode->i_sb;
 		int err, err2=0;
 
-		if (!capable(CAP_SYS_RESOURCE))
-			return -EPERM;
+		err = ext4_resize_begin(sb);
+		if (err)
+			return err;
 
 		if (copy_from_user(&input, (struct ext4_new_group_input __user *)arg,
 				sizeof(input)))

@@ -291,6 +294,7 @@ mext_out:
 		if (err == 0)
 			err = err2;
 		mnt_drop_write(filp->f_path.mnt);
+		ext4_resize_end(sb);
 
 		return err;
 	}

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 80bbc9c..0213f63 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -16,6 +16,25 @@
 
 #include "ext4_jbd2.h"
 
+int ext4_resize_begin(struct super_block *sb)
+{
+	int ret = 0;
+
+	if (!capable(CAP_SYS_RESOURCE))
+		return -EPERM;
+
+	if (test_and_set_bit_lock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags))
+		ret = -EBUSY;
+
+	return ret;
+}
+
+void ext4_resize_end(struct super_block *sb)
+{
+	clear_bit_unlock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags);
+	smp_mb__after_clear_bit();
+}
+
 #define outside(b, first, last)	((b) < (first) || (b) >= (last))
 #define inside(b, first, last)	((b) >= (first) && (b) < (last))
 

@@ -181,11 +200,7 @@ static int setup_new_group_blocks(struct super_block *sb,
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
-	mutex_lock(&sbi->s_resize_lock);
-	if (input->group != sbi->s_groups_count) {
-		err = -EBUSY;
-		goto exit_journal;
-	}
+	BUG_ON(input->group != sbi->s_groups_count);
 
 	if (IS_ERR(bh = bclean(handle, sb, input->block_bitmap))) {
 		err = PTR_ERR(bh);

@@ -285,7 +300,6 @@ exit_bh:
 	brelse(bh);
 
 exit_journal:
-	mutex_unlock(&sbi->s_resize_lock);
 	if ((err2 = ext4_journal_stop(handle)) && !err)
 		err = err2;
 

@@ -799,13 +813,6 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 		goto exit_put;
 	}
 
-	mutex_lock(&sbi->s_resize_lock);
-	if (input->group != sbi->s_groups_count) {
-		ext4_warning(sb, "multiple resizers run on filesystem!");
-		err = -EBUSY;
-		goto exit_journal;
-	}
-
 	if ((err = ext4_journal_get_write_access(handle, sbi->s_sbh)))
 		goto exit_journal;
 

@@ -829,7 +836,6 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
         /*
          * OK, now we've set up the new group.  Time to make it active.
          *
-         * We do not lock all allocations via s_resize_lock
          * so we have to be safe wrt. concurrent accesses the group
          * data.  So we need to be careful to set all of the relevant
          * group descriptor data etc. *before* we enable the group.

@@ -886,13 +892,9 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 	 *
 	 * The precise rules we use are:
 	 *
-	 * * Writers of s_groups_count *must* hold s_resize_lock
-	 * AND
 	 * * Writers must perform a smp_wmb() after updating all dependent
 	 *   data and before modifying the groups count
 	 *
-	 * * Readers must hold s_resize_lock over the access
-	 * OR
 	 * * Readers must perform an smp_rmb() after reading the groups count
 	 *   and before reading any dependent data.
 	 *

@@ -937,7 +939,6 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 	ext4_handle_dirty_super(handle, sb);
 
 exit_journal:
-	mutex_unlock(&sbi->s_resize_lock);
 	if ((err2 = ext4_journal_stop(handle)) && !err)
 		err = err2;
 	if (!err) {

@@ -972,9 +973,6 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	int err;
 	ext4_group_t group;
 
-	/* We don't need to worry about locking wrt other resizers just
-	 * yet: we're going to revalidate es->s_blocks_count after
-	 * taking the s_resize_lock below. */
 	o_blocks_count = ext4_blocks_count(es);
 
 	if (test_opt(sb, DEBUG))

@@ -995,7 +993,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 
 	if (n_blocks_count < o_blocks_count) {
 		ext4_warning(sb, "can't shrink FS - resize aborted");
-		return -EBUSY;
+		return -EINVAL;
 	}
 
 	/* Handle the remaining blocks in the last group only. */

@@ -1038,24 +1036,13 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 		goto exit_put;
 	}
 
-	mutex_lock(&EXT4_SB(sb)->s_resize_lock);
-	if (o_blocks_count != ext4_blocks_count(es)) {
-		ext4_warning(sb, "multiple resizers run on filesystem!");
-		mutex_unlock(&EXT4_SB(sb)->s_resize_lock);
-		ext4_journal_stop(handle);
-		err = -EBUSY;
-		goto exit_put;
-	}
-
 	if ((err = ext4_journal_get_write_access(handle,
 						 EXT4_SB(sb)->s_sbh))) {
 		ext4_warning(sb, "error %d on journal write access", err);
-		mutex_unlock(&EXT4_SB(sb)->s_resize_lock);
 		ext4_journal_stop(handle);
 		goto exit_put;
 	}
 	ext4_blocks_count_set(es, o_blocks_count + add);
-	mutex_unlock(&EXT4_SB(sb)->s_resize_lock);
 	ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
 	/* We add the blocks to the bitmap and set the group need init bit */

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7910e61..39c2992 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -3492,7 +3492,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 
 	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
 	mutex_init(&sbi->s_orphan_lock);
-	mutex_init(&sbi->s_resize_lock);
+	sbi->s_resize_flags = 0;
 
 	sb->s_root = NULL;
 

-- 
1.7.5.1

A filesystem with errors is not allowed to being resized, otherwise, it is
easy to destroy the filesystem.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0213f63..53d9795 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -23,6 +23,16 @@ int ext4_resize_begin(struct super_block *sb)
 	if (!capable(CAP_SYS_RESOURCE))
 		return -EPERM;
 
+	/*
+	 * We are not allowed to do online-resizing on a filesystem mounted
+	 * with error, because it can destroy the filesystem easily.
+	 */
+	if (EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS) {
+		ext4_warning(sb, "There are errors in the filesystem, "
+			     "so online resizing is not allowed\n");
+		return -EPERM;
+	}
+
 	if (test_and_set_bit_lock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags))
 		ret = -EBUSY;
 

-- 
1.7.5.1

This patch prevents a fs without journal from being resized, because
it is easy to detroy the fs.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 53d9795..33ab40d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -33,6 +33,16 @@ int ext4_resize_begin(struct super_block *sb)
 		return -EPERM;
 	}
 
+	/*
+	 * We are not allowed to do online-resizing on a filesystem without
+	 * journal, otherwise, it is easy to destroy the filesystem.
+	 */
+	if (!EXT4_SB(sb)->s_journal) {
+		ext4_warning(sb, "There is no journal for the filesystem, "
+			     "so online resizing is not allowed\n");
+		return -EPERM;
+	}
+
 	if (test_and_set_bit_lock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags))
 		ret = -EBUSY;
 

-- 
1.7.5.1

This patch renamed ext4_add_groupblocks() to ext4_group_add_blocks().

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/ext4.h    |    2 +-
 fs/ext4/mballoc.c |    4 ++--
 fs/ext4/resize.c  |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bb0f776..bbe81db 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -1799,7 +1799,7 @@ extern void ext4_free_blocks(handle_t *handle, struct inode *inode,
 			     unsigned long count, int flags);
 extern int ext4_mb_add_groupinfo(struct super_block *sb,
 		ext4_group_t i, struct ext4_group_desc *desc);
-extern void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
+extern void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 				ext4_fsblk_t block, unsigned long count);
 extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
 

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b97a2d2..ea80c0b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c

@@ -4656,7 +4656,7 @@ error_return:
 }
 
 /**
- * ext4_add_groupblocks() -- Add given blocks to an existing group
+ * ext4_group_add_blocks() -- Add given blocks to an existing group
  * @handle:			handle to this transaction
  * @sb:				super block
  * @block:			start physcial block to add to the block group

@@ -4664,7 +4664,7 @@ error_return:
  *
  * This marks the blocks as free in the bitmap and buddy.
  */
-void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
+void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 			 ext4_fsblk_t block, unsigned long count)
 {
 	struct buffer_head *bitmap_bh = NULL;

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 33ab40d..374fd1c 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -1066,7 +1066,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
 	/* We add the blocks to the bitmap and set the group need init bit */
-	ext4_add_groupblocks(handle, sb, o_blocks_count, add);
+	ext4_group_add_blocks(handle, sb, o_blocks_count, add);
 	ext4_handle_dirty_super(handle, sb);
 	ext4_debug("freed blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);

-- 
1.7.5.1

This patch lets ext4_group_add_blocks() return an error code if it fails,
so that upper functions can handle error correctly.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/ext4.h    |    2 +-
 fs/ext4/mballoc.c |   19 +++++++++++++------
 fs/ext4/resize.c  |   10 +++++++---
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bbe81db..da7ab48 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -1799,7 +1799,7 @@ extern void ext4_free_blocks(handle_t *handle, struct inode *inode,
 			     unsigned long count, int flags);
 extern int ext4_mb_add_groupinfo(struct super_block *sb,
 		ext4_group_t i, struct ext4_group_desc *desc);
-extern void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
+extern int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 				ext4_fsblk_t block, unsigned long count);
 extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
 

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ea80c0b..33c41e6 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c

@@ -4664,7 +4664,7 @@ error_return:
  *
  * This marks the blocks as free in the bitmap and buddy.
  */
-void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
+int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 			 ext4_fsblk_t block, unsigned long count)
 {
 	struct buffer_head *bitmap_bh = NULL;

@@ -4685,15 +4685,21 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 	 * Check to see if we are freeing blocks across a group
 	 * boundary.
 	 */
-	if (bit + count > EXT4_BLOCKS_PER_GROUP(sb))
-		goto error_return;
+	if (bit + count > EXT4_BLOCKS_PER_GROUP(sb)) {
+		ext4_warning(sb, "too much blocks added to group %u\n",
+			     block_group);
+		return -EINVAL;
+	}
 
 	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
 	if (!bitmap_bh)
-		goto error_return;
+		return -EIO;
+
 	desc = ext4_get_group_desc(sb, block_group, &gd_bh);
-	if (!desc)
+	if (!desc) {
+		err = -EIO;
 		goto error_return;
+	}
 
 	if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
 	    in_range(ext4_inode_bitmap(sb, desc), block, count) ||

@@ -4703,6 +4709,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 		ext4_error(sb, "Adding blocks in system zones - "
 			   "Block = %llu, count = %lu",
 			   block, count);
+		err = -EINVAL;
 		goto error_return;
 	}
 

@@ -4771,7 +4778,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 error_return:
 	brelse(bitmap_bh);
 	ext4_std_error(sb, err);
-	return;
+	return err;
 }
 
 /**

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 374fd1c..2e63376 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -990,7 +990,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	ext4_grpblk_t add;
 	struct buffer_head *bh;
 	handle_t *handle;
-	int err;
+	int err, err2;
 	ext4_group_t group;
 
 	o_blocks_count = ext4_blocks_count(es);

@@ -1066,11 +1066,15 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
 	/* We add the blocks to the bitmap and set the group need init bit */
-	ext4_group_add_blocks(handle, sb, o_blocks_count, add);
+	err = ext4_group_add_blocks(handle, sb, o_blocks_count, add);
 	ext4_handle_dirty_super(handle, sb);
 	ext4_debug("freed blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
-	if ((err = ext4_journal_stop(handle)))
+	err2 = ext4_journal_stop(handle);
+	if (!err && err2)
+		err = err2;
+
+	if (err)
 		goto exit_put;
 
 	if (test_opt(sb, DEBUG))

-- 
1.7.5.1

If ext4_group_add_blocks() is called with 0 block, it just return 0.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/mballoc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 33c41e6..a2af35b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c

@@ -4680,6 +4680,9 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 
 	ext4_debug("Adding block(s) %llu-%llu\n", block, block + count - 1);
 
+	if (count == 0)
+		return 0;
+
 	ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
 	/*
 	 * Check to see if we are freeing blocks across a group

-- 
1.7.5.1

This patch fixed a typo in ext4_group_extend().

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 2e63376..4089642 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -996,7 +996,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	o_blocks_count = ext4_blocks_count(es);
 
 	if (test_opt(sb, DEBUG))
-		printk(KERN_DEBUG "EXT4-fs: extending last group from %llu uto %llu blocks\n",
+		printk(KERN_DEBUG "EXT4-fs: extending last group from %llu to %llu blocks\n",
 		       o_blocks_count, n_blocks_count);
 
 	if (n_blocks_count == 0 || n_blocks_count == o_blocks_count)

-- 
1.7.5.1

This patch adds a function - ext4_set_btis() which can set multi-bits
each time, and lets setup_new_group_blocks() use ext4_set_bits().

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/ext4.h    |    1 +
 fs/ext4/mballoc.c |    5 +++++
 fs/ext4/resize.c  |   18 +++++++-----------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index da7ab48..43e7448 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -940,6 +940,7 @@ struct ext4_inode_info {
 #define ext4_find_next_zero_bit		find_next_zero_bit_le
 #define ext4_find_next_bit		find_next_bit_le
 
+extern void ext4_set_bits(void *bm, int cur, int len);
 /*
  * Maximal mount counts between two filesystem checks
  */

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a2af35b..971137d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c

@@ -1299,6 +1299,11 @@ static void mb_set_bits(void *bm, int cur, int len)
 	}
 }
 
+void ext4_set_bits(void *bm, int cur, int len)
+{
+	mb_set_bits(bm, cur, len);
+}
+
 static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
 			  int first, int count)
 {

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 4089642..c91653c 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -227,11 +227,6 @@ static int setup_new_group_blocks(struct super_block *sb,
 		goto exit_journal;
 	}
 
-	if (ext4_bg_has_super(sb, input->group)) {
-		ext4_debug("mark backup superblock %#04llx (+0)\n", start);
-		ext4_set_bit(0, bh->b_data);
-	}
-
 	/* Copy all of the GDT blocks into the backup in this group */
 	for (i = 0, bit = 1, block = start + 1;
 	     i < gdblocks; i++, block++, bit++) {

@@ -260,7 +255,6 @@ static int setup_new_group_blocks(struct super_block *sb,
 			brelse(gdb);
 			goto exit_bh;
 		}
-		ext4_set_bit(bit, bh->b_data);
 		brelse(gdb);
 	}
 

@@ -271,8 +265,11 @@ static int setup_new_group_blocks(struct super_block *sb,
 			       GFP_NOFS);
 	if (err)
 		goto exit_bh;
-	for (i = 0, bit = gdblocks + 1; i < reserved_gdb; i++, bit++)
-		ext4_set_bit(bit, bh->b_data);
+
+	if (ext4_bg_has_super(sb, input->group)) {
+		ext4_debug("mark backup group tables %#04llx (+0)\n", start);
+		ext4_set_bits(bh->b_data, 0, gdblocks + reserved_gdb + 1);
+	}
 
 	ext4_debug("mark block bitmap %#04llx (+%llu)\n", input->block_bitmap,
 		   input->block_bitmap - start);

@@ -288,9 +285,8 @@ static int setup_new_group_blocks(struct super_block *sb,
 	err = sb_issue_zeroout(sb, block, sbi->s_itb_per_group, GFP_NOFS);
 	if (err)
 		goto exit_bh;
-	for (i = 0, bit = input->inode_table - start;
-	     i < sbi->s_itb_per_group; i++, bit++)
-		ext4_set_bit(bit, bh->b_data);
+	ext4_set_bits(bh->b_data, input->inode_table - start,
+		      sbi->s_itb_per_group);
 
 	if ((err = extend_or_restart_transaction(handle, 2, bh)))
 		goto exit_bh;

-- 
1.7.5.1

This patch simplifies journal handling in ext4_setup_new_group_blocks().


Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c91653c..ac5b63d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -171,8 +171,7 @@ static struct buffer_head *bclean(handle_t *handle, struct super_block *sb,
  * If that fails, restart the transaction & regain write access for the
  * buffer head which is used for block_bitmap modifications.
  */
-static int extend_or_restart_transaction(handle_t *handle, int thresh,
-					 struct buffer_head *bh)
+static int extend_or_restart_transaction(handle_t *handle, int thresh)
 {
 	int err;
 

@@ -183,9 +182,8 @@ static int extend_or_restart_transaction(handle_t *handle, int thresh,
 	if (err < 0)
 		return err;
 	if (err) {
-		if ((err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA)))
-			return err;
-		if ((err = ext4_journal_get_write_access(handle, bh)))
+		err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA);
+		if (err)
 			return err;
 	}
 

@@ -222,29 +220,24 @@ static int setup_new_group_blocks(struct super_block *sb,
 
 	BUG_ON(input->group != sbi->s_groups_count);
 
-	if (IS_ERR(bh = bclean(handle, sb, input->block_bitmap))) {
-		err = PTR_ERR(bh);
-		goto exit_journal;
-	}
-
 	/* Copy all of the GDT blocks into the backup in this group */
 	for (i = 0, bit = 1, block = start + 1;
 	     i < gdblocks; i++, block++, bit++) {
 		struct buffer_head *gdb;
 
 		ext4_debug("update backup group %#04llx (+%d)\n", block, bit);
-
-		if ((err = extend_or_restart_transaction(handle, 1, bh)))
-			goto exit_bh;
+		err = extend_or_restart_transaction(handle, 1);
+		if (err)
+			goto exit_journal;
 
 		gdb = sb_getblk(sb, block);
 		if (!gdb) {
 			err = -EIO;
-			goto exit_bh;
+			goto exit_journal;
 		}
 		if ((err = ext4_journal_get_write_access(handle, gdb))) {
 			brelse(gdb);
-			goto exit_bh;
+			goto exit_journal;
 		}
 		lock_buffer(gdb);
 		memcpy(gdb->b_data, sbi->s_group_desc[i]->b_data, gdb->b_size);

@@ -253,7 +246,7 @@ static int setup_new_group_blocks(struct super_block *sb,
 		err = ext4_handle_dirty_metadata(handle, NULL, gdb);
 		if (unlikely(err)) {
 			brelse(gdb);
-			goto exit_bh;
+			goto exit_journal;
 		}
 		brelse(gdb);
 	}

@@ -264,7 +257,17 @@ static int setup_new_group_blocks(struct super_block *sb,
 	err = sb_issue_zeroout(sb, gdblocks + start + 1, reserved_gdb,
 			       GFP_NOFS);
 	if (err)
-		goto exit_bh;
+		goto exit_journal;
+
+	err = extend_or_restart_transaction(handle, 2);
+	if (err)
+		goto exit_journal;
+
+	bh = bclean(handle, sb, input->block_bitmap);
+	if (IS_ERR(bh)) {
+		err = PTR_ERR(bh);
+		goto exit_journal;
+	}
 
 	if (ext4_bg_has_super(sb, input->group)) {
 		ext4_debug("mark backup group tables %#04llx (+0)\n", start);

@@ -288,8 +291,6 @@ static int setup_new_group_blocks(struct super_block *sb,
 	ext4_set_bits(bh->b_data, input->inode_table - start,
 		      sbi->s_itb_per_group);
 
-	if ((err = extend_or_restart_transaction(handle, 2, bh)))
-		goto exit_bh;
 
 	ext4_mark_bitmap_end(input->blocks_count, sb->s_blocksize * 8,
 			     bh->b_data);

-- 
1.7.5.1

Nobody touchs blocks beyond the filesystem, there is no need to lock
the buffers.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index ac5b63d..e452bee 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -157,10 +157,8 @@ static struct buffer_head *bclean(handle_t *handle, struct super_block *sb,
 		brelse(bh);
 		bh = ERR_PTR(err);
 	} else {
-		lock_buffer(bh);
 		memset(bh->b_data, 0, sb->s_blocksize);
 		set_buffer_uptodate(bh);
-		unlock_buffer(bh);
 	}
 
 	return bh;

@@ -239,10 +237,8 @@ static int setup_new_group_blocks(struct super_block *sb,
 			brelse(gdb);
 			goto exit_journal;
 		}
-		lock_buffer(gdb);
 		memcpy(gdb->b_data, sbi->s_group_desc[i]->b_data, gdb->b_size);
 		set_buffer_uptodate(gdb);
-		unlock_buffer(gdb);
 		err = ext4_handle_dirty_metadata(handle, NULL, gdb);
 		if (unlikely(err)) {
 			brelse(gdb);

-- 
1.7.5.1

add_new_gdb() only needs the no. of a group, there is no need to pass
a pointer to struct ext4_new_group_data to add_new_gdb().

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index e452bee..52d0426 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -404,15 +404,15 @@ static int verify_reserved_gdb(struct super_block *sb,
  * fail once we start modifying the data on disk, because JBD has no rollback.
  */
 static int add_new_gdb(handle_t *handle, struct inode *inode,
-		       struct ext4_new_group_data *input,
-		       struct buffer_head **primary)
+		       ext4_group_t group)
 {
 	struct super_block *sb = inode->i_sb;
 	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
-	unsigned long gdb_num = input->group / EXT4_DESC_PER_BLOCK(sb);
+	unsigned long gdb_num = group / EXT4_DESC_PER_BLOCK(sb);
 	ext4_fsblk_t gdblock = EXT4_SB(sb)->s_sbh->b_blocknr + 1 + gdb_num;
 	struct buffer_head **o_group_desc, **n_group_desc;
 	struct buffer_head *dind;
+	struct buffer_head *gdb_bh;
 	int gdbackups;
 	struct ext4_iloc iloc;
 	__le32 *data;

@@ -435,11 +435,12 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 		return -EPERM;
 	}
 
-	*primary = sb_bread(sb, gdblock);
-	if (!*primary)
+	gdb_bh = sb_bread(sb, gdblock);
+	if (!gdb_bh)
 		return -EIO;
 
-	if ((gdbackups = verify_reserved_gdb(sb, *primary)) < 0) {
+	gdbackups = verify_reserved_gdb(sb, gdb_bh);
+	if (gdbackups < 0) {
 		err = gdbackups;
 		goto exit_bh;
 	}

@@ -454,7 +455,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	data = (__le32 *)dind->b_data;
 	if (le32_to_cpu(data[gdb_num % EXT4_ADDR_PER_BLOCK(sb)]) != gdblock) {
 		ext4_warning(sb, "new group %u GDT block %llu not reserved",
-			     input->group, gdblock);
+			     group, gdblock);
 		err = -EINVAL;
 		goto exit_dind;
 	}

@@ -463,7 +464,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	if (unlikely(err))
 		goto exit_dind;
 
-	err = ext4_journal_get_write_access(handle, *primary);
+	err = ext4_journal_get_write_access(handle, gdb_bh);
 	if (unlikely(err))
 		goto exit_sbh;
 

@@ -502,8 +503,8 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	}
 	inode->i_blocks -= (gdbackups + 1) * sb->s_blocksize >> 9;
 	ext4_mark_iloc_dirty(handle, inode, &iloc);
-	memset((*primary)->b_data, 0, sb->s_blocksize);
-	err = ext4_handle_dirty_metadata(handle, NULL, *primary);
+	memset(gdb_bh->b_data, 0, sb->s_blocksize);
+	err = ext4_handle_dirty_metadata(handle, NULL, gdb_bh);
 	if (unlikely(err)) {
 		ext4_std_error(sb, err);
 		goto exit_inode;

@@ -513,7 +514,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	o_group_desc = EXT4_SB(sb)->s_group_desc;
 	memcpy(n_group_desc, o_group_desc,
 	       EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
-	n_group_desc[gdb_num] = *primary;
+	n_group_desc[gdb_num] = gdb_bh;
 	EXT4_SB(sb)->s_group_desc = n_group_desc;
 	EXT4_SB(sb)->s_gdb_count++;
 	kfree(o_group_desc);

@@ -535,7 +536,7 @@ exit_sbh:
 exit_dind:
 	brelse(dind);
 exit_bh:
-	brelse(*primary);
+	brelse(gdb_bh);
 
 	ext4_debug("leaving with error %d\n", err);
 	return err;

@@ -843,8 +844,12 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 		if (reserved_gdb && ext4_bg_num_gdb(sb, input->group) &&
 		    (err = reserve_backup_gdb(handle, inode, input)))
 			goto exit_journal;
-	} else if ((err = add_new_gdb(handle, inode, input, &primary)))
-		goto exit_journal;
+	} else {
+		err = add_new_gdb(handle, inode, input->group);
+		if (err)
+			goto exit_journal;
+		primary = sbi->s_group_desc[gdb_num];
+	}
 
         /*
          * OK, now we've set up the new group.  Time to make it active.

-- 
1.7.5.1

reserve_backup_gdb() only needs the no. of a group, there is no need to pass
a pointer to struct ext4_new_group_data to it.

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 52d0426..3e790ad 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -556,7 +556,7 @@ exit_bh:
  * backup GDT blocks are stored in their reserved primary GDT block.
  */
 static int reserve_backup_gdb(handle_t *handle, struct inode *inode,
-			      struct ext4_new_group_data *input)
+			      ext4_group_t group)
 {
 	struct super_block *sb = inode->i_sb;
 	int reserved_gdb =le16_to_cpu(EXT4_SB(sb)->s_es->s_reserved_gdt_blocks);

@@ -627,7 +627,7 @@ static int reserve_backup_gdb(handle_t *handle, struct inode *inode,
 	 * Finally we can add each of the reserved backup GDT blocks from
 	 * the new group to its reserved primary GDT block.
 	 */
-	blk = input->group * EXT4_BLOCKS_PER_GROUP(sb);
+	blk = group * EXT4_BLOCKS_PER_GROUP(sb);
 	for (i = 0; i < reserved_gdb; i++) {
 		int err2;
 		data = (__le32 *)primary[i]->b_data;

@@ -841,9 +841,11 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 		if ((err = ext4_journal_get_write_access(handle, primary)))
 			goto exit_journal;
 
-		if (reserved_gdb && ext4_bg_num_gdb(sb, input->group) &&
-		    (err = reserve_backup_gdb(handle, inode, input)))
-			goto exit_journal;
+		if (reserved_gdb && ext4_bg_num_gdb(sb, input->group)) {
+			err = reserve_backup_gdb(handle, inode, input->group);
+			if (err)
+				goto exit_journal;
+		}
 	} else {
 		err = add_new_gdb(handle, inode, input->group);
 		if (err)

-- 
1.7.5.1

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

you removed an ext4_journal_get_write_access here, and I didn't see you
add it back anywhere.

Thanks
Tao

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

This patch prevents a fs without journal from being resized, because
it is easy to detroy the fs.

Why you want to do this?  You see any corruption?
At least in our product system, no-journal mode is heavily used and we
really don't want to disable this feature.

Thanks
Tao

On Mon, Jul 18, 2011 at 11:09 AM, Tao Ma [off-list ref] wrote:

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

quoted

This patch simplifies journal handling in ext4_setup_new_group_blocks().


Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c91653c..ac5b63d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -171,8 +171,7 @@ static struct buffer_head *bclean(handle_t *handle, struct super_block *sb,

  * If that fails, restart the transaction & regain write access for the
  * buffer head which is used for block_bitmap modifications.
  */
-static int extend_or_restart_transaction(handle_t *handle, int thresh,
-                                      struct buffer_head *bh)
+static int extend_or_restart_transaction(handle_t *handle, int thresh)
 {
      int err;

@@ -183,9 +182,8 @@ static int extend_or_restart_transaction(handle_t *handle, int thresh,

      if (err < 0)
              return err;
      if (err) {
-             if ((err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA)))
-                     return err;
-             if ((err = ext4_journal_get_write_access(handle, bh)))
+             err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA);
+             if (err)
                      return err;

you removed an ext4_journal_get_write_access here, and I didn't see you
add it back anywhere.

This get_write_access() was used to guarantee modification on the
block bitmap is in the new handle in previous code.  In previous code,
bitmap is modified everywhere in setup_group_blocks(),  actually, this
makes things complicated, in this patch, the modifications on block
bitmap are batched and are done by ext4_set_bits() after
extend_or_restart_transaction(), so it is ok.

Thanks,
Yongqiang.

Thanks
Tao

quoted

      }

@@ -222,29 +220,24 @@ static int setup_new_group_blocks(struct super_block *sb,

      BUG_ON(input->group != sbi->s_groups_count);

-     if (IS_ERR(bh = bclean(handle, sb, input->block_bitmap))) {
-             err = PTR_ERR(bh);
-             goto exit_journal;
-     }
-
      /* Copy all of the GDT blocks into the backup in this group */
      for (i = 0, bit = 1, block = start + 1;
           i < gdblocks; i++, block++, bit++) {
              struct buffer_head *gdb;

              ext4_debug("update backup group %#04llx (+%d)\n", block, bit);
-
-             if ((err = extend_or_restart_transaction(handle, 1, bh)))
-                     goto exit_bh;
+             err = extend_or_restart_transaction(handle, 1);
+             if (err)
+                     goto exit_journal;

              gdb = sb_getblk(sb, block);
              if (!gdb) {
                      err = -EIO;
-                     goto exit_bh;
+                     goto exit_journal;
              }
              if ((err = ext4_journal_get_write_access(handle, gdb))) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              lock_buffer(gdb);
              memcpy(gdb->b_data, sbi->s_group_desc[i]->b_data, gdb->b_size);

@@ -253,7 +246,7 @@ static int setup_new_group_blocks(struct super_block *sb,

              err = ext4_handle_dirty_metadata(handle, NULL, gdb);
              if (unlikely(err)) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              brelse(gdb);
      }

@@ -264,7 +257,17 @@ static int setup_new_group_blocks(struct super_block *sb,

      err = sb_issue_zeroout(sb, gdblocks + start + 1, reserved_gdb,
                             GFP_NOFS);
      if (err)
-             goto exit_bh;
+             goto exit_journal;
+
+     err = extend_or_restart_transaction(handle, 2);
+     if (err)
+             goto exit_journal;
+
+     bh = bclean(handle, sb, input->block_bitmap);
+     if (IS_ERR(bh)) {
+             err = PTR_ERR(bh);
+             goto exit_journal;
+     }

      if (ext4_bg_has_super(sb, input->group)) {
              ext4_debug("mark backup group tables %#04llx (+0)\n", start);

@@ -288,8 +291,6 @@ static int setup_new_group_blocks(struct super_block *sb,

      ext4_set_bits(bh->b_data, input->inode_table - start,
                    sbi->s_itb_per_group);

-     if ((err = extend_or_restart_transaction(handle, 2, bh)))
-             goto exit_bh;

      ext4_mark_bitmap_end(input->blocks_count, sb->s_blocksize * 8,
                           bh->b_data);

-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jul 18, 2011 at 11:17 AM, Tao Ma [off-list ref] wrote:

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

quoted

This patch prevents a fs without journal from being resized, because
it is easy to detroy the fs.

Why you want to do this?  You see any corruption?
At least in our product system, no-journal mode is heavily used and we
really don't want to disable this feature.

I did not see any corruption.  If there is no journal in a fs, then if
an error happens during online resizing, the filesystem will be
destroyed easily, I thought.  Just my thought:-)  It needs much more
feedbacks.

Thanks,
Yongqiang.

Thanks
Tao

quoted

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 53d9795..33ab40d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -33,6 +33,16 @@ int ext4_resize_begin(struct super_block *sb)

              return -EPERM;
      }

+     /*
+      * We are not allowed to do online-resizing on a filesystem without
+      * journal, otherwise, it is easy to destroy the filesystem.
+      */
+     if (!EXT4_SB(sb)->s_journal) {
+             ext4_warning(sb, "There is no journal for the filesystem, "
+                          "so online resizing is not allowed\n");
+             return -EPERM;
+     }
+
      if (test_and_set_bit_lock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags))
              ret = -EBUSY;

-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 07/18/2011 11:21 AM, Yongqiang Yang wrote:

On Mon, Jul 18, 2011 at 11:09 AM, Tao Ma [off-list ref] wrote:

quoted

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

quoted

This patch simplifies journal handling in ext4_setup_new_group_blocks().


Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c91653c..ac5b63d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -171,8 +171,7 @@ static struct buffer_head *bclean(handle_t *handle, struct super_block *sb,
  * If that fails, restart the transaction & regain write access for the
  * buffer head which is used for block_bitmap modifications.
  */
-static int extend_or_restart_transaction(handle_t *handle, int thresh,
-                                      struct buffer_head *bh)
+static int extend_or_restart_transaction(handle_t *handle, int thresh)
 {
      int err;

@@ -183,9 +182,8 @@ static int extend_or_restart_transaction(handle_t *handle, int thresh,
      if (err < 0)
              return err;
      if (err) {
-             if ((err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA)))
-                     return err;
-             if ((err = ext4_journal_get_write_access(handle, bh)))
+             err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA);
+             if (err)
                      return err;

you removed an ext4_journal_get_write_access here, and I didn't see you
add it back anywhere.

This get_write_access() was used to guarantee modification on the
block bitmap is in the new handle in previous code.  In previous code,
bitmap is modified everywhere in setup_group_blocks(),  actually, this
makes things complicated, in this patch, the modifications on block
bitmap are batched and are done by ext4_set_bits() after
extend_or_restart_transaction(), so it is ok.

oh, I see. You moved the initialization of bh after the extension of
journal. OK, but please do describe all your changes in more detail in
the commit log.

Thanks
Tao

Thanks,
Yongqiang.

quoted

Thanks
Tao

quoted

      }

@@ -222,29 +220,24 @@ static int setup_new_group_blocks(struct super_block *sb,

      BUG_ON(input->group != sbi->s_groups_count);

-     if (IS_ERR(bh = bclean(handle, sb, input->block_bitmap))) {
-             err = PTR_ERR(bh);
-             goto exit_journal;
-     }
-
      /* Copy all of the GDT blocks into the backup in this group */
      for (i = 0, bit = 1, block = start + 1;
           i < gdblocks; i++, block++, bit++) {
              struct buffer_head *gdb;

              ext4_debug("update backup group %#04llx (+%d)\n", block, bit);
-
-             if ((err = extend_or_restart_transaction(handle, 1, bh)))
-                     goto exit_bh;
+             err = extend_or_restart_transaction(handle, 1);
+             if (err)
+                     goto exit_journal;

              gdb = sb_getblk(sb, block);
              if (!gdb) {
                      err = -EIO;
-                     goto exit_bh;
+                     goto exit_journal;
              }
              if ((err = ext4_journal_get_write_access(handle, gdb))) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              lock_buffer(gdb);
              memcpy(gdb->b_data, sbi->s_group_desc[i]->b_data, gdb->b_size);

@@ -253,7 +246,7 @@ static int setup_new_group_blocks(struct super_block *sb,
              err = ext4_handle_dirty_metadata(handle, NULL, gdb);
              if (unlikely(err)) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              brelse(gdb);
      }

@@ -264,7 +257,17 @@ static int setup_new_group_blocks(struct super_block *sb,
      err = sb_issue_zeroout(sb, gdblocks + start + 1, reserved_gdb,
                             GFP_NOFS);
      if (err)
-             goto exit_bh;
+             goto exit_journal;
+
+     err = extend_or_restart_transaction(handle, 2);
+     if (err)
+             goto exit_journal;
+
+     bh = bclean(handle, sb, input->block_bitmap);
+     if (IS_ERR(bh)) {
+             err = PTR_ERR(bh);
+             goto exit_journal;
+     }

      if (ext4_bg_has_super(sb, input->group)) {
              ext4_debug("mark backup group tables %#04llx (+0)\n", start);

@@ -288,8 +291,6 @@ static int setup_new_group_blocks(struct super_block *sb,
      ext4_set_bits(bh->b_data, input->inode_table - start,
                    sbi->s_itb_per_group);

-     if ((err = extend_or_restart_transaction(handle, 2, bh)))
-             goto exit_bh;

      ext4_mark_bitmap_end(input->blocks_count, sb->s_blocksize * 8,
                           bh->b_data);

On Mon, Jul 18, 2011 at 11:28 AM, Yongqiang Yang [off-list ref] wrote:

On Mon, Jul 18, 2011 at 11:17 AM, Tao Ma [off-list ref] wrote:

quoted

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

quoted

This patch prevents a fs without journal from being resized, because
it is easy to detroy the fs.

Why you want to do this?  You see any corruption?
At least in our product system, no-journal mode is heavily used and we
really don't want to disable this feature.

Let's assume a situation without journal.  If the online resizing is
done successfully, it dirties super block and returns, then the added
groups could be used and some data are written to the added groups,
now comes an error before the super block are flushed. Could e2fsck
can find data in the added groups?  If not, this may bring something
strange to users.

It seems that super block should be flushed by online resizing, not
just be dirtied. :-)
Yongqiang.

I did not see any corruption.  If there is no journal in a fs, then if
an error happens during online resizing, the filesystem will be
destroyed easily, I thought.  Just my thought:-)  It needs much more
feedbacks.

Thanks,
Yongqiang.

quoted

Thanks
Tao

quoted

Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 53d9795..33ab40d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -33,6 +33,16 @@ int ext4_resize_begin(struct super_block *sb)

              return -EPERM;
      }

+     /*
+      * We are not allowed to do online-resizing on a filesystem without
+      * journal, otherwise, it is easy to destroy the filesystem.
+      */
+     if (!EXT4_SB(sb)->s_journal) {
+             ext4_warning(sb, "There is no journal for the filesystem, "
+                          "so online resizing is not allowed\n");
+             return -EPERM;
+     }
+
      if (test_and_set_bit_lock(EXT4_RESIZING, &EXT4_SB(sb)->s_resize_flags))
              ret = -EBUSY;

--
Best Wishes
Yongqiang Yang

-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jul 18, 2011 at 11:40 AM, Tao Ma [off-list ref] wrote:

On 07/18/2011 11:21 AM, Yongqiang Yang wrote:

quoted

On Mon, Jul 18, 2011 at 11:09 AM, Tao Ma [off-list ref] wrote:

quoted

On 07/18/2011 10:52 AM, Yongqiang Yang wrote:

quoted

This patch simplifies journal handling in ext4_setup_new_group_blocks().


Signed-off-by: Yongqiang Yang <redacted>
---
 fs/ext4/resize.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c91653c..ac5b63d 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -171,8 +171,7 @@ static struct buffer_head *bclean(handle_t *handle, struct super_block *sb,

  * If that fails, restart the transaction & regain write access for the
  * buffer head which is used for block_bitmap modifications.
  */
-static int extend_or_restart_transaction(handle_t *handle, int thresh,
-                                      struct buffer_head *bh)
+static int extend_or_restart_transaction(handle_t *handle, int thresh)
 {
      int err;

@@ -183,9 +182,8 @@ static int extend_or_restart_transaction(handle_t *handle, int thresh,

      if (err < 0)
              return err;
      if (err) {
-             if ((err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA)))
-                     return err;
-             if ((err = ext4_journal_get_write_access(handle, bh)))
+             err = ext4_journal_restart(handle, EXT4_MAX_TRANS_DATA);
+             if (err)
                      return err;

you removed an ext4_journal_get_write_access here, and I didn't see you
add it back anywhere.

This get_write_access() was used to guarantee modification on the
block bitmap is in the new handle in previous code.  In previous code,
bitmap is modified everywhere in setup_group_blocks(),  actually, this
makes things complicated, in this patch, the modifications on block
bitmap are batched and are done by ext4_set_bits() after
extend_or_restart_transaction(), so it is ok.

oh, I see. You moved the initialization of bh after the extension of
journal. OK, but please do describe all your changes in more detail in
the commit log.

Yes, my carelessness.

Thank you for your review.

Yongqiang.

Thanks
Tao

quoted

Thanks,
Yongqiang.

quoted

Thanks
Tao

quoted

      }

@@ -222,29 +220,24 @@ static int setup_new_group_blocks(struct super_block *sb,

      BUG_ON(input->group != sbi->s_groups_count);

-     if (IS_ERR(bh = bclean(handle, sb, input->block_bitmap))) {
-             err = PTR_ERR(bh);
-             goto exit_journal;
-     }
-
      /* Copy all of the GDT blocks into the backup in this group */
      for (i = 0, bit = 1, block = start + 1;
           i < gdblocks; i++, block++, bit++) {
              struct buffer_head *gdb;

              ext4_debug("update backup group %#04llx (+%d)\n", block, bit);
-
-             if ((err = extend_or_restart_transaction(handle, 1, bh)))
-                     goto exit_bh;
+             err = extend_or_restart_transaction(handle, 1);
+             if (err)
+                     goto exit_journal;

              gdb = sb_getblk(sb, block);
              if (!gdb) {
                      err = -EIO;
-                     goto exit_bh;
+                     goto exit_journal;
              }
              if ((err = ext4_journal_get_write_access(handle, gdb))) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              lock_buffer(gdb);
              memcpy(gdb->b_data, sbi->s_group_desc[i]->b_data, gdb->b_size);

@@ -253,7 +246,7 @@ static int setup_new_group_blocks(struct super_block *sb,

              err = ext4_handle_dirty_metadata(handle, NULL, gdb);
              if (unlikely(err)) {
                      brelse(gdb);
-                     goto exit_bh;
+                     goto exit_journal;
              }
              brelse(gdb);
      }

@@ -264,7 +257,17 @@ static int setup_new_group_blocks(struct super_block *sb,

      err = sb_issue_zeroout(sb, gdblocks + start + 1, reserved_gdb,
                             GFP_NOFS);
      if (err)
-             goto exit_bh;
+             goto exit_journal;
+
+     err = extend_or_restart_transaction(handle, 2);
+     if (err)
+             goto exit_journal;
+
+     bh = bclean(handle, sb, input->block_bitmap);
+     if (IS_ERR(bh)) {
+             err = PTR_ERR(bh);
+             goto exit_journal;
+     }

      if (ext4_bg_has_super(sb, input->group)) {
              ext4_debug("mark backup group tables %#04llx (+0)\n", start);

@@ -288,8 +291,6 @@ static int setup_new_group_blocks(struct super_block *sb,

      ext4_set_bits(bh->b_data, input->inode_table - start,
                    sbi->s_itb_per_group);

-     if ((err = extend_or_restart_transaction(handle, 2, bh)))
-             goto exit_bh;

      ext4_mark_bitmap_end(input->blocks_count, sb->s_blocksize * 8,
                           bh->b_data);

-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jul 18, 2011 at 5:52 AM, Yongqiang Yang [off-list ref] wrote:

Hi,

 This patch series prevents parallel resizer and fixes some error handling in
resize code.  Besides these, some code is simplified so that the code can be
used easily in new resize implementation.

You should probably mention that "new resize implementation" means:
let the kernel do all the resize work, gaining fast multi group resize,
flex_bg layout, and uninit block groups.
The new kernel resize implementation also prepares the ground for
supporting resize with features like big_alloc, 64bit and exclude_bitmap.

You should probably also mention that you have already completed the
"new resize implementation" and that you will post it shortly ;-)

 The patches are tested by resize2fs and e2fsck -fn.

[PATCH 01/12] ext4: prevent parallel resizers by atomic bit ops
[PATCH 02/12] ext4: prevent a fs with errors from being resized
[PATCH 03/12] ext4: prevent a fs without journal from being resized
[PATCH 04/12] ext4: rename ext4_add_groupblocks() to
[PATCH 05/12] ext4: let ext4_group_add_blocks return an error code
[PATCH 06/12] ext4: let ext4_group_add_blocks() handle 0 blocks
[PATCH 07/12] ext4: fix a typo in ext4_group_extend()
[PATCH 08/12] ext4: let setup_new_group_blocks set multi-bits each
[PATCH 09/12] ext4: simplify journal handling in
[PATCH 10/12] ext4: remove lock_buffer in bclean() and
[PATCH 11/12] ext4: simplify parameters of add_new_gdb()
[PATCH 12/12] ext4: simplify parameters of reserve_backup_gdb()

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:

This patch lets ext4_group_add_blocks() return an error code if it fails,
so that upper functions can handle error correctly.

This patch is somewhat incorrect, though it seems the existing code is
also incorrect, which isn't the fault of this patch, but should be
fixed if this code is being reworked.

When this code was originally written, this code could not fail
unless the filesystem had been marked read-only or the journal was
aborted.  It only freed blocks using ext3_free_blocks_sb(), which
only had error paths calling ext3_error().

It was purposely done that way because all of the failure cases
(e.g. not being able to read the block bitmap) were checked in
advance, before modifying any of the filesystem metadata.

In the case of this patch, ext4_blocks_count_set() is called to
add the new blocks to the total in the superblock.  If this code
fails, it doesn't try to reset the total number of blocks in the
superblock, which can cause e2fsck to be unhappy.

Looking at this code more closely, it seems that ext4_group_add_blocks()
(formerly ext4_add_groupblocks()) is now only used by the resize code,
which IMHO is wrong.  What it was doing in the past was using existing
code to "free" the blocks at the end of the block bitmap, as if a file
had just been deleted.  As it is now, it seems to be duplicating a lot
of what is in ext4_free_blocks().

It probably makes sense to try and get some consolidated helper
function that includes nearly all of the code in ext4_free_blocks between
"do_more:" and "error_return:" (all of the work of verifying the blocks
to be freed, updating the block bitmap and the buddy bitmap, updates the
group descriptors and superblock, and marks the blocks dirty in the
journal), but not the code that checks the writeback mode, updates quota,
or any of that (since this isn't really using an inode).

Cheers, Andreas

On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:

This patch adds a function - ext4_set_btis() which can set multi-bits
each time, and lets setup_new_group_blocks() use ext4_set_bits().


+void ext4_set_bits(void *bm, int cur, int len)
+{
+	mb_set_bits(bm, cur, len);
+}

Why not just rename mb_set_bits() to ext4_set_bits()?  That could be done
in one patch to avoid complexity.

Cheers, Andreas

On Mon, Jul 18, 2011 at 5:52 AM, Yongqiang Yang [off-list ref] wrote:

This patch renamed ext4_add_groupblocks() to ext4_group_add_blocks().

What is the reason for the rename?

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:

add_new_gdb() only needs the no. of a group, there is no need to pass
a pointer to struct ext4_new_group_data to add_new_gdb().

static int add_new_gdb(handle_t *handle, struct inode *inode,
-		       struct ext4_new_group_data *input,
-		       struct buffer_head **primary)
+		       ext4_group_t group)

This patch is changing the code in a subtle, but not incorrect way.
At a minimum, the commit comment should also mention why "primary"
can be removed as an argument.  That is because add_new_gdb() is
storing the bh pointer for the new group descriptor block into
s_group_desc[] internally:

Here...

and the caller can safely access this after the function returns.  It
would be incorrect to "optimize" this code to consolidate the setting
of "primary" from sbi->s_group_desc[gdb_num] before the if/else clause,
so this should get a comment that this array element is only valid after
add_new_gdb() returns.

Cheers, Andreas

On Mon, Jul 18, 2011 at 9:19 AM, Andreas Dilger [off-list ref] wrote:

On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:

quoted

This patch lets ext4_group_add_blocks() return an error code if it fails,
so that upper functions can handle error correctly.

This patch is somewhat incorrect, though it seems the existing code is
also incorrect, which isn't the fault of this patch, but should be
fixed if this code is being reworked.

When this code was originally written, this code could not fail
unless the filesystem had been marked read-only or the journal was
aborted.  It only freed blocks using ext3_free_blocks_sb(), which
only had error paths calling ext3_error().

It was purposely done that way because all of the failure cases
(e.g. not being able to read the block bitmap) were checked in
advance, before modifying any of the filesystem metadata.

In the case of this patch, ext4_blocks_count_set() is called to
add the new blocks to the total in the superblock.  If this code
fails, it doesn't try to reset the total number of blocks in the
superblock, which can cause e2fsck to be unhappy.

Looking at this code more closely, it seems that ext4_group_add_blocks()
(formerly ext4_add_groupblocks()) is now only used by the resize code,
which IMHO is wrong.  What it was doing in the past was using existing
code to "free" the blocks at the end of the block bitmap, as if a file
had just been deleted.  As it is now, it seems to be duplicating a lot
of what is in ext4_free_blocks().

This is my (git) blame.
Before I had my way with ext4_add_groupblocks() it was practically using
old remains of ext3_free_blocks_sb() code (see commit e73a347b).
To make things work better (without a rw_sem) I copied code from
ext4_free_blocks(), not messing with the latter on purpose.
Now that my changes have proven to work (?), the core freeing of
the blocks can be broken out to a helper function.

It probably makes sense to try and get some consolidated helper
function that includes nearly all of the code in ext4_free_blocks between
"do_more:" and "error_return:" (all of the work of verifying the blocks
to be freed, updating the block bitmap and the buddy bitmap, updates the
group descriptors and superblock, and marks the blocks dirty in the
journal), but not the code that checks the writeback mode, updates quota,
or any of that (since this isn't really using an inode).

quoted

Signed-off-by: Yongqiang Yang <redacted>
---
fs/ext4/ext4.h    |    2 +-
fs/ext4/mballoc.c |   19 +++++++++++++------
fs/ext4/resize.c  |   10 +++++++---
3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bbe81db..da7ab48 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -1799,7 +1799,7 @@ extern void ext4_free_blocks(handle_t *handle, struct inode *inode,

                           unsigned long count, int flags);
extern int ext4_mb_add_groupinfo(struct super_block *sb,
              ext4_group_t i, struct ext4_group_desc *desc);
-extern void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
+extern int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
                              ext4_fsblk_t block, unsigned long count);
extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ea80c0b..33c41e6 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c

@@ -4664,7 +4664,7 @@ error_return:

 *
 * This marks the blocks as free in the bitmap and buddy.
 */
-void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
+int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
                       ext4_fsblk_t block, unsigned long count)
{
      struct buffer_head *bitmap_bh = NULL;

@@ -4685,15 +4685,21 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,

       * Check to see if we are freeing blocks across a group
       * boundary.
       */
-     if (bit + count > EXT4_BLOCKS_PER_GROUP(sb))
-             goto error_return;
+     if (bit + count > EXT4_BLOCKS_PER_GROUP(sb)) {
+             ext4_warning(sb, "too much blocks added to group %u\n",
+                          block_group);
+             return -EINVAL;
+     }

      bitmap_bh = ext4_read_block_bitmap(sb, block_group);
      if (!bitmap_bh)
-             goto error_return;
+             return -EIO;
+
      desc = ext4_get_group_desc(sb, block_group, &gd_bh);
-     if (!desc)
+     if (!desc) {
+             err = -EIO;
              goto error_return;
+     }

      if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
          in_range(ext4_inode_bitmap(sb, desc), block, count) ||

@@ -4703,6 +4709,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,

              ext4_error(sb, "Adding blocks in system zones - "
                         "Block = %llu, count = %lu",
                         block, count);
+             err = -EINVAL;
              goto error_return;
      }

@@ -4771,7 +4778,7 @@ void ext4_group_add_blocks(handle_t *handle, struct super_block *sb,

error_return:
      brelse(bitmap_bh);
      ext4_std_error(sb, err);
-     return;
+     return err;
}

/**

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 374fd1c..2e63376 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c

@@ -990,7 +990,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,

      ext4_grpblk_t add;
      struct buffer_head *bh;
      handle_t *handle;
-     int err;
+     int err, err2;
      ext4_group_t group;

      o_blocks_count = ext4_blocks_count(es);

@@ -1066,11 +1066,15 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,

      ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
                 o_blocks_count + add);
      /* We add the blocks to the bitmap and set the group need init bit */
-     ext4_group_add_blocks(handle, sb, o_blocks_count, add);
+     err = ext4_group_add_blocks(handle, sb, o_blocks_count, add);
      ext4_handle_dirty_super(handle, sb);
      ext4_debug("freed blocks %llu through %llu\n", o_blocks_count,
                 o_blocks_count + add);
-     if ((err = ext4_journal_stop(handle)))
+     err2 = ext4_journal_stop(handle);
+     if (!err && err2)
+             err = err2;
+
+     if (err)
              goto exit_put;

      if (test_opt(sb, DEBUG))
--
1.7.5.1

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html