On Mon, Jun 28, 2021 at 07:27:25PM +0200, Andreas Gruenbacher wrote:
> (1) In iomap_readpage_actor, an iomap_page is attached to the page even
> for inline inodes. This is unnecessary because inline inodes don't need
> iomap_page objects. That alone wouldn't cause any real issues, but when
> iomap_read_inline_data copies the inline data into the page, it sets the
> PageUptodate flag without setting iop->uptodate, causing an
> inconsistency between the two. This will trigger a WARN_ON in
> iomap_page_release. The fix should be not to allocate iomap_page
> objects when reading from inline inodes (patch 1).
I don't have a problem with this patch.
> (2) When un-inlining an inode, we must allocate a page with an attached
> iomap_page object (iomap_page_create) and initialize the iop->uptodate
> bitmap (iomap_set_range_uptodate). We can't currently do that because
> iomap_page_create and iomap_set_range_uptodate are not exported. That
> could be fixed by exporting those functions, or by implementing an
> additional helper as in patch 2. Which of the two would you prefer?
Not hugely happy with either of these options, tbh. I'd rather we apply
a patch akin to this one (plucked from the folio tree), so won't apply:
@@ -1305,7 +1311,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
struct writeback_control *wbc, struct inode *inode,
struct folio *folio, loff_t end_pos)
{
- struct iomap_page *iop = to_iomap_page(folio);
+ struct iomap_page *iop = iomap_page_create(inode, folio);
struct iomap_ioend *ioend, *next;
unsigned len = i_blocksize(inode);
unsigned nblocks = i_blocks_per_folio(inode, folio);
@@ -1313,7 +1319,6 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
int error = 0, count = 0, i;
LIST_HEAD(submit_list);
- WARN_ON_ONCE(nblocks > 1 && !iop);
WARN_ON_ONCE(iop && atomic_read(&iop->write_bytes_pending) != 0);
/*
so permit pages without an iop to enter writeback and create an iop
*then*. Would that solve your problem?
> (3) We're not yet using iomap_page_mkwrite, so iomap_page objects don't
> get created on .page_mkwrite, either. Part of the reason is that
> iomap_page_mkwrite locks the page and then calls into the filesystem for
> uninlining and for allocating backing blocks. This conflicts with the
> gfs2 locking order: on gfs2, transactions must be started before locking
> any pages. We can fix that by calling iomap_page_create from
> gfs2_page_mkwrite, or by doing the uninlining and allocations before
> calling iomap_page_mkwrite. I've implemented option 2 for now; see
> here:
I think this might also solve this problem?
