[Ocfs2-commits] rev 769 - in trunk: . src

[svn-commits at oss.oracle.com]

Author: mfasheh
Date: 2004-03-10 12:26:08 -0600 (Wed, 10 Mar 2004)
New Revision: 769

Modified:
   trunk/TODO
   trunk/src/journal.c
Log:
* Knock a long standing item off the TODO list. This fixes a potential
  corruption due to improper cleanup of buffers before an
  abort_trans. Below is the comment from ocfs_abort_trans which explains
  the fixed bug.

/* There is a potential bug here which we may have to
 * resolve. What if you do a get_write_access on a buffer,
 * modify it, but error out before you've passed it to
 * dirty_metadata? We currently have to way to get to these
 * (unless we start tracking them much like dirty buffers). In
 * the worst case, the buffer could potentially hang around
 * the system and be picked up by another call and used as
 * though it were clean, even though it contains aborted
 * data!*/



Modified: trunk/TODO
===================================================================
--- trunk/TODO	2004-03-09 22:50:13 UTC (rev 768)
+++ trunk/TODO	2004-03-10 18:26:08 UTC (rev 769)
@@ -9,10 +9,6 @@
 * Move the journal file entry over to the next system slot which is unused.
   This should make downgrading back to ocfs version 1 easier.
 
-* Provide a solution for the potential problem with journal_access buffers
-  who haven't been journal_dirtied in abort_trans. Check out the comment in
-  abort_trans for more info on this.
-
 * fsck must be able to replay the journal
 
 * provide an ocfsmigrate utility to migrate v1 -> v2 and back.

Modified: trunk/src/journal.c
===================================================================
--- trunk/src/journal.c	2004-03-09 22:50:13 UTC (rev 768)
+++ trunk/src/journal.c	2004-03-10 18:26:08 UTC (rev 769)
@@ -451,41 +451,52 @@
 	int j;
 	ocfs_journal_copyout *co = NULL;
 	char *data;
+	bool dirtied;
 
 	LOG_ENTRY();
 
 	osb = handle->osb;
 	journal = &osb->journal;
 
-	/* There is a potential bug here which we may have to
-	 * resolve. What if you do a get_write_access on a buffer,
-	 * modify it, but error out before you've passed it to
-	 * dirty_metadata? We currently have to way to get to these
-	 * (unless we start tracking them much like dirty buffers). In
-	 * the worst case, the buffer could potentially hang around
-	 * the system and be picked up by another call and used as
-	 * though it were clean, even though it contains aborted
-	 * data!*/
+	if (handle->num_co < handle->num_buffs)
+		BUG();
 
-	/* Ok, we're aborting. For all dirtied buffers, copy our old
-	 * data back in. This should reverse what happened during the
-	 * transaction and revert us back.*/
-	for(i = 0; i < handle->num_buffs; i++) {
-		bh = handle->buffs[i];
-		
-		/* find the copyout. */
-		co = NULL;
-		for(j = 0; j < handle->num_co; j++)
-			if (handle->co_buffs[j].blocknr == bh->b_blocknr) {
-				co = &(handle->co_buffs[j]);
+	/* Ok, we're aborting. For all buffers I've seen, we want to
+	 * either forget it, or copy our old data back in. This should
+	 * revert what happened during the transaction.*/
+	for(i = 0; i < handle->num_co; i++) {
+		co = &(handle->co_buffs[i]);
+
+		/* Ok, search the dirtied buffers list for this
+		 * block. */
+		bh = NULL;
+		dirtied = true;
+		for(j = 0; j < handle->num_buffs; j++)
+			if (handle->buffs[j]->b_blocknr == co->blocknr) {
+				bh = handle->buffs[j];
 				break;
 			}
 
-		if (co == NULL)
-			BUG();
+		/* If we didn't have it then it was passed to
+		 * journal_access but never dirtied. We still have to
+		 * process it though as our abort paths may have left
+		 * the buffer with data that needs to be aborted */
+		if (!bh) {
+			/* log_trace_args for tracing, can safely be
+			 * turned off later. */
+			LOG_ERROR_ARGS("block %lu was modified but never "
+				       "dirtied!\n", co->blocknr);
+			bh = getblk(OCFS_GET_BLOCKDEV(osb->sb), co->blocknr,
+				    osb->sect_size);
+			if (bh == NULL)
+				BUG();
 
+			dirtied = false;
+		}
+
 		LOG_TRACE_ARGS("Aborting block %lu, forget=%u\n", co->blocknr,
 			       co->forget);
+
 		data = OCFS_BH_GET_DATA_WRITE(bh);
 		if (co->forget) {
 			/* journal_forget will perform a brelse for us */
@@ -497,6 +508,11 @@
 	                 * old. */
 			CLEAR_BH_SEQNUM(bh);
 
+			if (buffer_jbd(bh))
+				LOG_ERROR_ARGS("Buffer %lu has JBD bit set "
+					       "after a journal_forget!\n",
+					       bh->b_blocknr);
+
 			lock_buffer(bh);
 			clear_buffer_uptodate(bh);
 			clear_buffer_dirty(bh);
@@ -505,6 +521,12 @@
 			memcpy(data, co->data, bh->b_size);
 		}
 		OCFS_BH_PUT_DATA(bh);
+
+		/* we no longer need the buffer in this case. */
+		if (!dirtied) {
+			ocfs_clear_buffer_modified(bh);
+			brelse(bh);
+		}
 	}
 
 	/* done copying them, free it now. */
@@ -611,7 +633,7 @@
 	 * handle. 
 	 *
 	 * Note that we want to make a copy of the buffer on the 1st access
-	 * call as that when we know for sure it's clean. */
+	 * call as that's when we know for sure it's clean. */
 	for(i = 0; i < handle->num_co; i++)
 		if (handle->co_buffs[i].blocknr == bh->b_blocknr) {
 			found = true;