Ext3 users - Mar 2001 - Re: Bug in __invalidate_buffers?

I previously wrote:
> OK, my previous patch cleans up the ASSERT for invalidate_buffers()
> (modulo the fact that it was missing a ')' at the end of the line)
> but it hasn't really fixed the whole problem.  If a file write is in
> progress when invalidate_buffers() is called, I get an oops:
> The oops is caused from __invalidate_buffers() calling put_last_free(bh)
> on the device buffers (for buffers with b_count == 0 and not dirty), which
> must conflict somehow with what jfs expects of the state of a buffer.  It
> seems we would not have this problem if bh->b_count was non-zero.
I'm having a bit of a one-sided conversation here, but that's OK because
I
am at least able to stop the oops from happening (may not be 100% correct,
but it works for me).  Also, Stephen mentioned he is on holidays or
flying somewhere for a conference or something.

Anyhow, the below patch has fixed the invalidate_buffers() causing oops
problem for me (running LVM pvscan while writing into an ext3-0.0.6b fs).

Cheers, Andreas
==========================================================================---
fs/jfs/checkpoint.c.orig	Thu Mar  1 16:35:40 2001
+++ fs/jfs/checkpoint.c	Thu Mar 29 17:08:51 2001
@@ -106,8 +106,15 @@
 			lock_journal(journal);
 			return 1;
 		}
-		
-		if (!buffer_dirty(bh) && !buffer_jdirty(bh) &&
+
+		if (bh->b_dev == B_FREE) {
+			if (bh->b_cp_transaction) {
+				unlock_journal(journal);
+				journal_remove_checkpoint(bh);
+				lock_journal(journal);
+				return 1;
+			}
+		} else if (!buffer_dirty(bh) && !buffer_jdirty(bh) &&
 		    bh->b_list != BUF_CLEAN) {
 			unlock_journal(journal);
 			refile_buffer(bh);
--- fs/jfs/journal.c.orig	Thu Mar  1 16:14:59 2001
+++ fs/jfs/journal.c	Thu Mar 29 17:09:23 2001
@@ -242,9 +242,13 @@
 	do {
 		bh = next;
 		transaction->t_datalist = next = bh->b_tnext;
-		
-		if (!buffer_locked(bh) && 
-		    !buffer_dirty(bh)) {
+
+		if (bh->b_dev == B_FREE) {
+			journal_unfile_buffer(bh);
+			bh->b_transaction = NULL;
+			if (bh->b_cp_transaction)
+				journal_remove_checkpoint(bh);
+		} else if (!buffer_locked(bh) && !buffer_dirty(bh)) {
 			journal_unfile_buffer(bh);
 			bh->b_transaction = NULL;
 			unlock_journal(transaction->t_journal);
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Hi,

On Thu, Mar 29, 2001 at 05:44:16PM -0700, Andreas Dilger
wrote:
> I'm having a bit of a one-sided conversation here, but that's OK
because I
> am at least able to stop the oops from happening (may not be 100% correct,
> but it works for me).  Also, Stephen mentioned he is on holidays or
> flying somewhere for a conference or something.
Just been traveling for work, and I'm currently on holiday.  Right at
this moment I'm taking a train from New Orleans to Los Angeles (nearly
48 hours on the train --- we're stopped in El Paso as I write this!).  
> Anyhow, the below patch has fixed the invalidate_buffers() causing oops
> problem for me (running LVM pvscan while writing into an ext3-0.0.6b fs).
I'd much rather fix the buffer.c code.  Having journaling try to patch
up after somebody has deleted its buffer heads is very wrong, since we
risk the buffer journal lists getting badly corrupted if we allow
those buffers to be reused.

Does the patch below (untested, uncompiled!) work?

Eventually I'll be keeping an elevated b_count on _all_ buffers with
any journal metadata at all (as a side effect of separating that
metadata out into a separate journal_head struct), so this sort of
problem will just go away.  Nobody touches buffer_heads with non-zero
b_count.

Cheers,
 Stephen