On 11/18/10 12:28 AM, Andrew Morton wrote: > On Thu, 18 Nov 2010 17:00:00 +1100 Nick Piggin <npiggin@xxxxxxxxx> wrote: > >> On Wed, Nov 17, 2010 at 07:29:00PM -0800, Andrew Morton wrote: >>> On Wed, 17 Nov 2010 22:06:13 -0500 "Ted Ts'o" <tytso@xxxxxxx> wrote: >>> >>>> On Wed, Nov 17, 2010 at 05:10:57PM +1100, Nick Piggin wrote: >>>>> On Tue, Nov 16, 2010 at 11:05:52PM -0600, Eric Sandeen wrote: >>>>>> On 11/16/10 10:38 PM, Nick Piggin wrote: >>>>>>>> as for the locking problems ... sorry about that! >>>>>>> >>>>>>> That's no problem. So is that an ack? :) >>>>>>> >>>>>> >>>>>> I'd like to test it with the original case it was supposed to solve; will >>>>>> do that tomorrow. >>>>> >>>>> OK, but it shouldn't make much difference, unless there is a lot of >>>>> strange activity happening on the sb (like mount / umount / remount / >>>>> freeze / etc). >>>> >>>> This makes sense to me as well. >>>> >>>> Acked-by: "Theodore Ts'o" <tytso@xxxxxxx> >>>> >>>> So how do we want to send this patch to Linus? It's a writeback >>>> change, so through some mm tree? >>> >>> It's in my todo pile. Even though the patch sucks, but not as much as >>> its changelog does. Am not particularly happy merging an alleged >>> bugfix where the bug is, and I quote, "I saw a lock order warning on >>> ext4 trigger". I mean, wtf? How is anyone supposed to review the code >>> based on that?? Or to understand it a year from now? >> >> Sorry bout the confusion, it was supposed to be "i_mutex", and then it >> would have been a bit more obvious. >> >> >>> When I get to it I'll troll this email thread and might be able to >>> kludge together a description which might be able to fool people into >>> thinking it makes sense. >> >> "Lock order reversal between s_umount and i_mutex". >> >> i_mutex nests inside s_umount in some writeback paths (it was the end >> io handler to convert unwritten extents IIRC). But hmm, wouldn't that >> be a bug? We aren't allowed to take i_mutex inside writeback, are we? > > I'm not sure that s_umount versus i_mutex has come up before. > > Logically I'd expect i_mutex to nest inside s_umount. Because s_umount > is a per-superblock thing, and i_mutex is a per-file thing, and files > live under superblocks. Nesting s_umount outside i_mutex creates > complex deadlock graphs between the various i_mutexes, I think. > > Someone tell me if btrfs has the same bug, via its call to > writeback_inodes_sb_nr_if_idle()? > > I don't see why these functions need s_umount at all, if they're called > from within ->write_begin against an inode on that superblock. If the > superblock can get itself disappeared while we're running ->write_begin > on it, we have problems, no? > > In which case I'd suggest just removing the down_read(s_umount) and > specifying that the caller must pin the superblock via some means. > > Only we can't do that because we need to hold s_umount until the > bdi_queue_work() worker has done its work. > > The fact that a call to ->write_begin can randomly return with s_umount > held, to be randomly released at some random time in the future is a > bit ugly, isn't it? write_begin is a pretty low-level, per-inode > thing. > > It'd be better if we pinned these superblocks via refcounting, not via > holding s_umount but even then, having ->write_begin randomly bump sb > refcounts for random periods of time is still pretty ugly. > > What a pickle. > > Can we just delete writeback_inodes_sb_nr_if_idle() and > writeback_inodes_sb_if_idle()? The changelog for 17bd55d037a02 is > pretty handwavy - do we know that deleting these things would make a > jot of difference? Really? I thought it was pretty decent ;) Anyway, xfstests 204, "Test out ENOSPC flushing on small filesystems." shows the problem clearly, IIRC. I should have included that in the changelog, I suppose, sorry. -Eric > And why _do_ we need to hold s_umount during the bdi_queue_work() > handover? Would simply bumping s_count suffice? > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
