Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230421221249.1616168-1-dianders@chromium.org>
 <20230421151135.v2.1.I2b71e11264c5c214bc59744b9e13e4c353bc5714@changeid> <20230424082254.gopb4y2c7d65icpl@techsingularity.net>
In-Reply-To: <20230424082254.gopb4y2c7d65icpl@techsingularity.net>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 24 Apr 2023 09:22:55 -0700
Message-ID: <CAD=FV=XRMiDxKyLKGKL3ekk8CjRt6puT1PReD+pE21JGgF4TQw@mail.gmail.com>
Subject: Re: [PATCH v2 1/4] mm/filemap: Add folio_lock_timeout()
To:     Mel Gorman <mgorman@techsingularity.net>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>, Ying <ying.huang@intel.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Yu Zhao <yuzhao@google.com>, linux-fsdevel@vger.kernel.org,
        Matthew Wilcox <willy@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Mon, Apr 24, 2023 at 1:22=E2=80=AFAM Mel Gorman <mgorman@techsingularity=
.net> wrote:
>
> > @@ -1295,10 +1296,13 @@ static inline int folio_wait_bit_common(struct =
folio *folio, int bit_nr,
> >               /* Loop until we've been woken or interrupted */
> >               flags =3D smp_load_acquire(&wait->flags);
> >               if (!(flags & WQ_FLAG_WOKEN)) {
> > +                     if (!timeout)
> > +                             break;
> > +
>
> An io_schedule_timeout of 0 is valid so why the special handling? It's
> negative timeouts that cause schedule_timeout() to complain.

It's not expected that the caller passes in a timeout of 0 here. The
test here actually handles the case that the previous call to
io_schedule_timeout() returned 0. In my patch, after the call to
io_schedule_timeout() we unconditionally "continue" and end up back at
the top of the loop. The next time through the loop if we don't see
the WOKEN flag then we'll check for the two "error" conditions
(timeout or signal pending) and break for either of them.

To make it clearer, I'll add this comment for the next version:

/* Break if the last io_schedule_timeout() said no time left */