| Message ID | 20240103084126.513354-10-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/15] shmem: move the shmem_mapping assert into shmem_get_folio_gfp | expand |
On Wed, Jan 03, 2024 at 08:41:20AM +0000, Christoph Hellwig wrote: > XFS is generally used on 64-bit, non-highmem platforms and xfile > mappings are accessed all the time. Reduce our pain by not allowing > any highmem mappings in the xfile page cache and remove all the kmap > calls for it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/scrub/xfarray.c | 3 +-- > fs/xfs/scrub/xfile.c | 21 +++++++++------------ > 2 files changed, 10 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c > index f0f532c10a5acc..3a44700037924b 100644 > --- a/fs/xfs/scrub/xfarray.c > +++ b/fs/xfs/scrub/xfarray.c > @@ -580,7 +580,7 @@ xfarray_sort_get_page( > * xfile pages must never be mapped into userspace, so we skip the > * dcache flush when mapping the page. > */ > - si->page_kaddr = kmap_local_page(si->xfpage.page); > + si->page_kaddr = page_address(si->xfpage.page); > return 0; > } > > @@ -592,7 +592,6 @@ xfarray_sort_put_page( > if (!si->page_kaddr) > return 0; > > - kunmap_local(si->page_kaddr); > si->page_kaddr = NULL; > > return xfile_put_page(si->array->xfile, &si->xfpage); > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > index e872f4f0263f59..afbd205289e9b0 100644 > --- a/fs/xfs/scrub/xfile.c > +++ b/fs/xfs/scrub/xfile.c > @@ -77,6 +77,12 @@ xfile_create( > inode = file_inode(xf->file); > lockdep_set_class(&inode->i_rwsem, &xfile_i_mutex_key); > > + /* > + * We don't want to bother with kmapping data during repair, so don't > + * allow highmem pages to back this mapping. > + */ > + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER); Gonna be fun to see what happens on 32-bit. ;) But I do like getting rid of all the kmap overhead. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + > trace_xfile_create(xf); > > *xfilep = xf; > @@ -126,7 +132,6 @@ xfile_obj_load( > > pflags = memalloc_nofs_save(); > while (count > 0) { > - void *p, *kaddr; > unsigned int len; > > len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos)); > @@ -153,10 +158,7 @@ xfile_obj_load( > * xfile pages must never be mapped into userspace, so > * we skip the dcache flush. > */ > - kaddr = kmap_local_page(page); > - p = kaddr + offset_in_page(pos); > - memcpy(buf, p, len); > - kunmap_local(kaddr); > + memcpy(buf, page_address(page) + offset_in_page(pos), len); > put_page(page); > > advance: > @@ -221,14 +223,13 @@ xfile_obj_store( > * the dcache flush. If the page is not uptodate, zero it > * before writing data. > */ > - kaddr = kmap_local_page(page); > + kaddr = page_address(page); > if (!PageUptodate(page)) { > memset(kaddr, 0, PAGE_SIZE); > SetPageUptodate(page); > } > p = kaddr + offset_in_page(pos); > memcpy(p, buf, len); > - kunmap_local(kaddr); > > ret = aops->write_end(NULL, mapping, pos, len, len, page, > fsdata); > @@ -314,11 +315,7 @@ xfile_get_page( > * to the caller and make sure the backing store will hold on to them. > */ > if (!PageUptodate(page)) { > - void *kaddr; > - > - kaddr = kmap_local_page(page); > - memset(kaddr, 0, PAGE_SIZE); > - kunmap_local(kaddr); > + memset(page_address(page), 0, PAGE_SIZE); > SetPageUptodate(page); > } > > -- > 2.39.2 > >
On Wed, Jan 03, 2024 at 04:03:24PM -0800, Darrick J. Wong wrote: > > + /* > > + * We don't want to bother with kmapping data during repair, so don't > > + * allow highmem pages to back this mapping. > > + */ > > + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER); > > Gonna be fun to see what happens on 32-bit. ;) 32-bit with highmem, yes. I suspect we should just not allow online repair and scrub on that. I've in fact been tempted to see who would scream if we'd disallow XFS on 32-bit entirel, as that would simplify a lot of things.
On Thu, Jan 04, 2024 at 07:24:28AM +0100, Christoph Hellwig wrote: > On Wed, Jan 03, 2024 at 04:03:24PM -0800, Darrick J. Wong wrote: > > > + /* > > > + * We don't want to bother with kmapping data during repair, so don't > > > + * allow highmem pages to back this mapping. > > > + */ > > > + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER); > > > > Gonna be fun to see what happens on 32-bit. ;) > > 32-bit with highmem, yes. I suspect we should just not allow online > repair and scrub on that. I've in fact been tempted to see who would > scream if we'd disallow XFS on 32-bit entirel, as that would simplify > a lot of things. You and Dave both. :) I guess we could propose deprecating it like V4 and see if people come out of the woodwork. --D
diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c index f0f532c10a5acc..3a44700037924b 100644 --- a/fs/xfs/scrub/xfarray.c +++ b/fs/xfs/scrub/xfarray.c @@ -580,7 +580,7 @@ xfarray_sort_get_page( * xfile pages must never be mapped into userspace, so we skip the * dcache flush when mapping the page. */ - si->page_kaddr = kmap_local_page(si->xfpage.page); + si->page_kaddr = page_address(si->xfpage.page); return 0; } @@ -592,7 +592,6 @@ xfarray_sort_put_page( if (!si->page_kaddr) return 0; - kunmap_local(si->page_kaddr); si->page_kaddr = NULL; return xfile_put_page(si->array->xfile, &si->xfpage); diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c index e872f4f0263f59..afbd205289e9b0 100644 --- a/fs/xfs/scrub/xfile.c +++ b/fs/xfs/scrub/xfile.c @@ -77,6 +77,12 @@ xfile_create( inode = file_inode(xf->file); lockdep_set_class(&inode->i_rwsem, &xfile_i_mutex_key); + /* + * We don't want to bother with kmapping data during repair, so don't + * allow highmem pages to back this mapping. + */ + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER); + trace_xfile_create(xf); *xfilep = xf; @@ -126,7 +132,6 @@ xfile_obj_load( pflags = memalloc_nofs_save(); while (count > 0) { - void *p, *kaddr; unsigned int len; len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos)); @@ -153,10 +158,7 @@ xfile_obj_load( * xfile pages must never be mapped into userspace, so * we skip the dcache flush. */ - kaddr = kmap_local_page(page); - p = kaddr + offset_in_page(pos); - memcpy(buf, p, len); - kunmap_local(kaddr); + memcpy(buf, page_address(page) + offset_in_page(pos), len); put_page(page); advance: @@ -221,14 +223,13 @@ xfile_obj_store( * the dcache flush. If the page is not uptodate, zero it * before writing data. */ - kaddr = kmap_local_page(page); + kaddr = page_address(page); if (!PageUptodate(page)) { memset(kaddr, 0, PAGE_SIZE); SetPageUptodate(page); } p = kaddr + offset_in_page(pos); memcpy(p, buf, len); - kunmap_local(kaddr); ret = aops->write_end(NULL, mapping, pos, len, len, page, fsdata); @@ -314,11 +315,7 @@ xfile_get_page( * to the caller and make sure the backing store will hold on to them. */ if (!PageUptodate(page)) { - void *kaddr; - - kaddr = kmap_local_page(page); - memset(kaddr, 0, PAGE_SIZE); - kunmap_local(kaddr); + memset(page_address(page), 0, PAGE_SIZE); SetPageUptodate(page); }
XFS is generally used on 64-bit, non-highmem platforms and xfile mappings are accessed all the time. Reduce our pain by not allowing any highmem mappings in the xfile page cache and remove all the kmap calls for it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/scrub/xfarray.c | 3 +-- fs/xfs/scrub/xfile.c | 21 +++++++++------------ 2 files changed, 10 insertions(+), 14 deletions(-)