Messages in this thread |  | | Date | Sun, 17 Sep 2000 23:48:37 -0400 (EDT) | From | Alexander Viro <> | Subject | Re: The INN/mmap bug |
| |
On Sun, 17 Sep 2000, Linus Torvalds wrote:
> The basic problem is that right now we have code that does > > if (!page->buffers) > create_empty_buffers(page, inode, inode->i_sb->s_blocksize); > ... > if (!buffer_uptodate(bh)) > ll_rw_block(READ, 1, &bh); > > And this is WRONG. > > If the whole page is up-to-date, we must NOT try to read the buffer in > from disk, because the in-memory copy is always more up-to-date.
Yep.
> We need to fix the real bug - otherwise anybody doing both write() and > shared mmap's to the same file is going to be bit by it later on... > > The easy fix is probably to do something like > > /* Map the buffer */ > if (!buffer_mapped(bh)) { > ... > } > + /* If the page is up-to-date, so is the buffer */ > + if (Page_Uptodate(page)) > + set_bit(BH_Uptodate, &bh->b_state); > > /* Ok, now it was _truly_ not uptodate */ > if (!buffer_uptodate(bh)) > ll_rw_block(READ, 1, &bh); > > Comments? The above should fix block_write_full_page() automatically, as > well as fixing the other cases too - and actually improve performance at > the same time by getting rid of unnecessary re-reads. > > Looks fairly simple. It only happens in __block_prepare_write() and in > block_truncate_page(), so there's just two places to fix. > > Can you se anything else wrong?
Looks sane. But I really wonder if we could just do it in create_page_buffers() if page is up-to-date. OTOH it would require attempt to map them all. Comments?
That way we restore the bh state if buffer ring is recreated - IMO somewhat simpler...
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org Please read the FAQ at http://www.tux.org/lkml/
|  |