Messages in this thread | | | Date | Fri, 30 Aug 2019 10:09:03 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH block/for-next] writeback: add tracepoints for cgroup foreign writebacks |
| |
Hello, Jan.
On Fri, Aug 30, 2019 at 06:42:11PM +0200, Jan Kara wrote: > Well, but if you look at __set_page_dirty_nobuffers() it is careful. It > does: > > struct address_space *mapping = page_mapping(page); > > if (!mapping) { > bail > } > ... use mapping > > Exactly because page->mapping can become NULL under your hands if you don't > hold page lock. So I think you either need something similar in your > tracepoint or handle this in the caller.
So, account_page_dirtied() is called from two places.
__set_page_dirty() and __set_page_dirty_nobuffers(). The following is from the latter.
lock_page_memcg(page); if (!TestSetPageDirty(page)) { struct address_space *mapping = page_mapping(page); ...
if (!mapping) { unlock_page_memcg(page); return 1; }
xa_lock_irqsave(&mapping->i_pages, flags); BUG_ON(page_mapping(page) != mapping); WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page)); account_page_dirtied(page, mapping); ...
If I'm reading it right, it's saying that at this point if mapping exists after setting page dirty, it must not change while locking i_pages.
__set_page_dirty_nobuffers() is more brief but seems to be making the same assumption.
xa_lock_irqsave(&mapping->i_pages, flags); if (page->mapping) { /* Race with truncate? */ WARN_ON_ONCE(warn && !PageUptodate(page)); account_page_dirtied(page, mapping); __xa_set_mark(&mapping->i_pages, page_index(page), PAGECACHE_TAG_DIRTY); } xa_unlock_irqrestore(&mapping->i_pages, flags);
Both are clearly assuming that once i_pages is locked, mapping can't change. So, inside account_page_dirtied(), mapping clearly can't change. The TP in question - track_foreign_dirty - is invoked from mem_cgroup_track_foreign_dirty() which is only called from account_page_dirty(), so I'm failing to see how mapping would change there.
Thanks.
-- tejun
| |