| Message ID | 20181121215442.138545-1-yuzhao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mm: use swp_offset as key in shmem_replace_page() | expand |
On Wed, Nov 21, 2018 at 02:54:42PM -0700, Yu Zhao wrote: > We changed key of swap cache tree from swp_entry_t.val to > swp_offset. Need to do so in shmem_replace_page() as well. > > Fixes: f6ab1f7f6b2d ("mm, swap: use offset of swap entry as key of swap cache") > Cc: stable@vger.kernel.org # v4.9+ > Signed-off-by: Yu Zhao <yuzhao@google.com> Reviewed-by: Matthew Wilcox <willy@infradead.org>
On Wed, 21 Nov 2018, Yu Zhao wrote: > We changed key of swap cache tree from swp_entry_t.val to > swp_offset. Need to do so in shmem_replace_page() as well. > > Fixes: f6ab1f7f6b2d ("mm, swap: use offset of swap entry as key of swap cache") > Cc: stable@vger.kernel.org # v4.9+ > Signed-off-by: Yu Zhao <yuzhao@google.com> Acked-by: Hugh Dickins <hughd@google.com> Thanks! > --- > mm/shmem.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index d44991ea5ed4..42b70978e814 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1509,11 +1509,13 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp, > { > struct page *oldpage, *newpage; > struct address_space *swap_mapping; > + swp_entry_t entry; > pgoff_t swap_index; > int error; > > oldpage = *pagep; > - swap_index = page_private(oldpage); > + entry.val = page_private(oldpage); > + swap_index = swp_offset(entry); > swap_mapping = page_mapping(oldpage); > > /* > @@ -1532,7 +1534,7 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp, > __SetPageLocked(newpage); > __SetPageSwapBacked(newpage); > SetPageUptodate(newpage); > - set_page_private(newpage, swap_index); > + set_page_private(newpage, entry.val); > SetPageSwapCache(newpage); > > /* > -- > 2.19.1.1215.g8438c0b245-goog
On Wed, 21 Nov 2018 14:54:42 -0700 Yu Zhao <yuzhao@google.com> wrote: > We changed key of swap cache tree from swp_entry_t.val to > swp_offset. Need to do so in shmem_replace_page() as well. What are the user-visible effects of this change? > Fixes: f6ab1f7f6b2d ("mm, swap: use offset of swap entry as key of swap cache") > Cc: stable@vger.kernel.org # v4.9+ Please always provide the user-impact information when fixing bugs. This becomes especially important when proposing -stable backporting. Hugh said : shmem_replace_page() has been wrong since the day I wrote it: good : enough to work on swap "type" 0, which is all most people ever use : (especially those few who need shmem_replace_page() at all), but broken : once there are any non-0 swp_type bits set in the higher order bits. but we still don't have a description of "broken". Thanks.
On Wed, 21 Nov 2018, Andrew Morton wrote: > On Wed, 21 Nov 2018 14:54:42 -0700 Yu Zhao <yuzhao@google.com> wrote: > > > We changed key of swap cache tree from swp_entry_t.val to > > swp_offset. Need to do so in shmem_replace_page() as well. > > What are the user-visible effects of this change? Sorry, I don't know; and I don't know whether it's something Yu noticed by source inspection, or in trying to use it, perhaps in some new way: I assume the former, but he can add more info if the latter. shmem_replace_page() was added in 3.5, to allow Intel's GMA500 graphics driver to use the shmem support for GEM objects, despite its hardware addressing limitations. (I did once try to extend it to NUMA placement, but that turned out to be a waste, doing more harm than good.) My guess at user-visible effects would be that the screen goes blank or weird on GMA500 after a bout of swapping (because shmem_getpage_gfp() failed to bring back a part of the object). The code in shmem.c looks robust (amusing to see my "Is this possible? I think not" comment!), but I don't know how it plays out at the graphics driver end. > > > Fixes: f6ab1f7f6b2d ("mm, swap: use offset of swap entry as key of swap cache") > > Cc: stable@vger.kernel.org # v4.9+ > > Please always provide the user-impact information when fixing bugs. This > becomes especially important when proposing -stable backporting. So far as I know, nothing but GMA500 gets to use it, but perhaps other uses have been added since. We could ask around, but, frankly I'd prefer just to remove the Fixes and Cc stable tags if they're causing trouble. It's just a good simple bugfix, we thought stable would be glad of it. > > Hugh said > > : shmem_replace_page() has been wrong since the day I wrote it: good > : enough to work on swap "type" 0, which is all most people ever use > : (especially those few who need shmem_replace_page() at all), but broken > : once there are any non-0 swp_type bits set in the higher order bits. Yu then kindly restored my honour on that: I was misreading, it was not broken from the start, but got broken by the commit in 4.9. > > but we still don't have a description of "broken". > > Thanks. Hugh
diff --git a/mm/shmem.c b/mm/shmem.c index d44991ea5ed4..42b70978e814 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1509,11 +1509,13 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp, { struct page *oldpage, *newpage; struct address_space *swap_mapping; + swp_entry_t entry; pgoff_t swap_index; int error; oldpage = *pagep; - swap_index = page_private(oldpage); + entry.val = page_private(oldpage); + swap_index = swp_offset(entry); swap_mapping = page_mapping(oldpage); /* @@ -1532,7 +1534,7 @@ static int shmem_replace_page(struct page **pagep, gfp_t gfp, __SetPageLocked(newpage); __SetPageSwapBacked(newpage); SetPageUptodate(newpage); - set_page_private(newpage, swap_index); + set_page_private(newpage, entry.val); SetPageSwapCache(newpage); /*
We changed key of swap cache tree from swp_entry_t.val to swp_offset. Need to do so in shmem_replace_page() as well. Fixes: f6ab1f7f6b2d ("mm, swap: use offset of swap entry as key of swap cache") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Yu Zhao <yuzhao@google.com> --- mm/shmem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)