[PATCH] Fix race in do_get_write_access()

[PATCH] Fix race in do_get_write_access()

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

  Hello,

  attached patch should fix the following race:
     Proc 1                               Proc 2

     __flush_batch()
       ll_rw_block()
                                        do_get_write_access()
					   lock_buffer
                                             jh is only waiting for checkpoint
					     -> b_transaction == NULL ->
					     do nothing
                                           unlock_buffer
    test_set_buffer_locked()
    test_clear_buffer_dirty()
                                           __journal_file_buffer()
                                        change the data
    submit_bh()

  and we have sent wrong data to disk... We now clean the dirty buffer
flag under buffer lock in all cases and hence we know that whenever a buffer
is starting to be journaled we either finish the pending write-out
before attaching a buffer to a transaction or we won't write the buffer
until the transaction is going to be committed... Please apply.

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

[-- Attachment #2: jbd-2.6.12-3-early-writeout-fix.diff --]
[-- Type: text/plain, Size: 2228 bytes --]

The test in jbd_unexpected_dirty_buffer() is redundant - remove it.
Furthermore we have to clear the buffer dirty bit under the buffer lock
to prevent races with buffer write-out (and hence prevent returning
a buffer with IO happening).

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.12-2-ll_rw_block-fix/fs/jbd/transaction.c linux-2.6.12-3-early-writeout-fix/fs/jbd/transaction.c
--- linux-2.6.12-2-ll_rw_block-fix/fs/jbd/transaction.c	2005-06-28 13:26:18.000000000 +0200
+++ linux-2.6.12-3-early-writeout-fix/fs/jbd/transaction.c	2005-07-09 08:40:01.000000000 +0200
@@ -493,20 +493,17 @@ static void jbd_unexpected_dirty_buffer(
 	struct buffer_head *bh = jh2bh(jh);
 	int jlist;
 
-	if (buffer_dirty(bh)) {
-		/* If this buffer is one which might reasonably be dirty
-		 * --- ie. data, or not part of this journal --- then
-		 * we're OK to leave it alone, but otherwise we need to
-		 * move the dirty bit to the journal's own internal
-		 * JBDDirty bit. */
-		jlist = jh->b_jlist;
-
-		if (jlist == BJ_Metadata || jlist == BJ_Reserved || 
-		    jlist == BJ_Shadow || jlist == BJ_Forget) {
-			if (test_clear_buffer_dirty(jh2bh(jh))) {
-				set_bit(BH_JBDDirty, &jh2bh(jh)->b_state);
-			}
-		}
+	/* If this buffer is one which might reasonably be dirty
+	 * --- ie. data, or not part of this journal --- then
+	 * we're OK to leave it alone, but otherwise we need to
+	 * move the dirty bit to the journal's own internal
+	 * JBDDirty bit. */
+	jlist = jh->b_jlist;
+
+	if (jlist == BJ_Metadata || jlist == BJ_Reserved || 
+	    jlist == BJ_Shadow || jlist == BJ_Forget) {
+		if (test_clear_buffer_dirty(jh2bh(jh)))
+			set_bit(BH_JBDDirty, &jh2bh(jh)->b_state);
 	}
 }
 
@@ -574,9 +571,14 @@ repeat:
 			if (jh->b_next_transaction)
 				J_ASSERT_JH(jh, jh->b_next_transaction ==
 							transaction);
-			JBUFFER_TRACE(jh, "Unexpected dirty buffer");
-			jbd_unexpected_dirty_buffer(jh);
- 		}
+		}
+		/*
+		 * In any case we need to clean the dirty flag and we must
+		 * do it under the buffer lock to be sure we don't race
+		 * with running write-out.
+		 */
+		JBUFFER_TRACE(jh, "Unexpected dirty buffer");
+		jbd_unexpected_dirty_buffer(jh);
  	}
 
 	unlock_buffer(bh);

Re: [PATCH] Fix race in do_get_write_access()

[Stephen C. Tweedie]

Hi,

On Mon, 2005-07-11 at 17:10, Jan Kara wrote:

>   attached patch should fix the following race:
...
>   and we have sent wrong data to disk... We now clean the dirty buffer
> flag under buffer lock in all cases and hence we know that whenever a buffer
> is starting to be journaled we either finish the pending write-out
> before attaching a buffer to a transaction or we won't write the buffer
> until the transaction is going to be committed... Please apply.

Looks good to me.

Btw, how did you find this?  Were you able to reproduce this in
practice?

Cheers,
 Stephen

Re: [PATCH] Fix race in do_get_write_access()

[Jan Kara]

  Hello,

> On Mon, 2005-07-11 at 17:10, Jan Kara wrote:
> 
> >   attached patch should fix the following race:
> ...
> >   and we have sent wrong data to disk... We now clean the dirty buffer
> > flag under buffer lock in all cases and hence we know that whenever a buffer
> > is starting to be journaled we either finish the pending write-out
> > before attaching a buffer to a transaction or we won't write the buffer
> > until the transaction is going to be committed... Please apply.
> 
> Looks good to me.
> 
> Btw, how did you find this?  Were you able to reproduce this in
> practice?
  I was not able to reproduce the problem in practice (not that I'd try
very much). When I was fixing the checkpointing code I noted that
ll_rw_block() need not actually send data to disk because of buffer
being locked. So I started inspecting how do we actually use buffer
lock and found this problem...

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs