| Message ID | 20200709024808.18466-1-robbieko@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm : fix pte _PAGE_DIRTY bit when fallback migrate page | expand |
On 7/9/20 4:48 AM, robbieko wrote: > From: Robbie Ko <robbieko@synology.com> > > When a migrate page occurs, we first create a migration entry > to replace the original pte, and then go to fallback_migrate_page > to execute a writeout if the migratepage is not supported. > > In the writeout, we will clear the dirty bit of the page and use > page_mkclean to clear the dirty bit along with the corresponding pte, > but page_mkclean does not support migration entry. > > The page ditry bit is cleared, but the dirty bit of the pte still exists, > so if mmap continues to write, it will result in data loss. Curious, did you observe this data loss? What filesystem? If yes, it seems serious enough to CC stable and determine a Fixes: tag? > We fix the by first remove the migration entry and then clearing > the dirty bits of the page, which also clears the pte's dirty bits. > > Signed-off-by: Robbie Ko <robbieko@synology.com> > --- > mm/migrate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index f37729673558..5c407434b9ba 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -875,10 +875,6 @@ static int writeout(struct address_space *mapping, struct page *page) > /* No write method for the address space */ > return -EINVAL; > > - if (!clear_page_dirty_for_io(page)) > - /* Someone else already triggered a write */ > - return -EAGAIN; > - > /* > * A dirty page may imply that the underlying filesystem has > * the page on some queue. So the page must be clean for > @@ -889,6 +885,10 @@ static int writeout(struct address_space *mapping, struct page *page) > */ > remove_migration_ptes(page, page, false); > > + if (!clear_page_dirty_for_io(page)) > + /* Someone else already triggered a write */ > + return -EAGAIN; > + > rc = mapping->a_ops->writepage(page, &wbc); > > if (rc != AOP_WRITEPAGE_ACTIVATE) >
Vlastimil Babka 於 2020/7/10 下午11:31 寫道: > On 7/9/20 4:48 AM, robbieko wrote: >> From: Robbie Ko <robbieko@synology.com> >> >> When a migrate page occurs, we first create a migration entry >> to replace the original pte, and then go to fallback_migrate_page >> to execute a writeout if the migratepage is not supported. >> >> In the writeout, we will clear the dirty bit of the page and use >> page_mkclean to clear the dirty bit along with the corresponding pte, >> but page_mkclean does not support migration entry. >> >> The page ditry bit is cleared, but the dirty bit of the pte still exists, >> so if mmap continues to write, it will result in data loss. > Curious, did you observe this data loss? What filesystem? If yes, it seems > serious enough to > CC stable and determine a Fixes: tag? Yes, there is data loss. I'm using a btrfs environment, but not the following patch btrfs: implement migratepage callback for data pages https://git.kernel.org/pub/scm/linux/kernel /git/torvalds/linux.git/commit/?h=v5.8-rc5& id=f8e6608180a31cc72a23b74969da428da236dbd1 >> We fix the by first remove the migration entry and then clearing >> the dirty bits of the page, which also clears the pte's dirty bits. >> >> Signed-off-by: Robbie Ko <robbieko@synology.com> >> --- >> mm/migrate.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index f37729673558..5c407434b9ba 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -875,10 +875,6 @@ static int writeout(struct address_space *mapping, struct page *page) >> /* No write method for the address space */ >> return -EINVAL; >> >> - if (!clear_page_dirty_for_io(page)) >> - /* Someone else already triggered a write */ >> - return -EAGAIN; >> - >> /* >> * A dirty page may imply that the underlying filesystem has >> * the page on some queue. So the page must be clean for >> @@ -889,6 +885,10 @@ static int writeout(struct address_space *mapping, struct page *page) >> */ >> remove_migration_ptes(page, page, false); >> >> + if (!clear_page_dirty_for_io(page)) >> + /* Someone else already triggered a write */ >> + return -EAGAIN; >> + >> rc = mapping->a_ops->writepage(page, &wbc); >> >> if (rc != AOP_WRITEPAGE_ACTIVATE) >> >
On 7/13/20 3:57 AM, Robbie Ko wrote: > > Vlastimil Babka 於 2020/7/10 下午11:31 寫道: >> On 7/9/20 4:48 AM, robbieko wrote: >>> From: Robbie Ko <robbieko@synology.com> >>> >>> When a migrate page occurs, we first create a migration entry >>> to replace the original pte, and then go to fallback_migrate_page >>> to execute a writeout if the migratepage is not supported. >>> >>> In the writeout, we will clear the dirty bit of the page and use >>> page_mkclean to clear the dirty bit along with the corresponding pte, >>> but page_mkclean does not support migration entry. >>> >>> The page ditry bit is cleared, but the dirty bit of the pte still exists, >>> so if mmap continues to write, it will result in data loss. >> Curious, did you observe this data loss? What filesystem? If yes, it seems >> serious enough to >> CC stable and determine a Fixes: tag? > > Yes, there is data loss. > I'm using a btrfs environment, but not the following patch And the kernel is otherwise upstream? Which version? Anyway we better let btrfs guys know (+CC) even if the fix is in MM code. > btrfs: implement migratepage callback for data pages > https://git.kernel.org/pub/scm/linux/kernel > /git/torvalds/linux.git/commit/?h=v5.8-rc5& > id=f8e6608180a31cc72a23b74969da428da236dbd1 That's a new commit, so if this is really affecting upstream btrfs pre-5.8 we should either backport that commit, or your fix (after review). >>> We fix the by first remove the migration entry and then clearing >>> the dirty bits of the page, which also clears the pte's dirty bits. >>> >>> Signed-off-by: Robbie Ko <robbieko@synology.com> >>> --- >>> mm/migrate.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index f37729673558..5c407434b9ba 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -875,10 +875,6 @@ static int writeout(struct address_space *mapping, struct page *page) >>> /* No write method for the address space */ >>> return -EINVAL; >>> >>> - if (!clear_page_dirty_for_io(page)) >>> - /* Someone else already triggered a write */ >>> - return -EAGAIN; >>> - >>> /* >>> * A dirty page may imply that the underlying filesystem has >>> * the page on some queue. So the page must be clean for >>> @@ -889,6 +885,10 @@ static int writeout(struct address_space *mapping, struct page *page) >>> */ >>> remove_migration_ptes(page, page, false); >>> >>> + if (!clear_page_dirty_for_io(page)) >>> + /* Someone else already triggered a write */ >>> + return -EAGAIN; >>> + >>> rc = mapping->a_ops->writepage(page, &wbc); >>> >>> if (rc != AOP_WRITEPAGE_ACTIVATE) >>> >> >
On Tue, Jul 14, 2020 at 11:46:12AM +0200, Vlastimil Babka wrote: > On 7/13/20 3:57 AM, Robbie Ko wrote: > > > > Vlastimil Babka 於 2020/7/10 下午11:31 寫道: > >> On 7/9/20 4:48 AM, robbieko wrote: > >>> From: Robbie Ko <robbieko@synology.com> > >>> > >>> When a migrate page occurs, we first create a migration entry > >>> to replace the original pte, and then go to fallback_migrate_page > >>> to execute a writeout if the migratepage is not supported. > >>> > >>> In the writeout, we will clear the dirty bit of the page and use > >>> page_mkclean to clear the dirty bit along with the corresponding pte, > >>> but page_mkclean does not support migration entry. I don't follow the scenario. When we establish migration entries with try_to_unmap(), it transfers dirty bit from PTE to the page.
Vlastimil Babka 於 2020/7/14 下午5:46 寫道: > On 7/13/20 3:57 AM, Robbie Ko wrote: >> Vlastimil Babka 於 2020/7/10 下午11:31 寫道: >>> On 7/9/20 4:48 AM, robbieko wrote: >>>> From: Robbie Ko <robbieko@synology.com> >>>> >>>> When a migrate page occurs, we first create a migration entry >>>> to replace the original pte, and then go to fallback_migrate_page >>>> to execute a writeout if the migratepage is not supported. >>>> >>>> In the writeout, we will clear the dirty bit of the page and use >>>> page_mkclean to clear the dirty bit along with the corresponding pte, >>>> but page_mkclean does not support migration entry. >>>> >>>> The page ditry bit is cleared, but the dirty bit of the pte still exists, >>>> so if mmap continues to write, it will result in data loss. >>> Curious, did you observe this data loss? What filesystem? If yes, it seems >>> serious enough to >>> CC stable and determine a Fixes: tag? >> Yes, there is data loss. >> I'm using a btrfs environment, but not the following patch > And the kernel is otherwise upstream? Which version? > Anyway we better let btrfs guys know (+CC) even if the fix is in MM code. Kernel verion is 4.4. I think this is a bug that has been around for a long time. I think the problem is not limited to btrfs, as long as other fs have not implemented the migrationpage, they will encounter the problem. (Eg ecryptfs, fat, nfs...) >> btrfs: implement migratepage callback for data pages >> https://git.kernel.org/pub/scm/linux/kernel >> /git/torvalds/linux.git/commit/?h=v5.8-rc5& >> id=f8e6608180a31cc72a23b74969da428da236dbd1 > That's a new commit, so if this is really affecting upstream btrfs pre-5.8 we > should either backport that commit, or your fix (after review). > >>>> We fix the by first remove the migration entry and then clearing >>>> the dirty bits of the page, which also clears the pte's dirty bits. >>>> >>>> Signed-off-by: Robbie Ko <robbieko@synology.com> >>>> --- >>>> mm/migrate.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/mm/migrate.c b/mm/migrate.c >>>> index f37729673558..5c407434b9ba 100644 >>>> --- a/mm/migrate.c >>>> +++ b/mm/migrate.c >>>> @@ -875,10 +875,6 @@ static int writeout(struct address_space *mapping, struct page *page) >>>> /* No write method for the address space */ >>>> return -EINVAL; >>>> >>>> - if (!clear_page_dirty_for_io(page)) >>>> - /* Someone else already triggered a write */ >>>> - return -EAGAIN; >>>> - >>>> /* >>>> * A dirty page may imply that the underlying filesystem has >>>> * the page on some queue. So the page must be clean for >>>> @@ -889,6 +885,10 @@ static int writeout(struct address_space *mapping, struct page *page) >>>> */ >>>> remove_migration_ptes(page, page, false); >>>> >>>> + if (!clear_page_dirty_for_io(page)) >>>> + /* Someone else already triggered a write */ >>>> + return -EAGAIN; >>>> + >>>> rc = mapping->a_ops->writepage(page, &wbc); >>>> >>>> if (rc != AOP_WRITEPAGE_ACTIVATE) >>>> >
Kirill A. Shutemov 於 2020/7/14 下午6:19 寫道: > On Tue, Jul 14, 2020 at 11:46:12AM +0200, Vlastimil Babka wrote: >> On 7/13/20 3:57 AM, Robbie Ko wrote: >>> Vlastimil Babka 於 2020/7/10 下午11:31 寫道: >>>> On 7/9/20 4:48 AM, robbieko wrote: >>>>> From: Robbie Ko <robbieko@synology.com> >>>>> >>>>> When a migrate page occurs, we first create a migration entry >>>>> to replace the original pte, and then go to fallback_migrate_page >>>>> to execute a writeout if the migratepage is not supported. >>>>> >>>>> In the writeout, we will clear the dirty bit of the page and use >>>>> page_mkclean to clear the dirty bit along with the corresponding pte, >>>>> but page_mkclean does not support migration entry. > I don't follow the scenario. > > When we establish migration entries with try_to_unmap(), it transfers > dirty bit from PTE to the page. Sorry, I mean is _PAGE_RW with pte_write When we establish migration entries with try_to_unmap(), we create a migration entry, and if pte_write we set it to SWP_MIGRATION_WRITE, which will replace the migration entry with the original pte. When migratepage, we go to fallback_migrate_page to execute a writeout if the migratepage is not supported. In the writeout, we call clear_page_dirty_for_io to clear the dirty bit of the page and use page_mkclean to clear pte _PAGE_RW with pte_wrprotect in page_mkclean_one. However, page_mkclean_one does not support migration entries, so the migration entry is still SWP_MIGRATION_WRITE. In writeout, then we call remove_migration_ptes to remove the migration entry, because it is still SWP_MIGRATION_WRITE so set _PAGE_RW to pte via pte_mkwrite. Therefore, subsequent mmap wirte will not trigger page_mkwrite to cause data loss.
On Wed, Jul 15, 2020 at 10:45:39AM +0800, Robbie Ko wrote: > > Kirill A. Shutemov 於 2020/7/14 下午6:19 寫道: > > On Tue, Jul 14, 2020 at 11:46:12AM +0200, Vlastimil Babka wrote: > > > On 7/13/20 3:57 AM, Robbie Ko wrote: > > > > Vlastimil Babka 於 2020/7/10 下午11:31 寫道: > > > > > On 7/9/20 4:48 AM, robbieko wrote: > > > > > > From: Robbie Ko <robbieko@synology.com> > > > > > > > > > > > > When a migrate page occurs, we first create a migration entry > > > > > > to replace the original pte, and then go to fallback_migrate_page > > > > > > to execute a writeout if the migratepage is not supported. > > > > > > > > > > > > In the writeout, we will clear the dirty bit of the page and use > > > > > > page_mkclean to clear the dirty bit along with the corresponding pte, > > > > > > but page_mkclean does not support migration entry. > > I don't follow the scenario. > > > > When we establish migration entries with try_to_unmap(), it transfers > > dirty bit from PTE to the page. > > Sorry, I mean is _PAGE_RW with pte_write > > When we establish migration entries with try_to_unmap(), > we create a migration entry, and if pte_write we set it to SWP_MIGRATION_WRITE, > which will replace the migration entry with the original pte. > > When migratepage, we go to fallback_migrate_page to execute a writeout > if the migratepage is not supported. > > In the writeout, we call clear_page_dirty_for_io to clear the dirty bit of the page > and use page_mkclean to clear pte _PAGE_RW with pte_wrprotect in page_mkclean_one. > > However, page_mkclean_one does not support migration entries, so the > migration entry is still SWP_MIGRATION_WRITE. > > In writeout, then we call remove_migration_ptes to remove the migration entry, > because it is still SWP_MIGRATION_WRITE so set _PAGE_RW to pte via pte_mkwrite. > > Therefore, subsequent mmap wirte will not trigger page_mkwrite to cause data loss. Hm, okay. Folks, is there any good reason why try_to_unmap(TTU_MIGRATION) should not clear PTE (make the PTE none) for file page?
Kirill A. Shutemov 於 2020/7/15 下午4:11 寫道: > On Wed, Jul 15, 2020 at 10:45:39AM +0800, Robbie Ko wrote: >> Kirill A. Shutemov 於 2020/7/14 下午6:19 寫道: >>> On Tue, Jul 14, 2020 at 11:46:12AM +0200, Vlastimil Babka wrote: >>>> On 7/13/20 3:57 AM, Robbie Ko wrote: >>>>> Vlastimil Babka 於 2020/7/10 下午11:31 寫道: >>>>>> On 7/9/20 4:48 AM, robbieko wrote: >>>>>>> From: Robbie Ko <robbieko@synology.com> >>>>>>> >>>>>>> When a migrate page occurs, we first create a migration entry >>>>>>> to replace the original pte, and then go to fallback_migrate_page >>>>>>> to execute a writeout if the migratepage is not supported. >>>>>>> >>>>>>> In the writeout, we will clear the dirty bit of the page and use >>>>>>> page_mkclean to clear the dirty bit along with the corresponding pte, >>>>>>> but page_mkclean does not support migration entry. >>> I don't follow the scenario. >>> >>> When we establish migration entries with try_to_unmap(), it transfers >>> dirty bit from PTE to the page. >> Sorry, I mean is _PAGE_RW with pte_write >> >> When we establish migration entries with try_to_unmap(), >> we create a migration entry, and if pte_write we set it to SWP_MIGRATION_WRITE, >> which will replace the migration entry with the original pte. >> >> When migratepage, we go to fallback_migrate_page to execute a writeout >> if the migratepage is not supported. >> >> In the writeout, we call clear_page_dirty_for_io to clear the dirty bit of the page >> and use page_mkclean to clear pte _PAGE_RW with pte_wrprotect in page_mkclean_one. >> >> However, page_mkclean_one does not support migration entries, so the >> migration entry is still SWP_MIGRATION_WRITE. >> >> In writeout, then we call remove_migration_ptes to remove the migration entry, >> because it is still SWP_MIGRATION_WRITE so set _PAGE_RW to pte via pte_mkwrite. >> >> Therefore, subsequent mmap wirte will not trigger page_mkwrite to cause data loss. > Hm, okay. > > Folks, is there any good reason why try_to_unmap(TTU_MIGRATION) should not > clear PTE (make the PTE none) for file page? > This, I'm not sure. But I think that for the fs that support migratepage, when migratepage is finished, the page should still be dirty, and the pte should still have _PAGE_RW, when the next mmap write occurs, we don't need to trigger the page_mkwrite again.
On 16 Jul 2020, at 6:15, Robbie Ko wrote: > Kirill A. Shutemov 於 2020/7/15 下午4:11 寫道: >> On Wed, Jul 15, 2020 at 10:45:39AM +0800, Robbie Ko wrote: >>> Kirill A. Shutemov 於 2020/7/14 下午6:19 寫道: >>>> On Tue, Jul 14, 2020 at 11:46:12AM +0200, Vlastimil Babka wrote: >>>>> On 7/13/20 3:57 AM, Robbie Ko wrote: >>>>>> Vlastimil Babka 於 2020/7/10 下午11:31 寫道: >>>>>>> On 7/9/20 4:48 AM, robbieko wrote: >>>>>>>> From: Robbie Ko <robbieko@synology.com> >>>>>>>> >>>>>>>> When a migrate page occurs, we first create a migration entry >>>>>>>> to replace the original pte, and then go to >>>>>>>> fallback_migrate_page >>>>>>>> to execute a writeout if the migratepage is not supported. >>>>>>>> >>>>>>>> In the writeout, we will clear the dirty bit of the page and >>>>>>>> use >>>>>>>> page_mkclean to clear the dirty bit along with the >>>>>>>> corresponding pte, >>>>>>>> but page_mkclean does not support migration entry. >>>> I don't follow the scenario. >>>> >>>> When we establish migration entries with try_to_unmap(), it >>>> transfers >>>> dirty bit from PTE to the page. >>> Sorry, I mean is _PAGE_RW with pte_write >>> >>> When we establish migration entries with try_to_unmap(), >>> we create a migration entry, and if pte_write we set it to >>> SWP_MIGRATION_WRITE, >>> which will replace the migration entry with the original pte. >>> >>> When migratepage, we go to fallback_migrate_page to execute a >>> writeout >>> if the migratepage is not supported. >>> >>> In the writeout, we call clear_page_dirty_for_io to clear the dirty >>> bit of the page >>> and use page_mkclean to clear pte _PAGE_RW with pte_wrprotect in >>> page_mkclean_one. >>> >>> However, page_mkclean_one does not support migration entries, so the >>> migration entry is still SWP_MIGRATION_WRITE. >>> >>> In writeout, then we call remove_migration_ptes to remove the >>> migration entry, >>> because it is still SWP_MIGRATION_WRITE so set _PAGE_RW to pte via >>> pte_mkwrite. >>> >>> Therefore, subsequent mmap wirte will not trigger page_mkwrite to >>> cause data loss. >> Hm, okay. >> >> Folks, is there any good reason why try_to_unmap(TTU_MIGRATION) >> should not >> clear PTE (make the PTE none) for file page? >> > This, I'm not sure. > But I think that for the fs that support migratepage, when migratepage > is finished, > the page should still be dirty, and the pte should still have > _PAGE_RW, > when the next mmap write occurs, we don't need to trigger the > page_mkwrite again. I don’t know the page migration code well, but you’ll need this one as well on the 4.4 kernel you mentioned: commit 25f3c5021985e885292980d04a1423fd83c967bb Author: Chris Mason <clm@fb.com> Date: Tue Jan 21 11:51:42 2020 -0500 Btrfs: keep pages dirty when using btrfs_writepage_fixup_worker And this one as well: commit 7703bdd8d23e6ef057af3253958a793ec6066b28 Author: Chris Mason <clm@fb.com> Date: Wed Jun 20 07:56:11 2018 -0700 Btrfs: don't clean dirty pages during buffered writes With those two in place, we haven’t found lost data from the migration code, but we did see the fallback migration helper dirtying pages without going through page_mkwrite, which triggers the suboptimal btrfs fixup worker code path. This isn’t a yea or nay on the patch, just additional info. -chris
diff --git a/mm/migrate.c b/mm/migrate.c index f37729673558..5c407434b9ba 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -875,10 +875,6 @@ static int writeout(struct address_space *mapping, struct page *page) /* No write method for the address space */ return -EINVAL; - if (!clear_page_dirty_for_io(page)) - /* Someone else already triggered a write */ - return -EAGAIN; - /* * A dirty page may imply that the underlying filesystem has * the page on some queue. So the page must be clean for @@ -889,6 +885,10 @@ static int writeout(struct address_space *mapping, struct page *page) */ remove_migration_ptes(page, page, false); + if (!clear_page_dirty_for_io(page)) + /* Someone else already triggered a write */ + return -EAGAIN; + rc = mapping->a_ops->writepage(page, &wbc); if (rc != AOP_WRITEPAGE_ACTIVATE)