resize2fs: use directio when reading superblock

PATCH Superseded Krister Johansen 2 participants 7 messages Jun 5, 2023 – Aug 17, 2023

Share & tools

Krister Johansen

author

2023-06-05 23:11 UTC #0 lore raw

PATCH 1/1 resize2fs: use directio when reading superblock

Invocations of resize2fs intermittently report failure due to superblock checksum mismatches in this author's environment. This might happen a few times a week. The following script can make this happen within minutes. (It assumes /dev/nvme1n1 is available and not in use by anything else).

#!/usr/bin/bash
set -euxo pipefail

while true
do
parted /dev/nvme1n1 mklabel gpt mkpart primary 2048s 2099200s sleep .5
mkfs.ext4 /dev/nvme1n1p1
mount -t ext4 /dev/nvme1n1p1 /mnt
stress-ng --temp-path /mnt -D 4 &
STRESS_PID=$!
sleep 1
growpart /dev/nvme1n1 1
resize2fs /dev/nvme1n1p1
kill $STRESS_PID
wait $STRESS_PID
umount /mnt
wipefs -a /dev/nvme1n1p1
wipefs -a /dev/nvme1n1
done

After trying a few possible solutions, adding an O_DIRECT read to the open path in resize2fs eliminated the occurrences on test systems. ext2fs_open2 uses a negative count value when calling io_channel_read_blk to get the superblock. According to unix_read_block, negative offsets are to be read direct. However, when strace-ing a program without this fix, the underlying device was opened without O_DIRECT. Adding the flags in the patch ensures the device is opend with O_DIRECT and that the superblock read appears consistent.

Signed-off-by Krister Johansen <[email protected]>

---
 resize/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

resize/main.c +1 -1

index 94f5ec6d..b98af384 100644
@@ -410,7 +410,7 @@ int main (int argc, char ** argv)
 	if (!(mount_flags & EXT2_MF_MOUNTED) && !print_min_size)
 		io_flags = EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
 
-	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS;
+	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS | EXT2_FLAG_DIRECT_IO;
 	if (undo_file) {
 		retval = resize2fs_setup_tdb(device_name, undo_file, &io_ptr);
 		if (retval)

Signature · 1 line

2.25.1

Theodore Ts'o

2023-06-07 13:39 UTC #1 lore raw

Quoted 4 lines · Krister Johansen

Invocations of resize2fs intermittently report failure due to superblock checksum mismatches in this author's environment. This might happen a few times a week. The following script can make this happen within minutes. (It assumes /dev/nvme1n1 is available and not in use by anything else).

What version of e2fsprogs are you using, and what is your environment?

Are you perhaps trying to change the UUID of the file system (for example, in a cloud image environment) in parallel with resizing the file system to fit the size of the block device?

- Ted

Krister Johansen

author

2023-06-07 18:50 UTC #2 lore raw

Quoted 7 lines · Theodore Ts'o

Quoted 4 lines · Krister Johansen
Invocations of resize2fs intermittently report failure due to superblock checksum mismatches in this author's environment. This might happen a few times a week. The following script can make this happen within minutes. (It assumes /dev/nvme1n1 is available and not in use by anything else).
What version of e2fsprogs are you using, and what is your environment?

I hit this originally using e2fsprogs 1.45.5. That didn't have your patch for retrying the superblock read on checksum failure. I pulled that patch in initially, but it did not fully resolve the checksum mismatch error. The test provided in the report was using an EBS volume attached to an EC2 instance. (Let me know what additional environment details would be useful, if these are not).

Quoted 3 lines

Are you perhaps trying to change the UUID of the file system (for example, in a cloud image environment) in parallel with resizing the file system to fit the size of the block device?

The growpart / resize2fs in the reproducer are essentially verbatim from our system provisioning scripts. Unless those modify the UUID, we're not taking any explicit action to do so.

Thanks,

-K

Theodore Ts'o

2023-06-09 04:22 UTC #3 lore raw

Quoted 3 lines · Krister Johansen

The growpart / resize2fs in the reproducer are essentially verbatim from our system provisioning scripts. Unless those modify the UUID, we're not taking any explicit action to do so.

Ah, OK. OK, I'm guessing that your system provisioning scripts are attempting mess with the file system a lot (creating, deleting, etc.) files while trying to run resize2fs in parallel, then?

As far as your patch is concerned, resize2fs can do both off-line (unmounted) and on-line (mounted) resizes. And turning direct I/O unconditionally isn't a great idea for off-line resizes --- it will really trash the performance of the resize. Does this patch work for you instead?

- Ted

resize/main.c +2 -0

index 94f5ec6d..f914c050 100644
@@ -409,6 +409,8 @@ int main (int argc, char ** argv)
 
 	if (!(mount_flags & EXT2_MF_MOUNTED) && !print_min_size)
 		io_flags = EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if (mount_flags & EXT2_MF_MOUNTED)
+		io_flags |= EXT2_FLAG_DIRECT_IO;
 
 	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS;
 	if (undo_file) {

Krister Johansen

author Reviewed-by Tested-by

2023-06-10 02:11 UTC #4 lore raw

Hi Ted,

Quoted 8 lines · Theodore Ts'o

Quoted 3 lines · Krister Johansen
The growpart / resize2fs in the reproducer are essentially verbatim from our system provisioning scripts. Unless those modify the UUID, we're not taking any explicit action to do so.
Ah, OK. OK, I'm guessing that your system provisioning scripts are attempting mess with the file system a lot (creating, deleting, etc.) files while trying to run resize2fs in parallel, then?

The growpart and resize bits are triggered out of cloud-init when the machine initially boots. Some provisioning steps are machine-type depenent. The instances in which this problem have manifested are on machines where the initial provisioning has more to do. It's also on machine types that are frequently provisioned. I, unfortunately, only get to look at these machines once they break. It's been hard to say whether it's because some other step of the provisioning is happening in parallel, or if the probability is roughly the same, and these systems are hitting it because more come and go. I wish I had a better answer for you. :/

Quoted 4 lines

As far as your patch is concerned, resize2fs can do both off-line (unmounted) and on-line (mounted) resizes. And turning direct I/O unconditionally isn't a great idea for off-line resizes --- it will really trash the performance of the resize.

Thanks for the additional detail.

I also double-checked to make sure these systems had the following patch applied:

05c2c00f3769 ext4: protect superblock modifications with a buffer lock

And they do. Not sure if that's directly applicable to the online resize case though.

Quoted 1 line

Does this patch work for you instead?

Thanks, it does!

Quoted 7 lines

- Ted
resize/main.c +0 -0
index 94f5ec6d..f914c050 100644

resize/main.c int main (int argc, char ** argv) · line 416

Quoted 8 lines

resize/main.c +2 -0

@@ -409,6 +409,8 @@ int main (int argc, char ** argv)
 
 	if (!(mount_flags & EXT2_MF_MOUNTED) && !print_min_size)
 		io_flags = EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if (mount_flags & EXT2_MF_MOUNTED)
+		io_flags |= EXT2_FLAG_DIRECT_IO;
 
 	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS;
 	if (undo_file) {

If it counts:

Reviewed-by Krister Johansen <[email protected]>

Tested-by Krister Johansen <[email protected]>

Thanks again,

-K

Krister Johansen

author

2023-06-28 23:02 UTC #5 lore raw

Hi Ted,

Quoted 31 lines · Krister Johansen

Quoted 4 lines · Theodore Ts'o
As far as your patch is concerned, resize2fs can do both off-line (unmounted) and on-line (mounted) resizes. And turning direct I/O unconditionally isn't a great idea for off-line resizes --- it will really trash the performance of the resize.
Thanks for the additional detail.
I also double-checked to make sure these systems had the following patch applied:
05c2c00f3769 ext4: protect superblock modifications with a buffer lock
And they do. Not sure if that's directly applicable to the online resize case though.
Quoted 1 line
Does this patch work for you instead?
Thanks, it does!
Quoted 11 lines
resize/main.c +2 -0
index 94f5ec6d..f914c050 100644
@@ -409,6 +409,8 @@ int main (int argc, char ** argv)
 
 	if (!(mount_flags & EXT2_MF_MOUNTED) && !print_min_size)
 		io_flags = EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if (mount_flags & EXT2_MF_MOUNTED)
+		io_flags |= EXT2_FLAG_DIRECT_IO;
 

Quoted 7 lines

resize/main.c int main (int argc, char ** argv) · line 416
Quoted 2 lines
resize/main.c +0 -0
 	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS;
 	if (undo_file) {
If it counts:
Reviewed-by Krister Johansen <[email protected]>
Tested-by Krister Johansen <[email protected]>

Just wanted to check back on this. Should I send a v2 that incorporates the changes you suggested above?

Thanks,

-K

Krister Johansen

author

2023-08-17 00:38 UTC #6 lore raw

Hi Ted,

Quoted 39 lines · Krister Johansen

Hi Ted,
Quoted 36 lines · Krister Johansen
Quoted 4 lines · Theodore Ts'o
As far as your patch is concerned, resize2fs can do both off-line (unmounted) and on-line (mounted) resizes. And turning direct I/O unconditionally isn't a great idea for off-line resizes --- it will really trash the performance of the resize.
Thanks for the additional detail.
I also double-checked to make sure these systems had the following patch applied:
05c2c00f3769 ext4: protect superblock modifications with a buffer lock
And they do. Not sure if that's directly applicable to the online resize case though.
Quoted 1 line
Does this patch work for you instead?
Thanks, it does!
resize/main.c int main (int argc, char ** argv) · line 416
Quoted 13 lines
resize/main.c +2 -0
index 94f5ec6d..f914c050 100644
@@ -409,6 +409,8 @@ int main (int argc, char ** argv)
 
 	if (!(mount_flags & EXT2_MF_MOUNTED) && !print_min_size)
 		io_flags = EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if (mount_flags & EXT2_MF_MOUNTED)
+		io_flags |= EXT2_FLAG_DIRECT_IO;
 
 	io_flags |= EXT2_FLAG_64BITS | EXT2_FLAG_THREADS;
 	if (undo_file) {
If it counts:

Quoted 5 lines

Quoted 2 lines
Reviewed-by Krister Johansen <[email protected]>
Tested-by Krister Johansen <[email protected]>
Just wanted to check back on this. Should I send a v2 that incorporates the changes you suggested above?

I've been running this patch on our production systems for the past couple of months and haven't had any re-occurence of the bad superblock checksum error. It used to occur a few times a day.

Is there anything more I can do to help get this accepted into e2fsprogs?

Thanks very much,

-K