Re: Simple fix for swap_out (the 1st August patch)
Dr. Werner Fink (werner@suse.de)
Fri, 1 Aug 1997 22:16:28 +0200
> I can't comment on the swap code changes (haven't studied that yet), but
> I would like to point out what I believe is a mistake in
> pre-patch-2.0.31-2.
> In 2.0.29 and 2.0.30 buffer.c the condition for waking up bdflush looks
> like
>
> if (!(grow_buffers(GFP_ATOMIC,size)))
> wakeup_bdflush(1);
> needed -= PAGE_SIZE;
> goto repeat;
>
> This was changed to something like:
> if ((grow_buffers(GFP_ATOMIC,size)))
> needed -= PAGE_SIZE;
> else
> wakeup_bdflush(1);
> goto repeat;
>
> On the surface the original code looks erroneous, but intentional or
> not, the original code has the effect of reducing the memory goal _even
> if no memory was allocated_. This means that tasks trapped in
> "refill_freelist hell" have a chance of escaping under 2.0.29 or
> unpatched 2.0.30, but may be forever trapped under the new code.
>
> If you've looked at the changes I've made to 2.1.47 fs/buffer.c, it's
> important to get tasks out of refill_freelist even if they haven't
> reached their full quota of buffers. Otherwise, you can reach a
> deadlock state where the tasks wanting buffers have all fallen into
> refill_freelist, and none can make their quota.
>
> I would much prefer to see the code written as something like:
> grow_buffers(GFP_ATOMIC,size);
> if (free_list[BUFSIZE_INDEX(size)] != NULL)
> return;
> wakeup_bdflush(1);
> needed -= PAGE_SIZE;
> goto repeat;
>
> i.e a direct test of success, but at the very least would recommend that
> the original code be used.
>
Hmmm ... in my current patch I'm use the follwing statement:
if (nr_free_pages > 5) {
/* and repeat until we find something good */
if (grow_buffers(GFP_ATOMIC, size)) {
needed -= PAGE_SIZE;
goto repeat;
};
}
wakeup_bdflush(1);
that means if really low on mem or grow_buffers() fails wakeup_bdflush()
is called before refill_freelist() returns to the caller. Maybe it's
better after a wakeup_bdflush() call even if it is called with argument 1
for waiting on flushing dirty buffer to avoid trouble (race) with
find_candidate(). What do you think?
On the other hand the direct deadlock detection should added.
Any further comments?
Werner