Re: [PATCH] exclusive wakeup for lock_buffer

Re: [PATCH] exclusive wakeup for lock_buffer

[John Heil]

On Tue, 20 Feb 2001, Alan Cox wrote:

> Date: Tue, 20 Feb 2001 03:12:15 +0000 (GMT)
> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> To: Marcelo Tosatti <marcelo@conectiva.com.br>
> Cc: Linus Torvalds <torvalds@transmeta.com>,
>     lkml <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] exclusive wakeup for lock_buffer
> 
> > --- linux/include/linux/locks.h.orig	Mon Feb 19 23:16:50 2001
> > +++ linux/include/linux/locks.h	Mon Feb 19 23:21:48 2001
> > @@ -13,6 +13,7 @@
> >   * lock buffers.
> >   */
> >  extern void __wait_on_buffer(struct buffer_head *);
> > +extern void __lock_buffer(struct buffer_head *);
> 
> So are we starting 2.5 now ?

If per chance it's so, what's actually in plan for 2.5 so far?


> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-----------------------------------------------------------------
John Heil
South Coast Software
Custom systems software for UNIX and IBM MVS mainframes
1-714-774-6952
kerndev@sc-software.com
http://www.sc-software.com
-----------------------------------------------------------------

[PATCH] exclusive wakeup for lock_buffer

[Marcelo Tosatti]

Hi, 

The following patch makes lock_buffer() use the exclusive wakeup scheme
added in 2.3.

Against 2.4.2pre4.


--- linux/fs/buffer.c.orig	Mon Feb 19 23:13:08 2001
+++ linux/fs/buffer.c	Mon Feb 19 23:16:26 2001
@@ -142,13 +142,18 @@
  * if 'b_wait' is set before calling this, so that the queues aren't set
  * up unnecessarily.
  */
-void __wait_on_buffer(struct buffer_head * bh)
+void __wait_on_buffer(struct buffer_head * bh, int exclusive)
 {
 	struct task_struct *tsk = current;
 	DECLARE_WAITQUEUE(wait, tsk);
 
 	atomic_inc(&bh->b_count);
-	add_wait_queue(&bh->b_wait, &wait);
+
+	if (exclusive)
+		add_wait_queue_exclusive(&bh->b_wait, &wait);
+	else
+		add_wait_queue(&bh->b_wait, &wait);
+
 	do {
 		run_task_queue(&tq_disk);
 		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
@@ -2332,7 +2337,7 @@
 		tmp = tmp->b_this_page;
 		if (buffer_locked(p)) {
 			if (wait > 1)
-				__wait_on_buffer(p);
+				__wait_on_buffer(p, 0);
 		} else if (buffer_dirty(p))
 			ll_rw_block(WRITE, 1, &p);
 	} while (tmp != bh);
--- linux/include/linux/locks.h.orig	Mon Feb 19 23:21:10 2001
+++ linux/include/linux/locks.h	Mon Feb 19 23:17:42 2001
@@ -12,18 +12,18 @@
  * Buffer cache locking - note that interrupts may only unlock, not
  * lock buffers.
  */
-extern void __wait_on_buffer(struct buffer_head *);
+extern void __wait_on_buffer(struct buffer_head *, int);
 
 extern inline void wait_on_buffer(struct buffer_head * bh)
 {
 	if (test_bit(BH_Lock, &bh->b_state))
-		__wait_on_buffer(bh);
+		__wait_on_buffer(bh, 0);
 }
 
 extern inline void lock_buffer(struct buffer_head * bh)
 {
 	while (test_and_set_bit(BH_Lock, &bh->b_state))
-		__wait_on_buffer(bh);
+		__wait_on_buffer(bh, 1);
 }
 
 extern inline void unlock_buffer(struct buffer_head *bh)

Re: [PATCH] exclusive wakeup for lock_buffer

[Marcelo Tosatti]

On Mon, 19 Feb 2001, Linus Torvalds wrote:

> 
> 
> On Mon, 19 Feb 2001, Marcelo Tosatti wrote:
> > 
> > The following patch makes lock_buffer() use the exclusive wakeup scheme
> > added in 2.3.
> 
> Ugh, This is horrible.
> 
> You should NOT have one function that does two completely different things
> depending on a flag. That way lies madness and bad coding habits.
> 
> Just do two different functions - make one be "__wait_on_buffer()", and
> the other be "__lock_buffer()". See how the page functions work.
> 
> 		Linus

Ok. 

--- linux/include/linux/locks.h.orig	Mon Feb 19 23:16:50 2001
+++ linux/include/linux/locks.h	Mon Feb 19 23:21:48 2001
@@ -13,6 +13,7 @@
  * lock buffers.
  */
 extern void __wait_on_buffer(struct buffer_head *);
+extern void __lock_buffer(struct buffer_head *);
 
 extern inline void wait_on_buffer(struct buffer_head * bh)
 {
@@ -22,8 +23,8 @@
 
 extern inline void lock_buffer(struct buffer_head * bh)
 {
-	while (test_and_set_bit(BH_Lock, &bh->b_state))
-		__wait_on_buffer(bh);
+	if (test_and_set_bit(BH_Lock, &bh->b_state))
+		__lock_on_buffer(bh);
 }
 
 extern inline void unlock_buffer(struct buffer_head *bh)
--- linux/fs/buffer.c.orig	Mon Feb 19 23:09:31 2001
+++ linux/fs/buffer.c	Mon Feb 19 23:31:25 2001
@@ -161,6 +161,30 @@
 	atomic_dec(&bh->b_count);
 }
 
+void __lock_on_buffer(struct buffer_head * bh)
+{
+	struct task_struct *tsk = current;
+	DECLARE_WAITQUEUE(wait, tsk);
+
+	atomic_inc(&bh->b_count);
+	add_wait_queue_exclusive(&bh->b_wait, &wait);
+	for(;;) { 
+		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		if (test_bit(BH_Lock, &bh->b_state)) {
+			run_task_queue(&tq_disk);
+			schedule();
+			continue;
+		}
+
+		if (!test_and_set_bit(BH_Lock, &bh->b_state))
+			break;
+	} 
+	tsk->state = TASK_RUNNING;
+	remove_wait_queue(&bh->b_wait, &wait);
+	atomic_dec(&bh->b_count);
+}
+
+
 /* Call sync_buffers with wait!=0 to ensure that the call does not
  * return until all buffer writes have completed.  Sync() may return
  * before the writes have finished; fsync() may not.

Re: [PATCH] exclusive wakeup for lock_buffer

[Linus Torvalds]

On Mon, 19 Feb 2001, Marcelo Tosatti wrote:
> 
> The following patch makes lock_buffer() use the exclusive wakeup scheme
> added in 2.3.

Ugh, This is horrible.

You should NOT have one function that does two completely different things
depending on a flag. That way lies madness and bad coding habits.

Just do two different functions - make one be "__wait_on_buffer()", and
the other be "__lock_buffer()". See how the page functions work.

		Linus

Re: [PATCH] exclusive wakeup for lock_buffer

[Alan Cox]

> --- linux/include/linux/locks.h.orig	Mon Feb 19 23:16:50 2001
> +++ linux/include/linux/locks.h	Mon Feb 19 23:21:48 2001
> @@ -13,6 +13,7 @@
>   * lock buffers.
>   */
>  extern void __wait_on_buffer(struct buffer_head *);
> +extern void __lock_buffer(struct buffer_head *);

So are we starting 2.5 now ?

Re: [PATCH] exclusive wakeup for lock_buffer

[Marcelo Tosatti]

On Tue, 20 Feb 2001, Alan Cox wrote:

> > --- linux/include/linux/locks.h.orig	Mon Feb 19 23:16:50 2001
> > +++ linux/include/linux/locks.h	Mon Feb 19 23:21:48 2001
> > @@ -13,6 +13,7 @@
> >   * lock buffers.
> >   */
> >  extern void __wait_on_buffer(struct buffer_head *);
> > +extern void __lock_buffer(struct buffer_head *);
> 
> So are we starting 2.5 now ?

Alan,

This patch only avoids unecessary wakeups. It doesn't add any new
functionality.

Re: [PATCH] exclusive wakeup for lock_buffer

[Alan Cox]

> > >  extern void __wait_on_buffer(struct buffer_head *);
> > > +extern void __lock_buffer(struct buffer_head *);
> > 
> > So are we starting 2.5 now ?
> 
> Alan,
> 
> This patch only avoids unecessary wakeups. It doesn't add any new
> functionality.

I think making potentially very hard to debug changes to core code for
minor performance reasons alone isnt 2.4.x work. At least not yet

Re: [PATCH] exclusive wakeup for lock_buffer

[Ulf Carlsson]

> --- linux/include/linux/locks.h.orig	Mon Feb 19 23:16:50 2001
> +++ linux/include/linux/locks.h	Mon Feb 19 23:21:48 2001
> @@ -13,6 +13,7 @@
>   * lock buffers.
>   */
>  extern void __wait_on_buffer(struct buffer_head *);
> +extern void __lock_buffer(struct buffer_head *);

This doesn't match the function definition either.

Ulf