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:   Thu, 4 Aug 2022 10:45:44 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     bugzilla-daemon@kernel.org
Cc:     linux-ext4@vger.kernel.org
Subject: Re: [Bug 216322] Freezing of tasks failed after 60.004 seconds (1
 tasks refusing to freeze... task:fstrim  ext4_trim_fs - Dell XPS 13 9310
Message-ID: <YuvbmJRRUcemgPhp@mit.edu>
References: <bug-216322-13602@https.bugzilla.kernel.org/>
 <bug-216322-13602-2MvUDlAfJU@https.bugzilla.kernel.org/>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bug-216322-13602-2MvUDlAfJU@https.bugzilla.kernel.org/>
Precedence: bulk

On Thu, Aug 04, 2022 at 11:47:47AM +0000, bugzilla-daemon@kernel.org wrote:
> 
> I agree that the FITRIM interface is flawed in this way. But
> ext4_try_to_trim_range() actually does have fatal_signal_pending() and
> will return -ERESTARTSYS if that's true. Or did you have something else in
> mind?

The fatal_signal_pending() only checks for SIGKILL.  I'm not sure why
it returns ERESTARTSYS, since that's not applicable for a kill -9
signal.  The fake_signal_wake_up() function in kernel/freezer.c
doesn't send a fatal signal, so the fatal_signal_pending() check isn't
going to help here.

> Also in that case, I see no reason why we would not be able to adjust
> the fstrim_range to make it easier to re-start where we left off if
> we're going to return -ERESTARTSYS. I am missing something?

Well, we could adjust fstrim_range.start and fstrim_range.len to make
it easier to restart --- but that's only if we know for sure that
we're going to be restarting the system call.  So we need to break
some abstraction barriers since if the signal is one where based on
the sigaction flags, the system all is *not* restarted, then
fstrim_range.len is supposed to contain the number of bytes trimmed.

And even if the system call is restarted, there's no place to stash
the number of bytes trimmed so far, since fstrim_range.len is
overloaded.   This why the interface is so horrible...

> I have not had time to look deeply into the traces, but are you actually
> sure that we're not stuck in blkdev_issue_discard() instead?

I'm not 100% certain, but unless the block device has been put to
sleep first (in which case I think we would have noticed much sooner
since lots of other suspend-to-ram use cases would be failling --- in
writeback threads, for example), I'd be really surprised if we're
getting stuck there.

Even when we need to wait for the queue to be drained so there is
space to send the next discard, that shouldn't take 60+ seconds.

      	      	       		     	       - Ted