| Message ID | 1556274402-19018-1-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page-writeback: introduce tracepoint for wait_on_page_writeback | expand |
On Fri, 26 Apr 2019 18:26:42 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > Recently there're some hungtasks on our server due to > wait_on_page_writeback, and we want to know the details of this > PG_writeback, i.e. this page is writing back to which device. > But it is not so convenient to get the details. > > I think it would be better to introduce a tracepoint for diagnosing > the writeback details. Fair enough, I guess. > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -537,15 +537,7 @@ static inline int wait_on_page_locked_killable(struct page *page) > > extern void put_and_wait_on_page_locked(struct page *page); > > -/* > - * Wait for a page to complete writeback > - */ > -static inline void wait_on_page_writeback(struct page *page) > -{ > - if (PageWriteback(page)) > - wait_on_page_bit(page, PG_writeback); > -} > - > +void wait_on_page_writeback(struct page *page); > extern void end_page_writeback(struct page *page); > void wait_for_stable_page(struct page *page); > > ... > > +/* > + * Wait for a page to complete writeback > + */ > +void wait_on_page_writeback(struct page *page) > +{ > + if (PageWriteback(page)) { > + trace_wait_on_page_writeback(page, page_mapping(page)); > + wait_on_page_bit(page, PG_writeback); > + } > +} > +EXPORT_SYMBOL_GPL(wait_on_page_writeback); But this is a stealth change to the wait_on_page_writeback() licensing. I will get sad emails from developers of accidentally-broken out-of-tree filesystems. We can discuss changing the licensing, but this isn't the way to do it! --- a/mm/page-writeback.c~mm-page-writeback-introduce-tracepoint-for-wait_on_page_writeback-fix +++ a/mm/page-writeback.c @@ -2818,7 +2818,7 @@ void wait_on_page_writeback(struct page wait_on_page_bit(page, PG_writeback); } } -EXPORT_SYMBOL_GPL(wait_on_page_writeback); +EXPORT_SYMBOL(wait_on_page_writeback); /** * wait_for_stable_page() - wait for writeback to finish, if necessary.
On Sat, Apr 27, 2019 at 2:25 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 26 Apr 2019 18:26:42 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > Recently there're some hungtasks on our server due to > > wait_on_page_writeback, and we want to know the details of this > > PG_writeback, i.e. this page is writing back to which device. > > But it is not so convenient to get the details. > > > > I think it would be better to introduce a tracepoint for diagnosing > > the writeback details. > > Fair enough, I guess. > > > --- a/include/linux/pagemap.h > > +++ b/include/linux/pagemap.h > > @@ -537,15 +537,7 @@ static inline int wait_on_page_locked_killable(struct page *page) > > > > extern void put_and_wait_on_page_locked(struct page *page); > > > > -/* > > - * Wait for a page to complete writeback > > - */ > > -static inline void wait_on_page_writeback(struct page *page) > > -{ > > - if (PageWriteback(page)) > > - wait_on_page_bit(page, PG_writeback); > > -} > > - > > +void wait_on_page_writeback(struct page *page); > > extern void end_page_writeback(struct page *page); > > void wait_for_stable_page(struct page *page); > > > > ... > > > > +/* > > + * Wait for a page to complete writeback > > + */ > > +void wait_on_page_writeback(struct page *page) > > +{ > > + if (PageWriteback(page)) { > > + trace_wait_on_page_writeback(page, page_mapping(page)); > > + wait_on_page_bit(page, PG_writeback); > > + } > > +} > > +EXPORT_SYMBOL_GPL(wait_on_page_writeback); > > But this is a stealth change to the wait_on_page_writeback() licensing. > I will get sad emails from developers of accidentally-broken > out-of-tree filesystems. > > We can discuss changing the licensing, but this isn't the way to do it! > Got it. Thanks for your explatation. Thanks Yafang
On Fri 26-04-19 11:25:42, Andrew Morton wrote: > On Fri, 26 Apr 2019 18:26:42 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: [...] > > +/* > > + * Wait for a page to complete writeback > > + */ > > +void wait_on_page_writeback(struct page *page) > > +{ > > + if (PageWriteback(page)) { > > + trace_wait_on_page_writeback(page, page_mapping(page)); > > + wait_on_page_bit(page, PG_writeback); > > + } > > +} > > +EXPORT_SYMBOL_GPL(wait_on_page_writeback); > > But this is a stealth change to the wait_on_page_writeback() licensing. Why do we have to put that out of line in the first place? Btw. wait_on_page_bit is EXPORT_SYMBOL...
On Sun, 28 Apr 2019 23:05:38 +0200 Michal Hocko <mhocko@suse.com> wrote: > On Fri 26-04-19 11:25:42, Andrew Morton wrote: > > On Fri, 26 Apr 2019 18:26:42 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > [...] > > > +/* > > > + * Wait for a page to complete writeback > > > + */ > > > +void wait_on_page_writeback(struct page *page) > > > +{ > > > + if (PageWriteback(page)) { > > > + trace_wait_on_page_writeback(page, page_mapping(page)); > > > + wait_on_page_bit(page, PG_writeback); > > > + } > > > +} > > > +EXPORT_SYMBOL_GPL(wait_on_page_writeback); > > > > But this is a stealth change to the wait_on_page_writeback() licensing. > > Why do we have to put that out of line in the first place? Seems like a good thing to do from a size and icache-footprint POV. wait_on_page_writeback() has a ton of callsites and the allmodconfig out-of-line version is around 600 bytes of code (gack). > Btw. wait_on_page_bit is EXPORT_SYMBOL... OK, I'll leave wait_on_page_writeback() as EXPROT_SYMBOL().
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index f939e00..0f26c38 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -537,15 +537,7 @@ static inline int wait_on_page_locked_killable(struct page *page) extern void put_and_wait_on_page_locked(struct page *page); -/* - * Wait for a page to complete writeback - */ -static inline void wait_on_page_writeback(struct page *page) -{ - if (PageWriteback(page)) - wait_on_page_bit(page, PG_writeback); -} - +void wait_on_page_writeback(struct page *page); extern void end_page_writeback(struct page *page); void wait_for_stable_page(struct page *page); diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h index 32db72c..aa7f3ae 100644 --- a/include/trace/events/writeback.h +++ b/include/trace/events/writeback.h @@ -53,7 +53,7 @@ struct wb_writeback_work; -TRACE_EVENT(writeback_dirty_page, +DECLARE_EVENT_CLASS(writeback_page_template, TP_PROTO(struct page *page, struct address_space *mapping), @@ -79,6 +79,20 @@ ) ); +DEFINE_EVENT(writeback_page_template, writeback_dirty_page, + + TP_PROTO(struct page *page, struct address_space *mapping), + + TP_ARGS(page, mapping) +); + +DEFINE_EVENT(writeback_page_template, wait_on_page_writeback, + + TP_PROTO(struct page *page, struct address_space *mapping), + + TP_ARGS(page, mapping) +); + DECLARE_EVENT_CLASS(writeback_dirty_inode_template, TP_PROTO(struct inode *inode, int flags), diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 9f61dfe..0765648 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2808,6 +2808,18 @@ int __test_set_page_writeback(struct page *page, bool keep_write) } EXPORT_SYMBOL(__test_set_page_writeback); +/* + * Wait for a page to complete writeback + */ +void wait_on_page_writeback(struct page *page) +{ + if (PageWriteback(page)) { + trace_wait_on_page_writeback(page, page_mapping(page)); + wait_on_page_bit(page, PG_writeback); + } +} +EXPORT_SYMBOL_GPL(wait_on_page_writeback); + /** * wait_for_stable_page() - wait for writeback to finish, if necessary. * @page: The page to wait on.
Recently there're some hungtasks on our server due to wait_on_page_writeback, and we want to know the details of this PG_writeback, i.e. this page is writing back to which device. But it is not so convenient to get the details. I think it would be better to introduce a tracepoint for diagnosing the writeback details. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/pagemap.h | 10 +--------- include/trace/events/writeback.h | 16 +++++++++++++++- mm/page-writeback.c | 12 ++++++++++++ 3 files changed, 28 insertions(+), 10 deletions(-)