This patchset converts XFS & iomap to use folios, and gets them to a
state where they can handle multi-page folios. Applying these patches
is not yet sufficient to actually start using multi-page folios for
XFS; more page cache changes are needed. I don't anticipate needing to
touch XFS again until we're at the point where we want to convert the
aops to be type-safe. It completes an xfstests run with no unexpected
failures. Most of these patches have been posted before and I've retained
acks/reviews where I thought them reasonable. Some patches are new.
v2:
- Added review tags from Jens, Darrick & Christoph (thanks!)
- Added folio_zero_* wrappers around zero_user_*()
- Added a patch to rename AS_THP_SUPPORT
- Added a patch to convert __block_write_begin_int() to take a folio
- Split the iomap_add_to_ioend() patch into three
- Updated changelog of bio_add_folio() (Jens)
- Adjusted whitespace of bio patches (Christoph, Jens)
- Improved changelog of readahead conversion to explain why the put_page()
disappeared (Christoph)
- Add a patch to zero an entire folio at a time, instead of limiting to
a page
- Switch pos & end_pos back to being u64 from loff_t
- Call block_write_end() and ->page_done with the head page of the folio,
as that's what those functions expect.
I intend to push patch 1 upstream myself (before 5.16), but I've included
it here to avoid nasty messages from the build-bots. I can probably
persuade Linus to take patches 2-4 as well if Darrick's not comfortable
taking them as part of the iomap changes.
These changes are also available at:
git://git.infradead.org/users/willy/pagecache.git heads/folio-iomap
I intend to rebase that branch to include any further R-b tags (some of
the patches are new and don't have reviews).
Matthew Wilcox (Oracle) (28):
csky,sparc: Declare flush_dcache_folio()
mm: Add functions to zero portions of a folio
fs: Remove FS_THP_SUPPORT
fs: Rename AS_THP_SUPPORT and mapping_thp_support
block: Add bio_add_folio()
block: Add bio_for_each_folio_all()
fs/buffer: Convert __block_write_begin_int() to take a folio
iomap: Convert to_iomap_page to take a folio
iomap: Convert iomap_page_create to take a folio
iomap: Convert iomap_page_release to take a folio
iomap: Convert iomap_releasepage to use a folio
iomap: Add iomap_invalidate_folio
iomap: Pass the iomap_page into iomap_set_range_uptodate
iomap: Convert bio completions to use folios
iomap: Use folio offsets instead of page offsets
iomap: Convert iomap_read_inline_data to take a folio
iomap: Convert readahead and readpage to use a folio
iomap: Convert iomap_page_mkwrite to use a folio
iomap: Convert __iomap_zero_iter to use a folio
iomap: Convert iomap_write_begin() and iomap_write_end() to folios
iomap: Convert iomap_write_end_inline to take a folio
iomap,xfs: Convert ->discard_page to ->discard_folio
iomap: Simplify iomap_writepage_map()
iomap: Simplify iomap_do_writepage()
iomap: Convert iomap_add_to_ioend() to take a folio
iomap: Convert iomap_migrate_page() to use folios
iomap: Support multi-page folios in invalidatepage
xfs: Support multi-page folios
Documentation/core-api/kernel-api.rst | 1 +
arch/csky/abiv1/inc/abi/cacheflush.h | 1 +
arch/csky/abiv2/inc/abi/cacheflush.h | 2 +
arch/sparc/include/asm/cacheflush_32.h | 1 +
arch/sparc/include/asm/cacheflush_64.h | 1 +
block/bio.c | 22 ++
fs/buffer.c | 22 +-
fs/inode.c | 2 -
fs/internal.h | 2 +-
fs/iomap/buffered-io.c | 506 +++++++++++++------------
fs/xfs/xfs_aops.c | 24 +-
fs/xfs/xfs_icache.c | 2 +
include/linux/bio.h | 56 ++-
include/linux/fs.h | 1 -
include/linux/highmem.h | 44 ++-
include/linux/iomap.h | 3 +-
include/linux/pagemap.h | 26 +-
mm/highmem.c | 2 -
mm/shmem.c | 3 +-
19 files changed, 431 insertions(+), 290 deletions(-)
--
2.33.0
These architectures do not include asm-generic/cacheflush.h so need
to declare it themselves.
Fixes: 08b0b0059bf1 ("mm: Add flush_dcache_folio()")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
arch/csky/abiv1/inc/abi/cacheflush.h | 1 +
arch/csky/abiv2/inc/abi/cacheflush.h | 2 ++
arch/sparc/include/asm/cacheflush_32.h | 1 +
arch/sparc/include/asm/cacheflush_64.h | 1 +
4 files changed, 5 insertions(+)
@@ -25,6 +25,8 @@ static inline void flush_dcache_page(struct page *page)clear_bit(PG_dcache_clean,&page->flags);}+voidflush_dcache_folio(structfolio*folio);+#define flush_dcache_mmap_lock(mapping) do { } while (0)#define flush_dcache_mmap_unlock(mapping) do { } while (0)#define flush_icache_page(vma, page) do { } while (0)
These functions are wrappers around zero_user_segments(), which means
that zero_user_segments() can now be called for compound pages even when
CONFIG_TRANSPARENT_HUGEPAGE is disabled.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
include/linux/highmem.h | 44 ++++++++++++++++++++++++++++++++++++++---
mm/highmem.c | 2 --
2 files changed, 41 insertions(+), 5 deletions(-)
Instead of setting a bit in the fs_flags to set a bit in the
address_space, set the bit in the address_space directly.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/inode.c | 2 --
include/linux/fs.h | 1 -
include/linux/pagemap.h | 16 ++++++++++++++++
mm/shmem.c | 3 ++-
4 files changed, 18 insertions(+), 4 deletions(-)
These are now indicators of multi-page folio support, not THP support.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
include/linux/pagemap.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -34,7 +34,7 @@ enum mapping_flags {AS_EXITING=4,/* final truncate in progress *//* writeback related tags are not used */AS_NO_WRITEBACK_TAGS=5,-AS_THP_SUPPORT=6,/* THPs supported */+AS_LARGE_FOLIO_SUPPORT=6,};/**
This is a thin wrapper around bio_add_page(). The main advantage here
is the documentation that folios larger than 2GiB are not supported.
It's not currently possible to allocate folios that large, but if it
ever becomes possible, this function will fail gracefully instead of
doing I/O to the wrong bytes.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
block/bio.c | 22 ++++++++++++++++++++++
include/linux/bio.h | 3 ++-
2 files changed, 24 insertions(+), 1 deletion(-)
Allow callers to iterate over each folio instead of each page. The
bio need not have been constructed using folios originally.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
Documentation/core-api/kernel-api.rst | 1 +
include/linux/bio.h | 53 ++++++++++++++++++++++++++-
2 files changed, 53 insertions(+), 1 deletion(-)
@@ -260,6 +260,57 @@ static inline struct bio_vec *bio_last_bvec_all(struct bio *bio)return&bio->bi_io_vec[bio->bi_vcnt-1];}+/**+*structfolio_iter-Stateforiteratingallfoliosinabio.+*@folio:Thecurrentfoliowe'reiterating.NULLafterthelastfolio.+*@offset:Thebyteoffsetwithinthecurrentfolio.+*@length:Thenumberofbytesinthisiteration(willnotcrossfolio+*boundary).+*/+structfolio_iter{+structfolio*folio;+size_toffset;+size_tlength;+/* private: for use by the iterator */+size_t_seg_count;+int_i;+};++staticinlinevoidbio_first_folio(structfolio_iter*fi,structbio*bio,+inti)+{+structbio_vec*bvec=bio_first_bvec_all(bio)+i;++fi->folio=page_folio(bvec->bv_page);+fi->offset=bvec->bv_offset++PAGE_SIZE*(bvec->bv_page-&fi->folio->page);+fi->_seg_count=bvec->bv_len;+fi->length=min(folio_size(fi->folio)-fi->offset,fi->_seg_count);+fi->_i=i;+}++staticinlinevoidbio_next_folio(structfolio_iter*fi,structbio*bio)+{+fi->_seg_count-=fi->length;+if(fi->_seg_count){+fi->folio=folio_next(fi->folio);+fi->offset=0;+fi->length=min(folio_size(fi->folio),fi->_seg_count);+}elseif(fi->_i+1<bio->bi_vcnt){+bio_first_folio(fi,bio,fi->_i+1);+}else{+fi->folio=NULL;+}+}++/**+*bio_for_each_folio_all-Iterateovereachfolioinabio.+*@fi:structfolio_iterwhichisupdatedforeachfolio.+*@bio:structbiotoiterateover.+*/+#define bio_for_each_folio_all(fi, bio) \+for(bio_first_folio(&fi,bio,0);fi.folio;bio_next_folio(&fi,bio))+enumbip_flags{BIP_BLOCK_INTEGRITY=1<<0,/* block layer owns integrity data */BIP_MAPPED_INTEGRITY=1<<1,/* ref tag has been remapped */
There are no plans to convert buffer_head infrastructure to use multi-page
folios, but __block_write_begin_int() is called from iomap, and it's
more convenient and less error-prone if we pass in a folio from iomap.
It also has a nice saving of almost 200 bytes of code from removing
repeated calls to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/buffer.c | 22 +++++++++++-----------
fs/internal.h | 2 +-
fs/iomap/buffered-io.c | 7 +++++--
3 files changed, 17 insertions(+), 14 deletions(-)
The big comment about only using a head page can go away now that
it takes a folio argument.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
This function already assumed it was being passed a head page, so
just formalise that.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
@@ -256,7 +257,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,returnmin(iomap_read_inline_data(iter,page),length);/* zero post-eof blocks as the page may be mapped */-iop=iomap_page_create(iter->inode,page);+iop=iomap_page_create(iter->inode,folio);iomap_adjust_read_range(iter->inode,iop,&pos,length,&poff,&plen);if(plen==0)gotodone;
iomap_page_release() was also assuming that it was being passed a
head page.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
This is an address_space operation, so its argument must remain as a
struct page, but we can use a folio internally.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Keep iomap_invalidatepage around as a wrapper for use in address_space
operations.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 20 ++++++++++++--------
include/linux/iomap.h | 1 +
2 files changed, 13 insertions(+), 8 deletions(-)
All but one caller already has the iomap_page, so we can avoid getting
it again.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
Use bio_for_each_folio() to iterate over each folio in the bio
instead of iterating over each page.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 50 ++++++++++++++++++------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
@@ -1048,8 +1041,7 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error)boolquiet=bio_flagged(bio,BIO_QUIET);for(bio=&ioend->io_inline_bio;bio;bio=next){-structbio_vec*bv;-structbvec_iter_alliter_all;+structfolio_iterfi;/**Forthelastbio,bi_privatepointstotheioend,sowe
@@ -1060,10 +1052,10 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error)elsenext=bio->bi_private;-/* walk each page on bio, ending page IO on them */-bio_for_each_segment_all(bv,bio,iter_all)-iomap_finish_page_writeback(inode,bv->bv_page,error,-bv->bv_len);+/* walk all folios in bio, ending page IO on them */+bio_for_each_folio_all(fi,bio)+iomap_finish_folio_write(inode,fi.folio,fi.length,+error);bio_put(bio);}/* The ioend has been freed by bio_put() */
Pass a folio around instead of the page, and make sure the offset
is relative to the start of the folio instead of the start of a page.
Also use size_t for offset & length to make it clear that these are byte
counts, and to support >2GB folios in the future.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 78 ++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 38 deletions(-)
@@ -255,13 +256,13 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,/* zero post-eof blocks as the page may be mapped */iop=iomap_page_create(iter->inode,folio);-iomap_adjust_read_range(iter->inode,iop,&pos,length,&poff,&plen);+iomap_adjust_read_range(iter->inode,folio,&pos,length,&poff,&plen);if(plen==0)gotodone;if(iomap_block_needs_zeroing(iter,pos)){-zero_user(page,poff,plen);-iomap_set_range_uptodate(page,iop,poff,plen);+folio_zero_range(folio,poff,plen);+iomap_set_range_uptodate(folio,iop,poff,plen);gotodone;}
We still only support up to a single page of inline data (at least,
per call to iomap_read_inline_data()), but it can now be written into
the middle of a folio in case we decide to allocate a 16KiB page for
a file that's 8.1KiB in size.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -252,7 +251,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,sector_tsector;if(iomap->type==IOMAP_INLINE)-returnmin(iomap_read_inline_data(iter,page),length);+returnmin(iomap_read_inline_data(iter,folio),length);/* zero post-eof blocks as the page may be mapped */iop=iomap_page_create(iter->inode,folio);
@@ -586,12 +585,13 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,staticintiomap_write_begin_inline(conststructiomap_iter*iter,structpage*page){+structfolio*folio=page_folio(page);intret;/* needs more work for the tailpacking case; disable for now */if(WARN_ON_ONCE(iomap_iter_srcmap(iter)->offset!=0))return-EIO;-ret=iomap_read_inline_data(iter,page);+ret=iomap_read_inline_data(iter,folio);if(ret<0)returnret;return0;
Handle folios of arbitrary size instead of working in PAGE_SIZE units.
readahead_folio() decreases the page refcount for you, so this is not
quite a mechanical change.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 53 +++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 27 deletions(-)
If we write to any page in a folio, we have to mark the entire
folio as dirty, and potentially COW the entire folio, because it'll
all get written back as one unit.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
The zero iterator can work in folio-sized chunks instead of page-sized
chunks. This will save a lot of page cache lookups if the file is cached
in multi-page folios.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
fs/iomap/buffered-io.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
These functions still only work in PAGE_SIZE chunks, but there are
fewer conversions from tail to head pages as a result of this patch.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 66 ++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 35 deletions(-)
@@ -582,9 +581,8 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,}staticintiomap_write_begin_inline(conststructiomap_iter*iter,-structpage*page)+structfolio*folio){-structfolio*folio=page_folio(page);intret;/* needs more work for the tailpacking case; disable for now */
@@ -597,12 +595,12 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,}staticintiomap_write_begin(conststructiomap_iter*iter,loff_tpos,-unsignedlen,structpage**pagep)+size_tlen,structfolio**foliop){conststructiomap_page_ops*page_ops=iter->iomap.page_ops;conststructiomap*srcmap=iomap_iter_srcmap(iter);-structpage*page;structfolio*folio;+unsignedfgp=FGP_LOCK|FGP_WRITE|FGP_CREAT|FGP_STABLE|FGP_NOFS;intstatus=0;BUG_ON(pos+len>iter->iomap.offset+iter->iomap.length);
@@ -695,7 +691,7 @@ static size_t iomap_write_end_inline(const struct iomap_iter *iter,/* Returns the number of bytes copied. May be 0. Cannot be an errno. */staticsize_tiomap_write_end(structiomap_iter*iter,loff_tpos,size_tlen,-size_tcopied,structpage*page)+size_tcopied,structfolio*folio){conststructiomap_page_ops*page_ops=iter->iomap.page_ops;conststructiomap*srcmap=iomap_iter_srcmap(iter);
This conversion is only safe because iomap only supports writes to inline
data which starts at the beginning of the file.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
XFS has the only implementation of ->discard_page today, so convert it
to use folios in the same patch as converting the API.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 4 ++--
fs/xfs/xfs_aops.c | 24 ++++++++++++------------
include/linux/iomap.h | 2 +-
3 files changed, 15 insertions(+), 15 deletions(-)
Rename end_offset to end_pos and file_offset to pos to match the rest
of the file. Simplify the loop by calculating nblocks up front instead
of each time around the loop.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
Rename end_offset to end_pos and offset_into_page to poff to match the
rest of the file. Simplify the handling of the last page straddling
i_size by doing the EOF check based on the byte granularity i_size
instead of converting to a pgoff prematurely.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
@@ -1474,13 +1471,13 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)*memoryiszeroedwhenmapped,andwritestothatregionare*notwrittenouttothefile."*/-zero_user_segment(page,offset_into_page,PAGE_SIZE);+zero_user_segment(page,poff,PAGE_SIZE);/* Adjust the end_offset to the end of file */-end_offset=offset;+end_pos=isize;}-returniomap_writepage_map(wpc,wbc,inode,page,end_offset);+returniomap_writepage_map(wpc,wbc,inode,page,end_pos);redirty:redirty_page_for_writepage(wbc,page);
We still iterate one block at a time, but now we call compound_head()
less often.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 70 ++++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 36 deletions(-)
@@ -1471,17 +1471,15 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)*memoryiszeroedwhenmapped,andwritestothatregionare*notwrittenouttothefile."*/-zero_user_segment(page,poff,PAGE_SIZE);--/* Adjust the end_offset to the end of file */+folio_zero_segment(folio,poff,folio_size(folio));end_pos=isize;}-returniomap_writepage_map(wpc,wbc,inode,page,end_pos);+returniomap_writepage_map(wpc,wbc,inode,folio,end_pos);redirty:-redirty_page_for_writepage(wbc,page);-unlock_page(page);+folio_redirty_for_writepage(wbc,folio);+folio_unlock(folio);return0;}
The arguments are still pages for now, but we can use folios internally
and cut out a lot of calls to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
If we're punching a hole in a multi-page folio, we need to remove the
per-folio iomap data as the folio is about to be split and each page will
need its own. If a dirty folio is only partially-uptodate, the iomap
data contains the information about which blocks cannot be written back,
so assert that a dirty folio is fully uptodate.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/iomap/buffered-io.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -470,13 +470,18 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len)trace_iomap_invalidatepage(folio->mapping->host,offset,len);/*-*Ifwe'reinvalidatingtheentirepage,clearthedirtystatefromit-*andreleaseittoavoidunnecessarybuildupoftheLRU.+*Ifwe'reinvalidatingtheentirefolio,clearthedirtystate+*fromitandreleaseittoavoidunnecessarybuildupoftheLRU.*/if(offset==0&&len==folio_size(folio)){WARN_ON_ONCE(folio_test_writeback(folio));folio_cancel_dirty(folio);iomap_page_release(folio);+}elseif(folio_test_multi(folio)){+/* Must release the iop so the page can be split */+WARN_ON_ONCE(!folio_test_uptodate(folio)&&+folio_test_dirty(folio));+iomap_page_release(folio);}}EXPORT_SYMBOL_GPL(iomap_invalidate_folio);
Now that iomap has been converted, XFS is multi-page folio safe.
Indicate to the VFS that it can now create multi-page folios for XFS.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_icache.c | 2 ++
1 file changed, 2 insertions(+)
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-09 08:37:06
On Mon, Nov 08, 2021 at 04:05:24AM +0000, Matthew Wilcox (Oracle) wrote:
These architectures do not include asm-generic/cacheflush.h so need
to declare it themselves.
In mainline mm/util.c implements flush_dcache_folio unless
ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO is set. So I think you need to
define that for csky and sparc.
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-09 08:40:16
On Mon, Nov 08, 2021 at 04:05:25AM +0000, Matthew Wilcox (Oracle) wrote:
These functions are wrappers around zero_user_segments(), which means
that zero_user_segments() can now be called for compound pages even when
CONFIG_TRANSPARENT_HUGEPAGE is disabled.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Related note: the inline !HIGHMEM version should switch to page_address
instead of kmap_local_page to make the code more obvious.
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-09 08:43:20
On Mon, Nov 08, 2021 at 04:05:40AM +0000, Matthew Wilcox (Oracle) wrote:
Handle folios of arbitrary size instead of working in PAGE_SIZE units.
readahead_folio() decreases the page refcount for you, so this is not
quite a mechanical change.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-09 08:47:32
On Mon, Nov 08, 2021 at 04:05:42AM +0000, Matthew Wilcox (Oracle) wrote:
The zero iterator can work in folio-sized chunks instead of page-sized
chunks. This will save a lot of page cache lookups if the file is cached
in multi-page folios.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
but it will clash with my just sent series that decouples DAX
zeroing from buffered I/O zeroing and folds __iomap_zero_iter into
the caller.
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-15 15:56:09
On Tue, Nov 09, 2021 at 12:36:57AM -0800, Christoph Hellwig wrote:
On Mon, Nov 08, 2021 at 04:05:24AM +0000, Matthew Wilcox (Oracle) wrote:
quoted
These architectures do not include asm-generic/cacheflush.h so need
to declare it themselves.
In mainline mm/util.c implements flush_dcache_folio unless
ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO is set. So I think you need to
define that for csky and sparc.
There are three ways to implement flush_dcache_folio(). The first is
as a noop (this is what xtensa does, which is the only architecture
to define ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO; it's also done
automatically by asm-generic if the architecture doesn't define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE). The second is as a loop which calls
flush_dcache_page() for each page in the folio. That's the default
implementation which you found in mm/util.c. The third way, which I
hope architecture maintainers actually implement, is to just set the
needs-flush bit on the head page. But that requires knowledge of each
architecture; they need to check the needs-flush bit on the head page
instead of the precise page. So I've done the safe, slow thing for
all architectures. The only reason that csky and sparc are "special"
is that they don't include asm-generic/cacheflush.h and the buildbots
didn't catch that before the merge window.
I'm doing the exact same thing for csky and sparc that I did for
arc/arm/m68k/mips/nds32/nios2/parisc/sh. Nothing more, nothing less.
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-15 16:03:56
On Tue, Nov 09, 2021 at 12:41:19AM -0800, Christoph Hellwig wrote:
On Mon, Nov 08, 2021 at 04:05:27AM +0000, Matthew Wilcox (Oracle) wrote:
quoted
These are now indicators of multi-page folio support, not THP support.
Given that we don't use the large foltio term anywhere else this really
needs to grow a comment explaining what the flag means.
I think I prefer the term 'large' to 'multi'. What would you think to
this patch (not on top of any particular branch; just to show the scope
of it ...)
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-16 06:38:22
On Mon, Nov 15, 2021 at 03:54:47PM +0000, Matthew Wilcox wrote:
There are three ways to implement flush_dcache_folio(). The first is
as a noop (this is what xtensa does, which is the only architecture
to define ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO; it's also done
automatically by asm-generic if the architecture doesn't define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE). The second is as a loop which calls
flush_dcache_page() for each page in the folio. That's the default
implementation which you found in mm/util.c. The third way, which I
hope architecture maintainers actually implement, is to just set the
needs-flush bit on the head page. But that requires knowledge of each
architecture; they need to check the needs-flush bit on the head page
instead of the precise page. So I've done the safe, slow thing for
all architectures. The only reason that csky and sparc are "special"
is that they don't include asm-generic/cacheflush.h and the buildbots
didn't catch that before the merge window.
I'm doing the exact same thing for csky and sparc that I did for
arc/arm/m68k/mips/nds32/nios2/parisc/sh. Nothing more, nothing less.
I see how this works no, but it is pretty horrible. Why not something
simple like the patch below? If/when an architecture actually
wants to override flush_dcache_folio we can find out how to best do
it:
@@ -25,8 +25,6 @@ static inline void flush_dcache_page(struct page *page)clear_bit(PG_dcache_clean,&page->flags);}-voidflush_dcache_folio(structfolio*folio);-#define flush_dcache_mmap_lock(mapping) do { } while (0)#define flush_dcache_mmap_unlock(mapping) do { } while (0)#define flush_icache_page(vma, page) do { } while (0)
From: Christoph Hellwig <hch@infradead.org> Date: 2021-11-16 06:38:22
On Mon, Nov 15, 2021 at 04:03:22PM +0000, Matthew Wilcox wrote:
I think I prefer the term 'large' to 'multi'. What would you think to
this patch (not on top of any particular branch; just to show the scope
of it ...)
I don't really care either way. Just be consistent and maybe add a
comment here and there..
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-16 21:49:18
On Mon, Nov 15, 2021 at 10:33:01PM -0800, Christoph Hellwig wrote:
I see how this works no, but it is pretty horrible. Why not something
simple like the patch below? If/when an architecture actually
wants to override flush_dcache_folio we can find out how to best do
it:
I'll stick this one into -next and see if anything blows up:
From 14f55de74c68a3eb058cfdbf81414148b9bdaac7 Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Sat, 6 Nov 2021 17:13:35 -0400
Subject: [PATCH] Add linux/cacheflush.h
Many architectures do not include asm-generic/cacheflush.h, so turn
the includes on their head and add linux/cacheflush.h which includes
asm/cacheflush.h.
Move the flush_dcache_folio() declaration from asm-generic/cacheflush.h
to linux/cacheflush.h and change linux/highmem.h to include
linux/cacheflush.h instead of asm/cacheflush.h so that all necessary
places will see flush_dcache_folio().
More functions should have their default implementations moved in the
future, but those are for follow-on patches. This fixes csky, sparc and
sparc64 which were missed in the commit which added flush_dcache_folio().
Fixes: 08b0b0059bf1 ("mm: Add flush_dcache_folio()")
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
arch/arc/include/asm/cacheflush.h | 1 -
arch/arm/include/asm/cacheflush.h | 1 -
arch/m68k/include/asm/cacheflush_mm.h | 1 -
arch/mips/include/asm/cacheflush.h | 2 --
arch/nds32/include/asm/cacheflush.h | 1 -
arch/nios2/include/asm/cacheflush.h | 1 -
arch/parisc/include/asm/cacheflush.h | 1 -
arch/sh/include/asm/cacheflush.h | 1 -
arch/xtensa/include/asm/cacheflush.h | 3 ---
include/asm-generic/cacheflush.h | 6 ------
include/linux/cacheflush.h | 18 ++++++++++++++++++
include/linux/highmem.h | 3 +--
12 files changed, 19 insertions(+), 20 deletions(-)
create mode 100644 include/linux/cacheflush.h
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 02:20:48
On Mon, Nov 08, 2021 at 04:05:35AM +0000, Matthew Wilcox (Oracle) wrote:
Keep iomap_invalidatepage around as a wrapper for use in address_space
operations.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 02:24:26
On Mon, Nov 08, 2021 at 04:05:42AM +0000, Matthew Wilcox (Oracle) wrote:
The zero iterator can work in folio-sized chunks instead of page-sized
chunks. This will save a lot of page cache lookups if the file is cached
in multi-page folios.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
hch's dax decoupling series notwithstanding,
Though TBH I am kinda wondering how the two of you plan to resolve those
kinds of differences -- I haven't looked at that series, though I think
this one's been waiting in the wings for longer?
Heck, I wonder how Matthew plans to merge all this given that it touches
mm, fs, block, and iomap...?
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:31:29
On Mon, Nov 08, 2021 at 04:05:43AM +0000, Matthew Wilcox (Oracle) wrote:
quoted hunk
These functions still only work in PAGE_SIZE chunks, but there are
fewer conversions from tail to head pages as a result of this patch.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/iomap/buffered-io.c | 66 ++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 35 deletions(-)
Hrmm. In principle (or I guess even a subsequent patch), if we had
multi-page folios, could we simply loop the pages in the folio instead
of doing a single page and then calling back into iomap_write_begin to
get (probably) the same folio?
This looks like a fairly straightforward conversion, but I was wondering
about that one little point...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
quoted hunk
- status = iomap_write_end(iter, pos, bytes, copied, page);
+ status = iomap_write_end(iter, pos, bytes, copied, folio);
if (unlikely(copied != status))
iov_iter_revert(i, copied - status);
@@ -839,13 +837,13 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter) do { unsigned long offset = offset_in_page(pos); unsigned long bytes = min_t(loff_t, PAGE_SIZE - offset, length);- struct page *page;+ struct folio *folio;- status = iomap_write_begin(iter, pos, bytes, &page);+ status = iomap_write_begin(iter, pos, bytes, &folio); if (unlikely(status)) return status;- status = iomap_write_end(iter, pos, bytes, bytes, page);+ status = iomap_write_end(iter, pos, bytes, bytes, folio); if (WARN_ON_ONCE(status == 0)) return -EIO;
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:34:28
On Mon, Nov 08, 2021 at 04:05:48AM +0000, Matthew Wilcox (Oracle) wrote:
We still iterate one block at a time, but now we call compound_head()
less often.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
@@ -1471,17 +1471,15 @@ iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)*memoryiszeroedwhenmapped,andwritestothatregionare*notwrittenouttothefile."*/-zero_user_segment(page,poff,PAGE_SIZE);--/* Adjust the end_offset to the end of file */+folio_zero_segment(folio,poff,folio_size(folio));end_pos=isize;}-returniomap_writepage_map(wpc,wbc,inode,page,end_pos);+returniomap_writepage_map(wpc,wbc,inode,folio,end_pos);redirty:-redirty_page_for_writepage(wbc,page);-unlock_page(page);+folio_redirty_for_writepage(wbc,folio);+folio_unlock(folio);return0;}
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:35:38
On Mon, Nov 08, 2021 at 04:05:30AM +0000, Matthew Wilcox (Oracle) wrote:
There are no plans to convert buffer_head infrastructure to use multi-page
folios, but __block_write_begin_int() is called from iomap, and it's
more convenient and less error-prone if we pass in a folio from iomap.
It also has a nice saving of almost 200 bytes of code from removing
repeated calls to compound_head().
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Pretty straightforward,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:36:22
On Mon, Nov 08, 2021 at 04:05:26AM +0000, Matthew Wilcox (Oracle) wrote:
Instead of setting a bit in the fs_flags to set a bit in the
address_space, set the bit in the address_space directly.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Makes sense,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:45:30
On Mon, Nov 08, 2021 at 04:05:25AM +0000, Matthew Wilcox (Oracle) wrote:
quoted hunk
These functions are wrappers around zero_user_segments(), which means
that zero_user_segments() can now be called for compound pages even when
CONFIG_TRANSPARENT_HUGEPAGE is disabled.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
include/linux/highmem.h | 44 ++++++++++++++++++++++++++++++++++++++---
mm/highmem.c | 2 --
2 files changed, 41 insertions(+), 5 deletions(-)
At first I thought "Gee, this is wrong, end should be start+length-1!"
Then I looked at zero_user_segments and realized that despite the
parameter name "endi1", it really wants you to tell it the next byte.
Not the end byte of the range you want to zero.
Then I looked at the other two new functions and saw that you documented
this, and now I get why Linus ranted about this some time ago.
The code looks right, but the "end" names rankle me. Can we please
change them all? Or at least in the new functions, if you all already
fought a flamewar over this that I'm not aware of?
Almost-Reviewed-by: Darrick J. Wong [off-list ref]
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:48:12
On Mon, Nov 08, 2021 at 04:05:28AM +0000, Matthew Wilcox (Oracle) wrote:
This is a thin wrapper around bio_add_page(). The main advantage here
is the documentation that folios larger than 2GiB are not supported.
It's not currently possible to allocate folios that large, but if it
ever becomes possible, this function will fail gracefully instead of
doing I/O to the wrong bytes.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 04:48:40
On Mon, Nov 08, 2021 at 04:05:29AM +0000, Matthew Wilcox (Oracle) wrote:
Allow callers to iterate over each folio instead of each page. The
bio need not have been constructed using folios originally.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
@@ -260,6 +260,57 @@ static inline struct bio_vec *bio_last_bvec_all(struct bio *bio)return&bio->bi_io_vec[bio->bi_vcnt-1];}+/**+*structfolio_iter-Stateforiteratingallfoliosinabio.+*@folio:Thecurrentfoliowe'reiterating.NULLafterthelastfolio.+*@offset:Thebyteoffsetwithinthecurrentfolio.+*@length:Thenumberofbytesinthisiteration(willnotcrossfolio+*boundary).+*/+structfolio_iter{+structfolio*folio;+size_toffset;+size_tlength;+/* private: for use by the iterator */+size_t_seg_count;+int_i;+};++staticinlinevoidbio_first_folio(structfolio_iter*fi,structbio*bio,+inti)+{+structbio_vec*bvec=bio_first_bvec_all(bio)+i;++fi->folio=page_folio(bvec->bv_page);+fi->offset=bvec->bv_offset++PAGE_SIZE*(bvec->bv_page-&fi->folio->page);+fi->_seg_count=bvec->bv_len;+fi->length=min(folio_size(fi->folio)-fi->offset,fi->_seg_count);+fi->_i=i;+}++staticinlinevoidbio_next_folio(structfolio_iter*fi,structbio*bio)+{+fi->_seg_count-=fi->length;+if(fi->_seg_count){+fi->folio=folio_next(fi->folio);+fi->offset=0;+fi->length=min(folio_size(fi->folio),fi->_seg_count);+}elseif(fi->_i+1<bio->bi_vcnt){+bio_first_folio(fi,bio,fi->_i+1);+}else{+fi->folio=NULL;+}+}++/**+*bio_for_each_folio_all-Iterateovereachfolioinabio.+*@fi:structfolio_iterwhichisupdatedforeachfolio.+*@bio:structbiotoiterateover.+*/+#define bio_for_each_folio_all(fi, bio) \+for(bio_first_folio(&fi,bio,0);fi.folio;bio_next_folio(&fi,bio))+enumbip_flags{BIP_BLOCK_INTEGRITY=1<<0,/* block layer owns integrity data */BIP_MAPPED_INTEGRITY=1<<1,/* ref tag has been remapped */
On Wed, Nov 17, 2021 at 2:22 AM Matthew Wilcox [off-list ref] wrote:
On Mon, Nov 15, 2021 at 10:33:01PM -0800, Christoph Hellwig wrote:
quoted
I see how this works no, but it is pretty horrible. Why not something
simple like the patch below? If/when an architecture actually
wants to override flush_dcache_folio we can find out how to best do
it:
I'll stick this one into -next and see if anything blows up:
From 14f55de74c68a3eb058cfdbf81414148b9bdaac7 Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Sat, 6 Nov 2021 17:13:35 -0400
Subject: [PATCH] Add linux/cacheflush.h
Many architectures do not include asm-generic/cacheflush.h, so turn
the includes on their head and add linux/cacheflush.h which includes
asm/cacheflush.h.
Move the flush_dcache_folio() declaration from asm-generic/cacheflush.h
to linux/cacheflush.h and change linux/highmem.h to include
linux/cacheflush.h instead of asm/cacheflush.h so that all necessary
places will see flush_dcache_folio().
More functions should have their default implementations moved in the
future, but those are for follow-on patches. This fixes csky, sparc and
sparc64 which were missed in the commit which added flush_dcache_folio().
Fixes: 08b0b0059bf1 ("mm: Add flush_dcache_folio()")
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
arch/m68k/include/asm/cacheflush_mm.h | 1 -
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-17 14:07:05
On Tue, Nov 16, 2021 at 08:45:27PM -0800, Darrick J. Wong wrote:
quoted
+/**
+ * folio_zero_segment() - Zero a byte range in a folio.
+ * @folio: The folio to write to.
+ * @start: The first byte to zero.
+ * @end: One more than the last byte in the first range.
+ */
+static inline void folio_zero_segment(struct folio *folio,
+ size_t start, size_t end)
+{
+ zero_user_segments(&folio->page, start, end, 0, 0);
+}
+
+/**
+ * folio_zero_range() - Zero a byte range in a folio.
+ * @folio: The folio to write to.
+ * @start: The first byte to zero.
+ * @length: The number of bytes to zero.
+ */
+static inline void folio_zero_range(struct folio *folio,
+ size_t start, size_t length)
+{
+ zero_user_segments(&folio->page, start, start + length, 0, 0);
At first I thought "Gee, this is wrong, end should be start+length-1!"
Then I looked at zero_user_segments and realized that despite the
parameter name "endi1", it really wants you to tell it the next byte.
Not the end byte of the range you want to zero.
Then I looked at the other two new functions and saw that you documented
this, and now I get why Linus ranted about this some time ago.
The code looks right, but the "end" names rankle me. Can we please
change them all? Or at least in the new functions, if you all already
fought a flamewar over this that I'm not aware of?
Change them to what? I tend to use 'end' to mean 'excluded end' and
'max' to mean 'included end'. What would you call the excluded end?
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-17 14:21:23
On Tue, Nov 16, 2021 at 06:24:24PM -0800, Darrick J. Wong wrote:
On Mon, Nov 08, 2021 at 04:05:42AM +0000, Matthew Wilcox (Oracle) wrote:
quoted
The zero iterator can work in folio-sized chunks instead of page-sized
chunks. This will save a lot of page cache lookups if the file is cached
in multi-page folios.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
hch's dax decoupling series notwithstanding,
Though TBH I am kinda wondering how the two of you plan to resolve those
kinds of differences -- I haven't looked at that series, though I think
this one's been waiting in the wings for longer?
I haven't looked at that series either
Heck, I wonder how Matthew plans to merge all this given that it touches
mm, fs, block, and iomap...?
Hrmm. In principle (or I guess even a subsequent patch), if we had
multi-page folios, could we simply loop the pages in the folio instead
of doing a single page and then calling back into iomap_write_begin to
get (probably) the same folio?
This looks like a fairly straightforward conversion, but I was wondering
about that one little point...
Theoretically, yes, we should be able to do that. But all of this code
is pretty subtle ("What if we hit a page fault? What if we're writing
to part of this folio from an mmap of a different part of this folio?
What if it's !Uptodate? What if we hit this weird ARM super-mprotect
memory tag thing? What if ...") and, frankly, I got scared. So I've
left that as future work; someone else can try to wrap their brain around
all of this.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-17 17:07:11
On Wed, Nov 17, 2021 at 02:07:00PM +0000, Matthew Wilcox wrote:
On Tue, Nov 16, 2021 at 08:45:27PM -0800, Darrick J. Wong wrote:
quoted
quoted
+/**
+ * folio_zero_segment() - Zero a byte range in a folio.
+ * @folio: The folio to write to.
+ * @start: The first byte to zero.
+ * @end: One more than the last byte in the first range.
+ */
+static inline void folio_zero_segment(struct folio *folio,
+ size_t start, size_t end)
+{
+ zero_user_segments(&folio->page, start, end, 0, 0);
+}
+
+/**
+ * folio_zero_range() - Zero a byte range in a folio.
+ * @folio: The folio to write to.
+ * @start: The first byte to zero.
+ * @length: The number of bytes to zero.
+ */
+static inline void folio_zero_range(struct folio *folio,
+ size_t start, size_t length)
+{
+ zero_user_segments(&folio->page, start, start + length, 0, 0);
At first I thought "Gee, this is wrong, end should be start+length-1!"
Then I looked at zero_user_segments and realized that despite the
parameter name "endi1", it really wants you to tell it the next byte.
Not the end byte of the range you want to zero.
Then I looked at the other two new functions and saw that you documented
this, and now I get why Linus ranted about this some time ago.
The code looks right, but the "end" names rankle me. Can we please
change them all? Or at least in the new functions, if you all already
fought a flamewar over this that I'm not aware of?
Change them to what? I tend to use 'end' to mean 'excluded end' and
'max' to mean 'included end'. What would you call the excluded end?
I've started using 'next', or changing the code to make 'end' be the
last element in the range the caller wants to act upon. The thing is,
those are all iterators, so 'next' fits, whereas it doesn't fit so well
for range zeroing where that might have been all the zeroing we wanted
to do.
Though. 'xend' (shorthand for 'excluded end') is different enough to
signal that the reader should pay attention. Ok, how about xend then?
--D
Hrmm. In principle (or I guess even a subsequent patch), if we had
multi-page folios, could we simply loop the pages in the folio instead
of doing a single page and then calling back into iomap_write_begin to
get (probably) the same folio?
This looks like a fairly straightforward conversion, but I was wondering
about that one little point...
Theoretically, yes, we should be able to do that. But all of this code
is pretty subtle ("What if we hit a page fault? What if we're writing
to part of this folio from an mmap of a different part of this folio?
What if it's !Uptodate? What if we hit this weird ARM super-mprotect
memory tag thing? What if ...") and, frankly, I got scared. So I've
left that as future work; someone else can try to wrap their brain around
all of this.
<nod> That's roughly the same conclusion I came to -- conceptually we
could keep walking pages until we hit /any/ problem or other difference
with the first page that we don't feel like dealing with, and pass that
count to iomap_end... but no need to try that right this second.
Just checking that I grokked what's going on in this series. :)
--D
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-18 15:55:18
On Wed, Nov 17, 2021 at 09:07:07AM -0800, Darrick J. Wong wrote:
I've started using 'next', or changing the code to make 'end' be the
last element in the range the caller wants to act upon. The thing is,
those are all iterators, so 'next' fits, whereas it doesn't fit so well
for range zeroing where that might have been all the zeroing we wanted
to do.
Yeah, it doesn't really work so well for one of the patches in this
series:
if (buffer_new(bh)) {
...
folio_zero_segments(folio,
to, block_end,
block_start, from);
("zero between block_start and block_end, except for the region
specified by 'from' and 'to'"). Except that for some reason the
ranges are specified backwards, so it's not obvious what's going on.
Converting that to folio_zero_ranges() would be a possibility, at the
expense of complexity in the caller, or using 'max' instead of 'end'
would also add complexity to the callers.
Though. 'xend' (shorthand for 'excluded end') is different enough to
signal that the reader should pay attention. Ok, how about xend then?
offset, size_t len)
* folio_zero_segments() - Zero two byte ranges in a folio.
* @folio: The folio to write to.
* @start1: The first byte to zero.
- * @end1: One more than the last byte in the first range.
+ * @xend1: One more than the last byte in the first range.
* @start2: The first byte to zero in the second range.
- * @end2: One more than the last byte in the second range.
+ * @xend2: One more than the last byte in the second range.
*/
static inline void folio_zero_segments(struct folio *folio,
- size_t start1, size_t end1, size_t start2, size_t end2)
+ size_t start1, size_t xend1, size_t start2, size_t xend2)
{
- zero_user_segments(&folio->page, start1, end1, start2, end2);
+ zero_user_segments(&folio->page, start1, xend1, start2, xend2);
}
/**
* folio_zero_segment() - Zero a byte range in a folio.
* @folio: The folio to write to.
* @start: The first byte to zero.
- * @end: One more than the last byte in the first range.
+ * @xend: One more than the last byte to zero.
*/
static inline void folio_zero_segment(struct folio *folio,
- size_t start, size_t end)
+ size_t start, size_t xend)
{
- zero_user_segments(&folio->page, start, end, 0, 0);
+ zero_user_segments(&folio->page, start, xend, 0, 0);
}
/**
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-11-18 17:26:19
On Thu, Nov 18, 2021 at 03:55:12PM +0000, Matthew Wilcox wrote:
On Wed, Nov 17, 2021 at 09:07:07AM -0800, Darrick J. Wong wrote:
quoted
I've started using 'next', or changing the code to make 'end' be the
last element in the range the caller wants to act upon. The thing is,
those are all iterators, so 'next' fits, whereas it doesn't fit so well
for range zeroing where that might have been all the zeroing we wanted
to do.
Yeah, it doesn't really work so well for one of the patches in this
series:
if (buffer_new(bh)) {
...
folio_zero_segments(folio,
to, block_end,
block_start, from);
("zero between block_start and block_end, except for the region
specified by 'from' and 'to'"). Except that for some reason the
ranges are specified backwards, so it's not obvious what's going on.
Converting that to folio_zero_ranges() would be a possibility, at the
expense of complexity in the caller, or using 'max' instead of 'end'
would also add complexity to the callers.
The call above looks like it is preparing to copy some data into the
middle of a buffer by zero-initializing the bytes before and the bytes
after that middle region.
Admittedly my fs-addled brain actually finds this hot mess easier to
understand:
folio_zero_segments(folio, to, blocksize - 1, block_start, from - 1);
but I suppose the xend method involves less subtraction everywhere.
quoted hunk
quoted
Though. 'xend' (shorthand for 'excluded end') is different enough to
signal that the reader should pay attention. Ok, how about xend then?
offset, size_t len)
* folio_zero_segments() - Zero two byte ranges in a folio.
* @folio: The folio to write to.
* @start1: The first byte to zero.
- * @end1: One more than the last byte in the first range.
+ * @xend1: One more than the last byte in the first range.
* @start2: The first byte to zero in the second range.
- * @end2: One more than the last byte in the second range.
+ * @xend2: One more than the last byte in the second range.
*/
static inline void folio_zero_segments(struct folio *folio,
- size_t start1, size_t end1, size_t start2, size_t end2)
+ size_t start1, size_t xend1, size_t start2, size_t xend2)
{
- zero_user_segments(&folio->page, start1, end1, start2, end2);
+ zero_user_segments(&folio->page, start1, xend1, start2, xend2);
}
/**
* folio_zero_segment() - Zero a byte range in a folio.
* @folio: The folio to write to.
* @start: The first byte to zero.
- * @end: One more than the last byte in the first range.
+ * @xend: One more than the last byte to zero.
*/
static inline void folio_zero_segment(struct folio *folio,
- size_t start, size_t end)
+ size_t start, size_t xend)
{
- zero_user_segments(&folio->page, start, end, 0, 0);
+ zero_user_segments(&folio->page, start, xend, 0, 0);
Works for me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-18 20:08:54
On Thu, Nov 18, 2021 at 09:26:15AM -0800, Darrick J. Wong wrote:
On Thu, Nov 18, 2021 at 03:55:12PM +0000, Matthew Wilcox wrote:
quoted
if (buffer_new(bh)) {
...
folio_zero_segments(folio,
to, block_end,
block_start, from);
("zero between block_start and block_end, except for the region
specified by 'from' and 'to'"). Except that for some reason the
ranges are specified backwards, so it's not obvious what's going on.
Converting that to folio_zero_ranges() would be a possibility, at the
expense of complexity in the caller, or using 'max' instead of 'end'
would also add complexity to the callers.
The call above looks like it is preparing to copy some data into the
middle of a buffer by zero-initializing the bytes before and the bytes
after that middle region.
Admittedly my fs-addled brain actually finds this hot mess easier to
understand:
folio_zero_segments(folio, to, blocksize - 1, block_start, from - 1);
but I suppose the xend method involves less subtraction everywhere.
That's exactly what it's doing. It's kind of funny because it's an
abstraction that permits a micro-optimisation (removing potentially one
kmap() call), but removes the opportunity for a larger optimisation
(removing several, and also removing calls to flush_dcache_folio).
That is, we could rewrite __block_write_begin_int() as:
static void *kremap_folio(void *kaddr, struct folio *folio)
{
if (kaddr)
return kaddr;
/* buffer heads only support single page folios */
return kmap_local_folio(folio, 0);
}
+ void *kaddr = NULL;
...
- if (block_end > to || block_start < from)
- folio_zero_segments(folio,
- to, block_end,
- block_start, from);
+ if (from > block_start) {
+ kaddr = kremap_folio(kaddr, folio);
+ memset(kaddr + block_start, 0,
+ block_start - from);
+ }
+ if (block_end > to) {
+ kaddr = kremap_folio(kaddr, folio);
+ memset(kaddr + to, 0, block_end - to);
+ }
...
}
+ if (kaddr) {
+ kunmap_local(kaddr);
+ flush_dcache_folio(folio);
+ }
That way if there are multiple unmapped+new buffers, we only kmap/kunmap
once per page. I don't care to submit this as a patch though ... buffer
heads just need to go away. iomap can't use an optimisation like this;
it already reports all the contiguous unmapped blocks as a single extent,
and if you have multiple unmapped extents per page, well ... I'm sorry
for you, but the overhead of kmap/kunmap is the least of your problems.
This turned out to be buggy. Darrick and I figured out why his tests
were failing and mine weren't; this only shows up with a 4kB block
size filesystem and I was only testing with 1kB block size filesystems.
(at least on x86; I haven't figured out why it passes with 1kB block size
filesystems, so I'm not sure what would be true on other filesystems).
iomap_write_begin() is not prepared to deal with a length that spans a
page boundary. So I'm replacing this patch with the following patches
(whitespace damaged; pick them up from
https://git.infradead.org/users/willy/linux.git/tag/refs/tags/iomap-folio-5.17c
if you want to compile them):
commit 412212960b72
Author: Matthew Wilcox (Oracle) [off-list ref]
Date: Thu Dec 9 15:47:44 2021 -0500
iomap: Allow iomap_write_begin() to be called with the full length
In the future, we want write_begin to know the entire length of the
write so that it can choose to allocate large folios. Pass the full
length in from __iomap_zero_iter() and limit it where necessary.
Signed-off-by: Matthew Wilcox (Oracle) [off-list ref]
@@ -968,6 +968,9 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,structgfs2_sbd*sdp=GFS2_SB(inode);unsignedintblocks;+/* gfs2 does not support large folios yet */+if(len>PAGE_SIZE)+len=PAGE_SIZE;blocks=((pos&blockmask)+len+blockmask)>>inode->i_blkbits;returngfs2_trans_begin(sdp,RES_DINODE+blocks,0);}
The xfstests that Darrick identified as failing all passed. Running a
full sweep now; then I'll re-run with a 1kB filesystem to be sure that
still passes. Then I'll send another pull request.
After attempting the merge with Christoph's ill-timed refactoring,
I decided that eliding the use of 'bytes' here was the wrong approach,
because it very much needs to be put back in for the merge.
Here's the merge as I have it:
diff --cc fs/iomap/buffered-io.c
index f3176cf90351,d1aa0f0e7fd5..40356db3e856
From: Matthew Wilcox <willy@infradead.org> Date: 2021-12-13 18:08:53
On Sun, Dec 12, 2021 at 11:34:54PM -0800, Christoph Hellwig wrote:
On Fri, Dec 10, 2021 at 04:19:54PM +0000, Matthew Wilcox wrote:
quoted
After attempting the merge with Christoph's ill-timed refactoring,
I did give you a headsup before..
I thought that was going in via Darrick's tree. I had no idea Dan was
going to take it.
quoted
I decided that eliding the use of 'bytes' here was the wrong approach,
because it very much needs to be put back in for the merge.
Is there any good reason to not just delay the iomp_zero_iter folio
conversion for now?
It would hold up about half of the iomap folio conversion (~10 patches).
I don't understand what the benefit is of your patch series. Moving
filesystems away from being bdev based just doesn't seem interesting
to me. Having DAX as an optional feature that some bdevs have seems
like a far superior option.
This turned out to be buggy. Darrick and I figured out why his tests
were failing and mine weren't; this only shows up with a 4kB block
size filesystem and I was only testing with 1kB block size filesystems.
(at least on x86; I haven't figured out why it passes with 1kB block size
filesystems, so I'm not sure what would be true on other filesystems).
iomap_write_begin() is not prepared to deal with a length that spans a
page boundary. So I'm replacing this patch with the following patches
(whitespace damaged; pick them up from
https://git.infradead.org/users/willy/linux.git/tag/refs/tags/iomap-folio-5.17c
if you want to compile them):
commit 412212960b72
Author: Matthew Wilcox (Oracle) [off-list ref]
Date: Thu Dec 9 15:47:44 2021 -0500
iomap: Allow iomap_write_begin() to be called with the full length
In the future, we want write_begin to know the entire length of the
write so that it can choose to allocate large folios. Pass the full
length in from __iomap_zero_iter() and limit it where necessary.
Signed-off-by: Matthew Wilcox (Oracle) [off-list ref]
@@ -968,6 +968,9 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,structgfs2_sbd*sdp=GFS2_SB(inode);unsignedintblocks;+/* gfs2 does not support large folios yet */+if(len>PAGE_SIZE)+len=PAGE_SIZE;
This is awkward -- gfs2 doesn't set the mapping flag to indicate that it
supports large folios, so it should never be asked to deal with more
than a page at a time. Shouldn't iomap_write_begin clamp its len
argument to PAGE_SIZE at the start if the mapping doesn't have the large
folios flag set?
--D
The xfstests that Darrick identified as failing all passed. Running a
full sweep now; then I'll re-run with a 1kB filesystem to be sure that
still passes. Then I'll send another pull request.
From: Matthew Wilcox <willy@infradead.org> Date: 2021-12-16 20:43:59
On Thu, Dec 16, 2021 at 11:36:14AM -0800, Darrick J. Wong wrote:
quoted
+ /* gfs2 does not support large folios yet */
+ if (len > PAGE_SIZE)
+ len = PAGE_SIZE;
This is awkward -- gfs2 doesn't set the mapping flag to indicate that it
supports large folios, so it should never be asked to deal with more
than a page at a time. Shouldn't iomap_write_begin clamp its len
argument to PAGE_SIZE at the start if the mapping doesn't have the large
folios flag set?
You're right, this is awkward. And it's a bit of a beartrap for
another filesystem that wants to implement ->prepare_page in the
future.
@@ -968,9 +968,6 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,structgfs2_sbd*sdp=GFS2_SB(inode);unsignedintblocks;-/* gfs2 does not support large folios yet */-if(len>PAGE_SIZE)-len=PAGE_SIZE;blocks=((pos&blockmask)+len+blockmask)>>inode->i_blkbits;returngfs2_trans_begin(sdp,RES_DINODE+blocks,0);}