Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Wed, Feb 07, 2024 at 05:56:21PM -0800, Andrew Morton wrote: > On Thu, 11 Jan 2024 18:22:50 -0800 "Darrick J. Wong" > wrote: > > > On Thu, Jan 11, 2024 at 10:45:53PM +, Matthew Wilcox wrote: > > > On Thu, Jan 11, 2024 at 02:00:53PM -0800, Andrew Morton wrote: > > > > On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" > > > > wrote: > > > > > > > > > > > Fixing this will require a bit of an API change, and prefeably > > > > > > > sorting out > > > > > > > the hwpoison story for pages vs folio and where it is placed in > > > > > > > the shmem > > > > > > > API. For now use this one liner to disable large folios. > > > > > > > > > > > > > > Reported-by: Darrick J. Wong > > > > > > > Signed-off-by: Christoph Hellwig > > > > > > > > > > > > Can someone who knows more about shmem.c than I do please review > > > > > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > > > > > so that I can feel slightly more confident as hch and I sort > > > > > > through the > > > > > > xfile.c issues? > > > > > > > > > > > > For this patch, > > > > > > Reviewed-by: Darrick J. Wong > > > > > > > > > > ...except that I'm still getting 2M THPs even with this enabled, so I > > > > > guess either we get to fix it now, or create our own private tmpfs > > > > > mount > > > > > so that we can pass in huge=never, similar to what i915 does. :( > > > > > > > > What is "this"? Are you saying that $Subject doesn't work, or that the > > > > above-linked please-review patch doesn't work? > > > > > > shmem pays no attention to the mapping_large_folio_support() flag, > > > so the proposed fix doesn't work. It ought to, but it has its own way > > > of doing it that predates mapping_large_folio_support existing. > > > > Yep. It turned out to be easier to fix xfile.c to deal with large > > folios than I thought it would be. Or so I think. We'll see what > > happens on fstestscloud overnight. > > Where do we stand with this? Should I merge these two patches into > 6.8-rcX, cc:stable? This patchset doesn't actually fix the problem, so no, let's not merge it. For 6.9 we'll make xfile.c clean w.r.t. large folios: https://lore.kernel.org/linux-xfs/20240129143502.189370-1-...@lst.de/ I don't think we need a 6.8 backport since xfile.c is only used by an experimental feature that is default n in kconfig. --D
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Thu, 11 Jan 2024 18:22:50 -0800 "Darrick J. Wong" wrote: > On Thu, Jan 11, 2024 at 10:45:53PM +, Matthew Wilcox wrote: > > On Thu, Jan 11, 2024 at 02:00:53PM -0800, Andrew Morton wrote: > > > On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" > > > wrote: > > > > > > > > > Fixing this will require a bit of an API change, and prefeably > > > > > > sorting out > > > > > > the hwpoison story for pages vs folio and where it is placed in the > > > > > > shmem > > > > > > API. For now use this one liner to disable large folios. > > > > > > > > > > > > Reported-by: Darrick J. Wong > > > > > > Signed-off-by: Christoph Hellwig > > > > > > > > > > Can someone who knows more about shmem.c than I do please review > > > > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > > > > so that I can feel slightly more confident as hch and I sort through > > > > > the > > > > > xfile.c issues? > > > > > > > > > > For this patch, > > > > > Reviewed-by: Darrick J. Wong > > > > > > > > ...except that I'm still getting 2M THPs even with this enabled, so I > > > > guess either we get to fix it now, or create our own private tmpfs mount > > > > so that we can pass in huge=never, similar to what i915 does. :( > > > > > > What is "this"? Are you saying that $Subject doesn't work, or that the > > > above-linked please-review patch doesn't work? > > > > shmem pays no attention to the mapping_large_folio_support() flag, > > so the proposed fix doesn't work. It ought to, but it has its own way > > of doing it that predates mapping_large_folio_support existing. > > Yep. It turned out to be easier to fix xfile.c to deal with large > folios than I thought it would be. Or so I think. We'll see what > happens on fstestscloud overnight. Where do we stand with this? Should I merge these two patches into 6.8-rcX, cc:stable?
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Thu, Jan 11, 2024 at 10:45:53PM +, Matthew Wilcox wrote: > On Thu, Jan 11, 2024 at 02:00:53PM -0800, Andrew Morton wrote: > > On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" > > wrote: > > > > > > > Fixing this will require a bit of an API change, and prefeably > > > > > sorting out > > > > > the hwpoison story for pages vs folio and where it is placed in the > > > > > shmem > > > > > API. For now use this one liner to disable large folios. > > > > > > > > > > Reported-by: Darrick J. Wong > > > > > Signed-off-by: Christoph Hellwig > > > > > > > > Can someone who knows more about shmem.c than I do please review > > > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > > > so that I can feel slightly more confident as hch and I sort through the > > > > xfile.c issues? > > > > > > > > For this patch, > > > > Reviewed-by: Darrick J. Wong > > > > > > ...except that I'm still getting 2M THPs even with this enabled, so I > > > guess either we get to fix it now, or create our own private tmpfs mount > > > so that we can pass in huge=never, similar to what i915 does. :( > > > > What is "this"? Are you saying that $Subject doesn't work, or that the > > above-linked please-review patch doesn't work? > > shmem pays no attention to the mapping_large_folio_support() flag, > so the proposed fix doesn't work. It ought to, but it has its own way > of doing it that predates mapping_large_folio_support existing. Yep. It turned out to be easier to fix xfile.c to deal with large folios than I thought it would be. Or so I think. We'll see what happens on fstestscloud overnight. --D
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Thu, Jan 11, 2024 at 02:00:53PM -0800, Andrew Morton wrote: > On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" > wrote: > > > > > Fixing this will require a bit of an API change, and prefeably sorting > > > > out > > > > the hwpoison story for pages vs folio and where it is placed in the > > > > shmem > > > > API. For now use this one liner to disable large folios. > > > > > > > > Reported-by: Darrick J. Wong > > > > Signed-off-by: Christoph Hellwig > > > > > > Can someone who knows more about shmem.c than I do please review > > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > > so that I can feel slightly more confident as hch and I sort through the > > > xfile.c issues? > > > > > > For this patch, > > > Reviewed-by: Darrick J. Wong > > > > ...except that I'm still getting 2M THPs even with this enabled, so I > > guess either we get to fix it now, or create our own private tmpfs mount > > so that we can pass in huge=never, similar to what i915 does. :( > > What is "this"? Are you saying that $Subject doesn't work, or that the > above-linked please-review patch doesn't work? shmem pays no attention to the mapping_large_folio_support() flag, so the proposed fix doesn't work. It ought to, but it has its own way of doing it that predates mapping_large_folio_support existing.
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" wrote: > > > Fixing this will require a bit of an API change, and prefeably sorting out > > > the hwpoison story for pages vs folio and where it is placed in the shmem > > > API. For now use this one liner to disable large folios. > > > > > > Reported-by: Darrick J. Wong > > > Signed-off-by: Christoph Hellwig > > > > Can someone who knows more about shmem.c than I do please review > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > so that I can feel slightly more confident as hch and I sort through the > > xfile.c issues? > > > > For this patch, > > Reviewed-by: Darrick J. Wong > > ...except that I'm still getting 2M THPs even with this enabled, so I > guess either we get to fix it now, or create our own private tmpfs mount > so that we can pass in huge=never, similar to what i915 does. :( What is "this"? Are you saying that $Subject doesn't work, or that the above-linked please-review patch doesn't work?
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Wed, Jan 10, 2024 at 09:55:15AM -0800, Darrick J. Wong wrote: > On Wed, Jan 10, 2024 at 10:21:09AM +0100, Christoph Hellwig wrote: > > The xfarray code will crash if large folios are force enabled using: > > > >echo force > /sys/kernel/mm/transparent_hugepage/shmem_enabled > > > > Fixing this will require a bit of an API change, and prefeably sorting out > > the hwpoison story for pages vs folio and where it is placed in the shmem > > API. For now use this one liner to disable large folios. > > > > Reported-by: Darrick J. Wong > > Signed-off-by: Christoph Hellwig > > Can someone who knows more about shmem.c than I do please review > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > so that I can feel slightly more confident as hch and I sort through the > xfile.c issues? > > For this patch, > Reviewed-by: Darrick J. Wong ...except that I'm still getting 2M THPs even with this enabled, so I guess either we get to fix it now, or create our own private tmpfs mount so that we can pass in huge=never, similar to what i915 does. :( --D > --D > > > --- > > fs/xfs/scrub/xfile.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > > index 090c3ead43fdf1..1a8d1bedd0b0dc 100644 > > --- a/fs/xfs/scrub/xfile.c > > +++ b/fs/xfs/scrub/xfile.c > > @@ -94,6 +94,11 @@ xfile_create( > > > > lockdep_set_class(>i_rwsem, _i_mutex_key); > > > > + /* > > +* We're not quite ready for large folios yet. > > +*/ > > + mapping_clear_large_folios(inode->i_mapping); > > + > > trace_xfile_create(xf); > > > > *xfilep = xf; > > -- > > 2.39.2 > > > > >
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Wed, Jan 10, 2024 at 10:21:09AM +0100, Christoph Hellwig wrote: > The xfarray code will crash if large folios are force enabled using: > >echo force > /sys/kernel/mm/transparent_hugepage/shmem_enabled > > Fixing this will require a bit of an API change, and prefeably sorting out > the hwpoison story for pages vs folio and where it is placed in the shmem > API. For now use this one liner to disable large folios. > > Reported-by: Darrick J. Wong > Signed-off-by: Christoph Hellwig Can someone who knows more about shmem.c than I do please review https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ so that I can feel slightly more confident as hch and I sort through the xfile.c issues? For this patch, Reviewed-by: Darrick J. Wong --D > --- > fs/xfs/scrub/xfile.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > index 090c3ead43fdf1..1a8d1bedd0b0dc 100644 > --- a/fs/xfs/scrub/xfile.c > +++ b/fs/xfs/scrub/xfile.c > @@ -94,6 +94,11 @@ xfile_create( > > lockdep_set_class(>i_rwsem, _i_mutex_key); > > + /* > + * We're not quite ready for large folios yet. > + */ > + mapping_clear_large_folios(inode->i_mapping); > + > trace_xfile_create(xf); > > *xfilep = xf; > -- > 2.39.2 > >
[PATCH 2/2] xfs: disable large folio support in xfile_create
The xfarray code will crash if large folios are force enabled using: echo force > /sys/kernel/mm/transparent_hugepage/shmem_enabled Fixing this will require a bit of an API change, and prefeably sorting out the hwpoison story for pages vs folio and where it is placed in the shmem API. For now use this one liner to disable large folios. Reported-by: Darrick J. Wong Signed-off-by: Christoph Hellwig --- fs/xfs/scrub/xfile.c | 5 + 1 file changed, 5 insertions(+) diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c index 090c3ead43fdf1..1a8d1bedd0b0dc 100644 --- a/fs/xfs/scrub/xfile.c +++ b/fs/xfs/scrub/xfile.c @@ -94,6 +94,11 @@ xfile_create( lockdep_set_class(>i_rwsem, _i_mutex_key); + /* +* We're not quite ready for large folios yet. +*/ + mapping_clear_large_folios(inode->i_mapping); + trace_xfile_create(xf); *xfilep = xf; -- 2.39.2
