Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 16:02:20 -0700
From:   Krister Johansen <kjlx@templeofstupid.com>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     linux-ext4@vger.kernel.org
Subject: Re: [e2fsprogs PATCH] resize2fs: use directio when reading superblock
Message-ID: <20230628230220.GA1918@templeofstupid.com>
References: <20230605225221.GA5737@templeofstupid.com>
 <20230607133909.GA1309044@mit.edu>
 <20230607185041.GA2023@templeofstupid.com>
 <20230609042239.GA1436857@mit.edu>
 <20230610021131.GA6134@templeofstupid.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230610021131.GA6134@templeofstupid.com>
Precedence: bulk

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