correction: fs/buffer.c underlocking async pages

correction: fs/buffer.c underlocking async pages

[Stefan.Bader]

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
Stefan.Bader@de.ibm.com
Phone: +49 (7031) 16-2472
----------------------------------------------------------------------------------

  When all other means of communication fail, try words.

--------------------------------------------
diff -ruN old/fs/buffer.c new/fs/buffer.c
--- old/fs/buffer.c     Thu Jun 21 09:47:20 2001
+++ new/fs/buffer.c     Thu Jun 21 10:44:01 2001
@@ -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
--- old/include/linux/fs.h      Thu Jun 21 09:47:51 2001
+++ new/include/linux/fs.h      Thu Jun 21 09:48:20 2001
@@ -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
-------------------

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

On Thu, Jun 21, 2001 at 04:39:11PM +0200, Stefan.Bader@de.ibm.com wrote:
> 
> 
> 
> Hi,
> 
> I ran into some problems with buffer.c trying to unlock a page of

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

> 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

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:

--- 2.4.6pre5aa1/fs/buffer.c.~1~	Thu Jun 21 16:22:40 2001
+++ 2.4.6pre5aa1/fs/buffer.c	Thu Jun 21 17:05:18 2001
@@ -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

Re: correction: fs/buffer.c underlocking async pages

[Chris Mason]

On Thursday, June 21, 2001 05:08:13 PM +0200 Andrea Arcangeli
<andrea@suse.de> wrote:

> 
> 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.

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

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

On Thu, Jun 21, 2001 at 11:16:42AM -0400, Chris Mason wrote:
> 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.

correct

Andrea

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

On Thu, Jun 21, 2001 at 04:39:11PM +0200, Stefan.Bader@de.ibm.com wrote:
> diff -ruN old/fs/buffer.c new/fs/buffer.c
> --- old/fs/buffer.c     Thu Jun 21 09:47:20 2001
> +++ new/fs/buffer.c     Thu Jun 21 10:44:01 2001
> @@ -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
> --- old/include/linux/fs.h      Thu Jun 21 09:47:51 2001
> +++ new/include/linux/fs.h      Thu Jun 21 09:48:20 2001
> @@ -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

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

Re: correction: fs/buffer.c underlocking async pages

[Linus Torvalds]

On Thu, 21 Jun 2001, Andrea Arcangeli wrote:
>
> It seems we can more simply drop the tmp->b_end_io == end_buffer_io_async
> check enterely and safely.

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.

> +++ 2.4.6pre5aa1/fs/buffer.c	Thu Jun 21 17:05:18 2001
> @@ -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?

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

Re: correction: fs/buffer.c underlocking async pages

[Linus Torvalds]

On Thu, 21 Jun 2001, Andrea Arcangeli wrote:
>
> I think the patch is ok. We must have a way to track down which bh are
> actually getting read,

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

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

On Thu, Jun 21, 2001 at 09:54:47AM -0700, Linus Torvalds wrote:
> The above _will_ break. "tmp" may be locked due to the write - and the

indeed, I missed the pending writes sorry.

Andrea

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

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
> (unnecessary) new bit?

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

Re: correction: fs/buffer.c underlocking async pages

[Andrea Arcangeli]

On Thu, Jun 21, 2001 at 07:15:22PM +0200, Andrea Arcangeli wrote:
> 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
> > (unnecessary) new bit?
> 
> 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.

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:

--- bh-async/fs/buffer.c.~1~	Thu Jun 21 08:03:49 2001
+++ bh-async/fs/buffer.c	Thu Jun 21 18:13:46 2001
@@ -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);
--- bh-async/include/linux/fs.h.~1~	Thu Jun 21 08:03:56 2001
+++ bh-async/include/linux/fs.h	Thu Jun 21 18:10:38 2001
@@ -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

Re: correction: fs/buffer.c underlocking async pages

[Chris Mason]

On Thursday, June 21, 2001 07:15:22 PM +0200 Andrea Arcangeli
<andrea@suse.de> wrote:

> 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
>> (unnecessary) new bit?
> 
> 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.

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

Re: correction: fs/buffer.c underlocking async pages

[Linus Torvalds]

On Thu, 21 Jun 2001, Andrea Arcangeli wrote:

> 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
> > (unnecessary) new bit?
>
> 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.

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

Re: correction: fs/buffer.c underlocking async pages

[Stefan.Bader]

Chris Mason <mason@suse.com>
06/21/01 08:20 PM
Please respond to Chris Mason


        To:     Andrea Arcangeli <andrea@suse.de>, Linus Torvalds
<torvalds@transmeta.com>
        cc:     Stefan Bader/Germany/IBM@IBMDE,
linux-kernel@vger.kernel.org
        Subject:        Re: correction: fs/buffer.c underlocking async
pages







On Thursday, June 21, 2001 07:15:22 PM +0200 Andrea Arcangeli
<andrea@suse.de> wrote:

>> 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
>>> (unnecessary) new bit?
>>
>> 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.

>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

Yes, that was exactly the thing I tried to do. In my case some sort of
bookkeeping
how many IO's where mapped (to a certain path) and how many came back.
My assumption first was, if I am restoring the old pointers before I call
the original
function it should work.
After running into problems this patch was just my quick hack to try out
whether this
was the only place that I did not think of. I wouldn't insist on the exact
way to do it,
but since it worked for me, I thought it might be worth to discuss (or
even be useful
for other people... ;-)).

- Stefan

Linux for eServer development
Stefan.Bader@de.ibm.com
Phone: +49 (7031) 16-2472
----------------------------------------------------------------------------------

  When all other means of communication fail, try words.