Re: [patch] Real-Time Preemption, -RT-2.6.11-rc3-V0.7.38-01

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 14 Mar 2005, Steven Rostedt wrote:
>
> On Mon, 14 Mar 2005, Steven Rostedt wrote:
> >
> > I just downloaded -40 and applied my patch, compiled it with
> > PREEMPT_DESKTOP and data=ordered, ran it and everything seems OK, except
> > I'm getting the following...
> >
> > BUG: Unable to handle kernel NULL pointer dereference at virtual address
> > 00000000
> >  printing eip:
> > c0213438
> > *pde = 00000000
>
> [snip]
>
> >
> >
> > I'll see if this happens without the patch, and if so, then I'll look into
> > this further.
> >
>
> Well, I took out my patch and this bug didn't happen, so I guess it's may
> fault!  OK, I'll dig into it further.
>

Here's a new patch. All I did was move BUFFER_FNS(JournalHead,journalhead)
to inside the #ifdef CONFIG_PREEMPT_RT and my oops went away !?!  This
really bothers me since it just declares some functions and is not used
with CONFIG_PREEMPT_RT off.  I have no idea what's going on.

Lee, can you see if this still crashes for you.


Thanks,

-- Steve


diff -ur linux-2.6.11-final-V0.7.40-00.orig/fs/jbd/journal.c linux-2.6.11-final-V0.7.40-00/fs/jbd/journal.c
--- linux-2.6.11-final-V0.7.40-00.orig/fs/jbd/journal.c	2005-03-02 02:37:49.000000000 -0500
+++ linux-2.6.11-final-V0.7.40-00/fs/jbd/journal.c	2005-03-14 09:46:41.000000000 -0500
@@ -80,6 +80,10 @@
 EXPORT_SYMBOL(journal_try_to_free_buffers);
 EXPORT_SYMBOL(journal_force_commit);

+#ifdef CONFIG_PREEMPT_RT
+spinlock_t jbd_journal_head_lock = SPIN_LOCK_UNLOCKED;
+#endif
+
 static int journal_convert_superblock_v1(journal_t *, journal_superblock_t *);

 /*
@@ -1727,6 +1731,9 @@
 		jh = new_jh;
 		new_jh = NULL;		/* We consumed it */
 		set_buffer_jbd(bh);
+#ifdef CONFIG_PREEMPT_RT
+		spin_lock_init(&jh->b_state_lock);
+#endif
 		bh->b_private = jh;
 		jh->b_bh = bh;
 		get_bh(bh);
@@ -1767,26 +1774,34 @@
 		if (jh->b_transaction == NULL &&
 				jh->b_next_transaction == NULL &&
 				jh->b_cp_transaction == NULL) {
-			J_ASSERT_BH(bh, buffer_jbd(bh));
-			J_ASSERT_BH(bh, jh2bh(jh) == bh);
-			BUFFER_TRACE(bh, "remove journal_head");
-			if (jh->b_frozen_data) {
-				printk(KERN_WARNING "%s: freeing "
-						"b_frozen_data\n",
-						__FUNCTION__);
-				kfree(jh->b_frozen_data);
-			}
-			if (jh->b_committed_data) {
-				printk(KERN_WARNING "%s: freeing "
-						"b_committed_data\n",
-						__FUNCTION__);
-				kfree(jh->b_committed_data);
+#ifdef CONFIG_PREEMPT_RT
+			if (atomic_read(&jh->b_state_wait_count)) {
+				BUG_ON(buffer_journalhead(bh));
+				set_buffer_journalhead(bh);
+			} else
+#endif
+			{
+				J_ASSERT_BH(bh, buffer_jbd(bh));
+				J_ASSERT_BH(bh, jh2bh(jh) == bh);
+				BUFFER_TRACE(bh, "remove journal_head");
+				if (jh->b_frozen_data) {
+					printk(KERN_WARNING "%s: freeing "
+					       "b_frozen_data\n",
+					       __FUNCTION__);
+					kfree(jh->b_frozen_data);
+				}
+				if (jh->b_committed_data) {
+					printk(KERN_WARNING "%s: freeing "
+					       "b_committed_data\n",
+					       __FUNCTION__);
+					kfree(jh->b_committed_data);
+				}
+				bh->b_private = NULL;
+				jh->b_bh = NULL;	/* debug, really */
+				clear_buffer_jbd(bh);
+				__brelse(bh);
+				journal_free_journal_head(jh);
 			}
-			bh->b_private = NULL;
-			jh->b_bh = NULL;	/* debug, really */
-			clear_buffer_jbd(bh);
-			__brelse(bh);
-			journal_free_journal_head(jh);
 		} else {
 			BUFFER_TRACE(bh, "journal_head was locked");
 		}
diff -ur linux-2.6.11-final-V0.7.40-00.orig/fs/jbd/transaction.c linux-2.6.11-final-V0.7.40-00/fs/jbd/transaction.c
--- linux-2.6.11-final-V0.7.40-00.orig/fs/jbd/transaction.c	2005-03-02 02:37:53.000000000 -0500
+++ linux-2.6.11-final-V0.7.40-00/fs/jbd/transaction.c	2005-03-14 09:46:41.000000000 -0500
@@ -1207,11 +1207,17 @@

 	BUFFER_TRACE(bh, "entry");

+	/*
+	 * Is it OK to check to see if this isn't a jbd buffer outside of
+	 * locks? Now that jbd_lock_bh_state only works with jbd buffers
+	 * I sure hope so.
+	 */
+	if (!buffer_jbd(bh))
+		goto not_jbd;
+
 	jbd_lock_bh_state(bh);
 	spin_lock(&journal->j_list_lock);

-	if (!buffer_jbd(bh))
-		goto not_jbd;
 	jh = bh2jh(bh);

 	/* Critical error: attempting to delete a bitmap buffer, maybe?
@@ -1219,7 +1225,7 @@
 	if (!J_EXPECT_JH(jh, !jh->b_committed_data,
 			 "inconsistent data on disk")) {
 		err = -EIO;
-		goto not_jbd;
+		goto bad_jbd;
 	}

 	if (jh->b_transaction == handle->h_transaction) {
@@ -1274,9 +1280,11 @@
 		}
 	}

-not_jbd:
+
+bad_jbd:
 	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
+not_jbd:
 	__brelse(bh);
 	return err;
 }
diff -ur linux-2.6.11-final-V0.7.40-00.orig/include/linux/jbd.h linux-2.6.11-final-V0.7.40-00/include/linux/jbd.h
--- linux-2.6.11-final-V0.7.40-00.orig/include/linux/jbd.h	2005-03-02 02:38:19.000000000 -0500
+++ linux-2.6.11-final-V0.7.40-00/include/linux/jbd.h	2005-03-14 09:46:57.000000000 -0500
@@ -324,6 +324,68 @@
 	return bh->b_private;
 }

+void journal_remove_journal_head(struct buffer_head *bh);
+
+#ifdef CONFIG_PREEMPT_RT
+
+BUFFER_FNS(JournalHead,journalhead)
+
+extern spinlock_t jbd_journal_head_lock;
+
+static inline void jbd_lock_bh_state(struct buffer_head *bh)
+{
+	BUG_ON(!bh->b_private);
+	atomic_inc(&bh2jh(bh)->b_state_wait_count);
+	spin_lock(&bh2jh(bh)->b_state_lock);
+}
+
+static inline int jbd_trylock_bh_state(struct buffer_head *bh)
+{
+	int ret;
+
+	BUG_ON(!bh->b_private);
+
+	if ((ret = spin_trylock(&bh2jh(bh)->b_state_lock)))
+		atomic_inc(&bh2jh(bh)->b_state_wait_count);
+
+	return ret;
+}
+
+static inline int jbd_is_locked_bh_state(struct buffer_head *bh)
+{
+	return bh2jh(bh) ? spin_is_locked(&bh2jh(bh)->b_state_lock) : 0;
+}
+
+static inline void jbd_unlock_bh_state(struct buffer_head *bh)
+{
+	int rmjh = 0;
+
+	BUG_ON(!atomic_read(&bh2jh(bh)->b_state_wait_count));
+	atomic_dec(&bh2jh(bh)->b_state_wait_count);
+
+	if (buffer_journalhead(bh)) {
+		clear_buffer_journalhead(bh);
+		rmjh = 1;
+	}
+
+	spin_unlock(&bh2jh(bh)->b_state_lock);
+
+	if (rmjh)
+		journal_remove_journal_head(bh);
+}
+
+static inline void jbd_lock_bh_journal_head(struct buffer_head *bh)
+{
+	spin_lock(&jbd_journal_head_lock);
+}
+
+static inline void jbd_unlock_bh_journal_head(struct buffer_head *bh)
+{
+	spin_unlock(&jbd_journal_head_lock);
+}
+
+#else /* !CONFIG_PREEMPT_RT */
+
 static inline void jbd_lock_bh_state(struct buffer_head *bh)
 {
 	bit_spin_lock(BH_State, &bh->b_state);
@@ -354,6 +416,8 @@
 	bit_spin_unlock(BH_JournalHead, &bh->b_state);
 }

+#endif /* CONFIG_PREEMPT_RT */
+
 struct jbd_revoke_table_s;

 /**
@@ -918,7 +982,6 @@
  */
 struct journal_head *journal_add_journal_head(struct buffer_head *bh);
 struct journal_head *journal_grab_journal_head(struct buffer_head *bh);
-void journal_remove_journal_head(struct buffer_head *bh);
 void journal_put_journal_head(struct journal_head *jh);

 /*
diff -ur linux-2.6.11-final-V0.7.40-00.orig/include/linux/journal-head.h linux-2.6.11-final-V0.7.40-00/include/linux/journal-head.h
--- linux-2.6.11-final-V0.7.40-00.orig/include/linux/journal-head.h	2005-03-02 02:38:25.000000000 -0500
+++ linux-2.6.11-final-V0.7.40-00/include/linux/journal-head.h	2005-03-14 09:46:41.000000000 -0500
@@ -80,6 +80,16 @@
 	 * [j_list_lock]
 	 */
 	struct journal_head *b_cpnext, *b_cpprev;
+
+	/*
+	 * Lock the state of the buffer head.
+	 */
+	spinlock_t b_state_lock;
+
+	/*
+	 * Count the processes that want/have the state lock.
+	 */
+	atomic_t b_state_wait_count;
 };

 #endif		/* JOURNAL_HEAD_H_INCLUDED */
diff -ur linux-2.6.11-final-V0.7.40-00.orig/include/linux/spinlock.h linux-2.6.11-final-V0.7.40-00/include/linux/spinlock.h
--- linux-2.6.11-final-V0.7.40-00.orig/include/linux/spinlock.h	2005-03-14 06:00:54.000000000 -0500
+++ linux-2.6.11-final-V0.7.40-00/include/linux/spinlock.h	2005-03-14 09:46:41.053696484 -0500
@@ -774,6 +774,10 @@
 }))


+#ifndef CONFIG_PREEMPT_RT
+
+/* These are just plain evil! */
+
 /*
  *  bit-based spin_lock()
  *
@@ -789,10 +793,15 @@
 	 * busywait with less bus contention for a good time to
 	 * attempt to acquire the lock bit.
 	 */
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT)
-	while (test_and_set_bit(bitnum, addr))
-		while (test_bit(bitnum, addr))
+	preempt_disable();
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
+	while (test_and_set_bit(bitnum, addr)) {
+		while (test_bit(bitnum, addr)) {
+			preempt_enable();
 			cpu_relax();
+			preempt_disable();
+		}
+	}
 #endif
 	__acquire(bitlock);
 }
@@ -802,9 +811,12 @@
  */
 static inline int bit_spin_trylock(int bitnum, unsigned long *addr)
 {
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT)
-	if (test_and_set_bit(bitnum, addr))
+	preempt_disable();
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
+	if (test_and_set_bit(bitnum, addr)) {
+		preempt_enable();
 		return 0;
+	}
 #endif
 	__acquire(bitlock);
 	return 1;
@@ -815,11 +827,12 @@
  */
 static inline void bit_spin_unlock(int bitnum, unsigned long *addr)
 {
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT)
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
 	BUG_ON(!test_bit(bitnum, addr));
 	smp_mb__before_clear_bit();
 	clear_bit(bitnum, addr);
 #endif
+	preempt_enable();
 	__release(bitlock);
 }

@@ -828,12 +841,15 @@
  */
 static inline int bit_spin_is_locked(int bitnum, unsigned long *addr)
 {
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT)
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
 	return test_bit(bitnum, addr);
+#elif defined CONFIG_PREEMPT
+	return preempt_count();
 #else
 	return 1;
 #endif
 }
+#endif /* CONFIG_PREEMPT_RT */

 #define DEFINE_SPINLOCK(name) \
 	spinlock_t name __cacheline_aligned_in_smp = _SPIN_LOCK_UNLOCKED(name)