This is Jan's data=journal cleanup patch series, previously submitted
here[1] rebased on top of Linus's patches to address merge conflicts
with mm-stable, per this discussion[2].
[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/Y/[email protected]
While retesting this patch series, I've noticed a potential regression
which doesn't trigger before applying the last patch in this series
(Convert data=journal writeback to use ext4_writepages), but which
triggers a WARNING in generic/390 about half the time. I've gone back
and retested, and this was happening before the rebase.
Jan, could you take a look and (1) let me know what you think about my
patch conflict resolutions and (2) what you think about this warning
which is occasionally triggered by generic/390? Many thanks!
- Ted
generic/390 2s ... [00:08:04][ 2.708542] run fstests generic/390 at 2023-02-28 00:08:04
[ 2.871030] EXT4-fs: Warning: mounting with data=journal disables delayed allocation, dioread_nolock, O_DIRECT and fast_commit support!
[ 3.814748] ------------[ cut here ]------------
[ 3.816039] WARNING: CPU: 1 PID: 151 at fs/ext4/ext4_jbd2.c:75 ext4_journal_check_start+0x67/0xa0
[ 3.817902] CPU: 1 PID: 151 Comm: kworker/u4:3 Not tainted 6.2.0-xfstests-11603-g17b3ec378915 #953
[ 3.818285] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 3.818674] Workqueue: writeback wb_workfn (flush-254:32)
[ 3.818903] RIP: 0010:ext4_journal_check_start+0x67/0xa0
[ 3.819277] Code: 00 04 74 26 48 8b 90 28 02 00 00 31 c0 48 85 d2 74 07 8b 02 83 e0 02 75 15 5b c3 cc cc cc cc b8 fb ff ff ff 5b c3 cc cc cc cc <0f> 0b eb d6 44 8b 42 10 45 31 c9 68 60 57 59 82 48 89 df b9 01 00
[ 3.820069] RSP: 0018:ffffc900009f7930 EFLAGS: 00010246
[ 3.820301] RAX: ffff888009c65000 RBX: ffff888006ec1800 RCX: ffff88800665f000
[ 3.820605] RDX: 0000000000000000 RSI: 0000000000000044 RDI: ffffffff8259574c
[ 3.820908] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
[ 3.821213] R10: 0000000000000228 R11: 0000000000000000 R12: 0000000000000000
[ 3.821512] R13: 0000000000000002 R14: 000000000000097f R15: 0000000000000008
[ 3.821830] FS: 0000000000000000(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000
[ 3.822179] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3.822432] CR2: 00007f4ab2dce670 CR3: 0000000006764006 CR4: 0000000000770ee0
[ 3.822739] PKRU: 55555554
[ 3.822861] Call Trace:
[ 3.822972] <TASK>
[ 3.823070] __ext4_journal_start_sb+0x3f/0x190
[ 3.823267] mpage_prepare_extent_to_map+0x470/0x500
[ 3.823483] ext4_do_writepages+0x250/0x760
[ 3.823661] ext4_writepages+0x99/0x130
[ 3.823828] do_writepages+0xcf/0x1e0
[ 3.823988] ? fprop_fraction_percpu+0x2f/0x80
[ 3.824186] __writeback_single_inode+0x3d/0x280
[ 3.824389] writeback_sb_inodes+0x1ed/0x4b0
[ 3.824571] wb_writeback+0xdb/0x2f0
[ 3.824724] wb_do_writeback+0x87/0x2b0
[ 3.824890] ? set_worker_desc+0xc7/0xd0
[ 3.825060] wb_workfn+0x5f/0x260
[ 3.825205] ? ttwu_do_activate+0x83/0x1e0
[ 3.825382] ? _raw_spin_unlock_irqrestore+0xe/0x30
[ 3.825592] ? try_to_wake_up+0x275/0x480
[ 3.825774] process_one_work+0x1c3/0x3d0
[ 3.825949] worker_thread+0x51/0x3b0
[ 3.826108] ? __pfx_worker_thread+0x10/0x10
[ 3.826291] kthread+0xe7/0x110
[ 3.826430] ? __pfx_kthread+0x10/0x10
[ 3.826593] ret_from_fork+0x29/0x50
[ 3.826747] </TASK>
[ 3.826846] ---[ end trace 0000000000000000 ]---
[ 3.827056] ------------[ cut here ]------------
Jan Kara (7):
ext4: Update stale comment about write constraints
ext4: Use nr_to_write directly in mpage_prepare_extent_to_map()
ext4: Mark page for delayed dirtying only if it is pinned
ext4: Don't unlock page in ext4_bio_write_page()
ext4: Move page unlocking out of mpage_submit_page()
ext4: Move mpage_page_done() calls after error handling
ext4: Convert data=journal writeback to use ext4_writepages()
fs/ext4/inode.c | 410 +++++++++++-------------------------
fs/ext4/page-io.c | 10 +-
include/trace/events/ext4.h | 7 -
3 files changed, 126 insertions(+), 301 deletions(-)
--
2.31.0
From: Jan Kara <[email protected]>
In data=journal mode, page should be dirtied only when it has buffers
for checkpoint or it is writeably mapped. In the first case, we don't
need to do anything special. In the second case, page was already added
to the journal by ext4_page_mkwrite() and since transaction commit
writeprotects mapped pages again, page should be writeable (and thus
dirtied) only while it is part of the running transaction. So nothing
needs to be done either. The only special case is when someone pins the
page and uses this pin for modifying page data. So recognize this
special case and only then mark the page as having data that needs
adding to the journal.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 98dd50d7e72a..5c1d1b0bf5ee 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3669,24 +3669,26 @@ const struct iomap_ops ext4_iomap_report_ops = {
};
/*
- * Whenever the folio is being dirtied, corresponding buffers should already
- * be attached to the transaction (we take care of this in ext4_page_mkwrite()
- * and ext4_write_begin()). However we cannot move buffers to dirty transaction
- * lists here because ->dirty_folio is called under VFS locks and the folio
- * is not necessarily locked.
- *
- * We cannot just dirty the folio and leave attached buffers clean, because the
- * buffers' dirty state is "definitive". We cannot just set the buffers dirty
- * or jbddirty because all the journalling code will explode.
- *
- * So what we do is to mark the folio "pending dirty" and next time writepage
- * is called, propagate that into the buffers appropriately.
+ * For data=journal mode, folio should be marked dirty only when it was
+ * writeably mapped. When that happens, it was already attached to the
+ * transaction and marked as jbddirty (we take care of this in
+ * ext4_page_mkwrite()). On transaction commit, we writeprotect page mappings
+ * so we should have nothing to do here, except for the case when someone
+ * had the page pinned and dirtied the page through this pin (e.g. by doing
+ * direct IO to it). In that case we'd need to attach buffers here to the
+ * transaction but we cannot due to lock ordering. We cannot just dirty the
+ * folio and leave attached buffers clean, because the buffers' dirty state is
+ * "definitive". We cannot just set the buffers dirty or jbddirty because all
+ * the journalling code will explode. So what we do is to mark the folio
+ * "pending dirty" and next time ext4_writepages() is called, attach buffers
+ * to the transaction appropriately.
*/
static bool ext4_journalled_dirty_folio(struct address_space *mapping,
struct folio *folio)
{
WARN_ON_ONCE(!folio_buffers(folio));
- folio_set_checked(folio);
+ if (folio_maybe_dma_pinned(folio))
+ folio_set_checked(folio);
return filemap_dirty_folio(mapping, folio);
}
--
2.31.0
From: Jan Kara <[email protected]>
When looking up extent of pages to map in mpage_prepare_extent_to_map()
we count how many pages we still need to find in a copy of
wbc->nr_to_write counter. With more complex page handling for
data=journal mode, it will be easier to use wbc->nr_to_write directly so
that we don't forget to carry over changes back to nr_to_write counter.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 178978484acc..98dd50d7e72a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2580,7 +2580,6 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
struct address_space *mapping = mpd->inode->i_mapping;
struct folio_batch fbatch;
unsigned int nr_folios;
- long left = mpd->wbc->nr_to_write;
pgoff_t index = mpd->first_page;
pgoff_t end = mpd->last_page;
xa_mark_t tag;
@@ -2613,7 +2612,9 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
* newly appeared dirty pages, but have not synced all
* of the old dirty pages.
*/
- if (mpd->wbc->sync_mode == WB_SYNC_NONE && left <= 0)
+ if (mpd->wbc->sync_mode == WB_SYNC_NONE &&
+ mpd->wbc->nr_to_write <=
+ mpd->map.m_len >> (PAGE_SHIFT - blkbits))
goto out;
/* If we can't merge this page, we are done. */
@@ -2682,7 +2683,6 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
goto out;
err = 0;
}
- left -= folio_nr_pages(folio);
}
folio_batch_release(&fbatch);
cond_resched();
--
2.31.0
From: Jan Kara <[email protected]>
Do not unlock the written page in ext4_bio_write_page(). Instead leave
the page locked and unlock it in the callers. We'll need to keep the
page locked for data=journal writeback for a bit longer.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 2 ++
fs/ext4/page-io.c | 10 +++++-----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5c1d1b0bf5ee..27a11bed897e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2076,6 +2076,7 @@ static int ext4_writepage(struct page *page,
return -ENOMEM;
}
ret = ext4_bio_write_page(&io_submit, page, len);
+ unlock_page(page);
ext4_io_submit(&io_submit);
/* Drop io_end reference we got from init */
ext4_put_io_end_defer(io_submit.io_end);
@@ -2110,6 +2111,7 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
else
len = PAGE_SIZE;
err = ext4_bio_write_page(&mpd->io_submit, page, len);
+ unlock_page(page);
if (!err)
mpd->wbc->nr_to_write--;
mpd->first_page++;
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index beaec6d81074..3bc7c7c5b99d 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -500,7 +500,7 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
/* Nothing to submit? Just unlock the page... */
if (!nr_to_submit)
- goto unlock;
+ return 0;
bh = head = page_buffers(page);
@@ -548,7 +548,8 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
}
bh = bh->b_this_page;
} while (bh != head);
- goto unlock;
+
+ return ret;
}
}
@@ -564,7 +565,6 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
io_submit_add_bh(io, inode,
bounce_page ? bounce_page : page, bh);
} while ((bh = bh->b_this_page) != head);
-unlock:
- unlock_page(page);
- return ret;
+
+ return 0;
}
--
2.31.0
From: Jan Kara <[email protected]>
The comment above do_journal_get_write_access() is very stale. Most of
it just does not refer to what the function does today or how jbd2
works. The bit about transaction handling during write(2) is still
correct so just update the function names in that part and move the
comment to a more appropriate place.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 31 +++++++------------------------
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d251d705c276..178978484acc 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1004,30 +1004,6 @@ int ext4_walk_page_buffers(handle_t *handle, struct inode *inode,
return ret;
}
-/*
- * To preserve ordering, it is essential that the hole instantiation and
- * the data write be encapsulated in a single transaction. We cannot
- * close off a transaction and start a new one between the ext4_get_block()
- * and the commit_write(). So doing the jbd2_journal_start at the start of
- * prepare_write() is the right place.
- *
- * Also, this function can nest inside ext4_writepage(). In that case, we
- * *know* that ext4_writepage() has generated enough buffer credits to do the
- * whole page. So we won't block on the journal in that case, which is good,
- * because the caller may be PF_MEMALLOC.
- *
- * By accident, ext4 can be reentered when a transaction is open via
- * quota file writes. If we were to commit the transaction while thus
- * reentered, there can be a deadlock - we would be holding a quota
- * lock, and the commit would never complete if another thread had a
- * transaction open and was blocking on the quota lock - a ranking
- * violation.
- *
- * So what we do is to rely on the fact that jbd2_journal_stop/journal_start
- * will _not_ run commit under these circumstances because handle->h_ref
- * is elevated. We'll still have enough credits for the tiny quotafile
- * write.
- */
int do_journal_get_write_access(handle_t *handle, struct inode *inode,
struct buffer_head *bh)
{
@@ -1149,6 +1125,13 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
}
#endif
+/*
+ * To preserve ordering, it is essential that the hole instantiation and
+ * the data write be encapsulated in a single transaction. We cannot
+ * close off a transaction and start a new one between the ext4_get_block()
+ * and the ext4_write_end(). So doing the jbd2_journal_start at the start of
+ * ext4_write_begin() is the right place.
+ */
static int ext4_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len,
struct page **pagep, void **fsdata)
--
2.31.0
From: Jan Kara <[email protected]>
Move page unlocking during page writeback out of mpage_submit_page()
into the callers. This will allow writeback in data=journal mode to keep
the page locked for a bit longer. Since page unlocking it tightly
connected to increment of mpd->first_page (as that determines cleanup of
locked but unwritten pages), move page unlocking as well as
mpd->first_page handling into a helper function.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 27a11bed897e..a1fe5e3e7697 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2083,6 +2083,12 @@ static int ext4_writepage(struct page *page,
return ret;
}
+static void mpage_page_done(struct mpage_da_data *mpd, struct page *page)
+{
+ mpd->first_page++;
+ unlock_page(page);
+}
+
static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
{
int len;
@@ -2111,10 +2117,8 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)
else
len = PAGE_SIZE;
err = ext4_bio_write_page(&mpd->io_submit, page, len);
- unlock_page(page);
if (!err)
mpd->wbc->nr_to_write--;
- mpd->first_page++;
return err;
}
@@ -2226,6 +2230,7 @@ static int mpage_process_page_bufs(struct mpage_da_data *mpd,
/* So far everything mapped? Submit the page for IO. */
if (mpd->map.m_len == 0) {
err = mpage_submit_page(mpd, head->b_page);
+ mpage_page_done(mpd, head->b_page);
if (err < 0)
return err;
}
@@ -2357,6 +2362,7 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
goto out;
/* Page fully mapped - let IO run! */
err = mpage_submit_page(mpd, page);
+ mpage_page_done(mpd, page);
if (err < 0)
goto out;
}
@@ -2666,14 +2672,11 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
* modify metadata is simple. Just submit the page.
*/
if (!mpd->can_map) {
- if (ext4_page_nomap_can_writeout(&folio->page)) {
+ if (ext4_page_nomap_can_writeout(&folio->page))
err = mpage_submit_page(mpd, &folio->page);
- if (err < 0)
- goto out;
- } else {
- folio_unlock(folio);
- mpd->first_page += folio_nr_pages(folio);
- }
+ mpage_page_done(mpd, &folio->page);
+ if (err < 0)
+ goto out;
} else {
/* Add all dirty buffers to mpd */
lblk = ((ext4_lblk_t)folio->index) <<
--
2.31.0
From: Jan Kara <[email protected]>
In case mpage_submit_page() returns error, it doesn't really matter
whether we call mpage_page_done() and then return error or whether we
return directly because in that case page cleanup will be done by
mpage_release_unused_pages() instead. Logically, it makes more sense to
leave the cleanup to mpage_release_unused_pages() because we didn't
succeed in writing the page. So move mpage_page_done() calls after the
error handling.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a1fe5e3e7697..e6c453bf953e 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2230,9 +2230,9 @@ static int mpage_process_page_bufs(struct mpage_da_data *mpd,
/* So far everything mapped? Submit the page for IO. */
if (mpd->map.m_len == 0) {
err = mpage_submit_page(mpd, head->b_page);
- mpage_page_done(mpd, head->b_page);
if (err < 0)
return err;
+ mpage_page_done(mpd, head->b_page);
}
if (lblk >= blocks) {
mpd->scanned_until_end = 1;
@@ -2362,9 +2362,9 @@ static int mpage_map_and_submit_buffers(struct mpage_da_data *mpd)
goto out;
/* Page fully mapped - let IO run! */
err = mpage_submit_page(mpd, page);
- mpage_page_done(mpd, page);
if (err < 0)
goto out;
+ mpage_page_done(mpd, page);
}
folio_batch_release(&fbatch);
}
@@ -2672,11 +2672,12 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
* modify metadata is simple. Just submit the page.
*/
if (!mpd->can_map) {
- if (ext4_page_nomap_can_writeout(&folio->page))
+ if (ext4_page_nomap_can_writeout(&folio->page)) {
err = mpage_submit_page(mpd, &folio->page);
+ if (err < 0)
+ goto out;
+ }
mpage_page_done(mpd, &folio->page);
- if (err < 0)
- goto out;
} else {
/* Add all dirty buffers to mpd */
lblk = ((ext4_lblk_t)folio->index) <<
--
2.31.0
From: Jan Kara <[email protected]>
Add support for writeback of journalled data directly into
ext4_writepages() instead of offloading it to write_cache_pages(). This
actually significantly simplifies the code and reduces code duplication.
For checkpointing of committed data we can use ext4_writepages()
rightaway the same way as writeback of ordered data uses it on
transaction commit. For journalling of dirty mapped pages, we need to
add a special case to mpage_prepare_extent_to_map() to add all page
buffers to the journal.
Signed-off-by: Jan Kara <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/inode.c | 341 ++++++++++--------------------------
include/trace/events/ext4.h | 7 -
2 files changed, 91 insertions(+), 257 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e6c453bf953e..4a45d320fda8 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -136,7 +136,6 @@ static inline int ext4_begin_ordered_truncate(struct inode *inode,
new_size);
}
-static int __ext4_journalled_writepage(struct page *page, unsigned int len);
static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
int pextents);
@@ -1632,12 +1631,6 @@ static void ext4_print_free_blocks(struct inode *inode)
return;
}
-static int ext4_bh_delay_or_unwritten(handle_t *handle, struct inode *inode,
- struct buffer_head *bh)
-{
- return (buffer_delay(bh) || buffer_unwritten(bh)) && buffer_dirty(bh);
-}
-
/*
* ext4_insert_delayed_block - adds a delayed block to the extents status
* tree, incrementing the reserved cluster/block
@@ -1870,219 +1863,6 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
return 0;
}
-static int __ext4_journalled_writepage(struct page *page,
- unsigned int len)
-{
- struct address_space *mapping = page->mapping;
- struct inode *inode = mapping->host;
- handle_t *handle = NULL;
- int ret = 0, err = 0;
- int inline_data = ext4_has_inline_data(inode);
- struct buffer_head *inode_bh = NULL;
- loff_t size;
-
- ClearPageChecked(page);
-
- if (inline_data) {
- BUG_ON(page->index != 0);
- BUG_ON(len > ext4_get_max_inline_size(inode));
- inode_bh = ext4_journalled_write_inline_data(inode, len, page);
- if (inode_bh == NULL)
- goto out;
- }
- /*
- * We need to release the page lock before we start the
- * journal, so grab a reference so the page won't disappear
- * out from under us.
- */
- get_page(page);
- unlock_page(page);
-
- handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
- ext4_writepage_trans_blocks(inode));
- if (IS_ERR(handle)) {
- ret = PTR_ERR(handle);
- put_page(page);
- goto out_no_pagelock;
- }
- BUG_ON(!ext4_handle_valid(handle));
-
- lock_page(page);
- put_page(page);
- size = i_size_read(inode);
- if (page->mapping != mapping || page_offset(page) > size) {
- /* The page got truncated from under us */
- ext4_journal_stop(handle);
- ret = 0;
- goto out;
- }
-
- if (inline_data) {
- ret = ext4_mark_inode_dirty(handle, inode);
- } else {
- struct buffer_head *page_bufs = page_buffers(page);
-
- if (page->index == size >> PAGE_SHIFT)
- len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
-
- ret = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,
- NULL, do_journal_get_write_access);
-
- err = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,
- NULL, write_end_fn);
- }
- if (ret == 0)
- ret = err;
- err = ext4_jbd2_inode_add_write(handle, inode, page_offset(page), len);
- if (ret == 0)
- ret = err;
- EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
- err = ext4_journal_stop(handle);
- if (!ret)
- ret = err;
-
- ext4_set_inode_state(inode, EXT4_STATE_JDATA);
-out:
- unlock_page(page);
-out_no_pagelock:
- brelse(inode_bh);
- return ret;
-}
-
-/*
- * Note that we don't need to start a transaction unless we're journaling data
- * because we should have holes filled from ext4_page_mkwrite(). We even don't
- * need to file the inode to the transaction's list in ordered mode because if
- * we are writing back data added by write(), the inode is already there and if
- * we are writing back data modified via mmap(), no one guarantees in which
- * transaction the data will hit the disk. In case we are journaling data, we
- * cannot start transaction directly because transaction start ranks above page
- * lock so we have to do some magic.
- *
- * This function can get called via...
- * - ext4_writepages after taking page lock (have journal handle)
- * - journal_submit_inode_data_buffers (no journal handle)
- * - shrink_page_list via the kswapd/direct reclaim (no journal handle)
- * - grab_page_cache when doing write_begin (have journal handle)
- *
- * We don't do any block allocation in this function. If we have page with
- * multiple blocks we need to write those buffer_heads that are mapped. This
- * is important for mmaped based write. So if we do with blocksize 1K
- * truncate(f, 1024);
- * a = mmap(f, 0, 4096);
- * a[0] = 'a';
- * truncate(f, 4096);
- * we have in the page first buffer_head mapped via page_mkwrite call back
- * but other buffer_heads would be unmapped but dirty (dirty done via the
- * do_wp_page). So writepage should write the first block. If we modify
- * the mmap area beyond 1024 we will again get a page_fault and the
- * page_mkwrite callback will do the block allocation and mark the
- * buffer_heads mapped.
- *
- * We redirty the page if we have any buffer_heads that is either delay or
- * unwritten in the page.
- *
- * We can get recursively called as show below.
- *
- * ext4_writepage() -> kmalloc() -> __alloc_pages() -> page_launder() ->
- * ext4_writepage()
- *
- * But since we don't do any block allocation we should not deadlock.
- * Page also have the dirty flag cleared so we don't get recurive page_lock.
- */
-static int ext4_writepage(struct page *page,
- struct writeback_control *wbc)
-{
- struct folio *folio = page_folio(page);
- int ret = 0;
- loff_t size;
- unsigned int len;
- struct buffer_head *page_bufs = NULL;
- struct inode *inode = page->mapping->host;
- struct ext4_io_submit io_submit;
-
- if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
- folio_invalidate(folio, 0, folio_size(folio));
- folio_unlock(folio);
- return -EIO;
- }
-
- trace_ext4_writepage(page);
- size = i_size_read(inode);
- if (page->index == size >> PAGE_SHIFT &&
- !ext4_verity_in_progress(inode))
- len = size & ~PAGE_MASK;
- else
- len = PAGE_SIZE;
-
- /* Should never happen but for bugs in other kernel subsystems */
- if (!page_has_buffers(page)) {
- ext4_warning_inode(inode,
- "page %lu does not have buffers attached", page->index);
- ClearPageDirty(page);
- unlock_page(page);
- return 0;
- }
-
- page_bufs = page_buffers(page);
- /*
- * We cannot do block allocation or other extent handling in this
- * function. If there are buffers needing that, we have to redirty
- * the page. But we may reach here when we do a journal commit via
- * journal_submit_inode_data_buffers() and in that case we must write
- * allocated buffers to achieve data=ordered mode guarantees.
- *
- * Also, if there is only one buffer per page (the fs block
- * size == the page size), if one buffer needs block
- * allocation or needs to modify the extent tree to clear the
- * unwritten flag, we know that the page can't be written at
- * all, so we might as well refuse the write immediately.
- * Unfortunately if the block size != page size, we can't as
- * easily detect this case using ext4_walk_page_buffers(), but
- * for the extremely common case, this is an optimization that
- * skips a useless round trip through ext4_bio_write_page().
- */
- if (ext4_walk_page_buffers(NULL, inode, page_bufs, 0, len, NULL,
- ext4_bh_delay_or_unwritten)) {
- redirty_page_for_writepage(wbc, page);
- if ((current->flags & PF_MEMALLOC) ||
- (inode->i_sb->s_blocksize == PAGE_SIZE)) {
- /*
- * For memory cleaning there's no point in writing only
- * some buffers. So just bail out. Warn if we came here
- * from direct reclaim.
- */
- WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD))
- == PF_MEMALLOC);
- unlock_page(page);
- return 0;
- }
- }
-
- if (PageChecked(page) && ext4_should_journal_data(inode))
- /*
- * It's mmapped pagecache. Add buffers and journal it. There
- * doesn't seem much point in redirtying the page here.
- */
- return __ext4_journalled_writepage(page, len);
-
- ext4_io_submit_init(&io_submit, wbc);
- io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
- if (!io_submit.io_end) {
- redirty_page_for_writepage(wbc, page);
- unlock_page(page);
- return -ENOMEM;
- }
- ret = ext4_bio_write_page(&io_submit, page, len);
- unlock_page(page);
- ext4_io_submit(&io_submit);
- /* Drop io_end reference we got from init */
- ext4_put_io_end_defer(io_submit.io_end);
- return ret;
-}
-
static void mpage_page_done(struct mpage_da_data *mpd, struct page *page)
{
mpd->first_page++;
@@ -2563,6 +2343,50 @@ static bool ext4_page_nomap_can_writeout(struct page *page)
return false;
}
+static int ext4_journal_page_buffers(handle_t *handle, struct page *page,
+ int len)
+{
+ struct buffer_head *page_bufs = page_buffers(page);
+ struct inode *inode = page->mapping->host;
+ int ret, err;
+
+ ret = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,
+ NULL, do_journal_get_write_access);
+ err = ext4_walk_page_buffers(handle, inode, page_bufs, 0, len,
+ NULL, write_end_fn);
+ if (ret == 0)
+ ret = err;
+ err = ext4_jbd2_inode_add_write(handle, inode, page_offset(page), len);
+ if (ret == 0)
+ ret = err;
+ EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
+
+ ext4_set_inode_state(inode, EXT4_STATE_JDATA);
+
+ return ret;
+}
+
+static int mpage_journal_page_buffers(handle_t *handle,
+ struct mpage_da_data *mpd,
+ struct page *page)
+{
+ struct inode *inode = mpd->inode;
+ loff_t size = i_size_read(inode);
+ int len;
+
+ ClearPageChecked(page);
+ clear_page_dirty_for_io(page);
+ mpd->wbc->nr_to_write--;
+
+ if (page->index == size >> PAGE_SHIFT &&
+ !ext4_verity_in_progress(inode))
+ len = size & ~PAGE_MASK;
+ else
+ len = PAGE_SIZE;
+
+ return ext4_journal_page_buffers(handle, page, len);
+}
+
/*
* mpage_prepare_extent_to_map - find & lock contiguous range of dirty pages
* needing mapping, submit mapped pages
@@ -2595,11 +2419,20 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
int blkbits = mpd->inode->i_blkbits;
ext4_lblk_t lblk;
struct buffer_head *head;
+ handle_t *handle = NULL;
+ int bpp = ext4_journal_blocks_per_page(mpd->inode);
if (mpd->wbc->sync_mode == WB_SYNC_ALL || mpd->wbc->tagged_writepages)
tag = PAGECACHE_TAG_TOWRITE;
else
tag = PAGECACHE_TAG_DIRTY;
+
+ if (ext4_should_journal_data(mpd->inode)) {
+ handle = ext4_journal_start(mpd->inode, EXT4_HT_WRITE_PAGE,
+ bpp);
+ if (IS_ERR(handle))
+ return PTR_ERR(handle);
+ }
folio_batch_init(&fbatch);
mpd->map.m_len = 0;
mpd->next_page = index;
@@ -2629,6 +2462,13 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
if (mpd->map.m_len > 0 && mpd->next_page != folio->index)
goto out;
+ if (handle) {
+ err = ext4_journal_ensure_credits(handle, bpp,
+ 0);
+ if (err < 0)
+ goto out;
+ }
+
folio_lock(folio);
/*
* If the page is no longer dirty, or its mapping no
@@ -2668,8 +2508,15 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
mpd->first_page = folio->index;
mpd->next_page = folio->index + folio_nr_pages(folio);
/*
- * Writeout for transaction commit where we cannot
- * modify metadata is simple. Just submit the page.
+ * Writeout when we cannot modify metadata is simple.
+ * Just submit the page. For data=journal mode we
+ * first handle writeout of the page for checkpoint and
+ * only after that handle delayed page dirtying. This
+ * is crutial so that forcing a transaction commit and
+ * then calling filemap_write_and_wait() guarantees
+ * current state of data is in its final location. Such
+ * sequence is used for example by insert/collapse
+ * range operations before discarding the page cache.
*/
if (!mpd->can_map) {
if (ext4_page_nomap_can_writeout(&folio->page)) {
@@ -2677,6 +2524,13 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
if (err < 0)
goto out;
}
+ /* Pending dirtying of journalled data? */
+ if (PageChecked(&folio->page)) {
+ err = mpage_journal_page_buffers(handle,
+ mpd, &folio->page);
+ if (err < 0)
+ goto out;
+ }
mpage_page_done(mpd, &folio->page);
} else {
/* Add all dirty buffers to mpd */
@@ -2694,18 +2548,16 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
cond_resched();
}
mpd->scanned_until_end = 1;
+ if (handle)
+ ext4_journal_stop(handle);
return 0;
out:
folio_batch_release(&fbatch);
+ if (handle)
+ ext4_journal_stop(handle);
return err;
}
-static int ext4_writepage_cb(struct folio *folio, struct writeback_control *wbc,
- void *data)
-{
- return ext4_writepage(&folio->page, wbc);
-}
-
static int ext4_do_writepages(struct mpage_da_data *mpd)
{
struct writeback_control *wbc = mpd->wbc;
@@ -2731,13 +2583,6 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
if (!mapping->nrpages || !mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
goto out_writepages;
- if (ext4_should_journal_data(inode)) {
- blk_start_plug(&plug);
- ret = write_cache_pages(mapping, wbc, ext4_writepage_cb, NULL);
- blk_finish_plug(&plug);
- goto out_writepages;
- }
-
/*
* If the filesystem has aborted, it is read-only, so return
* right away instead of dumping stack traces later on that
@@ -2772,6 +2617,13 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
ext4_journal_stop(handle);
}
+ /*
+ * data=journal mode does not do delalloc so we just need to writeout /
+ * journal already mapped buffers
+ */
+ if (ext4_should_journal_data(inode))
+ mpd->can_map = 0;
+
if (ext4_should_dioread_nolock(inode)) {
/*
* We may need to convert up to one extent per block in
@@ -3148,9 +3000,8 @@ static int ext4_da_write_end(struct file *file,
* i_disksize since writeback will push i_disksize upto i_size
* eventually. If the end of the current write is > i_size and
* inside an allocated block (ext4_da_should_update_i_disksize()
- * check), we need to update i_disksize here as neither
- * ext4_writepage() nor certain ext4_writepages() paths not
- * allocating blocks update i_disksize.
+ * check), we need to update i_disksize here as certain
+ * ext4_writepages() paths not allocating blocks update i_disksize.
*
* Note that we defer inode dirtying to generic_write_end() /
* ext4_da_write_inline_data_end().
@@ -5371,7 +5222,7 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
* If the folio is fully truncated, we don't need to wait for any commit
* (and we even should not as __ext4_journalled_invalidate_folio() may
* strip all buffers from the folio but keep the folio dirty which can then
- * confuse e.g. concurrent ext4_writepage() seeing dirty folio without
+ * confuse e.g. concurrent ext4_writepages() seeing dirty folio without
* buffers). Also we don't need to wait for any commit if all buffers in
* the folio remain valid. This is most beneficial for the common case of
* blocksize == PAGESIZE.
@@ -6309,18 +6160,8 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
err = __block_write_begin(page, 0, len, ext4_get_block);
if (!err) {
ret = VM_FAULT_SIGBUS;
- if (ext4_walk_page_buffers(handle, inode,
- page_buffers(page), 0, len, NULL,
- do_journal_get_write_access))
- goto out_error;
- if (ext4_walk_page_buffers(handle, inode,
- page_buffers(page), 0, len, NULL,
- write_end_fn))
- goto out_error;
- if (ext4_jbd2_inode_add_write(handle, inode,
- page_offset(page), len))
+ if (ext4_journal_page_buffers(handle, page, len))
goto out_error;
- ext4_set_inode_state(inode, EXT4_STATE_JDATA);
} else {
unlock_page(page);
}
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 77b426ae0064..ebccf6a6aa1b 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -584,13 +584,6 @@ DECLARE_EVENT_CLASS(ext4__page_op,
(unsigned long) __entry->index)
);
-DEFINE_EVENT(ext4__page_op, ext4_writepage,
-
- TP_PROTO(struct page *page),
-
- TP_ARGS(page)
-);
-
DEFINE_EVENT(ext4__page_op, ext4_readpage,
TP_PROTO(struct page *page),
--
2.31.0
Hi Ted!
On Tue 28-02-23 00:13:12, Theodore Ts'o wrote:
> This is Jan's data=journal cleanup patch series, previously submitted
> here[1] rebased on top of Linus's patches to address merge conflicts
> with mm-stable, per this discussion[2].
>
> [1] https://lore.kernel.org/r/[email protected]
> [2] https://lore.kernel.org/r/Y/[email protected]
>
> While retesting this patch series, I've noticed a potential regression
> which doesn't trigger before applying the last patch in this series
> (Convert data=journal writeback to use ext4_writepages), but which
> triggers a WARNING in generic/390 about half the time. I've gone back
> and retested, and this was happening before the rebase.
>
> Jan, could you take a look and (1) let me know what you think about my
> patch conflict resolutions and (2) what you think about this warning
> which is occasionally triggered by generic/390? Many thanks!
I went through the patches and they look good to me. I have a side note
that obviously the code isn't quite going to work for folios larger than 1
page but for 1 page folios we should be fine.
Regarding the warning somehow there are dirty pages left after the
procedures freeze_super() goes through to flush all dirty data. That is not
too surprising since in data=journal mode pages get (as a surprise to
freezing code) dirtied when transaction commits. I thought we have this
covered by jbd2_journal_flush() call in ext4_freeze() but maybe there are
some stranded PageDirty bits left? It needs more debugging...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Feb 28, 2023 at 02:58:54PM +0100, Jan Kara wrote:
>
> Regarding the warning somehow there are dirty pages left after the
> procedures freeze_super() goes through to flush all dirty data. That is not
> too surprising since in data=journal mode pages get (as a surprise to
> freezing code) dirtied when transaction commits. I thought we have this
> covered by jbd2_journal_flush() call in ext4_freeze() but maybe there are
> some stranded PageDirty bits left? It needs more debugging...
So the question I have is whether this is a bug that was always there,
or perhaps code changes affected the timing, so that it was much more
likely to happen (although in my testing it's still only triggering
about 50% of the time).
This warning can mean that data can be lost, especially if someone was
doing hibernation, right? We can discuss this at today's ext4 video
conference, but I'm inclined to wait until the next merge cycle for
this patch series until we can figure out what's going on. Jan, what
do you think?
- Ted
On Thu 02-03-23 08:37:35, Theodore Ts'o wrote:
> On Tue, Feb 28, 2023 at 02:58:54PM +0100, Jan Kara wrote:
> >
> > Regarding the warning somehow there are dirty pages left after the
> > procedures freeze_super() goes through to flush all dirty data. That is not
> > too surprising since in data=journal mode pages get (as a surprise to
> > freezing code) dirtied when transaction commits. I thought we have this
> > covered by jbd2_journal_flush() call in ext4_freeze() but maybe there are
> > some stranded PageDirty bits left? It needs more debugging...
>
> So the question I have is whether this is a bug that was always there,
> or perhaps code changes affected the timing, so that it was much more
> likely to happen (although in my testing it's still only triggering
> about 50% of the time).
>
> This warning can mean that data can be lost, especially if someone was
> doing hibernation, right? We can discuss this at today's ext4 video
> conference, but I'm inclined to wait until the next merge cycle for
> this patch series until we can figure out what's going on. Jan, what
> do you think?
As we discussed in the meeting, I'll have a look into the failure. For now
I'd keep patches on hold as it is not urgent patchset.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri 03-03-23 10:29:39, Jan Kara wrote:
> On Thu 02-03-23 08:37:35, Theodore Ts'o wrote:
> > On Tue, Feb 28, 2023 at 02:58:54PM +0100, Jan Kara wrote:
> > >
> > > Regarding the warning somehow there are dirty pages left after the
> > > procedures freeze_super() goes through to flush all dirty data. That is not
> > > too surprising since in data=journal mode pages get (as a surprise to
> > > freezing code) dirtied when transaction commits. I thought we have this
> > > covered by jbd2_journal_flush() call in ext4_freeze() but maybe there are
> > > some stranded PageDirty bits left? It needs more debugging...
> >
> > So the question I have is whether this is a bug that was always there,
> > or perhaps code changes affected the timing, so that it was much more
> > likely to happen (although in my testing it's still only triggering
> > about 50% of the time).
> >
> > This warning can mean that data can be lost, especially if someone was
> > doing hibernation, right? We can discuss this at today's ext4 video
> > conference, but I'm inclined to wait until the next merge cycle for
> > this patch series until we can figure out what's going on. Jan, what
> > do you think?
>
> As we discussed in the meeting, I'll have a look into the failure. For now
> I'd keep patches on hold as it is not urgent patchset.
FWIW I'm able to reproduce the issue and also as I'm looking into the
freeze to it was always there in principle. But my patches likely made it
much more probable. I'll have a look what I can do...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri 03-03-23 11:38:06, Jan Kara wrote:
> On Fri 03-03-23 10:29:39, Jan Kara wrote:
> > On Thu 02-03-23 08:37:35, Theodore Ts'o wrote:
> > > On Tue, Feb 28, 2023 at 02:58:54PM +0100, Jan Kara wrote:
> > > >
> > > > Regarding the warning somehow there are dirty pages left after the
> > > > procedures freeze_super() goes through to flush all dirty data. That is not
> > > > too surprising since in data=journal mode pages get (as a surprise to
> > > > freezing code) dirtied when transaction commits. I thought we have this
> > > > covered by jbd2_journal_flush() call in ext4_freeze() but maybe there are
> > > > some stranded PageDirty bits left? It needs more debugging...
> > >
> > > So the question I have is whether this is a bug that was always there,
> > > or perhaps code changes affected the timing, so that it was much more
> > > likely to happen (although in my testing it's still only triggering
> > > about 50% of the time).
> > >
> > > This warning can mean that data can be lost, especially if someone was
> > > doing hibernation, right? We can discuss this at today's ext4 video
> > > conference, but I'm inclined to wait until the next merge cycle for
> > > this patch series until we can figure out what's going on. Jan, what
> > > do you think?
> >
> > As we discussed in the meeting, I'll have a look into the failure. For now
> > I'd keep patches on hold as it is not urgent patchset.
>
> FWIW I'm able to reproduce the issue and also as I'm looking into the
> freeze to it was always there in principle. But my patches likely made it
> much more probable. I'll have a look what I can do...
So in the end this was mostly a cosmetic issue (jbd2 checkpointing code
leaving dirty bits on pages whose buffer heads were already written). Patch
submitted.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, 28 Feb 2023 00:13:12 -0500, Theodore Ts'o wrote:
> This is Jan's data=journal cleanup patch series, previously submitted
> here[1] rebased on top of Linus's patches to address merge conflicts
> with mm-stable, per this discussion[2].
>
> [1] https://lore.kernel.org/r/[email protected]
> [2] https://lore.kernel.org/r/Y/[email protected]
>
> [...]
Applied, thanks!
[1/7] ext4: Update stale comment about write constraints
commit: e7a2d7ab32fd5bfe431d69df56eedf74c747dfa1
[2/7] ext4: Use nr_to_write directly in mpage_prepare_extent_to_map()
commit: 6e6213b505709902199d69f56f64bba3dbd867ff
[3/7] ext4: Mark page for delayed dirtying only if it is pinned
commit: 90f1929e5e5af0467572c81e6ac78ae20316f439
[4/7] ext4: Don't unlock page in ext4_bio_write_page()
commit: 2c892bdd69ce89e534c915e84c3e4ba98de9ef6c
[5/7] ext4: Move page unlocking out of mpage_submit_page()
commit: 22e00e971f55a0b7b375d937db614863e1a2e500
[6/7] ext4: Move mpage_page_done() calls after error handling
commit: 47c6f573b4ef9bf68ce45bd4e4c35ef56d326fe1
[7/7] ext4: Convert data=journal writeback to use ext4_writepages()
commit: f7233fb54d18b45b42d8f6ad6a95bd8641114c36
Best regards,
--
Theodore Ts'o <[email protected]>