On 09/27/2017 07:36 PM, Linus Torvalds wrote: > On Wed, Sep 27, 2017 at 5:41 AM, Jens Axboe <axboe@xxxxxxxxx> wrote: >> >> 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
