| Message ID | 20240129143502.189370-14-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/20] mm: move mapping_set_update out of <linux/swap.h> | expand |
On Mon, 29 Jan 2024, 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> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > 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 95ac14bceeadd6..d0f98a43b2ba0a 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 7e915385ef0011..a669ebbbc02d1d 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_KERNEL); I had been going to point you to inode_nohighmem(inode), which is how that is usually done. But I share Matthew's uncertainty about cpusets and the __GFP_HARDWALL inside GFP_USER: you may well be right to stick with GFP_KERNEL, just as you've done it here. I suppose it depends on whether you see this as a system operation or a user-invoked operation. I did also think that perhaps you could mask off __GFP_FS here, to avoid having to remember those memalloc_nofs_save()s elsewhere; but that's probably a bad idea, I doubt anywhere else does it that way. So, no change required, but I wanted to think about it. Hugh > + > trace_xfile_create(xf); > > *xfilep = xf; > @@ -126,7 +132,6 @@ xfile_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_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_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
diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c index 95ac14bceeadd6..d0f98a43b2ba0a 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 7e915385ef0011..a669ebbbc02d1d 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_KERNEL); + trace_xfile_create(xf); *xfilep = xf; @@ -126,7 +132,6 @@ xfile_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_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_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); }