On Tue, Nov 16, 2010 at 12:32:52PM -0800, Andrew Morton wrote:
> On Tue, 16 Nov 2010 22:00:58 +1100
> Nick Piggin <npiggin@xxxxxxxxx> wrote:
>
> > I saw a lock order warning on ext4 trigger. This should solve it.
>
> Send us the trace, please.
I lost it, sorry.
> The code comment implies that someone is calling down_read() under
> i_lock? That would be bad, and I'd expect it to have produced a
> might_sleep() warning, not a lockdep trace.
Sorry not i_lock, i_mutex. writeback_inodes_sb_if_idle is called by
ext4's write_begin function which is called with i_mutex held from
generic_file_buffered_write, I believe is the trace.
> And I don't see how we can call writeback_inodes_sb() under i_lock
> anyway, so I don't really have a clue what's going on here!
>
> > Raciness shouldn't matter much, because writeback can stop just
> > after we make the test and return anyway (so the API is racy anyway).
> >
> > Signed-off-by: Nick Piggin <npiggin@xxxxxxxxx>
> >
> > Index: linux-2.6/fs/fs-writeback.c
> > ===================================================================
> > --- linux-2.6.orig/fs/fs-writeback.c 2010-11-16 21:44:32.000000000 +1100
> > +++ linux-2.6/fs/fs-writeback.c 2010-11-16 21:49:37.000000000 +1100
> > @@ -1125,16 +1125,20 @@ EXPORT_SYMBOL(writeback_inodes_sb);
> > *
> > * Invoke writeback_inodes_sb if no writeback is currently underway.
> > * Returns 1 if writeback was started, 0 if not.
> > + *
> > + * May be called inside i_lock. May not start writeback if locks cannot
> > + * be acquired.
> > */
> > int writeback_inodes_sb_if_idle(struct super_block *sb)
> > {
> > if (!writeback_in_progress(sb->s_bdi)) {
> > - down_read(&sb->s_umount);
> > - writeback_inodes_sb(sb);
> > - up_read(&sb->s_umount);
> > - return 1;
> > - } else
> > - return 0;
> > + if (down_read_trylock(&sb->s_umount)) {
> > + writeback_inodes_sb(sb);
> > + up_read(&sb->s_umount);
> > + return 1;
> > + }
> > + }
> > + return 0;
>
> And it's pretty generous to describe a s/down_read/down_read_trylock/
> as a "fix". Terms like "bandaid" and "workaround" come to mind.
As much as the writeback_inodes_sb_if_idle API itself is a bandaid,
I suppose. (it doesn't do any rate limiting of the dirtier, it's racy,
it doesn't specify how much to writeback, it's synchronous, etc).
Anyway, I don't know, there's not much other option for 2.6.37 AFAIKS.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
