ext34_free_inode's mess

From 1857fc6c7349a67cf930e73b802427a138e43456 Mon Sep 17 00:00:00 2001

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:

fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Free inodes count wrong (35610, counted=35609).
Fix? no
...

I've started to look an inode bitmap manipulation code paths and found strange logic in ext{3,4}_free_inode functions

1) Group lock acquired twice for bitmap and for group_desc. There are not any advantage from this double locking, only error path(where the bit is already cleared) takes an advantage from this locking schema.
It is reasonable to batch it in to one locking block.
2) if we failed to read gdp then bh2 is undefined so may result in oops due to undefince pointer dereferance.
3) if we failed to get write_access to gdp we skip handle_dirty_metadata for inode_bitmap which is also a bug.

I've redesigned free_inode logic(see later two emails) and currently i'm not able to reproduce the bug, but i can not guarantee it is goes away.

From 1857fc6c7349a67cf930e73b802427a138e43456 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <[email protected]>
Date: Wed, 14 Apr 2010 14:53:47 +0400
Subject: [PATCH] xfstests-dev: add one more stress test

During stress testing we want to cover most of code paths. fsstress is very good for this purpose. But it has expandable nature (disk usage almost continually grow). So once we goes it no ENOSPC condition we will be where till the end.

But by running 'dd' in parallel we can regularly trigger ENOSPC but only for a limited periods of time.

This is my favorite stress test-case configuration.

---
 227     |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 227.out |    5 +++
 group   |    1 +
 3 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 227
 create mode 100644 227.out

227 new file +105 -0

new file mode 100755
index 0000000..d2b0c7d
@@ -0,0 +1,105 @@
+#! /bin/bash
+# FS QA Test No. 227
+#
+# Perform fsstress test with parallel dd
+# This proven to be a good stress test
+# * Continuous dd retult in ENOSPC condition but only for a limited periods
+#   of time.
+# * Fsstress test cover many code paths
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm -f $tmp.*
+}
+
+workout()
+{
+    # Disable bash job controll, to prevent message about killed task.
+    set +m
+
+    #Timing parameters
+    nr_iterations=5
+    kill_tries=20
+    echo Running fsstress. | tee -a $seq.full
+
+####################################################
+##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
+##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
+## 
+######################################################
+    mkdir -p $SCRATCH_MNT/fsstress
+    # It is reasonable to disable sync, otherwise most of tasks will simply
+    # stuck in that sync() call.
+    $FSSTRESS_PROG \
+	-d $SCRATCH_MNT/fsstress \
+	-p 100 -f sync=0  -n 9999999 > /dev/null 2>&1 &
+
+    echo Running ENOSPC hitters. | tee -a $seq.full
+    for ((i = 0; i < $nr_iterations; i++))
+    do
+	#Open with O_TRUNC and then write until error
+	#hit ENOSPC each time.
+	dd if=/dev/zero of=$SCRATCH_MNT/BIG_FILE bs=1M 2> /dev/null
+    done
+
+    for ((i = 0; i < $kill_tries; i++))
+    do
+	killall -r -q -TERM fsstress 2> /dev/null
+	sleep 1
+    done
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+rm -f $seq.full
+
+umount $TEST_DEV >/dev/null 2>&1
+umount $SCRATCH_DEV >/dev/null 2>&1
+echo "*** MKFS ***"                         >>$seq.full
+echo ""                                     >>$seq.full
+_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
+
+workout
+umount $SCRATCH_MNT
+echo 
+echo Checking filesystem
+_check_scratch_fs
+status=$?
+exit

227.out new file +5 -0

new file mode 100644
index 0000000..6a7342d
@@ -0,0 +1,5 @@
+QA output created by 227
+Running fsstress.
+Running ENOSPC hitters.
+
+Checking filesystem

group +1 -0

index 8d4a83a..81a2aa4 100644
@@ -339,3 +339,4 @@ deprecated
 223 auto quick
 224 auto
 225 auto quick
+227 rw auto prealloc enospc
\ No newline at end of file

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:
fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Free inodes count wrong (35610, counted=35609).
Fix? no
...

I've started to look an inode bitmap manipulation code paths and found strange logic in ext{3,4}_free_inode functions

1) Group lock acquired twice for bitmap and for group_desc. There are not any advantage from this double locking, only error path(where the bit is already cleared) takes an advantage from this locking schema.
It is reasonable to batch it in to one locking block.

2) if we failed to read gdp then bh2 is undefined so may result in oops due to undefince pointer dereferance.

3) if we failed to get write_access to gdp we skip handle_dirty_metadata for inode_bitmap which is also a bug.

Quoted 12 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:
fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Quoted 3 lines

Free inodes count wrong (35610, counted=35609).
Fix? no
...

Interesting. So some inode is marked as free although it is in
use, right? That sounds like a nasty bug - if you reproduce this again, could you use debugfs to find out what file type is that inode? It could help looking for the bug.

Quoted 9 lines

I've started to look an inode bitmap manipulation code paths and found strange logic in ext{3,4}_free_inode functions

1) Group lock acquired twice for bitmap and for group_desc. There are not any advantage from this double locking, only error path(where the bit is already cleared) takes an advantage from this locking schema.
It is reasonable to batch it in to one locking block.

I guess you think that this happens because we pass the lock parameter
to ext3_clear_bit_atomic. But if you would actually look at the definition of the function, you would see that it's hard to find an architecture that uses the lock. Most architectures just use atomic bitop to clear the bit. I actually fail to see why anyone would need the lock - probably Ted knows :).

Quoted 2 lines

2) if we failed to read gdp then bh2 is undefined so may result in oops due to undefince pointer dereferance.

No, because during mount time we check that all gdp pointers exist so
ext3_get_group_desc can never fail after the mount has succeeded.

Quoted 2 lines

3) if we failed to get write_access to gdp we skip handle_dirty_metadata for inode_bitmap which is also a bug.

It doesn't matter. At the moment ext3_journal_get_write_access fails we
abort the journal so no writes are allowed to the filesystem anyway. So modified bitmap has hardly any chance to get to disk and you have to run fsck to clean up the mess anyway...

Honza

I've finally automated my favorite testcase (see attachment), before i've run it by hand.

 227     |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 227.out |    5 +++
 group   |    1 +
 3 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 227
 create mode 100644 227.out

227 new file +0 -0

new file mode 100755
index 0000000..d2b0c7d

227 new file +26 -0

+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+here=`pwd`
+tmp=/tmp/$$

227 new file +53 -0

+##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
+##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
+## 
+######################################################
+    mkdir -p $SCRATCH_MNT/fsstress
+    # It is reasonable to disable sync, otherwise most of tasks will simply
+    # stuck in that sync() call.
+    $FSSTRESS_PROG \
+	-d $SCRATCH_MNT/fsstress \
+	-p 100 -f sync=0  -n 9999999 > /dev/null 2>&1 &
+
+    echo Running ENOSPC hitters. | tee -a $seq.full
+    for ((i = 0; i < $nr_iterations; i++))
+    do
+	#Open with O_TRUNC and then write until error
+	#hit ENOSPC each time.
+	dd if=/dev/zero of=$SCRATCH_MNT/BIG_FILE bs=1M 2> /dev/null
+    done
+
+    for ((i = 0; i < $kill_tries; i++))
+    do
+	killall -r -q -TERM fsstress 2> /dev/null
+	sleep 1
+    done
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+rm -f $seq.full
+
+umount $TEST_DEV >/dev/null 2>&1
+umount $SCRATCH_DEV >/dev/null 2>&1
+echo "*** MKFS ***"                         >>$seq.full
+echo ""                                     >>$seq.full
+_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
+
+workout
+umount $SCRATCH_MNT
+echo 
+echo Checking filesystem
+_check_scratch_fs
+status=$?
+exit

227.out new file +5 -0

new file mode 100644
index 0000000..6a7342d
@@ -0,0 +1,5 @@
+QA output created by 227
+Running fsstress.
+Running ENOSPC hitters.
+
+Checking filesystem

group +0 -0

index 8d4a83a..81a2aa4 100644

Quoted 12 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:
fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Quoted 3 lines

Free inodes count wrong (35610, counted=35609).
Fix? no
...

Interesting. So some inode is marked as free although it is in
use, right? That sounds like a nasty bug - if you reproduce this again, could you use debugfs to find out what file type is that inode? It could help looking for the bug.

Quoted 2 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.

Thanks! Feel free to cc: the xfs list since the patch hits xfstests. (I added it here)

Quoted 13 lines

 227     |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 227.out |    5 +++
 group   |    1 +
 3 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 227
 create mode 100644 227.out

227 new file +0 -0

new file mode 100755
index 0000000..d2b0c7d

227 line 8

Quoted 8 lines

227 new file +8 -0

@@ -0,0 +1,105 @@
+#! /bin/bash
+# FS QA Test No. 227
+#
+# Perform fsstress test with parallel dd
+# This proven to be a good stress test
+# * Continuous dd retult in ENOSPC condition but only for a limited periods
+#   of time.
+# * Fsstress test cover many code paths

just little editor nitpicks:

(quoted diff) +5 -0

+# Perform fsstress test with parallel dd
+# This is proven to be a good stress test
+# * Continuous dd results in ENOSPC condition but only for a limited period
+#   of time.
+# * Fsstress test covers many code paths

Quoted 32 lines

227 new file +30 -0

+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 Dmitry Monakhov.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{

227 line 44

Quoted 6 lines

227 new file +6 -0

+    rm -f $tmp.*
+}
+
+workout()
+{
+    # Disable bash job controll, to prevent message about killed task.

s/controll/control/

227 line 52

Quoted 9 lines

227 new file +8 -0

+    set +m
+
+    #Timing parameters
+    nr_iterations=5
+    kill_tries=20
+    echo Running fsstress. | tee -a $seq.full
+
+####################################################

What is all this for?

FWIW other fsstress tests use an $FSSTRESS_AVOID variable, where you can set the things you want to avoid easily

Quoted 70 lines

227 new file +53 -0

+##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
+##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
+## 
+######################################################
+    mkdir -p $SCRATCH_MNT/fsstress
+    # It is reasonable to disable sync, otherwise most of tasks will simply
+    # stuck in that sync() call.
+    $FSSTRESS_PROG \
+	-d $SCRATCH_MNT/fsstress \
+	-p 100 -f sync=0  -n 9999999 > /dev/null 2>&1 &
+
+    echo Running ENOSPC hitters. | tee -a $seq.full
+    for ((i = 0; i < $nr_iterations; i++))
+    do
+	#Open with O_TRUNC and then write until error
+	#hit ENOSPC each time.
+	dd if=/dev/zero of=$SCRATCH_MNT/BIG_FILE bs=1M 2> /dev/null
+    done
+
+    for ((i = 0; i < $kill_tries; i++))
+    do
+	killall -r -q -TERM fsstress 2> /dev/null
+	sleep 1
+    done
+}
+
+trap "_cleanup ; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+rm -f $seq.full
+
+umount $TEST_DEV >/dev/null 2>&1
+umount $SCRATCH_DEV >/dev/null 2>&1
+echo "*** MKFS ***"                         >>$seq.full
+echo ""                                     >>$seq.full
+_scratch_mkfs    >/dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount   >/dev/null 2>&1 || _fail "mount failed"
+
+workout
+umount $SCRATCH_MNT
+echo 
+echo Checking filesystem
+_check_scratch_fs
+status=$?
+exit

227.out new file +5 -0

new file mode 100644
index 0000000..6a7342d
@@ -0,0 +1,5 @@
+QA output created by 227
+Running fsstress.
+Running ENOSPC hitters.
+
+Checking filesystem

group +0 -0

index 8d4a83a..81a2aa4 100644
@@ -339,3 +339,4 @@ deprecated

group deprecated · line 342

Quoted 4 lines

group +1 -0

 223 auto quick
 224 auto
 225 auto quick
+227 rw auto prealloc enospc

Is this prealloc just because fsstress may run resvsp? FWIW, other fsstress tests aren't in that group, so this is as little inconsistent.

Quoted 2 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.

Thanks! Feel free to cc: the xfs list since the patch hits xfstests. (I added it here)

Quoted 13 lines

 227     |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 227.out |    5 +++
 group   |    1 +
 3 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100755 227
 create mode 100644 227.out

227 new file +0 -0

new file mode 100755
index 0000000..d2b0c7d

227 line 8

Quoted 8 lines

227 new file +8 -0

@@ -0,0 +1,105 @@
+#! /bin/bash
+# FS QA Test No. 227
+#
+# Perform fsstress test with parallel dd
+# This proven to be a good stress test
+# * Continuous dd retult in ENOSPC condition but only for a limited periods
+#   of time.
+# * Fsstress test cover many code paths

just little editor nitpicks:

227 line 52

Quoted 7 lines

227 new file +7 -0

+
+    #Timing parameters
+    nr_iterations=5
+    kill_tries=20
+    echo Running fsstress. | tee -a $seq.full
+
+####################################################

What is all this for?

FWIW other fsstress tests use an $FSSTRESS_AVOID variable, where you can set the things you want to avoid easily

Quoted 10 lines

227 new file +10 -0

+##    -f unresvsp=0 -f allocsp=0 -f freesp=0 \
+##    -f setxattr=0 -f attr_remove=0 -f attr_set=0 \
+## 
+######################################################
+    mkdir -p $SCRATCH_MNT/fsstress
+    # It is reasonable to disable sync, otherwise most of tasks will simply
+    # stuck in that sync() call.
+    $FSSTRESS_PROG \
+	-d $SCRATCH_MNT/fsstress \
+	-p 100 -f sync=0  -n 9999999 > /dev/null 2>&1 &

227 line 70

Quoted 8 lines

227 new file +8 -0

+
+    echo Running ENOSPC hitters. | tee -a $seq.full
+    for ((i = 0; i < $nr_iterations; i++))
+    do
+	#Open with O_TRUNC and then write until error
+	#hit ENOSPC each time.
+	dd if=/dev/zero of=$SCRATCH_MNT/BIG_FILE bs=1M 2> /dev/null
+    done

227 line 103

Quoted 5 lines

227 new file +5 -0

+workout
+umount $SCRATCH_MNT
+echo 
+echo Checking filesystem
+_check_scratch_fs

Quoted 16 lines · Jan Kara

Quoted 15 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:
fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Free inodes count wrong (35610, counted=35609).
Fix? no
...

Quoted 4 lines

Interesting. So some inode is marked as free although it is in
use, right? That sounds like a nasty bug - if you reproduce this again, could you use debugfs to find out what file type is that inode? It could help looking for the bug.

No problems,
wget http://download.openvz.org/~dmonakhov/junk/sdb2-2.bz2
In fact i've had even better image (with only 1 free inode in a group, but full bitmask) unfortunately i forgot to save it.

Quoted 15 lines

Quoted 8 lines

I've started to look an inode bitmap manipulation code paths and found strange logic in ext{3,4}_free_inode functions

1) Group lock acquired twice for bitmap and for group_desc. There are not any advantage from this double locking, only error path(where the bit is already cleared) takes an advantage from this locking schema.
It is reasonable to batch it in to one locking block.

I guess you think that this happens because we pass the lock parameter
to ext3_clear_bit_atomic. But if you would actually look at the definition of the function, you would see that it's hard to find an architecture that uses the lock. Most architectures just use atomic bitop to clear the bit. I actually fail to see why anyone would need the lock - probably Ted knows :).

Quoted 4 lines

Quoted 2 lines

2) if we failed to read gdp then bh2 is undefined so may result in oops due to undefince pointer dereferance.

No, because during mount time we check that all gdp pointers exist so
ext3_get_group_desc can never fail after the mount has succeeded.

Yes, that is right, why we have to check gdp to NULL when?

Quoted 18 lines · Dmitry Monakhov

Quoted 16 lines · Jan Kara

Quoted 15 lines · Dmitry Monakhov

I've finally automated my favorite testcase (see attachment), before i've run it by hand.
And sometimes i've saw following complain from fsck:
fsck.ext4 -f -n /dev/sdb2
...
Pass 5: Checking group summary information
Inode bitmap differences: -93582
Fix? no

Free inodes count wrong for group #12 (4634, counted=4633). Fix? no

Free inodes count wrong (35610, counted=35609).
Fix? no
...

Quoted 8 lines

Quoted 4 lines

Interesting. So some inode is marked as free although it is in
use, right? That sounds like a nasty bug - if you reproduce this again, could you use debugfs to find out what file type is that inode? It could help looking for the bug.

No problems,
wget http://download.openvz.org/~dmonakhov/junk/sdb2-2.bz2
In fact i've had even better image (with only 1 free inode in a group, but full bitmask) unfortunately i forgot to save it.

I've looked at it: So the problem is the other way around (I always
confuse this). The inode is properly deleted but the bit remains set in the bitmap. What is strange is that group descriptor counts are correct so it's really only the bitmap bit that is wrong. I've looked through the inode allocation and freeing code back and forth but I could not find a place where this could realistically happen.
So just for record:
Inode has mtime = ctime = atime = dtime (so it was really deleted), i_nlink = 0, it is a directory, i_disksize = 4096, i_blocks = 0. So indeed it looks that we were in ext4_mkdir, we failed to allocate the block for directory

and went to out_clear_inode (thus i_disksize remained to be set to 4096, otherwise it would be set to 0)... But how it happened that the bit in the bitmap didn't get cleared while the group descriptors were updated is beyond me.
Alternatively the inode could have been deleted just fine and later we
just set the bit in the inode bitmap and didn't update anything else. But even this does not seem to be possible to me...
Since you can reproduce it, good first step would be to

Quoted 20 lines

Quoted 20 lines

Quoted 9 lines

I've started to look an inode bitmap manipulation code paths and found strange logic in ext{3,4}_free_inode functions

1) Group lock acquired twice for bitmap and for group_desc. There are not any advantage from this double locking, only error path(where the bit is already cleared) takes an advantage from this locking schema.
It is reasonable to batch it in to one locking block.

I guess you think that this happens because we pass the lock parameter
to ext3_clear_bit_atomic. But if you would actually look at the definition of the function, you would see that it's hard to find an architecture that uses the lock. Most architectures just use atomic bitop to clear the bit. I actually fail to see why anyone would need the lock - probably Ted knows :).

Quoted 2 lines

2) if we failed to read gdp then bh2 is undefined so may result in oops due to undefince pointer dereferance.

No, because during mount time we check that all gdp pointers exist so
ext3_get_group_desc can never fail after the mount has succeeded.

Quoted 1 line

Yes, that is right, why we have to check gdp to NULL when?

Hmm, I've looked at the code again and I think the check is there mainly
to avoid Oops in case filesystem got corrupted and we computed some bogus group number. Not that I would see how that could happen in this particular case but in some other uses of ext3_get_group_desc it could happen. So moving the gdp check before we use bh2 probably makes some sence (although it's probably just a style cleanup in this case).

Ok, if we know that any error result in EIO or panic when let's just call it style cleanup(simplification), imho new code is more readable.

Here's my -V3 respin of this patch, which further cleans up the code and removes some duplicated code by only calling ext4_clear_bit() from one call site.

I think I'm about done for this, so if you agree with my improvements as improvements, it might be useful to port this back to ext3 version of this patch.

- Ted

ext4: clean up inode bitmaps manipulation in ext4_free_inode

From: Dmitry Monakhov <[email protected]>

- Reorganize locking scheme to batch two atomic operation in to one. This also allow us to state what healthy group must obey following rule ext4_free_inodes_count(sb, gdp) == ext4_count_free(inode_bitmap, NUM);
- Fix possible undefined pointer dereference.
- Even if group descriptor stats aren't accessible we have to update inode bitmaps.
- Move non-group members update out of group_lock.

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

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 fs/ext4/ialloc.c |   81 ++++++++++++++++++++++++-----------------------------
 1 files changed, 37 insertions(+), 44 deletions(-)

fs/ext4/ialloc.c +39 -46

index 57f6eef..52618d5 100644
@@ -240,56 +240,49 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
 	if (fatal)
 		goto error_return;
 
-	/* Ok, now we can actually update the inode bitmaps.. */
-	cleared = ext4_clear_bit_atomic(ext4_group_lock_ptr(sb, block_group),
-					bit, bitmap_bh->b_data);
-	if (!cleared)
-		ext4_error(sb, "bit already cleared for inode %lu", ino);
-	else {
-		gdp = ext4_get_group_desc(sb, block_group, &bh2);
-
+	fatal = -ESRCH;
+	gdp = ext4_get_group_desc(sb, block_group, &bh2);
+	if (gdp) {
 		BUFFER_TRACE(bh2, "get_write_access");
 		fatal = ext4_journal_get_write_access(handle, bh2);
-		if (fatal) goto error_return;
-
-		if (gdp) {
-			ext4_lock_group(sb, block_group);
-			count = ext4_free_inodes_count(sb, gdp) + 1;
-			ext4_free_inodes_set(sb, gdp, count);
-			if (is_directory) {
-				count = ext4_used_dirs_count(sb, gdp) - 1;
-				ext4_used_dirs_set(sb, gdp, count);
-				if (sbi->s_log_groups_per_flex) {
-					ext4_group_t f;
-
-					f = ext4_flex_group(sbi, block_group);
-					atomic_dec(&sbi->s_flex_groups[f].used_dirs);
-				}
+	}
+	ext4_lock_group(sb, block_group);
+	cleared = ext4_clear_bit(bit, bitmap_bh->b_data);
+	if (fatal || !cleared) {
+		ext4_unlock_group(sb, block_group);
+		goto out;
+	}
 
-			}
-			gdp->bg_checksum = ext4_group_desc_csum(sbi,
-							block_group, gdp);
-			ext4_unlock_group(sb, block_group);
-			percpu_counter_inc(&sbi->s_freeinodes_counter);
-			if (is_directory)
-				percpu_counter_dec(&sbi->s_dirs_counter);
-
-			if (sbi->s_log_groups_per_flex) {
-				ext4_group_t f;
-
-				f = ext4_flex_group(sbi, block_group);
-				atomic_inc(&sbi->s_flex_groups[f].free_inodes);
-			}
-		}
-		BUFFER_TRACE(bh2, "call ext4_handle_dirty_metadata");
-		err = ext4_handle_dirty_metadata(handle, NULL, bh2);
-		if (!fatal) fatal = err;
+	count = ext4_free_inodes_count(sb, gdp) + 1;
+	ext4_free_inodes_set(sb, gdp, count);
+	if (is_directory) {
+		count = ext4_used_dirs_count(sb, gdp) - 1;
+		ext4_used_dirs_set(sb, gdp, count);
+		percpu_counter_dec(&sbi->s_dirs_counter);
 	}
-	BUFFER_TRACE(bitmap_bh, "call ext4_handle_dirty_metadata");
-	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
-	if (!fatal)
-		fatal = err;
-	sb->s_dirt = 1;
+	gdp->bg_checksum = ext4_group_desc_csum(sbi, block_group, gdp);
+	ext4_unlock_group(sb, block_group);
+
+	percpu_counter_inc(&sbi->s_freeinodes_counter);
+	if (sbi->s_log_groups_per_flex) {
+		ext4_group_t f = ext4_flex_group(sbi, block_group);
+
+		atomic_inc(&sbi->s_flex_groups[f].free_inodes);
+		if (is_directory)
+			atomic_dec(&sbi->s_flex_groups[f].used_dirs);
+	}
+	BUFFER_TRACE(bh2, "call ext4_handle_dirty_metadata");
+	fatal = ext4_handle_dirty_metadata(handle, NULL, bh2);
+out:
+	if (cleared) {
+		BUFFER_TRACE(bitmap_bh, "call ext4_handle_dirty_metadata");
+		err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
+		if (!fatal)
+			fatal = err;
+		sb->s_dirt = 1;
+	} else
+		ext4_error(sb, "bit already cleared for inode %lu", ino);
+
 error_return:
 	brelse(bitmap_bh);
 	ext4_std_error(sb, fatal);