Re: highmem deadlock fix [was Re: VM in 2.4.10(+tweaks) vs.

[Linus Torvalds]

On Fri, 28 Sep 2001, Andrea Arcangeli wrote:
>
> The deadlock happens in the middle of write_locked_buffers when we hit
> an highmem buffer, so while allocating with GFP_NOHIGHIO we end doing
> sync_page_buffers on any page that isn't highmem, but that incidentally is one of the
> other next buffers in the array that we previously locked in
> write_some_buffers but that aren't in the I/O queue yet (so we'll wait
> forever since they depends on us to be written).

Interesting, indeed..

However, your patch is racy:

> --- 2.4.10aa2/fs/buffer.c.~1~	Wed Sep 26 18:45:29 2001
> +++ 2.4.10aa2/fs/buffer.c	Fri Sep 28 00:04:44 2001
> @@ -194,6 +194,7 @@
>  		struct buffer_head * bh = *array++;
>  		bh->b_end_io = end_buffer_io_sync;
>  		submit_bh(WRITE, bh);
> +		clear_bit(BH_Pending_IO, &bh->b_state);

No way can we clear the bit here, because the submit_bh() may have caused
the buffer to be unlocked and IO to have completed, and it is no longer
"owned" by us - somebody else might have started IO on it and we'd be
clearing the bit for the wrong user.

I would suggest a totally different approach: make the "can we wait for
existing buffer heads" condition a GFP bit the same way the HIGHIO thing
is a GFP bit, and just not set it for GFP_NOHIGHIO.

Thinking about it, I think GFP_NOIO also implies "we must not wait for
other buffers", because that could deadlock for _other_ things too, like
loop and NBD (which use NOIO to make sure that they don't recurse - but
that should also imply not waiting for themselves). The GFP_xxx approach
should fix those deadlocks too.

		Linus