correction: fs/buffer.c underlocking async pages

Hi,

I ran into some problems with buffer.c trying to unlock a page of async io buffer heads more
than once.
IMHO end_buffer_io_async() shouldn't rely on the value of b_end_io to decide if the whole
page can be unlocked. It would make it easier for other layers (well remappers like md or
lvm) to create an end_io chain without the need of allocating new buffer heads just for that.
Is the comparision on b_end_io really necessary? I would assume that all bh on the same
page belong to async io.
Otherwise, could something like below be done instead?

Linux for eServer development
[email protected]
Phone: +49 (7031) 16-2472
----------------------------------------------------------------------------------

When all other means of communication fail, try words.

--------------------------------------------

diff -ruN old/fs/buffer.c new/fs/buffer.c
@@ -798,11 +798,12 @@
         * that unlock the page..
         */
        spin_lock_irqsave(&page_uptodate_lock, flags);
+       clear_bit(BH_Async, &bh->b_state);
        unlock_buffer(bh);
        atomic_dec(&bh->b_count);
        tmp = bh->b_this_page;
        while (tmp != bh) {
-               if (tmp->b_end_io == end_buffer_io_async &&

buffer_locked(tmp))
+ if (test_bit(BH_Async, &tmp->b_state) &&
buffer_locked(tmp))

                        goto still_busy;
                tmp = tmp->b_this_page;
        }

@@ -834,6 +835,7 @@

 void set_buffer_async_io(struct buffer_head *bh) {
     bh->b_end_io = end_buffer_io_async ;

+ set_bit(BH_Async, &bh->b_state);
}

/*
@@ -1535,6 +1537,7 @@

        do {
                lock_buffer(bh);
                bh->b_end_io = end_buffer_io_async;
+               set_bit(BH_Async, &bh->b_state);
                atomic_inc(&bh->b_count);
                set_bit(BH_Uptodate, &bh->b_state);
                clear_bit(BH_Dirty, &bh->b_state);
@@ -1736,6 +1739,7 @@
                struct buffer_head * bh = arr[i];
                lock_buffer(bh);
                bh->b_end_io = end_buffer_io_async;
+               set_bit(BH_Async, &bh->b_state);
                atomic_inc(&bh->b_count);
        }

@@ -2182,6 +2186,7 @@

                bh->b_blocknr = *(b++);
                set_bit(BH_Mapped, &bh->b_state);
                bh->b_end_io = end_buffer_io_async;
+               set_bit(BH_Async, &bh->b_state);
                atomic_inc(&bh->b_count);
                bh = bh->b_this_page;
        } while (bh != head);

diff -ruN old/include/linux/fs.h new/include/linux/fs.h
@@ -207,6 +207,7 @@
 #define BH_Mapped      4       /* 1 if the buffer has a disk mapping */
 #define BH_New         5       /* 1 if the buffer is new and not yet

written out */
#define BH_Protected 6 /* 1 if the buffer is protected */
+#define BH_Async 7 /* 1 if the buffer is used for asyncronous io */

/*
* Try to keep the most commonly used fields in single cache lines (16
-------------------

sorry for the huge delay in the answer, I was going to answer your previous two emails very shortly (I didn't forgotten ;).

It seems we can more simply drop the tmp->b_end_io == end_buffer_io_async check enterely and safely. Possibly we could build a debugging logic to make sure nobody ever lock down a buffer mapped on a pagecache that is under async I/O (which in realty is "sync" I/O, you know the async/sync names of the kernel io callbacks are the opposite of realty ;).

The reason it seems safe to me is that when a pagecache is under async I/O (async in kernel terms) it says locked all the time until the last call of the async I/O callback, and _nobody_ is ever allowed to mess with the anon bh overlapped on the pagecache while the page stays locked down. As far as the async end_io callback is recalled it means the page is still locked down so we know if the end_io callback points to something else it's because of a underlying remapper, nobody else would be allowed to play the bh of a page locked down.

so in short:

@@ -850,7 +850,7 @@
 	atomic_dec(&bh->b_count);
 	tmp = bh->b_this_page;
 	while (tmp != bh) {
-		if (tmp->b_end_io == end_buffer_io_async && buffer_locked(tmp))
+		if (buffer_locked(tmp))
 			goto still_busy;
 		tmp = tmp->b_this_page;
 	}

can anybody see a problem in the above patch? Al, Ingo, Linus?

Andrea

Think of a mixture of fsync_inode_buffers and async i/o on page. Since fsync_inode_buffers uses ll_rw_block, if that end_io handler is the last to run the page never gets unlocked.

-chris

correct

Andrea

I doubt it.

Think about somebody who writes a partial page (but a full buffer). Somebody _else_ then reads the rest of the page. You'll have one buffer up-to-date (but possibly under write-back IO), and the others being read in asynchronously.

The above _will_ break. "tmp" may be locked due to the write - and the write will never call "end_buffer_io_async" because writes do not unlock the page. So if the write finishes last, you'll _never_ unlock the page.

I don't see why Stefan wants to change the current logic. The current logic is correct, and if there are double-unlock problems then those are due to some other bug.

Linus

indeed, I missed the pending writes sorry.

Andrea

I think the patch is ok. We must have a way to track down which bh are actually getting read, the others could be just uptodate and dirty and waiting kupdate to flush them under us (and we cannot defer the unlock of the page due those locked buffers under flush write-I/O or we deadlock).

Andrea

We _do_ have a way. The way is called "bh->b_end_io == end_io_async".

What's the problem with the existing code, and why do people want to add a (unnecessary) new bit?

Linus

On Thu, Jun 21, 2001 at 09:56:04AM -0700, Linus Torvalds wrote:
What's the problem with the existing code, and why do people want to add a

there's no problem with the existing code, what I understood is that they cannot overwrite the ->b_end_io callback in the lowlevel blkdev layer or the page will be unlocked too early.

Andrea

Just to be more explicit, the big problem is mixing different async callbacks on the same page. The patch would also allow things like this:

fs_specific_end_io() {
do something special
end_buffer_io_async()
}

-chris

Oh, fair enough.

I don't have any objections to the patch in that case, although it does end up being a 2.5.x issue as far as I'm concerned (and don't worry, 2.5.x looks like it will open in a week or two, so we're not talking about long timeframes).

Obviously 2.5.x code may be back-ported to 2.4.x later. That will be up to Alan.

Linus

I rediffed it a bit so the buffer_async and buffer_locked check can get merged in a single asm instruction by gcc and cleaned up the set_buffer_async_io logic a bit inside buffer.c:

@@ -806,11 +806,12 @@
 	 * that unlock the page..
 	 */
 	spin_lock_irqsave(&page_uptodate_lock, flags);
+	mark_buffer_async(bh, 0);
 	unlock_buffer(bh);
 	atomic_dec(&bh->b_count);
 	tmp = bh->b_this_page;
 	while (tmp != bh) {
-		if (tmp->b_end_io == end_buffer_io_async && buffer_locked(tmp))
+		if (buffer_async(tmp) && buffer_locked(tmp))
 			goto still_busy;
 		tmp = tmp->b_this_page;
 	}

@@ -840,8 +841,9 @@
 	return;
 }
 
-void set_buffer_async_io(struct buffer_head *bh) {
+inline void set_buffer_async_io(struct buffer_head *bh) {
     bh->b_end_io = end_buffer_io_async ;
+    mark_buffer_async(bh, 1);
 }
 
 /*

@@ -1531,7 +1533,7 @@
 	/* Stage 2: lock the buffers, mark them clean */
 	do {
 		lock_buffer(bh);
-		bh->b_end_io = end_buffer_io_async;
+		set_buffer_async_io(bh);
 		atomic_inc(&bh->b_count);
 		set_bit(BH_Uptodate, &bh->b_state);
 		clear_bit(BH_Dirty, &bh->b_state);

@@ -1732,7 +1734,7 @@
 	for (i = 0; i < nr; i++) {
 		struct buffer_head * bh = arr[i];
 		lock_buffer(bh);
-		bh->b_end_io = end_buffer_io_async;
+		set_buffer_async_io(bh);
 		atomic_inc(&bh->b_count);
 	}
 

@@ -2178,7 +2180,7 @@
 		lock_buffer(bh);
 		bh->b_blocknr = *(b++);
 		set_bit(BH_Mapped, &bh->b_state);
-		bh->b_end_io = end_buffer_io_async;
+		set_buffer_async_io(bh);
 		atomic_inc(&bh->b_count);
 		bh = bh->b_this_page;
 	} while (bh != head);

@@ -215,6 +215,7 @@
 	BH_Mapped,	/* 1 if the buffer has a disk mapping */
 	BH_New,		/* 1 if the buffer is new and not yet written out */
 	BH_Protected,	/* 1 if the buffer is protected */
+	BH_Async,	/* 1 if the buffer is under end_buffer_io_async I/O */
 
 	BH_PrivateStart,/* not a state bit, but the first bit available
 			 * for private allocation by other entities

@@ -275,6 +276,7 @@
 #define buffer_mapped(bh)	__buffer_state(bh,Mapped)
 #define buffer_new(bh)		__buffer_state(bh,New)
 #define buffer_protected(bh)	__buffer_state(bh,Protected)
+#define buffer_async(bh)	__buffer_state(bh,Async)
 
 #define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
 

@@ -1109,6 +1111,14 @@
 extern void FASTCALL(mark_buffer_dirty(struct buffer_head *bh));
 
 #define atomic_set_buffer_dirty(bh) test_and_set_bit(BH_Dirty, &(bh)->b_state)
+
+static inline void mark_buffer_async(struct buffer_head * bh, int on)
+{
+	if (on)
+		set_bit(BH_Async, &bh->b_state);
+	else
+		clear_bit(BH_Async, &bh->b_state);
+}
 
 /*
  * If an error happens during the make_request, this function

This way looks more robust to me.

However I want to add a very fat warning about not allocating a new bh:
never assume b_private and b_end_io and possibly otheres weren't remapped by another logical layer before you or the stacking will break badly, we want everything to remains transparent so we can stack multiple layers one on the top of the other.

Andrea