Messages in this thread Patches in this message | | | Date | Mon, 20 Dec 1999 15:31:08 +0100 | From | Jan Kara <> | Subject | Pagecache and inodecache bug in 2.3.33 |
| |
Hello.
I've found one bug in pagecache - in end_buffer_io_async() we badly detect when we should unlock the page. We check whether all buffers with end_buffer_io _async() handlers have use count 0. But for example in sync_buffers() we do wait_on_buffer() which will raise b_count and so if now finishes last ll_rw_block it will look and see that b_count on some buffer in page is 1 and so it won't unlock the page. But now nobody will remove the page lock... Attached patch changes the detection - we test whether all buffers with end_buffer_io_async() handler are unlocked (no IO on them). I think this should be safer as if buffer is locked, ll_rw_block() is doing IO and so it will call end_buffer_io_async() later...
I also notice one problem in inode cache. When we do remount we invalidate inodes after calling foo_remount(). But as invalidate simply discards all inodes with i_count == 0 and all their pages we can easily loose some data - - foo_remount (e.g. ext2_remount) might block. Then some other process modifies some file. Then remount wakes up and finishes and we junk modified data... Sync before invalidate_inodes() won't help as the same race might happen during sync... We can't also afford to sync inodes and data while invalidating as at that time inodes are unhashed and so we might have two copies of them in memory. I don't see any simple solution, how to make invalidate_inodes() safe so in attached patch there is only FIXME comment. Could someone tell me some case where we really need invalidating inodes on remount?
I have also another question: Is anybody currently working on locking inodes with read-write locks? They should fix those unpleasant truncate vs read races.
Honza.
--- linux/fs/buffer.c Thu Dec 16 22:16:14 1999 +++ linux/fs/buffer.c Sun Dec 19 18:05:28 1999 @@ -727,8 +727,7 @@ atomic_dec(&bh->b_count); tmp = bh->b_this_page; while (tmp != bh) { - if (atomic_read(&tmp->b_count) && - (tmp->b_end_io == end_buffer_io_async)) + if (tmp->b_end_io == end_buffer_io_async && buffer_locked(tmp)) goto still_busy; tmp = tmp->b_this_page; }--- linux/fs/super.c Thu Dec 16 22:16:15 1999 +++ linux/fs/super.c Sun Dec 19 21:06:07 1999 @@ -935,6 +935,8 @@ * Invalidate the inodes, as some mount options may be changed. * N.B. If we are changing media, we should check the return * from invalidate_inodes ... can't allow _any_ open files. + * FIXME: We can loose some data here as some modifications might + * happened during foo_remount_fs(). */ invalidate_inodes(sb);
| |