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.
--7AUc2qLy4jB3hD7Z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="buffer.c.diff"
--- 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;
}
--7AUc2qLy4jB3hD7Z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="super.c.diff"
--- 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);
--7AUc2qLy4jB3hD7Z--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/