Skip to contentSkip to conversation

ext4: fix fencepost error in lazytime optimization

PATCH In review Theodore Ts'o 2 participants 2 messages Jul 5, 2015 1 message not in the archive

Subsystems (2)
Stable branch ext4 file system
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Theodore Ts'o

author maintainer
#1 lore raw

PATCH 1/1 ext4: fix fencepost error in lazytime optimization

Commit 8f4d8558391: "ext4: fix lazytime optimization" was not a complete fix. In the case where the inode number is a multiple of 16, and we could still end up updating an inode with dirty timestamps written to the wrong inode on disk. Oops.

This can be easily reproduced by using generic/005 with a file system with metadata_csum and lazytime enabled.

Signed-off-by Theodore Ts'o <[email protected]>
Cc [email protected]
---
 fs/ext4/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/ext4/inode.c +6 -1
index e057c6f..4ad73d3 100644
@@ -4348,7 +4348,12 @@ static void ext4_update_other_inodes_time(struct super_block *sb,
 	int inode_size = EXT4_INODE_SIZE(sb);
 
 	oi.orig_ino = orig_ino;
-	ino = (orig_ino & ~(inodes_per_block - 1)) + 1;
+	/*
+	 * Calculate the first inode in the inode table block.  Inode
+	 * numbers are one-based.  That is, the first inode in a block
+	 * (assuming 4k blocks and 256 byte inodes) is (n*16 + 1).
+	 */
+	ino = ((orig_ino - 1) & ~(inodes_per_block - 1)) + 1;
 	for (i = 0; i < inodes_per_block; i++, ino++, buf += inode_size) {
 		if (ino == orig_ino)
 			continue;
Signature · 1 line
2.3.0
#2 lore raw

Hi,

Theodore Ts'o wrote on 07/05/2015 04:41:

Quoted 23 lines

Commit 8f4d8558391: "ext4: fix lazytime optimization" was not a complete fix. In the case where the inode number is a multiple of 16, and we could still end up updating an inode with dirty timestamps written to the wrong inode on disk. Oops.

This can be easily reproduced by using generic/005 with a file system with metadata_csum and lazytime enabled.

Signed-off-by Theodore Ts'o <[email protected]>
Cc [email protected]
---
 fs/ext4/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/ext4/inode.c +0 -0
index e057c6f..4ad73d3 100644
@@ -4348,7 +4348,12 @@ static void ext4_update_other_inodes_time(struct super_block *sb,
 	int inode_size = EXT4_INODE_SIZE(sb);
 
 	oi.orig_ino = orig_ino;

fs/ext4/inode.c static void ext4_update_other_inodes_time(struct super_block *sb, · line 4359

Quoted 10 lines
fs/ext4/inode.c +6 -1
-	ino = (orig_ino & ~(inodes_per_block - 1)) + 1;
+	/*
+	 * Calculate the first inode in the inode table block.  Inode
+	 * numbers are one-based.  That is, the first inode in a block
+	 * (assuming 4k blocks and 256 byte inodes) is (n*16 + 1).
+	 */
+	ino = ((orig_ino - 1) & ~(inodes_per_block - 1)) + 1;
 	for (i = 0; i < inodes_per_block; i++, ino++, buf += inode_size) {
 		if (ino == orig_ino)
 			continue;

thank you very much for the explanation. Now I think I understand:

/*
* Calculate the first inode in the original inode's inode table
* block. Inode numbers are one-based. That is, the first inode
* in a block is one plus the next-lowest to (orig_ino - 1)
* integral multiple of inodes_per_block. This can be calculated
* efficiently by bit masking since inodes_per_block is a power
* of 2 (assuming 4k blocks and 256 byte inodes).
*/

Regards,
Jörg.

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