| Message ID | 20240305161313.90954-1-zi.yan@sent.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [STABLE,v6.1.y] mm/migrate: set swap entry values of THP tail pages properly. | expand |
On 5 Mar 2024, at 11:13, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > The tail pages in a THP can have swap entry information stored in their > private field. When migrating to a new page, all tail pages of the new > page need to update ->private to avoid future data corruption. Corresponding swapcache entries need to be updated as well. e71769ae5260 ("mm: enable thp migration for shmem thp") fixed it already. Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > mm/migrate.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index c93dd6a31c31..c5968021fde0 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -423,8 +423,12 @@ int folio_migrate_mapping(struct address_space *mapping, > if (folio_test_swapbacked(folio)) { > __folio_set_swapbacked(newfolio); > if (folio_test_swapcache(folio)) { > + int i; > + > folio_set_swapcache(newfolio); > - newfolio->private = folio_get_private(folio); > + for (i = 0; i < nr; i++) > + set_page_private(folio_page(newfolio, i), > + page_private(folio_page(folio, i))); > } > entries = nr; > } else { > -- > 2.43.0 -- Best Regards, Yan, Zi
On 05.03.24 17:13, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > The tail pages in a THP can have swap entry information stored in their > private field. When migrating to a new page, all tail pages of the new > page need to update ->private to avoid future data corruption. > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > mm/migrate.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index c93dd6a31c31..c5968021fde0 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -423,8 +423,12 @@ int folio_migrate_mapping(struct address_space *mapping, > if (folio_test_swapbacked(folio)) { > __folio_set_swapbacked(newfolio); > if (folio_test_swapcache(folio)) { > + int i; > + > folio_set_swapcache(newfolio); > - newfolio->private = folio_get_private(folio); > + for (i = 0; i < nr; i++) > + set_page_private(folio_page(newfolio, i), > + page_private(folio_page(folio, i))); > } > entries = nr; > } else { Hopefully Charan can run the reproducer against this. Acked-by: David Hildenbrand <david@redhat.com>
Thanks David for various inputs on this patch!!. On 3/5/2024 9:47 PM, Zi Yan wrote: > On 5 Mar 2024, at 11:13, Zi Yan wrote: > >> From: Zi Yan <ziy@nvidia.com> >> >> The tail pages in a THP can have swap entry information stored in their >> private field. When migrating to a new page, all tail pages of the new >> page need to update ->private to avoid future data corruption. > > Corresponding swapcache entries need to be updated as well. > e71769ae5260 ("mm: enable thp migration for shmem thp") fixed it already. > > Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") > Thanks Zi Yan, for posting this patch. I think below tag too applicable? Closes: https://lore.kernel.org/linux-mm/1707814102-22682-1-git-send-email-quic_charante@quicinc.com/ > >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- >> mm/migrate.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index c93dd6a31c31..c5968021fde0 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -423,8 +423,12 @@ int folio_migrate_mapping(struct address_space *mapping, >> if (folio_test_swapbacked(folio)) { >> __folio_set_swapbacked(newfolio); >> if (folio_test_swapcache(folio)) { >> + int i; >> + >> folio_set_swapcache(newfolio); >> - newfolio->private = folio_get_private(folio); >> + for (i = 0; i < nr; i++) >> + set_page_private(folio_page(newfolio, i), >> + page_private(folio_page(folio, i))); >> } >> entries = nr; >> } else { >> -- >> 2.43.0 > > > -- > Best Regards, > Yan, Zi
On 5 Mar 2024, at 11:22, Charan Teja Kalla wrote: > Thanks David for various inputs on this patch!!. > > On 3/5/2024 9:47 PM, Zi Yan wrote: >> On 5 Mar 2024, at 11:13, Zi Yan wrote: >> >>> From: Zi Yan <ziy@nvidia.com> >>> >>> The tail pages in a THP can have swap entry information stored in their >>> private field. When migrating to a new page, all tail pages of the new >>> page need to update ->private to avoid future data corruption. >> >> Corresponding swapcache entries need to be updated as well. >> e71769ae5260 ("mm: enable thp migration for shmem thp") fixed it already. >> >> Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path") >> > > Thanks Zi Yan, for posting this patch. I think below tag too applicable? > > Closes: > https://lore.kernel.org/linux-mm/1707814102-22682-1-git-send-email-quic_charante@quicinc.com/ Right. Let me add it to other stable fixes I am going to send. Hi Greg, Could you add the information above (text, Fixes, and Closes) to this patch? Or do you want me to resend? Thanks. > >> >>> >>> Signed-off-by: Zi Yan <ziy@nvidia.com> >>> --- >>> mm/migrate.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index c93dd6a31c31..c5968021fde0 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -423,8 +423,12 @@ int folio_migrate_mapping(struct address_space *mapping, >>> if (folio_test_swapbacked(folio)) { >>> __folio_set_swapbacked(newfolio); >>> if (folio_test_swapcache(folio)) { >>> + int i; >>> + >>> folio_set_swapcache(newfolio); >>> - newfolio->private = folio_get_private(folio); >>> + for (i = 0; i < nr; i++) >>> + set_page_private(folio_page(newfolio, i), >>> + page_private(folio_page(folio, i))); >>> } >>> entries = nr; >>> } else { >>> -- >>> 2.43.0 >> >> >> -- >> Best Regards, >> Yan, Zi -- Best Regards, Yan, Zi
On Tue, Mar 05, 2024 at 11:13:13AM -0500, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > The tail pages in a THP can have swap entry information stored in their > private field. When migrating to a new page, all tail pages of the new > page need to update ->private to avoid future data corruption. > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > mm/migrate.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) What is the git commit id of this change in Linus's tree? thanks, greg k-h
On 05.03.24 23:04, Greg KH wrote: > On Tue, Mar 05, 2024 at 11:13:13AM -0500, Zi Yan wrote: >> From: Zi Yan <ziy@nvidia.com> >> >> The tail pages in a THP can have swap entry information stored in their >> private field. When migrating to a new page, all tail pages of the new >> page need to update ->private to avoid future data corruption. >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- >> mm/migrate.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) > > What is the git commit id of this change in Linus's tree? Unfortunately, we had to do stable-only versions, because the backport of the "accidental" fix that removes the per-subpage "private" information would be non-trivial, especially for pre-folio-converison times. The accidental fix is 07e09c483cbef2a252f75d95670755a0607288f5
On Tue, Mar 05, 2024 at 11:09:17PM +0100, David Hildenbrand wrote: > On 05.03.24 23:04, Greg KH wrote: > > On Tue, Mar 05, 2024 at 11:13:13AM -0500, Zi Yan wrote: > > > From: Zi Yan <ziy@nvidia.com> > > > > > > The tail pages in a THP can have swap entry information stored in their > > > private field. When migrating to a new page, all tail pages of the new > > > page need to update ->private to avoid future data corruption. > > > > > > Signed-off-by: Zi Yan <ziy@nvidia.com> > > > --- > > > mm/migrate.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > What is the git commit id of this change in Linus's tree? > > Unfortunately, we had to do stable-only versions, because the backport > of the "accidental" fix that removes the per-subpage "private" information > would be non-trivial, especially for pre-folio-converison times. > > The accidental fix is > > 07e09c483cbef2a252f75d95670755a0607288f5 None of that is obvious at all here, we need loads of documentation in the changelog text that says all of that please. thanks, greg k-h
On 5 Mar 2024, at 17:32, Greg KH wrote: > On Tue, Mar 05, 2024 at 11:09:17PM +0100, David Hildenbrand wrote: >> On 05.03.24 23:04, Greg KH wrote: >>> On Tue, Mar 05, 2024 at 11:13:13AM -0500, Zi Yan wrote: >>>> From: Zi Yan <ziy@nvidia.com> >>>> >>>> The tail pages in a THP can have swap entry information stored in their >>>> private field. When migrating to a new page, all tail pages of the new >>>> page need to update ->private to avoid future data corruption. >>>> >>>> Signed-off-by: Zi Yan <ziy@nvidia.com> >>>> --- >>>> mm/migrate.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> What is the git commit id of this change in Linus's tree? >> >> Unfortunately, we had to do stable-only versions, because the backport >> of the "accidental" fix that removes the per-subpage "private" information >> would be non-trivial, especially for pre-folio-converison times. >> >> The accidental fix is >> >> 07e09c483cbef2a252f75d95670755a0607288f5 > > None of that is obvious at all here, we need loads of documentation in > the changelog text that says all of that please. How about? Before 07e09c483cbe ("mm/huge_memory: work on folio->swap instead of page->private when splitting folio"), when a THP is added into swapcache, each of its subpages has its own swapcache entry and need ->private pointing to the right swapcache entry. THP added to swapcache function is added in 38d8b4e6bdc87 ("mm, THP, swap: delay splitting THP during swap out"). When THP migration was added in 616b8371539a6 ("mm: thp: enable thp migration in generic path"), it did not take care of swapcached THP's subpages, neither updated subpage's ->private nor replaced subpage pointer in the swapcache. Later, e71769ae5260 ("mm: enable thp migration for shmem thp") fixed swapcache update part. Now this patch fixes the subpage ->private update part. -- Best Regards, Yan, Zi
On Tue, Mar 05, 2024 at 06:13:39PM -0500, Zi Yan wrote: > On 5 Mar 2024, at 17:32, Greg KH wrote: > > > On Tue, Mar 05, 2024 at 11:09:17PM +0100, David Hildenbrand wrote: > >> On 05.03.24 23:04, Greg KH wrote: > >>> On Tue, Mar 05, 2024 at 11:13:13AM -0500, Zi Yan wrote: > >>>> From: Zi Yan <ziy@nvidia.com> > >>>> > >>>> The tail pages in a THP can have swap entry information stored in their > >>>> private field. When migrating to a new page, all tail pages of the new > >>>> page need to update ->private to avoid future data corruption. > >>>> > >>>> Signed-off-by: Zi Yan <ziy@nvidia.com> > >>>> --- > >>>> mm/migrate.c | 6 +++++- > >>>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> What is the git commit id of this change in Linus's tree? > >> > >> Unfortunately, we had to do stable-only versions, because the backport > >> of the "accidental" fix that removes the per-subpage "private" information > >> would be non-trivial, especially for pre-folio-converison times. > >> > >> The accidental fix is > >> > >> 07e09c483cbef2a252f75d95670755a0607288f5 > > > > None of that is obvious at all here, we need loads of documentation in > > the changelog text that says all of that please. > > How about? > > Before 07e09c483cbe ("mm/huge_memory: work on folio->swap instead of > page->private when splitting folio"), when a THP is added into swapcache, > each of its subpages has its own swapcache entry and need ->private pointing > to the right swapcache entry. THP added to swapcache function is added in > 38d8b4e6bdc87 ("mm, THP, swap: delay splitting THP during swap out"). > When THP migration was added in 616b8371539a6 ("mm: thp: enable thp migration in generic path"), it did not take care of swapcached THP's subpages, > neither updated subpage's ->private nor replaced subpage pointer in > the swapcache. Later, e71769ae5260 ("mm: enable thp migration for shmem thp") > fixed swapcache update part. Now this patch fixes the subpage ->private > update part. That's better than what is there now :) So yes, please resend all of these with the new text and then we can queue them up. thanks, greg k-h
diff --git a/mm/migrate.c b/mm/migrate.c index c93dd6a31c31..c5968021fde0 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -423,8 +423,12 @@ int folio_migrate_mapping(struct address_space *mapping, if (folio_test_swapbacked(folio)) { __folio_set_swapbacked(newfolio); if (folio_test_swapcache(folio)) { + int i; + folio_set_swapcache(newfolio); - newfolio->private = folio_get_private(folio); + for (i = 0; i < nr; i++) + set_page_private(folio_page(newfolio, i), + page_private(folio_page(folio, i))); } entries = nr; } else {