On Fri, Mar 09, 2012 at 11:15:46AM +0100, Jan Kara wrote: > On Sat 03-03-12 21:55:58, Wu Fengguang wrote: > > On Fri, Mar 02, 2012 at 11:57:00AM -0800, Andrew Morton wrote: > > > On Fri, 2 Mar 2012 18:39:51 +0800 > > > Fengguang Wu <fengguang.wu@xxxxxxxxx> wrote: > > > > > > > > And I agree it's unlikely but given enough time and people, I > > > > > believe someone finds a way to (inadvertedly) trigger this. > > > > > > > > Right. The pageout works could add lots more iput() to the flusher > > > > and turn some hidden statistical impossible bugs into real ones. > > > > > > > > Fortunately the "flusher deadlocks itself" case is easy to detect and > > > > prevent as illustrated in another email. > > > > > > It would be a heck of a lot safer and saner to avoid the iput(). We > > > know how to do this, so why not do it? > > > > My concern about the page lock is, it costs more code and sounds like > > hacking around something. It seems we (including me) have been trying > > to shun away from the iput() problem. Since it's unlikely we are to > > get rid of the already existing iput() calls from the flusher context, > > why not face the problem, sort it out and use it with confident in new > > code? > We can get rid of it in the current code - see my patch set. And also we > don't have to introduce new iput() with your patch set... I don't think > using ->writepage() directly on a locked page would be a good thing because > filesystems tend to ignore it completely (e.g. ext4 if it needs to do an > allocation, or btrfs) or are much less efficient than when ->writepages() > is used. So I'd prefer going through writeback_single_inode() as the rest > of flusher thread. Totally agreed. I was also not feeling good to use ->writepage() on the locked page. It looks very nice to pin the inode with I_SYNC rather than igrab or lock_page. > > Let me try it now. The only scheme iput() can deadlock the flusher is > > for the iput() path to come back to queue some work and wait for it. > Let me stop you right here. You severely underestimate the complexity of > filesystems :). Take for example ext4. To do truncate you need to start a > transaction, to start a transaction, you have to have a space in journal. > To have a space in journal, you may have to wait for any other process to > finish writing. If that process needs to wait for flusher thread to be able > to finish writing, you have a deadlock. And there are other implicit > dependencies like this. And it's similar for other filesystems as well. So > you really want to make flusher thread as light as possible with the > dependencies. Ah OK, please forgive my ignorance. Let's get rid of the existing iput()s in the flusher thread. Thanks, Fengguang -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>