Messages in this thread Patch in this message |  | | Date | Sun, 17 Sep 2000 22:12:47 -0400 (EDT) | From | Alexander Viro <> | Subject | Re: The INN/mmap bug |
| |
On Sun, 17 Sep 2000, Linus Torvalds wrote:
> > > On Sun, 17 Sep 2000, Marco d'Itri wrote: > > > > I added to block_write_full_page() the debugging code suggested by > > Alexander Viro: > > > > if (inode->i_dev == 0x305 && inode->i_ino == 48991) // Md > > printk("block_write_full_page: writing page %d, size %Ld\n", > > page->index, inode->i_size); > > > > and I have just been able to trigger the bug (two times): > > Bug? > > How do you think updates get written back to disk when innd has made > changes to its in-core file? > > Right. With block_write_full_page. > > > root@wonderland:vc/2:/var/lib/news$diff -u active active.ok > > --- active Sun Sep 17 16:09:01 2000 > > +++ active.ok Sun Sep 17 21:03:16 2000 > > @@ -1,11 +1,11 @@ > > control 0000004774 0000004775 y > > control.cancel 0000021917 0000021889 n > > junk 0000001777 0000001768 y > > -fido.ita.ridere 0000014632 0000014600 y > > +fido.ita.ridere 0000014634 0000014600 y > > So? innd got two new articles. > > The above is what you want. If the active file wouldn't get updated, you'd > never see any new articles ever again. > > Doesn't look like a bug to me.
It _is_ a bug, unfortunately. Look: he had taken a copy when the thing was running. I.e. took the copy of in-core page. After that he stopped innd and that had triggered block_write_full_page(). So far, so good, right? Except that the page contents didn't reach the disk.
How about the following: --- buffer.c Mon Sep 18 01:13:42 2000 +++ buffer.c.new Mon Sep 18 01:28:45 2000 @@ -1411,9 +1411,12 @@ */ static int __block_write_full_page(struct inode *inode, struct page *page, get_block_t *get_block) { - int err, i, need_balance_dirty = 0; + int err = -EIO, i, need_balance_dirty = 0; unsigned long block; struct buffer_head *bh, *head; + unsigned long end_block = (inode->i_size+inode->i_sb->s_blocksize-1) >> + inode->i_sb->s_blocksize_bits; + unsigned offset; if (!PageLocked(page)) BUG(); @@ -1423,6 +1426,9 @@ head = page->buffers; block = page->index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits); + /* Are we completely out? */ + if (block > end_block) + goto out; bh = head; i = 0; @@ -1451,6 +1457,8 @@ bh = bh->b_this_page; block++; + if (block > end_block) + break; } while (bh != head); if (need_balance_dirty) @@ -1823,31 +1831,7 @@ int block_write_full_page(struct page *page, get_block_t *get_block) { struct inode *inode = (struct inode*)page->mapping->host; - unsigned long end_index = inode->i_size >> PAGE_CACHE_SHIFT; - unsigned offset; - int err; - - /* easy case */ - if (page->index < end_index) - return __block_write_full_page(inode, page, get_block); - - /* things got complicated... */ - offset = inode->i_size & (PAGE_CACHE_SIZE-1); - /* OK, are we completely out? */ - if (page->index >= end_index+1 || !offset) - return -EIO; - /* Sigh... will have to work, then... */ - err = __block_prepare_write(inode, page, 0, offset, get_block); - if (!err) { - memset(page_address(page) + offset, 0, PAGE_CACHE_SIZE - offset); - flush_dcache_page(page); - __block_commit_write(inode,page,0,offset); -done: - kunmap(page); - return err; - } - ClearPageUptodate(page); - goto done; + return __block_write_full_page(inode, page, get_block); } int generic_block_bmap(struct address_space *mapping, long block, get_block_t *get_block) Unlike the current variant it does the right thing with ->b_end_io and is less intrusive (and shorter, BTW).
- 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/
|  |