| Message ID | 20221113162902.883850-3-hch@lst.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/9] extfat: remove ->writepage | expand |
On Sun 13-11-22 17:28:55, Christoph Hellwig wrote: > ->writepage is a very inefficient method to write back data, and only > used through write_cache_pages or a a fallback when no ->migrate_folio > method is present. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good! Feel free to add: Acked-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext2/inode.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index 918ab2f9e4c05..3b2e3e1e0fa25 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -869,11 +869,6 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > return ret; > } > > -static int ext2_writepage(struct page *page, struct writeback_control *wbc) > -{ > - return block_write_full_page(page, ext2_get_block, wbc); > -} > - > static int ext2_read_folio(struct file *file, struct folio *folio) > { > return mpage_read_folio(folio, ext2_get_block); > @@ -948,7 +943,6 @@ const struct address_space_operations ext2_aops = { > .invalidate_folio = block_invalidate_folio, > .read_folio = ext2_read_folio, > .readahead = ext2_readahead, > - .writepage = ext2_writepage, > .write_begin = ext2_write_begin, > .write_end = ext2_write_end, > .bmap = ext2_bmap, > -- > 2.30.2 >
On Mon, Nov 14, 2022 at 11:49:27AM +0100, Jan Kara wrote: > On Sun 13-11-22 17:28:55, Christoph Hellwig wrote: > > ->writepage is a very inefficient method to write back data, and only > > used through write_cache_pages or a a fallback when no ->migrate_folio > > method is present. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Looks good! Feel free to add: The testbot found a problem with this: ext2_commit_chunk calls write_one_page for the IS_DIRSYNC case, and write_one_page calls ->writepage. So I think I need to drop this one for now (none of the other file systems calls write_one_page). And then think what best to do about write_one_page/write_one_folio. I suspect just passing a writepage pointer to them might make most sense, as they are only used by a few file systems, and the calling convention with the locked page doesn't lend itself to using ->writepages.
On Wed 16-11-22 08:14:15, Christoph Hellwig wrote: > On Mon, Nov 14, 2022 at 11:49:27AM +0100, Jan Kara wrote: > > On Sun 13-11-22 17:28:55, Christoph Hellwig wrote: > > > ->writepage is a very inefficient method to write back data, and only > > > used through write_cache_pages or a a fallback when no ->migrate_folio > > > method is present. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > > Looks good! Feel free to add: > > The testbot found a problem with this: > > ext2_commit_chunk calls write_one_page for the IS_DIRSYNC case, > and write_one_page calls ->writepage. Right. > So I think I need to drop this one for now (none of the other > file systems calls write_one_page). And then think what best > to do about write_one_page/write_one_folio. I suspect just > passing a writepage pointer to them might make most sense, > as they are only used by a few file systems, and the calling > convention with the locked page doesn't lend itself to using > ->writepages. Looking at the code, IMO the write_one_page() looks somewhat premature anyway in that place. AFAICS we could handle the writeout using filemap_write_and_wait() if we moved it to somewhat later moment. So something like attached patch (only basic testing only so far)? Honza
On Wed, Nov 16, 2022 at 07:20:40PM +0100, Jan Kara wrote: > Looking at the code, IMO the write_one_page() looks somewhat premature > anyway in that place. AFAICS we could handle the writeout using > filemap_write_and_wait() if we moved it to somewhat later moment. So > something like attached patch (only basic testing only so far)? Yes, this looks sensible. Do you want to queue this one and the ext2 and udf patches from this series if the testing works fine? The same transformation should also be done for minix, sysfs and ufs. And a bunch of the others are probaby similar as well.
On Wed 16-11-22 22:31:47, Christoph Hellwig wrote: > On Wed, Nov 16, 2022 at 07:20:40PM +0100, Jan Kara wrote: > > Looking at the code, IMO the write_one_page() looks somewhat premature > > anyway in that place. AFAICS we could handle the writeout using > > filemap_write_and_wait() if we moved it to somewhat later moment. So > > something like attached patch (only basic testing only so far)? > > Yes, this looks sensible. Do you want to queue this one and the > ext2 and udf patches from this series if the testing works fine? OK, I'll take udf and ext2 patches through my tree. Honza
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c index 918ab2f9e4c05..3b2e3e1e0fa25 100644 --- a/fs/ext2/inode.c +++ b/fs/ext2/inode.c @@ -869,11 +869,6 @@ int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, return ret; } -static int ext2_writepage(struct page *page, struct writeback_control *wbc) -{ - return block_write_full_page(page, ext2_get_block, wbc); -} - static int ext2_read_folio(struct file *file, struct folio *folio) { return mpage_read_folio(folio, ext2_get_block); @@ -948,7 +943,6 @@ const struct address_space_operations ext2_aops = { .invalidate_folio = block_invalidate_folio, .read_folio = ext2_read_folio, .readahead = ext2_readahead, - .writepage = ext2_writepage, .write_begin = ext2_write_begin, .write_end = ext2_write_end, .bmap = ext2_bmap,
->writepage is a very inefficient method to write back data, and only used through write_cache_pages or a a fallback when no ->migrate_folio method is present. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/ext2/inode.c | 6 ------ 1 file changed, 6 deletions(-)