| Message ID | 20240103084126.513354-15-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| 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:25AM +0000, Christoph Hellwig wrote: > Rewrite xfile_obj_store to use xfile_get_page and xfile_put_page to > access the data in the shmem page cache instead of abusing the > shmem write_begin and write_end aops. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Much simpler, though I wonder if willy is going to have something to say about xfile.c continuing to pass pages around instead of folios. I /think/ that's ok since we actually need the physical base page for doing IO, right? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/scrub/xfile.c | 66 ++++++++------------------------------------ > 1 file changed, 11 insertions(+), 55 deletions(-) > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > index 3ed7fb82a4497b..987b03df241b02 100644 > --- a/fs/xfs/scrub/xfile.c > +++ b/fs/xfs/scrub/xfile.c > @@ -182,74 +182,30 @@ xfile_obj_store( > size_t count, > loff_t pos) > { > - struct inode *inode = file_inode(xf->file); > - struct address_space *mapping = inode->i_mapping; > - const struct address_space_operations *aops = mapping->a_ops; > - struct page *page = NULL; > - unsigned int pflags; > - int error = 0; > - > if (count > MAX_RW_COUNT) > return -ENOMEM; > - if (inode->i_sb->s_maxbytes - pos < count) > + if (file_inode(xf->file)->i_sb->s_maxbytes - pos < count) > return -ENOMEM; > > trace_xfile_obj_store(xf, pos, count); > > - pflags = memalloc_nofs_save(); > while (count > 0) { > - void *fsdata = NULL; > - void *p, *kaddr; > + struct page *page; > unsigned int len; > - int ret; > > len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos)); > + page = xfile_get_page(xf, pos, len, XFILE_ALLOC); > + if (IS_ERR(page)) > + return -ENOMEM; > + memcpy(page_address(page) + offset_in_page(pos), buf, len); > + xfile_put_page(xf, page); > > - /* > - * We call write_begin directly here to avoid all the freezer > - * protection lock-taking that happens in the normal path. > - * shmem doesn't support fs freeze, but lockdep doesn't know > - * that and will trip over that. > - */ > - error = aops->write_begin(NULL, mapping, pos, len, &page, > - &fsdata); > - if (error) { > - error = -ENOMEM; > - break; > - } > - > - /* > - * xfile pages must never be mapped into userspace, so we skip > - * the dcache flush. If the page is not uptodate, zero it > - * before writing data. > - */ > - kaddr = page_address(page); > - if (!PageUptodate(page)) { > - memset(kaddr, 0, PAGE_SIZE); > - SetPageUptodate(page); > - } > - p = kaddr + offset_in_page(pos); > - memcpy(p, buf, len); > - > - ret = aops->write_end(NULL, mapping, pos, len, len, page, > - fsdata); > - if (ret < 0) { > - error = -ENOMEM; > - break; > - } > - > - if (ret != len) { > - error = -ENOMEM; > - break; > - } > - > - count -= ret; > - pos += ret; > - buf += ret; > + count -= len; > + pos += len; > + buf += len; > } > - memalloc_nofs_restore(pflags); > > - return error; > + return 0; > } > > /* Find the next written area in the xfile data for a given offset. */ > -- > 2.39.2 > >
On Wed, Jan 03, 2024 at 04:20:24PM -0800, Darrick J. Wong wrote: > On Wed, Jan 03, 2024 at 08:41:25AM +0000, Christoph Hellwig wrote: > > Rewrite xfile_obj_store to use xfile_get_page and xfile_put_page to > > access the data in the shmem page cache instead of abusing the > > shmem write_begin and write_end aops. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Much simpler, though I wonder if willy is going to have something to say > about xfile.c continuing to pass pages around instead of folios. I > /think/ that's ok since we actually need the physical base page for > doing IO, right? Well, as mentioned in the cover letter I'd much prefer to return a folio here, but we'd first need to sort out the whole hwpoison flag mess for that first. There's also a few issues with the xfs-internal xfiles interface that need attention, but they're solvable.
diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c index 3ed7fb82a4497b..987b03df241b02 100644 --- a/fs/xfs/scrub/xfile.c +++ b/fs/xfs/scrub/xfile.c @@ -182,74 +182,30 @@ xfile_obj_store( size_t count, loff_t pos) { - struct inode *inode = file_inode(xf->file); - struct address_space *mapping = inode->i_mapping; - const struct address_space_operations *aops = mapping->a_ops; - struct page *page = NULL; - unsigned int pflags; - int error = 0; - if (count > MAX_RW_COUNT) return -ENOMEM; - if (inode->i_sb->s_maxbytes - pos < count) + if (file_inode(xf->file)->i_sb->s_maxbytes - pos < count) return -ENOMEM; trace_xfile_obj_store(xf, pos, count); - pflags = memalloc_nofs_save(); while (count > 0) { - void *fsdata = NULL; - void *p, *kaddr; + struct page *page; unsigned int len; - int ret; len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos)); + page = xfile_get_page(xf, pos, len, XFILE_ALLOC); + if (IS_ERR(page)) + return -ENOMEM; + memcpy(page_address(page) + offset_in_page(pos), buf, len); + xfile_put_page(xf, page); - /* - * We call write_begin directly here to avoid all the freezer - * protection lock-taking that happens in the normal path. - * shmem doesn't support fs freeze, but lockdep doesn't know - * that and will trip over that. - */ - error = aops->write_begin(NULL, mapping, pos, len, &page, - &fsdata); - if (error) { - error = -ENOMEM; - break; - } - - /* - * xfile pages must never be mapped into userspace, so we skip - * the dcache flush. If the page is not uptodate, zero it - * before writing data. - */ - kaddr = page_address(page); - if (!PageUptodate(page)) { - memset(kaddr, 0, PAGE_SIZE); - SetPageUptodate(page); - } - p = kaddr + offset_in_page(pos); - memcpy(p, buf, len); - - ret = aops->write_end(NULL, mapping, pos, len, len, page, - fsdata); - if (ret < 0) { - error = -ENOMEM; - break; - } - - if (ret != len) { - error = -ENOMEM; - break; - } - - count -= ret; - pos += ret; - buf += ret; + count -= len; + pos += len; + buf += len; } - memalloc_nofs_restore(pflags); - return error; + return 0; } /* Find the next written area in the xfile data for a given offset. */
Rewrite xfile_obj_store to use xfile_get_page and xfile_put_page to access the data in the shmem page cache instead of abusing the shmem write_begin and write_end aops. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/scrub/xfile.c | 66 ++++++++------------------------------------ 1 file changed, 11 insertions(+), 55 deletions(-)