Re: [PATCH] not sleep while holding a locked page in block_truncate_page

From: Marcelo Tosatti (marcelo@conectiva.com.br)
Date: Thu Dec 28 2000 - 11:25:56 EST

On Thu, 28 Dec 2000, Linus Torvalds wrote:

>
>
> On Thu, 28 Dec 2000, Marcelo Tosatti wrote:
> >
> > Hi Linus,
> >
> > block_truncate_page() function unecessarily calls mark_buffer_dirty(),
> > which may wait on bdflush, while holding a locked page.
>
> Good catch. It should be ok to sleep for bdflush while holding the page,
> but at the same time it's certainly preferable _not_ to do that.
>
> bdflush should not need any locks that we hold, so it shouldn't have
> deadlocked. How did you find this? Just reading the source, or have you
> seen any real problems?

Just reading the code.

> If the latter, maybe there's something that tries to get a FS lock
> when it shouldn't?

No, its not a deadlock. Its just a potential performance
problem.

Actually, ext2 is full of calls to mark_buffer_dirty() while holding the
superblock lock. Juan Quintela (which is being CC'ed) has a patch to
minimize the contention of the superblock lock by calling balance_dirty()
only after the sb lock gets unlocked all over ext2. Would you accept that
patch for 2.4?

Moreover, it seems mark_buffer_dirty does not makes a lot of sense wrt
balance_dirty: 

---

/* atomic version, the user must call balance_dirty() by hand
   as soon as it become possible to block */
void __mark_buffer_dirty(struct buffer_head *bh)
{
        if (!atomic_set_buffer_dirty(bh))
                __mark_dirty(bh);
}

void mark_buffer_dirty(struct buffer_head *bh)
{
        __mark_buffer_dirty(bh);
        balance_dirty(bh->b_dev);
}

--- 

If we call mark_buffer_dirty() on an already dirty buffer, we may sleep
waiting for bdflush even if we haven't caused _any_ real disk IO (because
the buffer was already dirty anyway).

I think it makes more sense if we only call balance_dirty if we actually
caused real disk IO.

Would you accept a patch to change that situation by making
__mark_buffer_dirty return the old dirty bit value and make
mark_buffer_dirty only sleep on bdflush if we dirtied a clean buffer?

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

This archive was generated by hypermail 2b29 : Sun Dec 31 2000 - 21:00:11 EST