Re: [patch 2/3] fs: buffer don't PageUptodate without page locked

[Andrew Morton]

On Tue,  6 Feb 2007 09:02:23 +0100 (CET) Nick Piggin <npiggin@xxxxxxx> wrote:

> __block_write_full_page is calling SetPageUptodate without the page locked.
> This is unusual, but not incorrect, as PG_writeback is still set.
> 
> However with the previous patch, this is now a problem: so don't bother
> setting the page uptodate in this case (it is weird that the write path
> does such a thing anyway). Instead just leave it to the read side to bring
> the page uptodate when it notices that all buffers are uptodate.
> 
> Signed-off-by: Nick Piggin <npiggin@xxxxxxx>
> 
> Index: linux-2.6/fs/buffer.c
> ===================================================================
> --- linux-2.6.orig/fs/buffer.c
> +++ linux-2.6/fs/buffer.c
> @@ -1679,6 +1679,7 @@ static int __block_write_full_page(struc
>  	 */
>  	BUG_ON(PageWriteback(page));
>  	set_page_writeback(page);
> +	unlock_page(page);
>  
>  	do {
>  		struct buffer_head *next = bh->b_this_page;
> @@ -1688,7 +1689,6 @@ static int __block_write_full_page(struc
>  		}
>  		bh = next;
>  	} while (bh != head);
> -	unlock_page(page);
>  
>  	err = 0;
>  done:

Why this change?  Without looking at it too hard, it seems that if
submit_bh() completes synchronously, this thread can end up playing with
the buffers on a non-locked, non-PageWriteback page.  Someone else could
whip the buffers away and oops?

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