Re: [patch] set_blocksize/invalidate_buffers race fixes for 2.2.12-final

Ingo Molnar (mingo@redhat.com)
Tue, 24 Aug 1999 13:31:19 -0400 (EDT)


On Wed, 18 Aug 1999, Andrea Arcangeli wrote:

> The new set_blocksize and cache_drop_behind in the buffer code inserted in
> 2.2.12-final is suspect to me.
>
> The rule is always been to check the b_count to know if somebody is
> waiting in the waitqueue. So all hte checks for
> waitqueue_active(&bh->b_wait) looks messy to me and seems to suggest that
> somebody broken the rule.

no, it's just that the RAID code has been around during all those buffer.c
rewrites, and it tried to be very careful and overparanoid. It might not
be necessery anymore.

> Also calling try_to_free_buffers is not clean from a VM point of view
> IMHO. The other buffers in the page may be referenced/young and so freeing
> them unconditionally we'll plain break the VM aging algorithm.

i think you have not understood in what context that code is used. take a
look at buffer_lowprio() and how the BH_LowPrio bit lives.

> Also I can't understand this comment:
>
> * so we can drop all unused buffers. Careful, bforget alone is
> * unsafe, we must be 100% sure that at the end of bforget() we will
> * really have no (new) users of this buffer.

this is an old comment, just ignore it. (there is no bforget in
cache_drop_behind anymore)

> Then this code seems messy:
>
> /*
> * lets be mega-conservative about what to free:
> */
> if (!(bh->b_dev != dev) &&
> !(bh->b_size == size) &&
> [..]
> {
> try_to_free_buffers()
> } else {
> put the buffer in the freelist directly
> }
>
> If the b_dev and b_size _can't_ change under us. If we are there we just
> know that b_dev and b_size are the interesting ones and we should
> invalidate and then free such buffer without other checks. Really I do the
> additional checks as well but I panic if b_dev or b_size changes under me
> while I am sleeping in wait_on_buffer().

in RAID there can be a set_blocksize() called while the device has active
bhs. This never happens with a 'simple device'. I know it is hacky, but
the 'users' of the bhs that get their device's size changed under them are
also aware of this and thus the thing is safe.

> Patch against 2.2.12-final (untested on raid):

-- mingo

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/