[e2fsprogs PATCH] resize2fs: use directio when reading superblock

[e2fsprogs PATCH] resize2fs: use directio when reading superblock

[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).

   #!/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 <kjlx@templeofstupid.com>
---
 resize/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/resize/main.c b/resize/main.c
index 94f5ec6d..b98af384 100644
--- a/resize/main.c
+++ b/resize/main.c
@@ -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)
-- 
2.25.1

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Theodore Ts'o]

On Mon, Jun 05, 2023 at 03:52:21PM -0700, Krister Johansen wrote:
> 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

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Krister Johansen]

On Wed, Jun 07, 2023 at 09:39:09AM -0400, Theodore Ts'o wrote:
> On Mon, Jun 05, 2023 at 03:52:21PM -0700, Krister Johansen wrote:
> > 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).

> 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

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Theodore Ts'o]

On Wed, Jun 07, 2023 at 11:50:41AM -0700, Krister Johansen wrote:
> 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

diff --git a/resize/main.c b/resize/main.c
index 94f5ec6d..f914c050 100644
--- a/resize/main.c
+++ b/resize/main.c
@@ -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) {

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Krister Johansen]

Hi Ted,

On Fri, Jun 09, 2023 at 12:22:39AM -0400, Theodore Ts'o wrote:
> On Wed, Jun 07, 2023 at 11:50:41AM -0700, Krister Johansen wrote:
> > 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. :/

> 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.

> Does this patch work for you instead?

Thanks, it does!

> 					- Ted
> 
> diff --git a/resize/main.c b/resize/main.c
> index 94f5ec6d..f914c050 100644
> --- a/resize/main.c
> +++ b/resize/main.c
> @@ -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 <kjlx@templeofstupid.com>
Tested-by: Krister Johansen <kjlx@templeofstupid.com>

Thanks again,

-K

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Krister Johansen]

Hi Ted,

On Fri, Jun 09, 2023 at 07:11:31PM -0700, Krister Johansen wrote:
> On Fri, Jun 09, 2023 at 12:22:39AM -0400, Theodore Ts'o wrote:
> > 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.
> 
> > Does this patch work for you instead?
> 
> Thanks, it does!
> 
> > diff --git a/resize/main.c b/resize/main.c
> > index 94f5ec6d..f914c050 100644
> > --- a/resize/main.c
> > +++ b/resize/main.c
> > @@ -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 <kjlx@templeofstupid.com>
> Tested-by: Krister Johansen <kjlx@templeofstupid.com>

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

Thanks,

-K

Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock

[Krister Johansen]

Hi Ted,

On Wed, Jun 28, 2023 at 04:02:20PM -0700, Krister Johansen wrote:
> Hi Ted,
> 
> On Fri, Jun 09, 2023 at 07:11:31PM -0700, Krister Johansen wrote:
> > On Fri, Jun 09, 2023 at 12:22:39AM -0400, Theodore Ts'o wrote:
> > > 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.
> > 
> > > Does this patch work for you instead?
> > 
> > Thanks, it does!
> > 
> > > diff --git a/resize/main.c b/resize/main.c
> > > index 94f5ec6d..f914c050 100644
> > > --- a/resize/main.c
> > > +++ b/resize/main.c
> > > @@ -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 <kjlx@templeofstupid.com>
> > Tested-by: Krister Johansen <kjlx@templeofstupid.com>
> 
> 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