On Fri, Dec 17, 2010 at 02:52:50PM +0800, Hugh Dickins wrote: > On Fri, 17 Dec 2010, Wu Fengguang wrote: > > On Mon, Dec 13, 2010 at 10:47:08PM +0800, Wu, Fengguang wrote: > > > > > + TP_fast_assign( > > > + strlcpy(__entry->bdi, > > > + dev_name(mapping->backing_dev_info->dev), 32); > > > + __entry->ino = mapping->host->i_ino; > > > > I got an oops against the above line on shmem. Can be fixed by the > > below patch, but still not 100% confident.. > > > > Thanks, > > Fengguang > > --- > > Subject: writeback fix dereferencing NULL shmem mapping->host > > Date: Thu Dec 16 22:22:00 CST 2010 > > > > The oops happens when doing "cp /proc/vmstat /dev/shm". It seems to be > > triggered on accessing host->i_ino, since the offset of i_ino is exactly > > 0x50. However I'm afraid the problem is not fully understand > > > > 1) it's not normal that tmpfs will have mapping->host == NULL > > > > 2) I tried removing the dereference as the below diff, however it > > didn't stop the oops. This is very weird. > > > > TRACE_EVENT balance_dirty_state: > > > > TP_fast_assign( > > strlcpy(__entry->bdi, > > dev_name(mapping->backing_dev_info->dev), 32); > > I believe this line above is actually the problem: you can imagine that > tmpfs leaves backing_dev_info->dev NULL, and dev_name() appears to Ah, I didn't notice that obvious fact in shmem_backing_dev_info.. > access dev->init_name at 64-bit offset 0x50 down struct device. And it's such a coincident that the two lines accessed different struct members both with offset 0x50 :) > > - __entry->ino = mapping->host->i_ino; > > __entry->nr_dirty = nr_dirty; > > __entry->nr_writeback = nr_writeback; > > __entry->nr_unstable = nr_unstable; > ... > > > > CC: Hugh Dickins <hugh.dickins@xxxxxxxxxxxxx> > > I prefer hughd@xxxxxxxxxx, but the tiscali address survived unexpectedly. OK, just updated my alias db. > > Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx> > > --- > > mm/page-writeback.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > --- linux-next.orig/mm/page-writeback.c 2010-12-17 09:30:11.000000000 +0800 > > +++ linux-next/mm/page-writeback.c 2010-12-17 09:31:05.000000000 +0800 > > @@ -907,6 +907,9 @@ void balance_dirty_pages_ratelimited_nr( > > { > > struct backing_dev_info *bdi = mapping->backing_dev_info; > > > > + if (!mapping_cap_writeback_dirty(mapping)) > > + return; > > + > > current->nr_dirtied += nr_pages_dirtied; > > > > if (unlikely(!current->nr_dirtied_pause)) > > That would not really be the right patch to fix your oops, but it Then it will also avoid oops in another tracepoint balance_dirty_pages. ([PATCH 21/35] writeback: trace balance_dirty_pages() in this series) I skipped the backing_dev_info->dev check partly because it's also referenced in tracepoint balance_dirty_pages. So I did this cure-all change that makes sense in itself :) > or something like would be a very sensible patch in its own right: > looking back through old patches I never got around to sending in, > I can see I had a very similar one two years ago, to save wasting > time on dirty page accounting here when it's inappropriate. It's a pity you didn't submit it. > Though mine was testing !mapping_cap_account_dirty(mapping). Sorry I didn't check whether to use mapping_cap_writeback_dirty() or mapping_cap_account_dirty() -- I just used a random one of them. Some double checking shows that the end results are the same as for now: all related parts set both flags at the same time with BDI_CAP_NO_ACCT_AND_WRITEBACK. However it does look more sane to use bdi_cap_account_dirty(bdi). I'll switch to it. Thank you! Thanks, Fengguang -- 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