On 09/27/2017 07:36 PM, Linus Torvalds wrote:

On Wed, Sep 27, 2017 at 5:41 AM, Jens Axboe [off-list ref] wrote:

quoted

So I reworked the series, to include three prep patches that end up
killing off free_more_memory(). This means that we don't have to do the
1024 -> 0 change in there. On top of that, I added a separate bit to
manage range cyclic vs non range cyclic flush all work. This means that
we don't have to worry about the laptop case either.

I think that should quell any of the concerns in the patchset, you can
find the new series here:

http://git.kernel.dk/cgit/linux-block/log/?h=wb-start-all

Yeah, this I feel more confident about.

It still has some subtle changes (nr_pages is slightly different for
laptop mode, and the whole "we rely on the VM to not return NULL") but
I cannot for the life of me imagine that those changes are noticeable
or meaningful.

So now that last commit that limits the pending flushes is the only
one that seems to matter, and the one you wanted to build up to. It's
not a subtle change, but that's the one that fixes the actual bug you
see, so now the rest of the series really looks like "prep-work for
the bug fix".

Of course, with the change to support range_cycling for the laptop
mode case, there's actually possibly _two_ pending full flushes (the
range-cyclic and the "all" one), so that last changelog may not be
entirely accurate.

Yeah good point. I did modify it a little bit, but I should make that
clear throughout that changelog. Functionally it's identical as far as
the bug is concerned, 1 or 2 is a far cry from hundreds of thousands or
millions.

Long-term, I would prefer to maybe make the "range_whole" case to
always be range-cyclic, because I suspect that's the better behavior,
but I think that series is the one that changes existing semantics the
least for the existing cases, so I think your approach is better for
now.

I agree, I was actually contemplating whether to make that change or
not. I don't see a reason to ever NOT do range cyclic writeback for the
full range request. Then we could kill the second wb->state bit and get
back to just having the one in flight.

As far as I can tell, the cases that do not set range_cyclic right now are:

 - wakeup_flusher_threads()

 - sync_inodes_sb()

and in neither case does that lack of range_cyclic really seem to make
any sense.

It might be a purely historical artifact (an _old_ one: that
range_cyclic logic goes back to 2006, as far as I can tell).

But there might also be something I'm missing.

Anyway, your series looks fine to me.

Thanks for taking a look. I feel like the change should be made to do
range cyclic flushes for any request that wants to start writeback on
the full range, I can't think of a case where that make a difference.
Then we can kill the extra bit, and more importantly, kill this part:

        if (reason == WB_REASON_LAPTOP_TIMER)                                   
                range_cyclic = true;

which is a bit of a hack. I'll respin with that, and repost the series.

-- 
Jens Axboe