Re: [PATCH] ext4: Make ext4_lazyinit_thread freezable.

From: "Darrick J. Wong" <djwong@kernel.org>
To: Lalith Rajendran <lalithkraj@google.com>
Cc: tytso@mit.edu, dilger.kernel@dilger.ca,
	linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
	xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] ext4: Make ext4_lazyinit_thread freezable.
Date: Tue, 23 Aug 2022 09:03:45 -0700	[thread overview]
Message-ID: <YwT6YelUyY+fjkT4@magnolia> (raw)
In-Reply-To: <CANscXK018_PSLPSeTyne7D+KWHa74kyHmdJ8Bxfxyf=5YMPSfw@mail.gmail.com>

On Tue, Aug 23, 2022 at 08:37:43AM -0700, Lalith Rajendran wrote:
> Thank you for the comment.
> Can you provide more details on how
> https://lore.kernel.org/linux-fsdevel/20210417001026.23858-1-mcgrof@kernel.org/
> is
> related to your comment to freeze all filesystems in order?

This all got started a long time ago[0] when I noticed that suspend often
fails on my laptop because the kthread freezer will shut down some of
XFS' low level background kernel threads before higher level background
threads have been put to sleep.  In this case, suspend kills the buffer
IO completion thread before the XFS log worker, which means the log
worker stalls trying to write the log contents back to disk:

[0] https://lore.kernel.org/linux-xfs/20170203010401.GR9134@birch.djwong.org/

So I then proposed a patch[1] to mark the buffer completion workqueue as
non-freezeable, which was rejected because while that will bandaid the
suspend problem, it'll create bigger problems with hibernation because
the log and buffer workers can keep running while hibernate tries to
write the state of those threads out to disk:

[1] https://lore.kernel.org/linux-xfs/20170327204611.GA4864@birch.djwong.org/

That came with a suggestion that the power management code should freeze
the filesystems before trying to put kthreads to sleep.  User programs
will go to sleep trying to get write access, and all fs background
threads will already be idle.

Luis Chamberlain offered to pick up that work[2] back in 2017 to figure
out how to freeze filesystems prior to suspend.  His first attempt
simply walked the supers list in reverse order, and drew quite a few
comments.  The second version[3] improved on that.

[2] https://lore.kernel.org/all/20171003185313.1017-1-mcgrof@kernel.org/T/#u
[3] https://lore.kernel.org/all/20171129232356.28296-1-mcgrof@kernel.org/T/#u

Four years went by (I don't blame Luis; I've not had time to get back to
this either) and his most recent posting[4] I think tried to restart the
discussion.

[4] https://lore.kernel.org/linux-fsdevel/20210417001026.23858-1-mcgrof@kernel.org/

So that's where things stand now.  I've idly wondered if a better
approach would be to hook filesystems into the device model so that
filesystems could watch for suspend notifications propagating down the
device tree and turn that into a fsfreeze, but as I mentioned, I've had
no time to figure out how that would really work since I'm not that
familiar with how power management works in the kernel.  I don't have a
clue where you'd attach a network filesystem.

--D

> Thanks,
> Lalith
> 
> On Thu, Aug 18, 2022 at 5:08 PM Darrick J. Wong <djwong@kernel.org> wrote:
> 
> > On Thu, Aug 18, 2022 at 09:40:49PM +0000, Lalith Rajendran wrote:
> > > ext4_lazyinit_thread is not set freezable. Hence when the thread calls
> > > try_to_freeze it doesn't freeze during suspend and continues to send
> > > requests to the storage during suspend, resulting in suspend failures.
> >
> > Maybe we should just make suspend freeze all the filesystems in order?
> >
> >
> > https://lore.kernel.org/linux-fsdevel/20210417001026.23858-1-mcgrof@kernel.org/
> >
> > --D
> >
> > > Signed-off-by: Lalith Rajendran <lalithkraj@google.com>
> > > ---
> > >  fs/ext4/super.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index 9a66abcca1a85..d77e0904a1327 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -3767,6 +3767,7 @@ static int ext4_lazyinit_thread(void *arg)
> > >       unsigned long next_wakeup, cur;
> > >
> > >       BUG_ON(NULL == eli);
> > > +     set_freezable();
> > >
> > >  cont_thread:
> > >       while (true) {
> > > --
> > > 2.31.0
> > >
> >