Re: Oops while rebalancing, now unmountable.

Re: Oops while rebalancing, now unmountable.

[Christoph Hellwig]

On Sun, Nov 14, 2010 at 09:42:06PM +0100, Andrea Arcangeli wrote:
> btrfs misses this:
> 
> +       .migratepage    = btree_migratepage,
> 
> It's a bug that can trigger upstream too (not only with THP) if there
> are hugepage allocations (like while incrasing nr_hugepages). Chris
> already fixed it with an experimental patch.

If the lack of an obscure method causes data corruption something
is seriously wrong with THP.  At least from the 10.000 foot view
I can't quite figure what the exact issue is, though.
fallback_migrate_page seems to do the right thing to me for that
case.

Btw, there's also another issue with the page migration code when used
for filesystem pages.  If directly calls into ->writepage instead
of using the flusher threads.  On most filesystems this will
"only" cause nasty I/O patterns, but on ext4 for example it will
be more nasty as ext3 doesn't do conversions from delayed allocations to
real ones.  So unless you're doing a lot of overwrites it will be
hard to make any progress in writeout().

Btw, what codepath does THP call migrate_pages from?  If you don't
use an explicit thread writeout will be a no-op on btrfs and XFS, too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Andrea Arcangeli]

On Sun, Nov 14, 2010 at 05:00:18PM -0500, Christoph Hellwig wrote:
> On Sun, Nov 14, 2010 at 09:42:06PM +0100, Andrea Arcangeli wrote:
> > btrfs misses this:
> > 
> > +       .migratepage    = btree_migratepage,
> > 
> > It's a bug that can trigger upstream too (not only with THP) if there
> > are hugepage allocations (like while incrasing nr_hugepages). Chris
> > already fixed it with an experimental patch.
> 
> If the lack of an obscure method causes data corruption something
> is seriously wrong with THP.  At least from the 10.000 foot view

I just wrote above that it can happen upstream without THP. It's not
THP related at all. THP is the consumer, this is a problem in migrate
that will trigger as well with migrate_pages or all other possible
migration APIs.

If more people would be using hugetlbfs they would have noticed
without THP.

> I can't quite figure what the exact issue is, though.
> fallback_migrate_page seems to do the right thing to me for that
> case.
> 
> Btw, there's also another issue with the page migration code when used
> for filesystem pages.  If directly calls into ->writepage instead
> of using the flusher threads.  On most filesystems this will
> "only" cause nasty I/O patterns, but on ext4 for example it will
> be more nasty as ext3 doesn't do conversions from delayed allocations to
> real ones.  So unless you're doing a lot of overwrites it will be
> hard to make any progress in writeout().

+static int btree_migratepage(struct address_space *mapping,
+                       struct page *newpage, struct page *page)
+{
+       /*
+        * we can't safely write a btree page from here,
+        * we haven't done the locking hook
+        */
+       if (PageDirty(page))
+               return -EAGAIN;

fallback_migrate_page would call writeout() which is apparently not
ok in btrfs for locking issues leading to corruption.

> Btw, what codepath does THP call migrate_pages from?  If you don't
> use an explicit thread writeout will be a no-op on btrfs and XFS, too.

THP never calls migrate_pages, it's memory compaction that calls it
from inside alloc_pages(order=9). It got noticed only with THP because
it makes more frequent hugepage allocations than nr_hugepages in
hugetlbfs (and maybe there are more THP users already).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Christoph Hellwig]

On Sun, Nov 14, 2010 at 11:12:22PM +0100, Andrea Arcangeli wrote:
> I just wrote above that it can happen upstream without THP. It's not
> THP related at all. THP is the consumer, this is a problem in migrate
> that will trigger as well with migrate_pages or all other possible
> migration APIs.
> 
> If more people would be using hugetlbfs they would have noticed
> without THP.

Okay, it seems THP is really just the messenger for bad VM practices
here.

> +static int btree_migratepage(struct address_space *mapping,
> +                       struct page *newpage, struct page *page)
> +{
> +       /*
> +        * we can't safely write a btree page from here,
> +        * we haven't done the locking hook
> +        */
> +       if (PageDirty(page))
> +               return -EAGAIN;
> 
> fallback_migrate_page would call writeout() which is apparently not
> ok in btrfs for locking issues leading to corruption.

Hmm, it seems the issue for that particular problem is indeedin btrfs.
If it needs external locking for writing out data it should not
implement ->writepage to start with.  Chris, can you explain what's
going on with the btree code? It's pretty funny both in the
btree_writepage which goes directly into extent_write_full_page
if PF_MEMALLOC is not set, but otherwise does much more complicated
work, and also in btree_writepages which skips various WB_SYNC_NONE,
including the very weird check for for_kupdate.

What's the story behing all this and the corruption that Andrea found?

> > Btw, what codepath does THP call migrate_pages from?  If you don't
> > use an explicit thread writeout will be a no-op on btrfs and XFS, too.
> 
> THP never calls migrate_pages, it's memory compaction that calls it
> from inside alloc_pages(order=9). It got noticed only with THP because
> it makes more frequent hugepage allocations than nr_hugepages in
> hugetlbfs (and maybe there are more THP users already).

Well, s/THP/compaction/ and the same problem applies.  The modern
filesystem all have stopped from writeback happening either at all
or at least for the delalloc case from direct reclaim.  Calling
into this code from alloc_pages for filesystem backed pages is thus
rather pointless.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Chris Mason]

Excerpts from Christoph Hellwig's message of 2010-11-15 13:23:14 -0500:
> On Sun, Nov 14, 2010 at 11:12:22PM +0100, Andrea Arcangeli wrote:
> > I just wrote above that it can happen upstream without THP. It's not
> > THP related at all. THP is the consumer, this is a problem in migrate
> > that will trigger as well with migrate_pages or all other possible
> > migration APIs.
> > 
> > If more people would be using hugetlbfs they would have noticed
> > without THP.
> 
> Okay, it seems THP is really just the messenger for bad VM practices
> here.
> 
> > +static int btree_migratepage(struct address_space *mapping,
> > +                       struct page *newpage, struct page *page)
> > +{
> > +       /*
> > +        * we can't safely write a btree page from here,
> > +        * we haven't done the locking hook
> > +        */
> > +       if (PageDirty(page))
> > +               return -EAGAIN;
> > 
> > fallback_migrate_page would call writeout() which is apparently not
> > ok in btrfs for locking issues leading to corruption.
> 
> Hmm, it seems the issue for that particular problem is indeedin btrfs.
> If it needs external locking for writing out data it should not
> implement ->writepage to start with.  Chris, can you explain what's
> going on with the btree code? It's pretty funny both in the
> btree_writepage which goes directly into extent_write_full_page
> if PF_MEMALLOC is not set, but otherwise does much more complicated
> work, and also in btree_writepages which skips various WB_SYNC_NONE,
> including the very weird check for for_kupdate.

So, I had THP + a patched btrfs running all weekend and I can safely say
I've fixed this one now. 

> 
> What's the story behing all this and the corruption that Andrea found?

For the metadata blocks, btrfs gets into a problematic lock inversion
where it needs to record that a block has been written so that it will
be properly recowed when someone tries to change it again.

Basically the rule for btree_writepage:

1) lock the extent buffer (different from the page)
2) mark the metadata block as written
3) lock the page
4) call writepage

Btrfs does this correctly everywhere it uses writepage, and everyone
else either uses writepages or is PF_MEMALLOC, except for the page
migration code, which just jumps to step 4.

So, my current fix adds a migrate page hook and adds a warning into the
code to make sure we protest loudly when the block isn't marked as
written.  Since this shakedown worked well, I'm changing the warning to
a BUG().

The check for kupdate in btree_writepages is different.  Once we write
something, we have to do a good amount of work in order to modify it
again.  The btrfs log commits make sure that we write metadata from time
to time, so we don't really need help from the flusher threads unless.

We also don't want to waste time writing metadata from
balance_dirty_pages.  It'll just make more allocations later as we
wander around and recow things, and it is much more likely to be seeky
than the file IO.  So we setup a threshold where we don't bother doing
metadata IO unless there is a good amount pending.

I'm fine with removing the metadata writepage entirely, it didn't use to
have this many rules and it seems like a better idea to have it not
there at all.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Christoph Hellwig]

On Mon, Nov 15, 2010 at 01:46:02PM -0500, Chris Mason wrote:
> For the metadata blocks, btrfs gets into a problematic lock inversion
> where it needs to record that a block has been written so that it will
> be properly recowed when someone tries to change it again.
> 
> Basically the rule for btree_writepage:
> 
> 1) lock the extent buffer (different from the page)
> 2) mark the metadata block as written
> 3) lock the page
> 4) call writepage
> 
> Btrfs does this correctly everywhere it uses writepage, and everyone
> else either uses writepages or is PF_MEMALLOC, except for the page
> migration code, which just jumps to step 4.
>
> So, my current fix adds a migrate page hook and adds a warning into the
> code to make sure we protest loudly when the block isn't marked as
> written.  Since this shakedown worked well, I'm changing the warning to
> a BUG().
> 

This sounds to me like you shouldn't bother to use ->writepage
for the case that adheres to your locking protocol, but just call into
extent_write_full_page directly.  ->writepage is supposed to directly
callable from the VM, and not require filesystems specific calling
conventions.  Just calling extent_write_full_page directly and
making btree_writepage do the PF_MEMALLOC unconditionally should
also fix the page migration corruption.  And at the same time
making btree_writepage future proof.

Btw, magic like the one there currently does need at least a long
describing comment.

> The check for kupdate in btree_writepages is different.  Once we write
> something, we have to do a good amount of work in order to modify it
> again.  The btrfs log commits make sure that we write metadata from time
> to time, so we don't really need help from the flusher threads unless.
>
> We also don't want to waste time writing metadata from
> balance_dirty_pages.  It'll just make more allocations later as we
> wander around and recow things, and it is much more likely to be seeky
> than the file IO.  So we setup a threshold where we don't bother doing
> metadata IO unless there is a good amount pending.
> 
> I'm fine with removing the metadata writepage entirely, it didn't use to
> have this many rules and it seems like a better idea to have it not
> there at all.

for_kupdate only covers a tiny subset of the flusher threads, as it's
only set for the older_than_this still writeback.  It doesn't cover
regular percentage background reclaim not other asynchronous activity
from the flusher threads, like wakeup_flusher_threads or the laptop-mode
I/O completion.

At the very least it should check for_kupdate || for_background to cover
all background writeback, which is what the few other uses of
for_kupdate already do, but I suspect you simply want to not mark
the btree inode as hashed in the inode hash and skip background
writeback completely.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Shane Shrybman]

On Mon, 2010-11-15 at 13:46 -0500, Chris Mason wrote:
> Excerpts from Christoph Hellwig's message of 2010-11-15 13:23:14 -0500:
> > On Sun, Nov 14, 2010 at 11:12:22PM +0100, Andrea Arcangeli wrote:
> > > I just wrote above that it can happen upstream without THP. It's not
> > > THP related at all. THP is the consumer, this is a problem in migrate
> > > that will trigger as well with migrate_pages or all other possible
> > > migration APIs.
> > > 
> > > If more people would be using hugetlbfs they would have noticed
> > > without THP.
> > 
> > Okay, it seems THP is really just the messenger for bad VM practices
> > here.
> > 
> > > +static int btree_migratepage(struct address_space *mapping,
> > > +                       struct page *newpage, struct page *page)
> > > +{
> > > +       /*
> > > +        * we can't safely write a btree page from here,
> > > +        * we haven't done the locking hook
> > > +        */
> > > +       if (PageDirty(page))
> > > +               return -EAGAIN;
> > > 
> > > fallback_migrate_page would call writeout() which is apparently not
> > > ok in btrfs for locking issues leading to corruption.
> > 
> > Hmm, it seems the issue for that particular problem is indeedin btrfs.
> > If it needs external locking for writing out data it should not
> > implement ->writepage to start with.  Chris, can you explain what's
> > going on with the btree code? It's pretty funny both in the
> > btree_writepage which goes directly into extent_write_full_page
> > if PF_MEMALLOC is not set, but otherwise does much more complicated
> > work, and also in btree_writepages which skips various WB_SYNC_NONE,
> > including the very weird check for for_kupdate.
> 
> So, I had THP + a patched btrfs running all weekend and I can safely say
> I've fixed this one now. 
> 

That seems like good news!

Is that btrfs patch available somewhere?

Where does this leave the existing corrupted btrfs'?

Thanks guys,

Shane

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Andrea Arcangeli]

On Mon, Nov 15, 2010 at 01:23:14PM -0500, Christoph Hellwig wrote:
> On Sun, Nov 14, 2010 at 11:12:22PM +0100, Andrea Arcangeli wrote:
> > +static int btree_migratepage(struct address_space *mapping,
> > +                       struct page *newpage, struct page *page)
> > +{
> > +       /*
> > +        * we can't safely write a btree page from here,
> > +        * we haven't done the locking hook
> > +        */
> > +       if (PageDirty(page))
> > +               return -EAGAIN;
> > 
> > fallback_migrate_page would call writeout() which is apparently not
> > ok in btrfs for locking issues leading to corruption.
> 
> Hmm, it seems the issue for that particular problem is indeedin btrfs.

I've been reading the writeout() in mm/migrate.c and I wonder if maybe
that should have been WB_SYNC_ALL or if we miss a
wait_on_page_writeback in after ->writepage() returns? Can you have a
look there? We check the PG_writeback bit when the page is not dirty
(well before fallback_migrate_page is called), but after calling
writeout() we don't return to wait on PG_writeback. We make sure to
hold the page lock after ->writepage returns but that doesn't mean
PG_writeback isn't still set.

> If it needs external locking for writing out data it should not
> implement ->writepage to start with.  Chris, can you explain what's
> going on with the btree code? It's pretty funny both in the
> btree_writepage which goes directly into extent_write_full_page
> if PF_MEMALLOC is not set, but otherwise does much more complicated
> work, and also in btree_writepages which skips various WB_SYNC_NONE,
> including the very weird check for for_kupdate.
> 
> What's the story behing all this and the corruption that Andrea found?
> 
> > > Btw, what codepath does THP call migrate_pages from?  If you don't
> > > use an explicit thread writeout will be a no-op on btrfs and XFS, too.
> > 
> > THP never calls migrate_pages, it's memory compaction that calls it
> > from inside alloc_pages(order=9). It got noticed only with THP because
> > it makes more frequent hugepage allocations than nr_hugepages in
> > hugetlbfs (and maybe there are more THP users already).
> 
> Well, s/THP/compaction/ and the same problem applies.  The modern
> filesystem all have stopped from writeback happening either at all
> or at least for the delalloc case from direct reclaim.  Calling
> into this code from alloc_pages for filesystem backed pages is thus
> rather pointless.

Compaction practically only happens in the context of the task
allocating memory (in my tree it is also used by kswapd). Not
immediate to ask a separate daemon to invoke it. Not sure why this
should screw delalloc. Compaction isn't freeing any memory at all,
it's not reclaim. It just defragments and moves stuff around and it
may have to write dirty pages to do so.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Chris Mason]

Excerpts from Andrea Arcangeli's message of 2010-11-15 13:46:57 -0500:
> On Mon, Nov 15, 2010 at 01:23:14PM -0500, Christoph Hellwig wrote:
> > On Sun, Nov 14, 2010 at 11:12:22PM +0100, Andrea Arcangeli wrote:
> > > +static int btree_migratepage(struct address_space *mapping,
> > > +                       struct page *newpage, struct page *page)
> > > +{
> > > +       /*
> > > +        * we can't safely write a btree page from here,
> > > +        * we haven't done the locking hook
> > > +        */
> > > +       if (PageDirty(page))
> > > +               return -EAGAIN;
> > > 
> > > fallback_migrate_page would call writeout() which is apparently not
> > > ok in btrfs for locking issues leading to corruption.
> > 
> > Hmm, it seems the issue for that particular problem is indeedin btrfs.
> 
> I've been reading the writeout() in mm/migrate.c and I wonder if maybe
> that should have been WB_SYNC_ALL or if we miss a
> wait_on_page_writeback in after ->writepage() returns? Can you have a
> look there? We check the PG_writeback bit when the page is not dirty
> (well before fallback_migrate_page is called), but after calling
> writeout() we don't return to wait on PG_writeback. We make sure to
> hold the page lock after ->writepage returns but that doesn't mean
> PG_writeback isn't still set.

It always returns either -EIO or -EAGAIN, so the caller will try again
and then end up waiting on PageWriteback?

> 
> > If it needs external locking for writing out data it should not
> > implement ->writepage to start with.  Chris, can you explain what's
> > going on with the btree code? It's pretty funny both in the
> > btree_writepage which goes directly into extent_write_full_page
> > if PF_MEMALLOC is not set, but otherwise does much more complicated
> > work, and also in btree_writepages which skips various WB_SYNC_NONE,
> > including the very weird check for for_kupdate.
> > 
> > What's the story behing all this and the corruption that Andrea found?
> > 
> > > > Btw, what codepath does THP call migrate_pages from?  If you don't
> > > > use an explicit thread writeout will be a no-op on btrfs and XFS, too.
> > > 
> > > THP never calls migrate_pages, it's memory compaction that calls it
> > > from inside alloc_pages(order=9). It got noticed only with THP because
> > > it makes more frequent hugepage allocations than nr_hugepages in
> > > hugetlbfs (and maybe there are more THP users already).
> > 
> > Well, s/THP/compaction/ and the same problem applies.  The modern
> > filesystem all have stopped from writeback happening either at all
> > or at least for the delalloc case from direct reclaim.  Calling
> > into this code from alloc_pages for filesystem backed pages is thus
> > rather pointless.
> 
> Compaction practically only happens in the context of the task
> allocating memory (in my tree it is also used by kswapd). Not
> immediate to ask a separate daemon to invoke it. Not sure why this
> should screw delalloc. Compaction isn't freeing any memory at all,
> it's not reclaim. It just defragments and moves stuff around and it
> may have to write dirty pages to do so.

The short version is that when the VM jumps in and starts doing single
page IO, we badly fragment files.  The FS wants writepages for
everything, so that we can do smart delayed allocation and so that we
can write out things in units bigger than 4KB.

We most recently hashed this out in threads with Mel Gorman around
balance_dirty_pages, but it has a very big impact on performance.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Andrea Arcangeli]

On Mon, Nov 15, 2010 at 02:03:55PM -0500, Chris Mason wrote:
> It always returns either -EIO or -EAGAIN, so the caller will try again
> and then end up waiting on PageWriteback?

Returning any error from ->writepage will make writeout return -EIO so
aborting the migration for that page. If no error is returned from
->writepage, writeout will return -EAGAIN the caller will try again
after wait_on_page_writeback. I think I misread the code when in prev
mail I worried about not waiting on PG_writeback after writeout()... :)

So the ideal would be not to return errors when ->writepage submitted
the writeback I/O successfully but if it returns -EIO/-EAGAIN there's
no risk whatsoever (except compaction will be less effective).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Christoph Hellwig]

On Mon, Nov 15, 2010 at 07:46:57PM +0100, Andrea Arcangeli wrote:
> I've been reading the writeout() in mm/migrate.c and I wonder if maybe
> that should have been WB_SYNC_ALL or if we miss a
> wait_on_page_writeback in after ->writepage() returns? Can you have a
> look there? We check the PG_writeback bit when the page is not dirty
> (well before fallback_migrate_page is called), but after calling
> writeout() we don't return to wait on PG_writeback. We make sure to
> hold the page lock after ->writepage returns but that doesn't mean
> PG_writeback isn't still set.

I didn't even notice that, but the WB_SYNC_NONE does indeed seem
buggy to me.  If we set the sync_mode to WB_SYNC_NONE filesystem
can and frequently do trylock operations and might just skip to
write it out completely.

So we defintively do need to change writeout to do a WB_SYNC_ALL
writeback.  In addition to that we'll also need the
wait_on_page_writeback call to make sure we actually wait for I/O
to finish.

Also what protects us from updating the page while we write it out?
PG_writeback on many filesystems doesn't protect writes from modifying
the in-flight buffer, and just locking the page after ->writepage
is racy without a check that nothing changed.

> Compaction practically only happens in the context of the task
> allocating memory (in my tree it is also used by kswapd). Not
> immediate to ask a separate daemon to invoke it. Not sure why this
> should screw delalloc. Compaction isn't freeing any memory at all,
> it's not reclaim. It just defragments and moves stuff around and it
> may have to write dirty pages to do so.

kswapd is fine.  Other task allocation memory are direct reclaimers.
Direct reclaim through the filesystem delalloc conversion and the I/O
stack guarantees you stack overflows, that's why filesystems refuse
to do anything in ->writepage for this case.  btrfs and XFS have
explicit checks for PF_MEMALLOC (with a carve out for kswapd in XFS),
and ext4 only writes already allocated blocks in ->writepage but never
does delalloc conversions.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Chris Mason]

Excerpts from Christoph Hellwig's message of 2010-11-15 14:12:04 -0500:
> On Mon, Nov 15, 2010 at 07:46:57PM +0100, Andrea Arcangeli wrote:
> > I've been reading the writeout() in mm/migrate.c and I wonder if maybe
> > that should have been WB_SYNC_ALL or if we miss a
> > wait_on_page_writeback in after ->writepage() returns? Can you have a
> > look there? We check the PG_writeback bit when the page is not dirty
> > (well before fallback_migrate_page is called), but after calling
> > writeout() we don't return to wait on PG_writeback. We make sure to
> > hold the page lock after ->writepage returns but that doesn't mean
> > PG_writeback isn't still set.
> 
> I didn't even notice that, but the WB_SYNC_NONE does indeed seem
> buggy to me.  If we set the sync_mode to WB_SYNC_NONE filesystem
> can and frequently do trylock operations and might just skip to
> write it out completely.
> 
> So we defintively do need to change writeout to do a WB_SYNC_ALL
> writeback.  In addition to that we'll also need the
> wait_on_page_writeback call to make sure we actually wait for I/O
> to finish.
> 
> Also what protects us from updating the page while we write it out?
> PG_writeback on many filesystems doesn't protect writes from modifying
> the in-flight buffer, and just locking the page after ->writepage
> is racy without a check that nothing changed.
> 

Oh, I should have thought of that.  Btrfs (and most of the time xfs?)
will wait on PageWriteback internally, but for the ext crowd we're in
trouble.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Andrea Arcangeli]

On Mon, Nov 15, 2010 at 02:12:04PM -0500, Christoph Hellwig wrote:
> I didn't even notice that, but the WB_SYNC_NONE does indeed seem
> buggy to me.  If we set the sync_mode to WB_SYNC_NONE filesystem
> can and frequently do trylock operations and might just skip to
> write it out completely.

Scary stuff, so WB_SYNC_NONE wouldn't submit the dirty part of the
page down for I/O, so that it's all clean after wait_on_page_writeback
returns? (well of course unless the dirty bit was set again)

> So we defintively do need to change writeout to do a WB_SYNC_ALL
> writeback.  In addition to that we'll also need the
> wait_on_page_writeback call to make sure we actually wait for I/O
> to finish.

Ok that is ok... I misread it sorry. But the writeback must be started
by WB_SYNC_NONE (or _ALL) for wait_on_page_writeback to be effective.

migration will abort if ->writepage returns error, that's safe
though. It will retry calling on wait_on_page_writeback only if
->writepage returns 0.

> Also what protects us from updating the page while we write it out?
> PG_writeback on many filesystems doesn't protect writes from modifying
> the in-flight buffer, and just locking the page after ->writepage
> is racy without a check that nothing changed.

migrate established migration ptes already so nobody can write to the
page through pagetables. The only thing left is O_DIRECT which is also
taken care by the page count check in migrate_page_move_mapping,
before migrate_page called by fallback_migrate_page can succeed. So
nothing can be modifying the page if we go ahead with migrate_page
(and no pte dirty bit can happen either). The page is also locked down
for the whole migration so all writes syscalls should be stopped.

> kswapd is fine.  Other task allocation memory are direct reclaimers.
> Direct reclaim through the filesystem delalloc conversion and the I/O
> stack guarantees you stack overflows, that's why filesystems refuse
> to do anything in ->writepage for this case.  btrfs and XFS have
> explicit checks for PF_MEMALLOC (with a carve out for kswapd in XFS),
> and ext4 only writes already allocated blocks in ->writepage but never
> does delalloc conversions.

I didn't realize the stack overflow issue was specific to delalloc. I
think it's ok here to skip ->writepage for delalloc, it's not
mandatory, memory compaction isn't supposed to do much I/O anyway,
it's supposed to copy ram instead. Sure it'd be more reliable to
submit I/O but it's going to work pretty well, plus compaction will be
retried again later by khugepaged once every 10 sec. kswapd actually
with THP will not do anything because THP allocations are run with
__GFP_NO_KSWAPD to avoid kswapd to waste cpu by trying in the
background hard to create hugepages if 90% of ram goes in anonymous
memory (and there are background anon allocations that would wakeup
kswapd) but only 80% can be allocated as 2M contiguous beacuse 20% was
at some point allocated in slab caches.

In short with THP it's khugepaged that is supposed to run the
->writepage in migrate.c and it will run it once every 10 sec even
when it fails (and not in a 100% cpu wasting loop like kswapd), so if
you did something magic for kswapd in XFS you should do for khugepaged
too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Oops while rebalancing, now unmountable.

[Christoph Hellwig]

On Mon, Nov 15, 2010 at 08:29:14PM +0100, Andrea Arcangeli wrote:
> Scary stuff, so WB_SYNC_NONE wouldn't submit the dirty part of the
> page down for I/O, so that it's all clean after wait_on_page_writeback
> returns? (well of course unless the dirty bit was set again)

It might not if we have lock contention or other resource starvation.
That's the reason why WB_SYNC_NONE was added - to not block the flusher
threads.

> I didn't realize the stack overflow issue was specific to delalloc.

It's not.  It's specific to direct reclaim.  Only ext4 special cases
delalloc, but I'm not sure if that's intentional or just an accidental
side effect of the mess that the ext4 writeback code is.

> In short with THP it's khugepaged that is supposed to run the
> ->writepage in migrate.c and it will run it once every 10 sec even
> when it fails (and not in a 100% cpu wasting loop like kswapd), so if
> you did something magic for kswapd in XFS you should do for khugepaged
> too.

If you have a PF_ flag for it that's easy to add once it goes into
mainline.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>