[1/5] ext4: fix race with setting free_inode/clusters_counter

Message ID 1527594317-9214-1-git-send-email-wshilong1991@gmail.com
State New
Headers show
Series
  • [1/5] ext4: fix race with setting free_inode/clusters_counter
Related show

Commit Message

Wang Shilong May 29, 2018, 11:45 a.m.
From: Wang Shilong <wshilong@ddn.com>

Whenever we hit block or inode bitmap corruptions we set
bit and then reduce this block group free inode/clusters
counter to expose right available space.

However some of ext4_mark_group_bitmap_corrupted() is called
inside group spinlock, some are not, this could make it happen
that we double reduce one block group free counters from system.

Always hold group spinlock for it could fix it, but it looks
a little heavy, we could use test_and_set_bit() to fix race
problems here.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/super.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Andreas Dilger June 4, 2018, 6 p.m. | #1
On May 29, 2018, at 5:45 AM, Wang Shilong <wangshilong1991@gmail.com> wrote:
> 
> From: Wang Shilong <wshilong@ddn.com>
> 
> Whenever we hit block or inode bitmap corruptions we set
> bit and then reduce this block group free inode/clusters
> counter to expose right available space.
> 
> However some of ext4_mark_group_bitmap_corrupted() is called
> inside group spinlock, some are not, this could make it happen
> that we double reduce one block group free counters from system.
> 
> Always hold group spinlock for it could fix it, but it looks
> a little heavy, we could use test_and_set_bit() to fix race
> problems here.
> 
> Signed-off-by: Wang Shilong <wshilong@ddn.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/super.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c1c5c87..d6fa6cf 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -770,26 +770,26 @@ void ext4_mark_group_bitmap_corrupted(struct super_block *sb,
> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
> 	struct ext4_group_info *grp = ext4_get_group_info(sb, group);
> 	struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);
> +	int ret;
> 
> -	if ((flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) &&
> -	    !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) {
> -		percpu_counter_sub(&sbi->s_freeclusters_counter,
> -					grp->bb_free);
> -		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
> -			&grp->bb_state);
> +	if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) {
> +		ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
> +					    &grp->bb_state);
> +		if (!ret)
> +			percpu_counter_sub(&sbi->s_freeclusters_counter,
> +					   grp->bb_free);
> 	}
> 
> -	if ((flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) &&
> -	    !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
> -		if (gdp) {
> +	if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) {
> +		ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
> +					    &grp->bb_state);
> +		if (!ret && gdp) {
> 			int count;
> 
> 			count = ext4_free_inodes_count(sb, gdp);
> 			percpu_counter_sub(&sbi->s_freeinodes_counter,
> 					   count);
> 		}
> -		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
> -			&grp->bb_state);
> 	}
> }
> 
> --
> 1.8.3.1
> 


Cheers, Andreas

Patch

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c1c5c87..d6fa6cf 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -770,26 +770,26 @@  void ext4_mark_group_bitmap_corrupted(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_group_info *grp = ext4_get_group_info(sb, group);
 	struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);
+	int ret;
 
-	if ((flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) &&
-	    !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) {
-		percpu_counter_sub(&sbi->s_freeclusters_counter,
-					grp->bb_free);
-		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
-			&grp->bb_state);
+	if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT) {
+		ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
+					    &grp->bb_state);
+		if (!ret)
+			percpu_counter_sub(&sbi->s_freeclusters_counter,
+					   grp->bb_free);
 	}
 
-	if ((flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) &&
-	    !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
-		if (gdp) {
+	if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT) {
+		ret = ext4_test_and_set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
+					    &grp->bb_state);
+		if (!ret && gdp) {
 			int count;
 
 			count = ext4_free_inodes_count(sb, gdp);
 			percpu_counter_sub(&sbi->s_freeinodes_counter,
 					   count);
 		}
-		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
-			&grp->bb_state);
 	}
 }