On Mon, Jun 28, 2021 at 7:40 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> 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?
It probably would. Let me do some testing based on that.
> > (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?
Probably yes.
Thanks,
Andreas
