Here's an updated version of this patch queue. Changes since v5 [*]:
* A new iomap-internal __iomap_get_folio() helper was added.
* The previous iomap-internal iomap_put_folio() helper was renamed to
__iomap_put_folio() to mirror __iomap_get_folio().
* The comment describing struct iomap_folio_ops was still referring to
pages instead of folios in two places.
Is this good enough for iomap-for-next now, please?
Thanks,
Andreas
[*] https://lore.kernel.org/linux-xfs/[email protected]/
Andreas Gruenbacher (10):
iomap: Add __iomap_put_folio helper
iomap/gfs2: Unlock and put folio in page_done handler
iomap: Rename page_done handler to put_folio
iomap: Add iomap_get_folio helper
iomap/gfs2: Get page in page_prepare handler
iomap: Add __iomap_get_folio helper
iomap: Rename page_prepare handler to get_folio
iomap/xfs: Eliminate the iomap_valid handler
iomap: Rename page_ops to folio_ops
xfs: Make xfs_iomap_folio_ops static
fs/gfs2/bmap.c | 38 ++++++++++-----
fs/iomap/buffered-io.c | 105 +++++++++++++++++++++++------------------
fs/xfs/xfs_iomap.c | 41 +++++++++++-----
include/linux/iomap.h | 50 +++++++++-----------
4 files changed, 134 insertions(+), 100 deletions(-)
--
2.38.1
The ->page_done() handler in struct iomap_page_ops is now somewhat
misnamed in that it mainly deals with unlocking and putting a folio, so
rename it to ->put_folio().
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/gfs2/bmap.c | 4 ++--
fs/iomap/buffered-io.c | 4 ++--
include/linux/iomap.h | 12 ++++++------
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 46206286ad42..0c041459677b 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -967,7 +967,7 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
return gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
}
-static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
+static void gfs2_iomap_put_folio(struct inode *inode, loff_t pos,
unsigned copied, struct folio *folio)
{
struct gfs2_trans *tr = current->journal_info;
@@ -994,7 +994,7 @@ static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
static const struct iomap_page_ops gfs2_iomap_page_ops = {
.page_prepare = gfs2_iomap_page_prepare,
- .page_done = gfs2_iomap_page_done,
+ .put_folio = gfs2_iomap_put_folio,
};
static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index a9082078e4ed..d4b444e44861 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -580,8 +580,8 @@ static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
{
const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
- if (page_ops && page_ops->page_done) {
- page_ops->page_done(iter->inode, pos, ret, folio);
+ if (page_ops && page_ops->put_folio) {
+ page_ops->put_folio(iter->inode, pos, ret, folio);
} else if (folio) {
folio_unlock(folio);
folio_put(folio);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 743e2a909162..ecf815b34d51 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -126,18 +126,18 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
/*
* When a filesystem sets page_ops in an iomap mapping it returns, page_prepare
- * and page_done will be called for each page written to. This only applies to
- * buffered writes as unbuffered writes will not typically have pages
+ * and put_folio will be called for each folio written to. This only applies
+ * to buffered writes as unbuffered writes will not typically have folios
* associated with them.
*
- * When page_prepare succeeds, page_done will always be called to do any
- * cleanup work necessary. In that page_done call, @folio will be NULL if the
- * associated folio could not be obtained. When folio is not NULL, page_done
+ * When page_prepare succeeds, put_folio will always be called to do any
+ * cleanup work necessary. In that put_folio call, @folio will be NULL if the
+ * associated folio could not be obtained. When folio is not NULL, put_folio
* is responsible for unlocking and putting the folio.
*/
struct iomap_page_ops {
int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len);
- void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
+ void (*put_folio)(struct inode *inode, loff_t pos, unsigned copied,
struct folio *folio);
/*
--
2.38.1
Add an __iomap_put_folio() helper to encapsulate unlocking the folio,
calling ->page_done(), and putting the folio. Use the new helper in
iomap_write_begin() and iomap_write_end().
This effectively doesn't change the way the code works, but prepares for
successive improvements.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/iomap/buffered-io.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 356193e44cf0..c045689b6af8 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -575,6 +575,19 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
return 0;
}
+static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
+ struct folio *folio)
+{
+ const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
+
+ if (folio)
+ folio_unlock(folio);
+ if (page_ops && page_ops->page_done)
+ page_ops->page_done(iter->inode, pos, ret, &folio->page);
+ if (folio)
+ folio_put(folio);
+}
+
static int iomap_write_begin_inline(const struct iomap_iter *iter,
struct folio *folio)
{
@@ -616,7 +629,8 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
fgp, mapping_gfp_mask(iter->inode->i_mapping));
if (!folio) {
status = (iter->flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOMEM;
- goto out_no_page;
+ __iomap_put_folio(iter, pos, 0, NULL);
+ return status;
}
/*
@@ -656,13 +670,9 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
return 0;
out_unlock:
- folio_unlock(folio);
- folio_put(folio);
+ __iomap_put_folio(iter, pos, 0, folio);
iomap_write_failed(iter->inode, pos, len);
-out_no_page:
- if (page_ops && page_ops->page_done)
- page_ops->page_done(iter->inode, pos, 0, NULL);
return status;
}
@@ -712,7 +722,6 @@ static size_t iomap_write_end_inline(const struct iomap_iter *iter,
static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
size_t copied, struct folio *folio)
{
- const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
const struct iomap *srcmap = iomap_iter_srcmap(iter);
loff_t old_size = iter->inode->i_size;
size_t ret;
@@ -735,14 +744,10 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
i_size_write(iter->inode, pos + ret);
iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
}
- folio_unlock(folio);
+ __iomap_put_folio(iter, pos, ret, folio);
if (old_size < pos)
pagecache_isize_extended(iter->inode, old_size, pos);
- if (page_ops && page_ops->page_done)
- page_ops->page_done(iter->inode, pos, ret, &folio->page);
- folio_put(folio);
-
if (ret < len)
iomap_write_failed(iter->inode, pos + ret, len - ret);
return ret;
--
2.38.1
Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
handler and validating the mapping there.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/iomap/buffered-io.c | 26 +++++---------------------
fs/xfs/xfs_iomap.c | 37 ++++++++++++++++++++++++++-----------
include/linux/iomap.h | 23 ++++++-----------------
3 files changed, 37 insertions(+), 49 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 006ddf933948..72dfbc3cb086 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -638,10 +638,9 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
size_t len, struct folio **foliop)
{
- const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
const struct iomap *srcmap = iomap_iter_srcmap(iter);
struct folio *folio;
- int status = 0;
+ int status;
BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
if (srcmap != &iter->iomap)
@@ -654,27 +653,12 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
folio = __iomap_get_folio(iter, pos, len);
- if (IS_ERR(folio))
- return PTR_ERR(folio);
-
- /*
- * Now we have a locked folio, before we do anything with it we need to
- * check that the iomap we have cached is not stale. The inode extent
- * mapping can change due to concurrent IO in flight (e.g.
- * IOMAP_UNWRITTEN state can change and memory reclaim could have
- * reclaimed a previously partially written page at this index after IO
- * completion before this write reaches this file offset) and hence we
- * could do the wrong thing here (zero a page range incorrectly or fail
- * to zero) and corrupt data.
- */
- if (page_ops && page_ops->iomap_valid) {
- bool iomap_valid = page_ops->iomap_valid(iter->inode,
- &iter->iomap);
- if (!iomap_valid) {
+ if (IS_ERR(folio)) {
+ if (folio == ERR_PTR(-ESTALE)) {
iter->iomap.flags |= IOMAP_F_STALE;
- status = 0;
- goto out_unlock;
+ return 0;
}
+ return PTR_ERR(folio);
}
if (pos + len > folio_pos(folio) + folio_size(folio))
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 669c1bc5c3a7..d0bf99539180 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -62,29 +62,44 @@ xfs_iomap_inode_sequence(
return cookie | READ_ONCE(ip->i_df.if_seq);
}
-/*
- * Check that the iomap passed to us is still valid for the given offset and
- * length.
- */
-static bool
-xfs_iomap_valid(
- struct inode *inode,
- const struct iomap *iomap)
+static struct folio *
+xfs_get_folio(
+ struct iomap_iter *iter,
+ loff_t pos,
+ unsigned len)
{
+ struct inode *inode = iter->inode;
+ struct iomap *iomap = &iter->iomap;
struct xfs_inode *ip = XFS_I(inode);
+ struct folio *folio;
+ folio = iomap_get_folio(iter, pos);
+ if (IS_ERR(folio))
+ return folio;
+
+ /*
+ * Now that we have a locked folio, we need to check that the iomap we
+ * have cached is not stale. The inode extent mapping can change due to
+ * concurrent IO in flight (e.g., IOMAP_UNWRITTEN state can change and
+ * memory reclaim could have reclaimed a previously partially written
+ * page at this index after IO completion before this write reaches
+ * this file offset) and hence we could do the wrong thing here (zero a
+ * page range incorrectly or fail to zero) and corrupt data.
+ */
if (iomap->validity_cookie !=
xfs_iomap_inode_sequence(ip, iomap->flags)) {
trace_xfs_iomap_invalid(ip, iomap);
- return false;
+ folio_unlock(folio);
+ folio_put(folio);
+ return ERR_PTR(-ESTALE);
}
XFS_ERRORTAG_DELAY(ip->i_mount, XFS_ERRTAG_WRITE_DELAY_MS);
- return true;
+ return folio;
}
const struct iomap_page_ops xfs_iomap_page_ops = {
- .iomap_valid = xfs_iomap_valid,
+ .get_folio = xfs_get_folio,
};
int
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index da226032aedc..0ae2cddbedd6 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -134,29 +134,18 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
* When get_folio succeeds, put_folio will always be called to do any
* cleanup work necessary. put_folio is responsible for unlocking and putting
* @folio.
+ *
+ * When an iomap is created, the filesystem can store internal state (e.g., a
+ * sequence number) in iomap->validity_cookie. The get_folio handler can use
+ * this validity cookie to detect when the iomap needs to be refreshed because
+ * it is no longer up to date. In that case, the function should return
+ * ERR_PTR(-ESTALE) to retry the operation with a fresh mapping.
*/
struct iomap_page_ops {
struct folio *(*get_folio)(struct iomap_iter *iter, loff_t pos,
unsigned len);
void (*put_folio)(struct inode *inode, loff_t pos, unsigned copied,
struct folio *folio);
-
- /*
- * Check that the cached iomap still maps correctly to the filesystem's
- * internal extent map. FS internal extent maps can change while iomap
- * is iterating a cached iomap, so this hook allows iomap to detect that
- * the iomap needs to be refreshed during a long running write
- * operation.
- *
- * The filesystem can store internal state (e.g. a sequence number) in
- * iomap->validity_cookie when the iomap is first mapped to be able to
- * detect changes between mapping time and whenever .iomap_valid() is
- * called.
- *
- * This is called with the folio over the specified file position held
- * locked by the iomap code.
- */
- bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap);
};
/*
--
2.38.1
The ->page_prepare() handler in struct iomap_page_ops is now somewhat
misnamed, so rename it to ->get_folio().
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/gfs2/bmap.c | 6 +++---
fs/iomap/buffered-io.c | 4 ++--
include/linux/iomap.h | 6 +++---
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 41349e09558b..d3adb715ac8c 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -957,7 +957,7 @@ static int __gfs2_iomap_get(struct inode *inode, loff_t pos, loff_t length,
}
static struct folio *
-gfs2_iomap_page_prepare(struct iomap_iter *iter, loff_t pos, unsigned len)
+gfs2_iomap_get_folio(struct iomap_iter *iter, loff_t pos, unsigned len)
{
struct inode *inode = iter->inode;
unsigned int blockmask = i_blocksize(inode) - 1;
@@ -998,7 +998,7 @@ static void gfs2_iomap_put_folio(struct inode *inode, loff_t pos,
}
static const struct iomap_page_ops gfs2_iomap_page_ops = {
- .page_prepare = gfs2_iomap_page_prepare,
+ .get_folio = gfs2_iomap_get_folio,
.put_folio = gfs2_iomap_put_folio,
};
@@ -1291,7 +1291,7 @@ int gfs2_alloc_extent(struct inode *inode, u64 lblock, u64 *dblock,
/*
* NOTE: Never call gfs2_block_zero_range with an open transaction because it
* uses iomap write to perform its actions, which begin their own transactions
- * (iomap_begin, page_prepare, etc.)
+ * (iomap_begin, get_folio, etc.)
*/
static int gfs2_block_zero_range(struct inode *inode, loff_t from,
unsigned int length)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 666107c3a385..006ddf933948 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -607,8 +607,8 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
{
const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
- if (page_ops && page_ops->page_prepare)
- return page_ops->page_prepare(iter, pos, len);
+ if (page_ops && page_ops->get_folio)
+ return page_ops->get_folio(iter, pos, len);
else
return iomap_get_folio(iter, pos);
}
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index d50501781856..da226032aedc 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -126,17 +126,17 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
}
/*
- * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare
+ * When a filesystem sets page_ops in an iomap mapping it returns, get_folio
* and put_folio will be called for each folio written to. This only applies
* to buffered writes as unbuffered writes will not typically have folios
* associated with them.
*
- * When page_prepare succeeds, put_folio will always be called to do any
+ * When get_folio succeeds, put_folio will always be called to do any
* cleanup work necessary. put_folio is responsible for unlocking and putting
* @folio.
*/
struct iomap_page_ops {
- struct folio *(*page_prepare)(struct iomap_iter *iter, loff_t pos,
+ struct folio *(*get_folio)(struct iomap_iter *iter, loff_t pos,
unsigned len);
void (*put_folio)(struct inode *inode, loff_t pos, unsigned copied,
struct folio *folio);
--
2.38.1
When an iomap defines a ->page_done() handler in its page_ops, delegate
unlocking the folio and putting the folio reference to that handler.
This allows to fix a race between journaled data writes and folio
writeback in gfs2: before this change, gfs2_iomap_page_done() was called
after unlocking the folio, so writeback could start writing back the
folio's buffers before they could be marked for writing to the journal.
Also, try_to_free_buffers() could free the buffers before
gfs2_iomap_page_done() was done adding the buffers to the current
current transaction. With this change, gfs2_iomap_page_done() adds the
buffers to the current transaction while the folio is still locked, so
the problems described above can no longer occur.
The only current user of ->page_done() is gfs2, so other filesystems are
not affected. To catch out any out-of-tree users, switch from a page to
a folio in ->page_done().
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/gfs2/bmap.c | 15 ++++++++++++---
fs/iomap/buffered-io.c | 8 ++++----
include/linux/iomap.h | 7 ++++---
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index e7537fd305dd..46206286ad42 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -968,14 +968,23 @@ static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
}
static void gfs2_iomap_page_done(struct inode *inode, loff_t pos,
- unsigned copied, struct page *page)
+ unsigned copied, struct folio *folio)
{
struct gfs2_trans *tr = current->journal_info;
struct gfs2_inode *ip = GFS2_I(inode);
struct gfs2_sbd *sdp = GFS2_SB(inode);
- if (page && !gfs2_is_stuffed(ip))
- gfs2_page_add_databufs(ip, page, offset_in_page(pos), copied);
+ if (!folio) {
+ gfs2_trans_end(sdp);
+ return;
+ }
+
+ if (!gfs2_is_stuffed(ip))
+ gfs2_page_add_databufs(ip, &folio->page, offset_in_page(pos),
+ copied);
+
+ folio_unlock(folio);
+ folio_put(folio);
if (tr->tr_num_buf_new)
__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index c045689b6af8..a9082078e4ed 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -580,12 +580,12 @@ static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
{
const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
- if (folio)
+ if (page_ops && page_ops->page_done) {
+ page_ops->page_done(iter->inode, pos, ret, folio);
+ } else if (folio) {
folio_unlock(folio);
- if (page_ops && page_ops->page_done)
- page_ops->page_done(iter->inode, pos, ret, &folio->page);
- if (folio)
folio_put(folio);
+ }
}
static int iomap_write_begin_inline(const struct iomap_iter *iter,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 0983dfc9a203..743e2a909162 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -131,13 +131,14 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
* associated with them.
*
* When page_prepare succeeds, page_done will always be called to do any
- * cleanup work necessary. In that page_done call, @page will be NULL if the
- * associated page could not be obtained.
+ * cleanup work necessary. In that page_done call, @folio will be NULL if the
+ * associated folio could not be obtained. When folio is not NULL, page_done
+ * is responsible for unlocking and putting the folio.
*/
struct iomap_page_ops {
int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len);
void (*page_done)(struct inode *inode, loff_t pos, unsigned copied,
- struct page *page);
+ struct folio *folio);
/*
* Check that the cached iomap still maps correctly to the filesystem's
--
2.38.1
The operations in struct page_ops all operate on folios, so rename
struct page_ops to struct folio_ops.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/gfs2/bmap.c | 4 ++--
fs/iomap/buffered-io.c | 12 ++++++------
fs/xfs/xfs_iomap.c | 4 ++--
include/linux/iomap.h | 8 ++++----
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index d3adb715ac8c..e191ecfb1fde 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -997,7 +997,7 @@ static void gfs2_iomap_put_folio(struct inode *inode, loff_t pos,
gfs2_trans_end(sdp);
}
-static const struct iomap_page_ops gfs2_iomap_page_ops = {
+static const struct iomap_folio_ops gfs2_iomap_folio_ops = {
.get_folio = gfs2_iomap_get_folio,
.put_folio = gfs2_iomap_put_folio,
};
@@ -1075,7 +1075,7 @@ static int gfs2_iomap_begin_write(struct inode *inode, loff_t pos,
}
if (gfs2_is_stuffed(ip) || gfs2_is_jdata(ip))
- iomap->page_ops = &gfs2_iomap_page_ops;
+ iomap->folio_ops = &gfs2_iomap_folio_ops;
return 0;
out_trans_end:
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 72dfbc3cb086..dacc7c80b20d 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -605,10 +605,10 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
size_t len)
{
- const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
+ const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
- if (page_ops && page_ops->get_folio)
- return page_ops->get_folio(iter, pos, len);
+ if (folio_ops && folio_ops->get_folio)
+ return folio_ops->get_folio(iter, pos, len);
else
return iomap_get_folio(iter, pos);
}
@@ -616,10 +616,10 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
struct folio *folio)
{
- const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
+ const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
- if (page_ops && page_ops->put_folio) {
- page_ops->put_folio(iter->inode, pos, ret, folio);
+ if (folio_ops && folio_ops->put_folio) {
+ folio_ops->put_folio(iter->inode, pos, ret, folio);
} else {
folio_unlock(folio);
folio_put(folio);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index d0bf99539180..5bddf31e21eb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -98,7 +98,7 @@ xfs_get_folio(
return folio;
}
-const struct iomap_page_ops xfs_iomap_page_ops = {
+const struct iomap_folio_ops xfs_iomap_folio_ops = {
.get_folio = xfs_get_folio,
};
@@ -148,7 +148,7 @@ xfs_bmbt_to_iomap(
iomap->flags |= IOMAP_F_DIRTY;
iomap->validity_cookie = sequence_cookie;
- iomap->page_ops = &xfs_iomap_page_ops;
+ iomap->folio_ops = &xfs_iomap_folio_ops;
return 0;
}
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 0ae2cddbedd6..3e6c34b03c89 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -86,7 +86,7 @@ struct vm_fault;
*/
#define IOMAP_NULL_ADDR -1ULL /* addr is not valid */
-struct iomap_page_ops;
+struct iomap_folio_ops;
struct iomap {
u64 addr; /* disk offset of mapping, bytes */
@@ -98,7 +98,7 @@ struct iomap {
struct dax_device *dax_dev; /* dax_dev for dax operations */
void *inline_data;
void *private; /* filesystem private */
- const struct iomap_page_ops *page_ops;
+ const struct iomap_folio_ops *folio_ops;
u64 validity_cookie; /* used with .iomap_valid() */
};
@@ -126,7 +126,7 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
}
/*
- * When a filesystem sets page_ops in an iomap mapping it returns, get_folio
+ * When a filesystem sets folio_ops in an iomap mapping it returns, get_folio
* and put_folio will be called for each folio written to. This only applies
* to buffered writes as unbuffered writes will not typically have folios
* associated with them.
@@ -141,7 +141,7 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
* it is no longer up to date. In that case, the function should return
* ERR_PTR(-ESTALE) to retry the operation with a fresh mapping.
*/
-struct iomap_page_ops {
+struct iomap_folio_ops {
struct folio *(*get_folio)(struct iomap_iter *iter, loff_t pos,
unsigned len);
void (*put_folio)(struct inode *inode, loff_t pos, unsigned copied,
--
2.38.1
Add an __iomap_get_folio() helper as the counterpart of the existing
__iomap_put_folio() helper. Use the new helper in iomap_write_begin().
Not a functional change.
Signed-off-by: Andreas Gruenbacher <[email protected]>
---
fs/iomap/buffered-io.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 418519dea2ce..666107c3a385 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -602,6 +602,17 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
return 0;
}
+static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
+ size_t len)
+{
+ const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
+
+ if (page_ops && page_ops->page_prepare)
+ return page_ops->page_prepare(iter, pos, len);
+ else
+ return iomap_get_folio(iter, pos);
+}
+
static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
struct folio *folio)
{
@@ -642,10 +653,7 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
if (!mapping_large_folio_support(iter->inode->i_mapping))
len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
- if (page_ops && page_ops->page_prepare)
- folio = page_ops->page_prepare(iter, pos, len);
- else
- folio = iomap_get_folio(iter, pos);
+ folio = __iomap_get_folio(iter, pos, len);
if (IS_ERR(folio))
return PTR_ERR(folio);
--
2.38.1
Variable xfs_iomap_folio_ops isn't used outside xfs_iomap.c, so it
should be static.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/xfs/xfs_iomap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 5bddf31e21eb..7d1795a9c742 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -98,7 +98,7 @@ xfs_get_folio(
return folio;
}
-const struct iomap_folio_ops xfs_iomap_folio_ops = {
+static const struct iomap_folio_ops xfs_iomap_folio_ops = {
.get_folio = xfs_get_folio,
};
--
2.38.1
Change the iomap ->page_prepare() handler to get and return a locked
folio instead of doing that in iomap_write_begin(). This allows to
recover from out-of-memory situations in ->page_prepare(), which
eliminates the corresponding error handling code in iomap_write_begin().
The ->put_folio() handler now also isn't called with NULL as the folio
value anymore.
Filesystems are expected to use the iomap_get_folio() helper for getting
locked folios in their ->page_prepare() handlers.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
fs/gfs2/bmap.c | 21 +++++++++++++--------
fs/iomap/buffered-io.c | 17 ++++++-----------
include/linux/iomap.h | 9 +++++----
3 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 0c041459677b..41349e09558b 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -956,15 +956,25 @@ static int __gfs2_iomap_get(struct inode *inode, loff_t pos, loff_t length,
goto out;
}
-static int gfs2_iomap_page_prepare(struct inode *inode, loff_t pos,
- unsigned len)
+static struct folio *
+gfs2_iomap_page_prepare(struct iomap_iter *iter, loff_t pos, unsigned len)
{
+ struct inode *inode = iter->inode;
unsigned int blockmask = i_blocksize(inode) - 1;
struct gfs2_sbd *sdp = GFS2_SB(inode);
unsigned int blocks;
+ struct folio *folio;
+ int status;
blocks = ((pos & blockmask) + len + blockmask) >> inode->i_blkbits;
- return gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
+ status = gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
+ if (status)
+ return ERR_PTR(status);
+
+ folio = iomap_get_folio(iter, pos);
+ if (IS_ERR(folio))
+ gfs2_trans_end(sdp);
+ return folio;
}
static void gfs2_iomap_put_folio(struct inode *inode, loff_t pos,
@@ -974,11 +984,6 @@ static void gfs2_iomap_put_folio(struct inode *inode, loff_t pos,
struct gfs2_inode *ip = GFS2_I(inode);
struct gfs2_sbd *sdp = GFS2_SB(inode);
- if (!folio) {
- gfs2_trans_end(sdp);
- return;
- }
-
if (!gfs2_is_stuffed(ip))
gfs2_page_add_databufs(ip, &folio->page, offset_in_page(pos),
copied);
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index de4a8e5f721a..418519dea2ce 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -609,7 +609,7 @@ static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
if (page_ops && page_ops->put_folio) {
page_ops->put_folio(iter->inode, pos, ret, folio);
- } else if (folio) {
+ } else {
folio_unlock(folio);
folio_put(folio);
}
@@ -642,17 +642,12 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
if (!mapping_large_folio_support(iter->inode->i_mapping))
len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
- if (page_ops && page_ops->page_prepare) {
- status = page_ops->page_prepare(iter->inode, pos, len);
- if (status)
- return status;
- }
-
- folio = iomap_get_folio(iter, pos);
- if (IS_ERR(folio)) {
- __iomap_put_folio(iter, pos, 0, NULL);
+ if (page_ops && page_ops->page_prepare)
+ folio = page_ops->page_prepare(iter, pos, len);
+ else
+ folio = iomap_get_folio(iter, pos);
+ if (IS_ERR(folio))
return PTR_ERR(folio);
- }
/*
* Now we have a locked folio, before we do anything with it we need to
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 188d14e786a4..d50501781856 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -13,6 +13,7 @@
struct address_space;
struct fiemap_extent_info;
struct inode;
+struct iomap_iter;
struct iomap_dio;
struct iomap_writepage_ctx;
struct iov_iter;
@@ -131,12 +132,12 @@ static inline bool iomap_inline_data_valid(const struct iomap *iomap)
* associated with them.
*
* When page_prepare succeeds, put_folio will always be called to do any
- * cleanup work necessary. In that put_folio call, @folio will be NULL if the
- * associated folio could not be obtained. When folio is not NULL, put_folio
- * is responsible for unlocking and putting the folio.
+ * cleanup work necessary. put_folio is responsible for unlocking and putting
+ * @folio.
*/
struct iomap_page_ops {
- int (*page_prepare)(struct inode *inode, loff_t pos, unsigned len);
+ struct folio *(*page_prepare)(struct iomap_iter *iter, loff_t pos,
+ unsigned len);
void (*put_folio)(struct inode *inode, loff_t pos, unsigned copied,
struct folio *folio);
--
2.38.1
On Sun, Jan 08, 2023 at 08:40:32PM +0100, Andreas Gruenbacher wrote:
> Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
> handler and validating the mapping there.
>
> Signed-off-by: Andreas Gruenbacher <[email protected]>
I think this is wrong.
The ->iomap_valid() function handles a fundamental architectural
issue with cached iomaps: the iomap can become stale at any time
whilst it is in use by the iomap core code.
The current problem it solves in the iomap_write_begin() path has to
do with writeback and memory reclaim races over unwritten extents,
but the general case is that we must be able to check the iomap
at any point in time to assess it's validity.
Indeed, we also have this same "iomap valid check" functionality in the
writeback code as cached iomaps can become stale due to racing
writeback, truncated, etc. But you wouldn't know it by looking at the iomap
writeback code - this is currently hidden by XFS by embedding
the checks into the iomap writeback ->map_blocks function.
That is, the first thing that xfs_map_blocks() does is check if the
cached iomap is valid, and if it is valid it returns immediately and
the iomap writeback code uses it without question.
The reason that this is embedded like this is that the iomap did not
have a validity cookie field in it, and so the validity information
was wrapped around the outside of the iomap_writepage_ctx and the
filesystem has to decode it from that private wrapping structure.
However, the validity information iin the structure wrapper is
indentical to the iomap validity cookie, and so the direction I've
been working towards is to replace this implicit, hidden cached
iomap validity check with an explicit ->iomap_valid call and then
only call ->map_blocks if the validity check fails (or is not
implemented).
I want to use the same code for all the iomap validity checks in all
the iomap core code - this is an iomap issue, the conditions where
we need to check for iomap validity are different for depending on
the iomap context being run, and the checks are not necessarily
dependent on first having locked a folio.
Yes, the validity cookie needs to be decoded by the filesystem, but
that does not dictate where the validity checking needs to be done
by the iomap core.
Hence I think removing ->iomap_valid is a big step backwards for the
iomap core code - the iomap core needs to be able to formally verify
the iomap is valid at any point in time, not just at the point in
time a folio in the page cache has been locked...
-Dave.
--
Dave Chinner
[email protected]
On Sun, Jan 8, 2023 at 10:59 PM Dave Chinner <[email protected]> wrote:
> On Sun, Jan 08, 2023 at 08:40:32PM +0100, Andreas Gruenbacher wrote:
> > Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
> > handler and validating the mapping there.
> >
> > Signed-off-by: Andreas Gruenbacher <[email protected]>
>
> I think this is wrong.
>
> The ->iomap_valid() function handles a fundamental architectural
> issue with cached iomaps: the iomap can become stale at any time
> whilst it is in use by the iomap core code.
>
> The current problem it solves in the iomap_write_begin() path has to
> do with writeback and memory reclaim races over unwritten extents,
> but the general case is that we must be able to check the iomap
> at any point in time to assess it's validity.
>
> Indeed, we also have this same "iomap valid check" functionality in the
> writeback code as cached iomaps can become stale due to racing
> writeback, truncated, etc. But you wouldn't know it by looking at the iomap
> writeback code - this is currently hidden by XFS by embedding
> the checks into the iomap writeback ->map_blocks function.
>
> That is, the first thing that xfs_map_blocks() does is check if the
> cached iomap is valid, and if it is valid it returns immediately and
> the iomap writeback code uses it without question.
>
> The reason that this is embedded like this is that the iomap did not
> have a validity cookie field in it, and so the validity information
> was wrapped around the outside of the iomap_writepage_ctx and the
> filesystem has to decode it from that private wrapping structure.
>
> However, the validity information iin the structure wrapper is
> indentical to the iomap validity cookie,
Then could that part of the xfs code be converted to use
iomap->validity_cookie so that struct iomap_writepage_ctx can be
eliminated?
> and so the direction I've
> been working towards is to replace this implicit, hidden cached
> iomap validity check with an explicit ->iomap_valid call and then
> only call ->map_blocks if the validity check fails (or is not
> implemented).
>
> I want to use the same code for all the iomap validity checks in all
> the iomap core code - this is an iomap issue, the conditions where
> we need to check for iomap validity are different for depending on
> the iomap context being run, and the checks are not necessarily
> dependent on first having locked a folio.
>
> Yes, the validity cookie needs to be decoded by the filesystem, but
> that does not dictate where the validity checking needs to be done
> by the iomap core.
>
> Hence I think removing ->iomap_valid is a big step backwards for the
> iomap core code - the iomap core needs to be able to formally verify
> the iomap is valid at any point in time, not just at the point in
> time a folio in the page cache has been locked...
We don't need to validate an iomap "at any time". It's two specific
places in the code in which we need to check, and we're not going to
end up with ten more such places tomorrow. I'd prefer to keep those
filesystem internals in the filesystem specific code instead of
exposing them to the iomap layer. But that's just me ...
If we ignore this particular commit for now, do you have any
objections to the patches in this series? If not, it would be great if
we could add the other patches to iomap-for-next.
By the way, I'm still not sure if gfs2 is affected by this whole iomap
validation drama given that it neither implements unwritten extents
nor delayed allocation. This is a mess.
Thanks,
Andreas
On Mon, Jan 09, 2023 at 07:45:27PM +0100, Andreas Gruenbacher wrote:
> On Sun, Jan 8, 2023 at 10:59 PM Dave Chinner <[email protected]> wrote:
> > On Sun, Jan 08, 2023 at 08:40:32PM +0100, Andreas Gruenbacher wrote:
> > > Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
> > > handler and validating the mapping there.
> > >
> > > Signed-off-by: Andreas Gruenbacher <[email protected]>
> >
> > I think this is wrong.
> >
> > The ->iomap_valid() function handles a fundamental architectural
> > issue with cached iomaps: the iomap can become stale at any time
> > whilst it is in use by the iomap core code.
> >
> > The current problem it solves in the iomap_write_begin() path has to
> > do with writeback and memory reclaim races over unwritten extents,
> > but the general case is that we must be able to check the iomap
> > at any point in time to assess it's validity.
> >
> > Indeed, we also have this same "iomap valid check" functionality in the
> > writeback code as cached iomaps can become stale due to racing
> > writeback, truncated, etc. But you wouldn't know it by looking at the iomap
> > writeback code - this is currently hidden by XFS by embedding
> > the checks into the iomap writeback ->map_blocks function.
> >
> > That is, the first thing that xfs_map_blocks() does is check if the
> > cached iomap is valid, and if it is valid it returns immediately and
> > the iomap writeback code uses it without question.
> >
> > The reason that this is embedded like this is that the iomap did not
> > have a validity cookie field in it, and so the validity information
> > was wrapped around the outside of the iomap_writepage_ctx and the
> > filesystem has to decode it from that private wrapping structure.
> >
> > However, the validity information iin the structure wrapper is
> > indentical to the iomap validity cookie,
>
> Then could that part of the xfs code be converted to use
> iomap->validity_cookie so that struct iomap_writepage_ctx can be
> eliminated?
Yes, that is the plan.
>
> > and so the direction I've
> > been working towards is to replace this implicit, hidden cached
> > iomap validity check with an explicit ->iomap_valid call and then
> > only call ->map_blocks if the validity check fails (or is not
> > implemented).
> >
> > I want to use the same code for all the iomap validity checks in all
> > the iomap core code - this is an iomap issue, the conditions where
> > we need to check for iomap validity are different for depending on
> > the iomap context being run, and the checks are not necessarily
> > dependent on first having locked a folio.
> >
> > Yes, the validity cookie needs to be decoded by the filesystem, but
> > that does not dictate where the validity checking needs to be done
> > by the iomap core.
> >
> > Hence I think removing ->iomap_valid is a big step backwards for the
> > iomap core code - the iomap core needs to be able to formally verify
> > the iomap is valid at any point in time, not just at the point in
> > time a folio in the page cache has been locked...
>
> We don't need to validate an iomap "at any time". It's two specific
> places in the code in which we need to check, and we're not going to
> end up with ten more such places tomorrow.
Not immediately, but that doesn't change the fact this is not a
filesystem specific issue - it's an inherent characteristic of
cached iomaps and unsynchronised extent state changes that occur
outside exclusive inode->i_rwsem IO context (e.g. in writeback and
IO completion contexts).
Racing mmap + buffered writes can expose these state changes as the
iomap bufferred write IO path is not serialised against the iomap
mmap IO path except via folio locks. Hence a mmap page fault can
invalidate a cached buffered write iomap by causing a hole ->
unwritten, hole -> delalloc or hole -> written conversion in the
middle of the buffered write range. The buffered write still has a
hole mapping cached for that entire range, and it is now incorrect.
If the mmap write happens to change extent state at the trailing
edge of a partial buffered write, data corruption will occur if we
race just right with writeback and memory reclaim. I'm pretty sure
that this corruption can be reporduced on gfs2 if we try hard enough
- generic/346 triggers the mmap/write race condition, all that is
needed from that point is for writeback and reclaiming pages at
exactly the right time...
> I'd prefer to keep those
> filesystem internals in the filesystem specific code instead of
> exposing them to the iomap layer. But that's just me ...
My point is that there is nothing XFS specific about these stale
cached iomap race conditions, nor is it specifically related to
folio locking. The folio locking inversions w.r.t. iomap caching and
the interactions with writeback and reclaim are simply the
manifestation that brought the issue to our attention.
This is why I think hiding iomap validation filesystem specific page
cache allocation/lookup functions is entirely the wrong layer to be
doing iomap validity checks. Especially as it prevents us from
adding more validity checks in the core infrastructure when we need
them in future.
AFAIC, an iomap must carry with it a method for checking
that it is still valid. We need it in the write path, we need it in
the writeback path. If we want to relax the restrictions on clone
operations (e.g. shared locking on the source file), we'll need to
be able to detect stale cached iomaps in those paths, too. And I
haven't really thought through all the implications of shared
locking on buffered writes yet, but that may well require more
checks in other places as well.
> If we ignore this particular commit for now, do you have any
> objections to the patches in this series? If not, it would be great if
> we could add the other patches to iomap-for-next.
I still don't like moving page cache operations into individual
filesystems, but for the moment I can live with the IOMAP_NOCREATE
hack to drill iomap state through the filesystem without the
filesystem being aware of it.
> By the way, I'm still not sure if gfs2 is affected by this whole iomap
> validation drama given that it neither implements unwritten extents
> nor delayed allocation. This is a mess.
See above - I'm pretty sure it will be, but it may be very difficult
to expose. After all, it's taken several years before anyone noticed
this issue with XFS, even though we were aware of the issue of stale
cached iomaps causing data corruption in the writeback path....
Cheers,
Dave.
--
Dave Chinner
[email protected]
Am Mo., 9. Jan. 2023 um 23:58 Uhr schrieb Dave Chinner <[email protected]>:
> On Mon, Jan 09, 2023 at 07:45:27PM +0100, Andreas Gruenbacher wrote:
> > On Sun, Jan 8, 2023 at 10:59 PM Dave Chinner <[email protected]> wrote:
> > > On Sun, Jan 08, 2023 at 08:40:32PM +0100, Andreas Gruenbacher wrote:
> > > > Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
> > > > handler and validating the mapping there.
> > > >
> > > > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > >
> > > I think this is wrong.
> > >
> > > The ->iomap_valid() function handles a fundamental architectural
> > > issue with cached iomaps: the iomap can become stale at any time
> > > whilst it is in use by the iomap core code.
> > >
> > > The current problem it solves in the iomap_write_begin() path has to
> > > do with writeback and memory reclaim races over unwritten extents,
> > > but the general case is that we must be able to check the iomap
> > > at any point in time to assess it's validity.
> > >
> > > Indeed, we also have this same "iomap valid check" functionality in the
> > > writeback code as cached iomaps can become stale due to racing
> > > writeback, truncated, etc. But you wouldn't know it by looking at the iomap
> > > writeback code - this is currently hidden by XFS by embedding
> > > the checks into the iomap writeback ->map_blocks function.
> > >
> > > That is, the first thing that xfs_map_blocks() does is check if the
> > > cached iomap is valid, and if it is valid it returns immediately and
> > > the iomap writeback code uses it without question.
> > >
> > > The reason that this is embedded like this is that the iomap did not
> > > have a validity cookie field in it, and so the validity information
> > > was wrapped around the outside of the iomap_writepage_ctx and the
> > > filesystem has to decode it from that private wrapping structure.
> > >
> > > However, the validity information iin the structure wrapper is
> > > indentical to the iomap validity cookie,
> >
> > Then could that part of the xfs code be converted to use
> > iomap->validity_cookie so that struct iomap_writepage_ctx can be
> > eliminated?
>
> Yes, that is the plan.
>
> >
> > > and so the direction I've
> > > been working towards is to replace this implicit, hidden cached
> > > iomap validity check with an explicit ->iomap_valid call and then
> > > only call ->map_blocks if the validity check fails (or is not
> > > implemented).
> > >
> > > I want to use the same code for all the iomap validity checks in all
> > > the iomap core code - this is an iomap issue, the conditions where
> > > we need to check for iomap validity are different for depending on
> > > the iomap context being run, and the checks are not necessarily
> > > dependent on first having locked a folio.
> > >
> > > Yes, the validity cookie needs to be decoded by the filesystem, but
> > > that does not dictate where the validity checking needs to be done
> > > by the iomap core.
> > >
> > > Hence I think removing ->iomap_valid is a big step backwards for the
> > > iomap core code - the iomap core needs to be able to formally verify
> > > the iomap is valid at any point in time, not just at the point in
> > > time a folio in the page cache has been locked...
> >
> > We don't need to validate an iomap "at any time". It's two specific
> > places in the code in which we need to check, and we're not going to
> > end up with ten more such places tomorrow.
>
> Not immediately, but that doesn't change the fact this is not a
> filesystem specific issue - it's an inherent characteristic of
> cached iomaps and unsynchronised extent state changes that occur
> outside exclusive inode->i_rwsem IO context (e.g. in writeback and
> IO completion contexts).
>
> Racing mmap + buffered writes can expose these state changes as the
> iomap bufferred write IO path is not serialised against the iomap
> mmap IO path except via folio locks. Hence a mmap page fault can
> invalidate a cached buffered write iomap by causing a hole ->
> unwritten, hole -> delalloc or hole -> written conversion in the
> middle of the buffered write range. The buffered write still has a
> hole mapping cached for that entire range, and it is now incorrect.
>
> If the mmap write happens to change extent state at the trailing
> edge of a partial buffered write, data corruption will occur if we
> race just right with writeback and memory reclaim. I'm pretty sure
> that this corruption can be reporduced on gfs2 if we try hard enough
> - generic/346 triggers the mmap/write race condition, all that is
> needed from that point is for writeback and reclaiming pages at
> exactly the right time...
>
> > I'd prefer to keep those
> > filesystem internals in the filesystem specific code instead of
> > exposing them to the iomap layer. But that's just me ...
>
> My point is that there is nothing XFS specific about these stale
> cached iomap race conditions, nor is it specifically related to
> folio locking. The folio locking inversions w.r.t. iomap caching and
> the interactions with writeback and reclaim are simply the
> manifestation that brought the issue to our attention.
>
> This is why I think hiding iomap validation filesystem specific page
> cache allocation/lookup functions is entirely the wrong layer to be
> doing iomap validity checks. Especially as it prevents us from
> adding more validity checks in the core infrastructure when we need
> them in future.
>
> AFAIC, an iomap must carry with it a method for checking
> that it is still valid. We need it in the write path, we need it in
> the writeback path. If we want to relax the restrictions on clone
> operations (e.g. shared locking on the source file), we'll need to
> be able to detect stale cached iomaps in those paths, too. And I
> haven't really thought through all the implications of shared
> locking on buffered writes yet, but that may well require more
> checks in other places as well.
>
> > If we ignore this particular commit for now, do you have any
> > objections to the patches in this series? If not, it would be great if
> > we could add the other patches to iomap-for-next.
>
> I still don't like moving page cache operations into individual
> filesystems, but for the moment I can live with the IOMAP_NOCREATE
> hack to drill iomap state through the filesystem without the
> filesystem being aware of it.
Alright, works for me. Darrick?
> > By the way, I'm still not sure if gfs2 is affected by this whole iomap
> > validation drama given that it neither implements unwritten extents
> > nor delayed allocation. This is a mess.
>
> See above - I'm pretty sure it will be, but it may be very difficult
> to expose. After all, it's taken several years before anyone noticed
> this issue with XFS, even though we were aware of the issue of stale
> cached iomaps causing data corruption in the writeback path....
Okay, that's all pretty ugly. Thanks a lot for the detailed explanation.
Cheers,
Andreas
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]
On Sun, Jan 08, 2023 at 08:40:30PM +0100, Andreas Gruenbacher wrote:
> +static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
> + size_t len)
> +{
> + const struct iomap_page_ops *page_ops = iter->iomap.page_ops;
> +
> + if (page_ops && page_ops->page_prepare)
> + return page_ops->page_prepare(iter, pos, len);
> + else
> + return iomap_get_folio(iter, pos);
Nit: No need for an else after the return.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Mon, Jan 09, 2023 at 08:59:11AM +1100, Dave Chinner wrote:
> Indeed, we also have this same "iomap valid check" functionality in the
> writeback code as cached iomaps can become stale due to racing
> writeback, truncated, etc. But you wouldn't know it by looking at the iomap
> writeback code - this is currently hidden by XFS by embedding
> the checks into the iomap writeback ->map_blocks function.
And that's in many ways a good thing, as it avoids various callouts
that are expensive and confusing. Just like how this patch gets it
right by not having a mess of badly interacting callbacks, but
one that ensures that the page is ready.
> Hence I think removing ->iomap_valid is a big step backwards for the
> iomap core code - the iomap core needs to be able to formally verify
> the iomap is valid at any point in time, not just at the point in
> time a folio in the page cache has been locked...
For using it anywhere else but the buffered write path it is in the
wrong place to start with, and nonwithstanding my above concern I
can't relaly think of a good place and prototype for such a valid
callback to actually cover all use cases.
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Jan 10, 2023 at 02:09:07AM +0100, Andreas Gr?nbacher wrote:
> Am Mo., 9. Jan. 2023 um 23:58 Uhr schrieb Dave Chinner <[email protected]>:
> > On Mon, Jan 09, 2023 at 07:45:27PM +0100, Andreas Gruenbacher wrote:
> > > On Sun, Jan 8, 2023 at 10:59 PM Dave Chinner <[email protected]> wrote:
> > > > On Sun, Jan 08, 2023 at 08:40:32PM +0100, Andreas Gruenbacher wrote:
> > > > > Eliminate the ->iomap_valid() handler by switching to a ->get_folio()
> > > > > handler and validating the mapping there.
> > > > >
> > > > > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > > >
> > > > I think this is wrong.
> > > >
> > > > The ->iomap_valid() function handles a fundamental architectural
> > > > issue with cached iomaps: the iomap can become stale at any time
> > > > whilst it is in use by the iomap core code.
> > > >
> > > > The current problem it solves in the iomap_write_begin() path has to
> > > > do with writeback and memory reclaim races over unwritten extents,
> > > > but the general case is that we must be able to check the iomap
> > > > at any point in time to assess it's validity.
> > > >
> > > > Indeed, we also have this same "iomap valid check" functionality in the
> > > > writeback code as cached iomaps can become stale due to racing
> > > > writeback, truncated, etc. But you wouldn't know it by looking at the iomap
> > > > writeback code - this is currently hidden by XFS by embedding
> > > > the checks into the iomap writeback ->map_blocks function.
> > > >
> > > > That is, the first thing that xfs_map_blocks() does is check if the
> > > > cached iomap is valid, and if it is valid it returns immediately and
> > > > the iomap writeback code uses it without question.
> > > >
> > > > The reason that this is embedded like this is that the iomap did not
> > > > have a validity cookie field in it, and so the validity information
> > > > was wrapped around the outside of the iomap_writepage_ctx and the
> > > > filesystem has to decode it from that private wrapping structure.
> > > >
> > > > However, the validity information iin the structure wrapper is
> > > > indentical to the iomap validity cookie,
> > >
> > > Then could that part of the xfs code be converted to use
> > > iomap->validity_cookie so that struct iomap_writepage_ctx can be
> > > eliminated?
> >
> > Yes, that is the plan.
> >
> > >
> > > > and so the direction I've
> > > > been working towards is to replace this implicit, hidden cached
> > > > iomap validity check with an explicit ->iomap_valid call and then
> > > > only call ->map_blocks if the validity check fails (or is not
> > > > implemented).
> > > >
> > > > I want to use the same code for all the iomap validity checks in all
> > > > the iomap core code - this is an iomap issue, the conditions where
> > > > we need to check for iomap validity are different for depending on
> > > > the iomap context being run, and the checks are not necessarily
> > > > dependent on first having locked a folio.
> > > >
> > > > Yes, the validity cookie needs to be decoded by the filesystem, but
> > > > that does not dictate where the validity checking needs to be done
> > > > by the iomap core.
> > > >
> > > > Hence I think removing ->iomap_valid is a big step backwards for the
> > > > iomap core code - the iomap core needs to be able to formally verify
> > > > the iomap is valid at any point in time, not just at the point in
> > > > time a folio in the page cache has been locked...
> > >
> > > We don't need to validate an iomap "at any time". It's two specific
> > > places in the code in which we need to check, and we're not going to
> > > end up with ten more such places tomorrow.
> >
> > Not immediately, but that doesn't change the fact this is not a
> > filesystem specific issue - it's an inherent characteristic of
> > cached iomaps and unsynchronised extent state changes that occur
> > outside exclusive inode->i_rwsem IO context (e.g. in writeback and
> > IO completion contexts).
> >
> > Racing mmap + buffered writes can expose these state changes as the
> > iomap bufferred write IO path is not serialised against the iomap
> > mmap IO path except via folio locks. Hence a mmap page fault can
> > invalidate a cached buffered write iomap by causing a hole ->
> > unwritten, hole -> delalloc or hole -> written conversion in the
> > middle of the buffered write range. The buffered write still has a
> > hole mapping cached for that entire range, and it is now incorrect.
> >
> > If the mmap write happens to change extent state at the trailing
> > edge of a partial buffered write, data corruption will occur if we
> > race just right with writeback and memory reclaim. I'm pretty sure
> > that this corruption can be reporduced on gfs2 if we try hard enough
> > - generic/346 triggers the mmap/write race condition, all that is
> > needed from that point is for writeback and reclaiming pages at
> > exactly the right time...
> >
> > > I'd prefer to keep those
> > > filesystem internals in the filesystem specific code instead of
> > > exposing them to the iomap layer. But that's just me ...
> >
> > My point is that there is nothing XFS specific about these stale
> > cached iomap race conditions, nor is it specifically related to
> > folio locking. The folio locking inversions w.r.t. iomap caching and
> > the interactions with writeback and reclaim are simply the
> > manifestation that brought the issue to our attention.
> >
> > This is why I think hiding iomap validation filesystem specific page
> > cache allocation/lookup functions is entirely the wrong layer to be
> > doing iomap validity checks. Especially as it prevents us from
> > adding more validity checks in the core infrastructure when we need
> > them in future.
> >
> > AFAIC, an iomap must carry with it a method for checking
> > that it is still valid. We need it in the write path, we need it in
> > the writeback path. If we want to relax the restrictions on clone
> > operations (e.g. shared locking on the source file), we'll need to
> > be able to detect stale cached iomaps in those paths, too. And I
> > haven't really thought through all the implications of shared
> > locking on buffered writes yet, but that may well require more
> > checks in other places as well.
> >
> > > If we ignore this particular commit for now, do you have any
> > > objections to the patches in this series? If not, it would be great if
> > > we could add the other patches to iomap-for-next.
> >
> > I still don't like moving page cache operations into individual
> > filesystems, but for the moment I can live with the IOMAP_NOCREATE
> > hack to drill iomap state through the filesystem without the
> > filesystem being aware of it.
>
> Alright, works for me. Darrick?
Works for me too.
I've wondered if IOMAP_NOCREATE could be useful for more things (e.g.
determining if part of a file has been cached) though I've not thought
of a good usecase for that. Maybe something along the lines of a
"userspace wants us to redirty this critical file after fsync returned
EIO" type thing?
> > > By the way, I'm still not sure if gfs2 is affected by this whole iomap
> > > validation drama given that it neither implements unwritten extents
> > > nor delayed allocation. This is a mess.
> >
> > See above - I'm pretty sure it will be, but it may be very difficult
> > to expose. After all, it's taken several years before anyone noticed
> > this issue with XFS, even though we were aware of the issue of stale
> > cached iomaps causing data corruption in the writeback path....
>
> Okay, that's all pretty ugly. Thanks a lot for the detailed explanation.
I don't have any objections to pulling everything except patches 8 and
10 for testing this week. I find myself more in agreement with
Christoph and Andreas that whoever gets the folio is also responsible
for knowing if revalidating the mapping is necessary and then doing it.
However, I still have enough questions about the mapping revalidation to
make that a separate discussion.
Questions, namely:
1. Does zonefs need to revalidate mappings? The mappings are 1:1 so I
don't think it does, but OTOH zone pointer management might complicate
that.
2. How about porting the writeback iomap validation to use this
mechanism? (I suspect Dave might already be working on this...)
2. Do we need to revalidate mappings for directio writes? I think the
answer is no (for xfs) because the ->iomap_begin call will allocate
whatever blocks are needed and truncate/punch/reflink block on the
iolock while the directio writes are pending, so you'll never end up
with a stale mapping. But I don't know if that statement applies
generally...
--D
> Cheers,
> Andreas
>
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > [email protected]
On Sun, Jan 15, 2023 at 09:29:58AM -0800, Darrick J. Wong wrote:
> I don't have any objections to pulling everything except patches 8 and
> 10 for testing this week.
That would be great. I now have a series to return the ERR_PTR
from __filemap_get_folio which will cause a minor conflict, but
I think that's easy enough for Linux to handle.
>
> 1. Does zonefs need to revalidate mappings? The mappings are 1:1 so I
> don't think it does, but OTOH zone pointer management might complicate
> that.
Adding Damien.
> 2. How about porting the writeback iomap validation to use this
> mechanism? (I suspect Dave might already be working on this...)
What is "this mechanism"? Do you mean the here removed ->iomap_valid
? writeback calls into ->map_blocks for every block while under the
folio lock, so the validation can (and for XFS currently is) done
in that. Moving it out into a separate method with extra indirect
functiona call overhead and interactions between the methods seems
like a retrograde step to me.
> 2. Do we need to revalidate mappings for directio writes? I think the
> answer is no (for xfs) because the ->iomap_begin call will allocate
> whatever blocks are needed and truncate/punch/reflink block on the
> iolock while the directio writes are pending, so you'll never end up
> with a stale mapping.
Yes.
On 1/18/23 16:21, Christoph Hellwig wrote:
> On Sun, Jan 15, 2023 at 09:29:58AM -0800, Darrick J. Wong wrote:
>> I don't have any objections to pulling everything except patches 8 and
>> 10 for testing this week.
>
> That would be great. I now have a series to return the ERR_PTR
> from __filemap_get_folio which will cause a minor conflict, but
> I think that's easy enough for Linux to handle.
>
>>
>> 1. Does zonefs need to revalidate mappings? The mappings are 1:1 so I
>> don't think it does, but OTOH zone pointer management might complicate
>> that.
>
> Adding Damien.
zonefs has a static mapping of file blocks that never changes and is fully
populated up to a file max size from mount. So zonefs is not using the
iomap_valid page operation. In fact, zonefs is not even using struct
iomap_page_ops.
>
>> 2. How about porting the writeback iomap validation to use this
>> mechanism? (I suspect Dave might already be working on this...)
>
> What is "this mechanism"? Do you mean the here removed ->iomap_valid
> ? writeback calls into ->map_blocks for every block while under the
> folio lock, so the validation can (and for XFS currently is) done
> in that. Moving it out into a separate method with extra indirect
> functiona call overhead and interactions between the methods seems
> like a retrograde step to me.
>
>> 2. Do we need to revalidate mappings for directio writes? I think the
>> answer is no (for xfs) because the ->iomap_begin call will allocate
>> whatever blocks are needed and truncate/punch/reflink block on the
>> iolock while the directio writes are pending, so you'll never end up
>> with a stale mapping.
>
> Yes.
--
Damien Le Moal
Western Digital Research
On Tue, Jan 17, 2023 at 11:21:38PM -0800, Christoph Hellwig wrote:
> On Sun, Jan 15, 2023 at 09:29:58AM -0800, Darrick J. Wong wrote:
> > I don't have any objections to pulling everything except patches 8 and
> > 10 for testing this week.
>
> That would be great. I now have a series to return the ERR_PTR
> from __filemap_get_folio which will cause a minor conflict, but
> I think that's easy enough for Linux to handle.
Ok, done.
> >
> > 1. Does zonefs need to revalidate mappings? The mappings are 1:1 so I
> > don't think it does, but OTOH zone pointer management might complicate
> > that.
>
> Adding Damien.
>
> > 2. How about porting the writeback iomap validation to use this
> > mechanism? (I suspect Dave might already be working on this...)
>
> What is "this mechanism"? Do you mean the here removed ->iomap_valid
> ? writeback calls into ->map_blocks for every block while under the
> folio lock, so the validation can (and for XFS currently is) done
> in that. Moving it out into a separate method with extra indirect
> functiona call overhead and interactions between the methods seems
> like a retrograde step to me.
Sorry, I should've been more specific -- can xfs writeback use the
validity cookie in struct iomap and thereby get rid of struct
xfs_writepage_ctx entirely?
> > 2. Do we need to revalidate mappings for directio writes? I think the
> > answer is no (for xfs) because the ->iomap_begin call will allocate
> > whatever blocks are needed and truncate/punch/reflink block on the
> > iolock while the directio writes are pending, so you'll never end up
> > with a stale mapping.
>
> Yes.
Er... yes as in "Yes, we *do* need to revalidate directio writes", or
"Yes, your reasoning is correct"?
--D
Am Mi., 18. Jan. 2023 um 20:04 Uhr schrieb Darrick J. Wong <[email protected]>:
>
> On Tue, Jan 17, 2023 at 11:21:38PM -0800, Christoph Hellwig wrote:
> > On Sun, Jan 15, 2023 at 09:29:58AM -0800, Darrick J. Wong wrote:
> > > I don't have any objections to pulling everything except patches 8 and
> > > 10 for testing this week.
> >
> > That would be great. I now have a series to return the ERR_PTR
> > from __filemap_get_folio which will cause a minor conflict, but
> > I think that's easy enough for Linux to handle.
>
> Ok, done.
>
> > >
> > > 1. Does zonefs need to revalidate mappings? The mappings are 1:1 so I
> > > don't think it does, but OTOH zone pointer management might complicate
> > > that.
> >
> > Adding Damien.
> >
> > > 2. How about porting the writeback iomap validation to use this
> > > mechanism? (I suspect Dave might already be working on this...)
> >
> > What is "this mechanism"? Do you mean the here removed ->iomap_valid
> > ? writeback calls into ->map_blocks for every block while under the
> > folio lock, so the validation can (and for XFS currently is) done
> > in that. Moving it out into a separate method with extra indirect
> > functiona call overhead and interactions between the methods seems
> > like a retrograde step to me.
>
> Sorry, I should've been more specific -- can xfs writeback use the
> validity cookie in struct iomap and thereby get rid of struct
> xfs_writepage_ctx entirely?
Already asked and answered in the same thread:
https://lore.kernel.org/linux-fsdevel/[email protected]/
> > > 2. Do we need to revalidate mappings for directio writes? I think the
> > > answer is no (for xfs) because the ->iomap_begin call will allocate
> > > whatever blocks are needed and truncate/punch/reflink block on the
> > > iolock while the directio writes are pending, so you'll never end up
> > > with a stale mapping.
> >
> > Yes.
>
> Er... yes as in "Yes, we *do* need to revalidate directio writes", or
> "Yes, your reasoning is correct"?
>
> --D
On Sun, Jan 15, 2023 at 09:29:58AM -0800, Darrick J. Wong wrote:
> 2. Do we need to revalidate mappings for directio writes? I think the
> answer is no (for xfs) because the ->iomap_begin call will allocate
> whatever blocks are needed and truncate/punch/reflink block on the
> iolock while the directio writes are pending, so you'll never end up
> with a stale mapping. But I don't know if that statement applies
> generally...
The issue is not truncate/punch/reflink for either DIO or buffered
IO - the issue that leads to stale iomaps is async extent state.
i.e. IO completion doing unwritten extent conversion.
For DIO, AIO doesn't hold the IOLOCK at all when completion is run
(like buffered writeback), but non-AIO DIO writes hold the IOLOCK
shared while waiting for completion. This means that we can have DIO
submission and completion still running concurrently, and so stale
iomaps are a definite possibility.
From my notes when I looked at this:
1. the race condition for a DIO write mapping go stale is an
overlapping DIO completion and converting the block from unwritten
to written, and then the dio write incorrectly issuing sub-block
zeroing because the mapping is now stale.
2. DIO read into a hole or unwritten extent zeroes the entire range
in the user buffer in one operation. If this is a large range, this
could race with small DIO writes within that range that have
completed
3. There is a window between dio write completion doing unwritten
extent conversion (by ->end_io) and the page cache being
invalidated, providing a window where buffered read maps can be
stale and incorrect read behaviour exposed to userpace before
the page cache is invalidated.
These all stem from IO having overlapping ranges, which is largely
unsupported but can't be entirely prevented (e.g. backup
applications running in the background). Largely the problems are
confined to sub-block IOs. i.e. when sub-block DIO writes to the
same block are being performed, we have the possiblity that one
write completes whilst the other is deciding what to zero, unaware
that the range is now MAPPED rather than UNWRITTEN.
We currently avoid issues with sub-block dio writes by using
IOMAP_DIO_OVERWRITE_ONLY with shared locking. This ensures that the
unaligned IO fits entirely within a MAPPED extent so no sub-block
zeroing is required. If allocation or sub-block zeroing is required,
then we force the filesystem to fall back to exclusive IO locking
and wait for all concurrent DIO in flight to complete so that it
can't race with any other DIO write that might cause the map to
become stale while we are doing the zeroing.
This does not avoid potential issues with DIO write vs buffered
read, nor DIO write vs mmap IO. It's not totally clear to me
whether we need ->iomap_valid checks in the buffered read paths
to avoid the completion races with DIO writes, but there are windows
there where cached iomaps could be considered stale....
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Sun, Jan 08, 2023 at 08:40:29PM +0100, Andreas Gruenbacher wrote:
> +static struct folio *
> +gfs2_iomap_page_prepare(struct iomap_iter *iter, loff_t pos, unsigned len)
> {
> + struct inode *inode = iter->inode;
> unsigned int blockmask = i_blocksize(inode) - 1;
> struct gfs2_sbd *sdp = GFS2_SB(inode);
> unsigned int blocks;
> + struct folio *folio;
> + int status;
>
> blocks = ((pos & blockmask) + len + blockmask) >> inode->i_blkbits;
> - return gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
> + status = gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
> + if (status)
> + return ERR_PTR(status);
> +
> + folio = iomap_get_folio(iter, pos);
> + if (IS_ERR(folio))
> + gfs2_trans_end(sdp);
> + return folio;
> }
Hi Andreas,
I didn't think to mention this at the time, but I was reading through
buffered-io.c and this jumped out at me. For filesystems which support
folios, we pass the entire length of the write (or at least the length
of the remaining iomap length). That's intended to allow us to decide
how large a folio to allocate at some point in the future.
For GFS2, we do this:
if (!mapping_large_folio_support(iter->inode->i_mapping))
len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
I'd like to drop that and pass the full length of the write to
->get_folio(). It looks like you'll have to clamp it yourself at this
point. I am kind of curious why you do one transaction per page --
I would have thought you'd rather do one transaction for the entire write.
On Tue, Jan 31, 2023 at 8:37 PM Matthew Wilcox <[email protected]> wrote:
> On Sun, Jan 08, 2023 at 08:40:29PM +0100, Andreas Gruenbacher wrote:
> > +static struct folio *
> > +gfs2_iomap_page_prepare(struct iomap_iter *iter, loff_t pos, unsigned len)
> > {
> > + struct inode *inode = iter->inode;
> > unsigned int blockmask = i_blocksize(inode) - 1;
> > struct gfs2_sbd *sdp = GFS2_SB(inode);
> > unsigned int blocks;
> > + struct folio *folio;
> > + int status;
> >
> > blocks = ((pos & blockmask) + len + blockmask) >> inode->i_blkbits;
> > - return gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
> > + status = gfs2_trans_begin(sdp, RES_DINODE + blocks, 0);
> > + if (status)
> > + return ERR_PTR(status);
> > +
> > + folio = iomap_get_folio(iter, pos);
> > + if (IS_ERR(folio))
> > + gfs2_trans_end(sdp);
> > + return folio;
> > }
>
> Hi Andreas,
Hello,
> I didn't think to mention this at the time, but I was reading through
> buffered-io.c and this jumped out at me. For filesystems which support
> folios, we pass the entire length of the write (or at least the length
> of the remaining iomap length). That's intended to allow us to decide
> how large a folio to allocate at some point in the future.
>
> For GFS2, we do this:
>
> if (!mapping_large_folio_support(iter->inode->i_mapping))
> len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
>
> I'd like to drop that and pass the full length of the write to
> ->get_folio(). It looks like you'll have to clamp it yourself at this
> point.
sounds reasonable to me.
I see that gfs2_page_add_databufs() hasn't been folio-ized yet, but it
looks like it might just work anway. So gfs2_iomap_get_folio() ...
gfs2_iomap_put_folio() should, in principle, work for requests bigger
than PAGE_SIZE.
Is there a reasonable way of trying it out?
We still want to keep the transaction size somewhat reasonable, but
the maximum size gfs2_iomap_begin() will return for a write is 509
blocks on a 4k-block filesystem, or slightly less than 2 MiB, which
should be fine.
> I am kind of curious why you do one transaction per page --
> I would have thought you'd rather do one transaction for the entire write.
Only for journaled data writes. We could probably do bigger
transactions even in that case, but we'd rather get rid of data
journaling than encourage it, so we're also not spending a lot of time
on optimizing this case.
Thanks,
Andreas