These patches are carefully plucked from the THP series. I would like
them to hit 5.10 to make the THP patchset merge easier. Some of these
are just generic improvements that make sense on their own terms, but
the overall intent is to support THPs in iomap.
I'll send another patch series later today which are the changes to
iomap which don't pay their own way until we actually have THPs in the
page cache. I would like those to be reviewed with an eye to merging
them into 5.11.
Matthew Wilcox (Oracle) (9):
iomap: Fix misplaced page flushing
fs: Introduce i_blocks_per_page
iomap: Use kzalloc to allocate iomap_page
iomap: Use bitmap ops to set uptodate bits
iomap: Support arbitrarily many blocks per page
iomap: Convert read_count to byte count
iomap: Convert write_count to byte count
iomap: Convert iomap_write_end types
iomap: Change calling convention for zeroing
fs/dax.c | 13 ++--
fs/iomap/buffered-io.c | 145 ++++++++++++++++------------------------
fs/jfs/jfs_metapage.c | 2 +-
fs/xfs/xfs_aops.c | 2 +-
include/linux/dax.h | 3 +-
include/linux/pagemap.h | 16 +++++
6 files changed, 83 insertions(+), 98 deletions(-)
--
2.28.0
If iomap_unshare_actor() unshares to an inline iomap, the page was
not being flushed. block_write_end() and __iomap_write_end() already
contain flushes, so adding it to iomap_write_end_inline() seems like
the best place. That means we can remove it from iomap_write_actor().
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/iomap/buffered-io.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index bcfc288dba3f..cffd575e57b6 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -715,6 +715,7 @@ iomap_write_end_inline(struct inode *inode, struct page *page,
{
void *addr;
+ flush_dcache_page(page);
WARN_ON_ONCE(!PageUptodate(page));
BUG_ON(pos + copied > PAGE_SIZE - offset_in_page(iomap->inline_data));
@@ -811,8 +812,6 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
- flush_dcache_page(page);
-
status = iomap_write_end(inode, pos, bytes, copied, page, iomap,
srcmap);
if (unlikely(status < 0))
--
2.28.0
Instead of counting bio segments, count the number of bytes submitted.
This insulates us from the block layer's definition of what a 'same page'
is, which is not necessarily clear once THPs are involved.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/iomap/buffered-io.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index c9b44f86d166..8c6187a6f34e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1050,7 +1050,7 @@ EXPORT_SYMBOL_GPL(iomap_page_mkwrite);
static void
iomap_finish_page_writeback(struct inode *inode, struct page *page,
- int error)
+ int error, unsigned int len)
{
struct iomap_page *iop = to_iomap_page(page);
@@ -1062,7 +1062,7 @@ iomap_finish_page_writeback(struct inode *inode, struct page *page,
WARN_ON_ONCE(i_blocks_per_page(inode, page) > 1 && !iop);
WARN_ON_ONCE(iop && atomic_read(&iop->write_count) <= 0);
- if (!iop || atomic_dec_and_test(&iop->write_count))
+ if (!iop || atomic_sub_and_test(len, &iop->write_count))
end_page_writeback(page);
}
@@ -1096,7 +1096,8 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error)
/* 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);
+ iomap_finish_page_writeback(inode, bv->bv_page, error,
+ bv->bv_len);
bio_put(bio);
}
/* The ioend has been freed by bio_put() */
@@ -1312,8 +1313,8 @@ iomap_add_to_ioend(struct inode *inode, loff_t offset, struct page *page,
merged = __bio_try_merge_page(wpc->ioend->io_bio, page, len, poff,
&same_page);
- if (iop && !same_page)
- atomic_inc(&iop->write_count);
+ if (iop)
+ atomic_add(len, &iop->write_count);
if (!merged) {
if (bio_full(wpc->ioend->io_bio, len)) {
--
2.28.0
We can skip most of the initialisation, although spinlocks still
need explicit initialisation as architectures may use a non-zero
value to indicate unlocked. The comment is no longer useful as
attach_page_private() handles the refcount now.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/iomap/buffered-io.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 13d5cdab8dcd..639d54a4177e 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -49,16 +49,8 @@ iomap_page_create(struct inode *inode, struct page *page)
if (iop || i_blocks_per_page(inode, page) <= 1)
return iop;
- iop = kmalloc(sizeof(*iop), GFP_NOFS | __GFP_NOFAIL);
- atomic_set(&iop->read_count, 0);
- atomic_set(&iop->write_count, 0);
+ iop = kzalloc(sizeof(*iop), GFP_NOFS | __GFP_NOFAIL);
spin_lock_init(&iop->uptodate_lock);
- bitmap_zero(iop->uptodate, PAGE_SIZE / SECTOR_SIZE);
-
- /*
- * migrate_page_move_mapping() assumes that pages with private data have
- * their count elevated by 1.
- */
attach_page_private(page, iop);
return iop;
}
--
2.28.0
On Mon, Aug 24, 2020 at 03:55:02PM +0100, Matthew Wilcox (Oracle) wrote:
> If iomap_unshare_actor() unshares to an inline iomap, the page was
> not being flushed. block_write_end() and __iomap_write_end() already
> contain flushes, so adding it to iomap_write_end_inline() seems like
> the best place. That means we can remove it from iomap_write_actor().
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> fs/iomap/buffered-io.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
looks good.
Reviewed-by: Dave Chinner <[email protected]>
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Mon, Aug 24, 2020 at 03:55:04PM +0100, Matthew Wilcox (Oracle) wrote:
> We can skip most of the initialisation, although spinlocks still
> need explicit initialisation as architectures may use a non-zero
> value to indicate unlocked. The comment is no longer useful as
> attach_page_private() handles the refcount now.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> fs/iomap/buffered-io.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
The sooner this goes in the better :)
Reviewed-by: Dave Chinner <[email protected]>
--
Dave Chinner
[email protected]
On Mon, Aug 24, 2020 at 03:55:02PM +0100, Matthew Wilcox (Oracle) wrote:
> If iomap_unshare_actor() unshares to an inline iomap, the page was
> not being flushed. block_write_end() and __iomap_write_end() already
> contain flushes, so adding it to iomap_write_end_inline() seems like
> the best place. That means we can remove it from iomap_write_actor().
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Seems reasonable to me...
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/iomap/buffered-io.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index bcfc288dba3f..cffd575e57b6 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -715,6 +715,7 @@ iomap_write_end_inline(struct inode *inode, struct page *page,
> {
> void *addr;
>
> + flush_dcache_page(page);
> WARN_ON_ONCE(!PageUptodate(page));
> BUG_ON(pos + copied > PAGE_SIZE - offset_in_page(iomap->inline_data));
>
> @@ -811,8 +812,6 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>
> copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
>
> - flush_dcache_page(page);
> -
> status = iomap_write_end(inode, pos, bytes, copied, page, iomap,
> srcmap);
> if (unlikely(status < 0))
> --
> 2.28.0
>
On Mon, Aug 24, 2020 at 03:55:04PM +0100, Matthew Wilcox (Oracle) wrote:
> We can skip most of the initialisation, although spinlocks still
> need explicit initialisation as architectures may use a non-zero
> value to indicate unlocked. The comment is no longer useful as
> attach_page_private() handles the refcount now.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
Looks good to me,
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> ---
> fs/iomap/buffered-io.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 13d5cdab8dcd..639d54a4177e 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -49,16 +49,8 @@ iomap_page_create(struct inode *inode, struct page *page)
> if (iop || i_blocks_per_page(inode, page) <= 1)
> return iop;
>
> - iop = kmalloc(sizeof(*iop), GFP_NOFS | __GFP_NOFAIL);
> - atomic_set(&iop->read_count, 0);
> - atomic_set(&iop->write_count, 0);
> + iop = kzalloc(sizeof(*iop), GFP_NOFS | __GFP_NOFAIL);
> spin_lock_init(&iop->uptodate_lock);
> - bitmap_zero(iop->uptodate, PAGE_SIZE / SECTOR_SIZE);
> -
> - /*
> - * migrate_page_move_mapping() assumes that pages with private data have
> - * their count elevated by 1.
> - */
> attach_page_private(page, iop);
> return iop;
> }
> --
> 2.28.0
>
On Mon, Aug 24, 2020 at 03:55:02PM +0100, Matthew Wilcox (Oracle) wrote:
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index bcfc288dba3f..cffd575e57b6 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -715,6 +715,7 @@ iomap_write_end_inline(struct inode *inode, struct page *page,
> {
> void *addr;
>
> + flush_dcache_page(page);
> WARN_ON_ONCE(!PageUptodate(page));
> BUG_ON(pos + copied > PAGE_SIZE - offset_in_page(iomap->inline_data));
Please move the call down below the asserts.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Mon, Aug 24, 2020 at 03:55:08PM +0100, Matthew Wilcox (Oracle) wrote:
> Instead of counting bio segments, count the number of bytes submitted.
> This insulates us from the block layer's definition of what a 'same page'
> is, which is not necessarily clear once THPs are involved.
Looks good (module the field naming as comment on the previous patch):
Reviewed-by: Christoph Hellwig <[email protected]>