Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1622051 > unrolled thread

[PATCH v2 06/17] mm: doc comment for scary spot in write_one_page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-12 14:10 +0200
    Re: [PATCH v2 06/17] mm: doc comment for scary spot in  write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-12 15:10 +0200
      Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page Matthew Wilcox <willy@infradead.org> - 2017-04-12 16:40 +0200
        Re: [PATCH v2 06/17] mm: doc comment for scary spot in  write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-12 18:00 +0200
          Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page NeilBrown <neilb@suse.com> - 2017-04-12 23:40 +0200
            Re: [PATCH v2 06/17] mm: doc comment for scary spot in  write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-13 01:00 +0200

#1622051 — [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page

Not sure what to do here just yet.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 mm/page-writeback.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index de0dbf12e2c1..3ac8399dc984 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
 		ret = mapping->a_ops->writepage(page, &wbc);
 		if (ret == 0) {
 			wait_on_page_writeback(page);
+			/*
+			 * FIXME: is this racy? What guarantees that PG_error
+			 * will still be set once we get around to checking it?
+			 * What if writeback fails, but then a read is issued
+			 * before we check this, and that calls ClearPageError?
+			 */
 			if (PageError(page))
 				ret = -EIO;
 		}
-- 
2.9.3

[toc] | [next] | [standalone]

#1622126 — Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page

On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
> Not sure what to do here just yet.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  mm/page-writeback.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index de0dbf12e2c1..3ac8399dc984 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
>  		ret = mapping->a_ops->writepage(page, &wbc);
>  		if (ret == 0) {
>  			wait_on_page_writeback(page);
> +			/*
> +			 * FIXME: is this racy? What guarantees that PG_error
> +			 * will still be set once we get around to checking it?
> +			 * What if writeback fails, but then a read is issued
> +			 * before we check this, and that calls ClearPageError?
> +			 */
>  			if (PageError(page))
>  				ret = -EIO;
>  		}

Ahh, we are always under the page lock here, and this is generally used
for writing out directory pages anyway. I'm fine with dropping this
patch unless someone else sees a problem here.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1622193

On Wed, Apr 12, 2017 at 09:01:34AM -0400, Jeff Layton wrote:
> On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
> > Not sure what to do here just yet.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  mm/page-writeback.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index de0dbf12e2c1..3ac8399dc984 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
> >  		ret = mapping->a_ops->writepage(page, &wbc);
> >  		if (ret == 0) {
> >  			wait_on_page_writeback(page);
> > +			/*
> > +			 * FIXME: is this racy? What guarantees that PG_error
> > +			 * will still be set once we get around to checking it?
> > +			 * What if writeback fails, but then a read is issued
> > +			 * before we check this, and that calls ClearPageError?
> > +			 */
> >  			if (PageError(page))
> >  				ret = -EIO;
> >  		}
> 
> Ahh, we are always under the page lock here, and this is generally used
> for writing out directory pages anyway. I'm fine with dropping this
> patch unless someone else sees a problem here.

->writepage drops the page lock.  We're still holding a refcount on this
page, but that's not going to prevent read being called.  But maybe the
filesystem won't call read on a page that's marked as PageError?

[toc] | [prev] | [next] | [standalone]

#1622268 — Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page

On Wed, 2017-04-12 at 07:38 -0700, Matthew Wilcox wrote:
> On Wed, Apr 12, 2017 at 09:01:34AM -0400, Jeff Layton wrote:
> > On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
> > > Not sure what to do here just yet.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  mm/page-writeback.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > index de0dbf12e2c1..3ac8399dc984 100644
> > > --- a/mm/page-writeback.c
> > > +++ b/mm/page-writeback.c
> > > @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
> > >  		ret = mapping->a_ops->writepage(page, &wbc);
> > >  		if (ret == 0) {
> > >  			wait_on_page_writeback(page);
> > > +			/*
> > > +			 * FIXME: is this racy? What guarantees that PG_error
> > > +			 * will still be set once we get around to checking it?
> > > +			 * What if writeback fails, but then a read is issued
> > > +			 * before we check this, and that calls ClearPageError?
> > > +			 */
> > >  			if (PageError(page))
> > >  				ret = -EIO;
> > >  		}
> > 
> > Ahh, we are always under the page lock here, and this is generally used
> > for writing out directory pages anyway. I'm fine with dropping this
> > patch unless someone else sees a problem here.
> 
> ->writepage drops the page lock.  We're still holding a refcount on this
> page, but that's not going to prevent read being called.  But maybe the
> filesystem won't call read on a page that's marked as PageError?

Hard to be sure there. I really wonder if that check is needed at all,
the more I look at it. After all, we are calling writepage with
WB_SYNC_ALL so we should get an error there.

Is it also possible these pages could be written back before that point
(due to memory pressure or something) and that fail?

Maybe we should just have a call to filemap_check_errors on exiting
this function?

With the the wb_err_t based stuff, we could change it to sample the
wb_err early, and then use that to see if an error has occurred since
then. Maybe we should even allow callers to pass a wb_err_t in here, so
we can report errors that have occurred since a known point?
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1622571

[Multipart message — attachments visible in raw view] — view raw

On Wed, Apr 12 2017, Jeff Layton wrote:

> On Wed, 2017-04-12 at 07:38 -0700, Matthew Wilcox wrote:
>> On Wed, Apr 12, 2017 at 09:01:34AM -0400, Jeff Layton wrote:
>> > On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
>> > > Not sure what to do here just yet.
>> > > 
>> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> > > ---
>> > >  mm/page-writeback.c | 6 ++++++
>> > >  1 file changed, 6 insertions(+)
>> > > 
>> > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> > > index de0dbf12e2c1..3ac8399dc984 100644
>> > > --- a/mm/page-writeback.c
>> > > +++ b/mm/page-writeback.c
>> > > @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
>> > >  		ret = mapping->a_ops->writepage(page, &wbc);
>> > >  		if (ret == 0) {
>> > >  			wait_on_page_writeback(page);
>> > > +			/*
>> > > +			 * FIXME: is this racy? What guarantees that PG_error
>> > > +			 * will still be set once we get around to checking it?
>> > > +			 * What if writeback fails, but then a read is issued
>> > > +			 * before we check this, and that calls ClearPageError?
>> > > +			 */
>> > >  			if (PageError(page))
>> > >  				ret = -EIO;
>> > >  		}
>> > 
>> > Ahh, we are always under the page lock here, and this is generally used
>> > for writing out directory pages anyway. I'm fine with dropping this
>> > patch unless someone else sees a problem here.
>> 
>> ->writepage drops the page lock.  We're still holding a refcount on this
>> page, but that's not going to prevent read being called.  But maybe the
>> filesystem won't call read on a page that's marked as PageError?
>
> Hard to be sure there. I really wonder if that check is needed at all,
> the more I look at it. After all, we are calling writepage with
> WB_SYNC_ALL so we should get an error there.

WB_SYNC_ALL doesn't cause writepage to wait.  It might case it to ask
for REQ_SYNC, so the write requests gets priority in the block layer.
WB_SYNC_ALL does cause writepages (with an 's') to wait.
(At least, that is how I read the code).

>
> Is it also possible these pages could be written back before that point
> (due to memory pressure or something) and that fail?

Probably, in which case clear_page_dirty_for_io() will fail and
write_one_page() will just unlock the page.

>
> Maybe we should just have a call to filemap_check_errors on exiting
> this function?

I'm leaning in that direction.

>
> With the the wb_err_t based stuff, we could change it to sample the
> wb_err early, and then use that to see if an error has occurred since
> then. Maybe we should even allow callers to pass a wb_err_t in here, so
> we can report errors that have occurred since a known point?

That feels to me like over-engineering.  We would need to
unconditionally call writepage() for that to work.

We seem to be agreed that write errors for buffered writes are reported
per-address-space.  To get per-page errors you have to use direct IO.
Let's focus on that policy and make it work.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1622606 — Re: [PATCH v2 06/17] mm: doc comment for scary spot in write_one_page

On Thu, 2017-04-13 at 07:36 +1000, NeilBrown wrote:
> On Wed, Apr 12 2017, Jeff Layton wrote:
> 
> > On Wed, 2017-04-12 at 07:38 -0700, Matthew Wilcox wrote:
> > > On Wed, Apr 12, 2017 at 09:01:34AM -0400, Jeff Layton wrote:
> > > > On Wed, 2017-04-12 at 08:06 -0400, Jeff Layton wrote:
> > > > > Not sure what to do here just yet.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > ---
> > > > >  mm/page-writeback.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > > > > index de0dbf12e2c1..3ac8399dc984 100644
> > > > > --- a/mm/page-writeback.c
> > > > > +++ b/mm/page-writeback.c
> > > > > @@ -2388,6 +2388,12 @@ int write_one_page(struct page *page)
> > > > >  		ret = mapping->a_ops->writepage(page, &wbc);
> > > > >  		if (ret == 0) {
> > > > >  			wait_on_page_writeback(page);
> > > > > +			/*
> > > > > +			 * FIXME: is this racy? What guarantees that PG_error
> > > > > +			 * will still be set once we get around to checking it?
> > > > > +			 * What if writeback fails, but then a read is issued
> > > > > +			 * before we check this, and that calls ClearPageError?
> > > > > +			 */
> > > > >  			if (PageError(page))
> > > > >  				ret = -EIO;
> > > > >  		}
> > > > 
> > > > Ahh, we are always under the page lock here, and this is generally used
> > > > for writing out directory pages anyway. I'm fine with dropping this
> > > > patch unless someone else sees a problem here.
> > > 
> > > ->writepage drops the page lock.  We're still holding a refcount on this
> > > page, but that's not going to prevent read being called.  But maybe the
> > > filesystem won't call read on a page that's marked as PageError?
> > 
> > Hard to be sure there. I really wonder if that check is needed at all,
> > the more I look at it. After all, we are calling writepage with
> > WB_SYNC_ALL so we should get an error there.
> 
> WB_SYNC_ALL doesn't cause writepage to wait.  It might case it to ask
> for REQ_SYNC, so the write requests gets priority in the block layer.
> WB_SYNC_ALL does cause writepages (with an 's') to wait.
> (At least, that is how I read the code).
> 

Yeah, I realized that after I sent it. If it waited, we wouldn't need
to wait_on_page_writeback there.

> > 
> > Is it also possible these pages could be written back before that point
> > (due to memory pressure or something) and that fail?
> 
> Probably, in which case clear_page_dirty_for_io() will fail and
> write_one_page() will just unlock the page.
> 

Right. So we're already potentially missing errors here for writeback
if it happened to occur before we called clear_page_dirty_for_io.

> > 
> > Maybe we should just have a call to filemap_check_errors on exiting
> > this function?
> 
> I'm leaning in that direction.
> 
> > With the the wb_err_t based stuff, we could change it to sample the
> > wb_err early, and then use that to see if an error has occurred since
> > then. Maybe we should even allow callers to pass a wb_err_t in here, so
> > we can report errors that have occurred since a known point?
> 
> That feels to me like over-engineering.  We would need to
> unconditionally call writepage() for that to work.
> 
> We seem to be agreed that write errors for buffered writes are reported
> per-address-space.  To get per-page errors you have to use direct IO.
> Let's focus on that policy and make it work.
>

That makes sense. I don't think we'd need to always call writepage
there though. We just need to call filemap_check_wb_error even in the
case where writepage wasn't called. IOW, we want to know if there was a
writeback error on the inode, even if it occurred before we tried to
clear the dirty flag.

So, with the new API, we just have to ensure that we sample the
wb_err_t at an earlier time, and then pass that value into
write_one_page so that it can use it to detect the errors since then.

For something like JFS (which apparently uses this function for its fs
journal), the "since" value could be the value at the time that the
journal was last flushed. Then you know whether something failed
regardless of whether it was synced out to disk before this function
was called.

Most of the other callers use this for synchronous directory changes,
so those might want to keep a wb_err_t value in their inode structures,
depending on what they're looking for there. Again, I think this is a
place where we need some input from the fs maintainers.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web