linux-ext4 - Re: [PATCH] e2fsck: Release clusters only once in release_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180316152429.GA25794@thunk.org>
Date:   Fri, 16 Mar 2018 11:24:29 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     harshads <harshads@...gle.com>
Cc:     linux-ext4@...r.kernel.org
Subject: Re: [PATCH] e2fsck: Release clusters only once in
 release_inode_blocks

On Tue, Mar 13, 2018 at 04:19:07PM -0700, harshads wrote:
> While releasing inode blocks, if bigalloc feature is turned on,
> release clusters only once. We do it by remembering the last released
> cluster while iterating through blocks and releasing a cluster only if
> it is not already released.
> 
> Signed-off-by: Harshad Shirwadkar <harshads@...gle.com>

Thanks, applied.

For future reference, this is my local preference and a tiny nit-pick,
but I prefer the first word after the colon to be uncapitalized (since
it isn't necessarily, and in most cases shouldn't be, a full complete
sentence).  So I would prefer:

    e2fsck: release clusters only once in release_inode_blocks

Also, either included in the same commit, or in a separate commit
(especially if several commits in a sequence are needed to fix a bug),
it's a really good idea to create a regression test.  This is also
useful because it means that you know you can create as small file
system to use a simple, pure regression test.  For example:

commit a126f79a0eee38762d8e3d110281ae86322de444
Author: Theodore Ts'o <tytso@....edu>
Date:   Fri Mar 16 11:19:02 2018 -0400

    tests: add new test f_bigalloc_orphan_list
    
    This is a regression test for the bugfix found in commit 707599bf:
    "e2fsck: release clusters only once in release_inode_blocks"
    
    Signed-off-by: Theodore Ts'o <tytso@....edu>

diff --git a/tests/f_bigalloc_orphan_list/expect.1 b/tests/f_bigalloc_orphan_list/expect.1
new file mode 100644
index 000000000..622620d06
--- /dev/null
+++ b/tests/f_bigalloc_orphan_list/expect.1
@@ -0,0 +1,10 @@
+Clearing orphaned inode 12 (uid=0, gid=0, mode=0100644, size=28672)
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 11/64 files (0.0% non-contiguous), 96/1024 blocks
+Exit status is 0
diff --git a/tests/f_bigalloc_orphan_list/expect.2 b/tests/f_bigalloc_orphan_list/expect.2
new file mode 100644
index 000000000..30392d428
--- /dev/null
+++ b/tests/f_bigalloc_orphan_list/expect.2
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 11/64 files (0.0% non-contiguous), 96/1024 blocks
+Exit status is 0
diff --git a/tests/f_bigalloc_orphan_list/name b/tests/f_bigalloc_orphan_list/name
new file mode 100644
index 000000000..5d9566ff9
--- /dev/null
+++ b/tests/f_bigalloc_orphan_list/name
@@ -0,0 +1 @@
+orphaned list handling with bigalloc file systems
diff --git a/tests/f_bigalloc_orphan_list/script b/tests/f_bigalloc_orphan_list/script
new file mode 100644
index 000000000..1508bf126
--- /dev/null
+++ b/tests/f_bigalloc_orphan_list/script
@@ -0,0 +1,28 @@
+if test -x $DEBUGFS_EXE; then
+
+SKIP_GUNZIP="true"
+TEST_DATA="$test_name.tmp"
+
+dd if=$TEST_BITS of=$TEST_DATA bs=28k count=1 > /dev/null 2>&1
+
+touch $TMPFILE
+mke2fs -Fq -t ext4 -O bigalloc $TMPFILE 1M > /dev/null 2>&1
+debugfs -w $TMPFILE << EOF > /dev/null 2>&1
+write $TEST_DATA testfile
+set_inode_field testfile links_count 0
+set_inode_field testfile bmap[0] 0
+set_inode_field testfile bmap[2] 0
+set_super_value last_orphan 12
+unlink testfile
+quit
+EOF
+
+. $cmd_dir/run_e2fsck
+
+rm -f $TEST_DATA
+
+unset E2FSCK_TIME TEST_DATA
+
+else #if test -x $DEBUGFS_EXE; then
+	echo "$test_name: $test_description: skipped"
+fi