[PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[Yafang Shao]

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(-)

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.
-- 
1.8.3.1

Re: [PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[Andrew Morton]

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.
_

Re: [PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[Yafang Shao]

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

Re: [PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[Michal Hocko]

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...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page-writeback: introduce tracepoint for wait_on_page_writeback

[Andrew Morton]

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().