On Wed, Oct 16, 2019 at 09:07:21AM +1100, Dave Chinner wrote:
> On Tue, Oct 15, 2019 at 05:43:42PM +0200, Christoph Hellwig wrote:
> > Take the xfs writeback code and move it to fs/iomap. A new structure
> > with three methods is added as the abstraction from the generic writeback
> > code to the file system. These methods are used to map blocks, submit an
> > ioend, and cancel a page that encountered an error before it was added to
> > an ioend.
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> > fs/iomap/buffered-io.c | 564 ++++++++++++++++++++++++++++++++++-
> > fs/iomap/trace.h | 39 +++
> > fs/xfs/xfs_aops.c | 662 ++++-------------------------------------
> > fs/xfs/xfs_aops.h | 17 --
> > fs/xfs/xfs_super.c | 11 +-
> > fs/xfs/xfs_trace.h | 39 ---
> > include/linux/iomap.h | 59 ++++
> > 7 files changed, 722 insertions(+), 669 deletions(-)
> .....
> > @@ -468,6 +471,8 @@ EXPORT_SYMBOL_GPL(iomap_is_partially_uptodate);
> > int
> > iomap_releasepage(struct page *page, gfp_t gfp_mask)
> > {
> > + trace_iomap_releasepage(page->mapping->host, page, 0, 0);
> > +
> > /*
> > * mm accommodates an old ext3 case where clean pages might not have had
> > * the dirty bit cleared. Thus, it can send actual dirty pages to
> > @@ -483,6 +488,8 @@ EXPORT_SYMBOL_GPL(iomap_releasepage);
> > void
> > iomap_invalidatepage(struct page *page, unsigned int offset, unsigned int len)
> > {
> > + trace_iomap_invalidatepage(page->mapping->host, page, offset, len);
> > +
>
> These tracepoints should be split out into a separate patch like
> the readpage(s) tracepoints. Maybe just lift all the non-writeback
> ones in a single patch...
>
> > /*
> > * If we are invalidating the entire page, clear the dirty state from it
> > * and release it to avoid unnecessary buildup of the LRU.
> > @@ -1084,3 +1091,558 @@ vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops)
> > return block_page_mkwrite_return(ret);
> > }
> > EXPORT_SYMBOL_GPL(iomap_page_mkwrite);
> > +
> > +static void
> > +iomap_finish_page_writeback(struct inode *inode, struct bio_vec *bvec,
> > + int error)
> > +{
> > + struct iomap_page *iop = to_iomap_page(bvec->bv_page);
> > +
> > + if (error) {
> > + SetPageError(bvec->bv_page);
> > + mapping_set_error(inode->i_mapping, -EIO);
> > + }
> > +
> > + WARN_ON_ONCE(i_blocksize(inode) < PAGE_SIZE && !iop);
> > + WARN_ON_ONCE(iop && atomic_read(&iop->write_count) <= 0);
> > +
> > + if (!iop || atomic_dec_and_test(&iop->write_count))
> > + end_page_writeback(bvec->bv_page);
> > +}
>
> Can we just pass the struct page into this function?
>
> .....
>
> > +/*
> > + * Submit the bio for an ioend. We are passed an ioend with a bio attached to
> > + * it, and we submit that bio. The ioend may be used for multiple bio
> > + * submissions, so we only want to allocate an append transaction for the ioend
> > + * once. In the case of multiple bio submission, each bio will take an IO
>
> This needs to be changed to describe what wpc->ops->submit_ioend()
> is used for rather than what XFS might use this hook for.
>
> > + * reference to the ioend to ensure that the ioend completion is only done once
> > + * all bios have been submitted and the ioend is really done.
> > + *
> > + * If @error is non-zero, it means that we have a situation where some part of
> > + * the submission process has failed after we have marked paged for writeback
> > + * and unlocked them. In this situation, we need to fail the bio and ioend
> > + * rather than submit it to IO. This typically only happens on a filesystem
> > + * shutdown.
> > + */
> > +static int
> > +iomap_submit_ioend(struct iomap_writepage_ctx *wpc, struct iomap_ioend *ioend,
> > + int error)
> > +{
> > + ioend->io_bio->bi_private = ioend;
> > + ioend->io_bio->bi_end_io = iomap_writepage_end_bio;
> > +
> > + if (wpc->ops->submit_ioend)
> > + error = wpc->ops->submit_ioend(ioend, error);
>
> I'm not sure that "submit_ioend" is the best name for this method,
> as it is a pre-bio-submission hook, not an actual IO submission
> method. "prepare_ioend_for_submit" is more descriptive, but probably
> too long. wpc->ops->prepare_submit(ioend, error) reads pretty well,
> though...
->prepare_submission() ?
--D
> > + if (error) {
> > + /*
> > + * If we are failing the IO now, just mark the ioend with an
> > + * error and finish it. This will run IO completion immediately
> > + * as there is only one reference to the ioend at this point in
> > + * time.
> > + */
> > + ioend->io_bio->bi_status = errno_to_blk_status(error);
> > + bio_endio(ioend->io_bio);
> > + return error;
> > + }
> > +
> > + submit_bio(ioend->io_bio);
> > + return 0;
> > +}
>
> .....
> > +/*
> > + * We implement an immediate ioend submission policy here to avoid needing to
> > + * chain multiple ioends and hence nest mempool allocations which can violate
> > + * forward progress guarantees we need to provide. The current ioend we are
> > + * adding blocks to is cached on the writepage context, and if the new block
>
> adding pages to ... , and if the new block mapping
>
> > + * does not append to the cached ioend it will create a new ioend and cache that
> > + * instead.
> > + *
> > + * If a new ioend is created and cached, the old ioend is returned and queued
> > + * locally for submission once the entire page is processed or an error has been
> > + * detected. While ioends are submitted immediately after they are completed,
> > + * batching optimisations are provided by higher level block plugging.
> > + *
> > + * At the end of a writeback pass, there will be a cached ioend remaining on the
> > + * writepage context that the caller will need to submit.
> > + */
> > +static int
> > +iomap_writepage_map(struct iomap_writepage_ctx *wpc,
> > + struct writeback_control *wbc, struct inode *inode,
> > + struct page *page, u64 end_offset)
> > +{
> > + struct iomap_page *iop = to_iomap_page(page);
> > + struct iomap_ioend *ioend, *next;
> > + unsigned len = i_blocksize(inode);
> > + u64 file_offset; /* file offset of page */
> > + int error = 0, count = 0, i;
> > + LIST_HEAD(submit_list);
> > +
> > + WARN_ON_ONCE(i_blocksize(inode) < PAGE_SIZE && !iop);
> > + WARN_ON_ONCE(iop && atomic_read(&iop->write_count) != 0);
> > +
> > + /*
> > + * Walk through the page to find areas to write back. If we run off the
> > + * end of the current map or find the current map invalid, grab a new
> > + * one.
> > + */
> > + for (i = 0, file_offset = page_offset(page);
> > + i < (PAGE_SIZE >> inode->i_blkbits) && file_offset < end_offset;
> > + i++, file_offset += len) {
> > + if (iop && !test_bit(i, iop->uptodate))
> > + continue;
> > +
> > + error = wpc->ops->map_blocks(wpc, inode, file_offset);
> > + if (error)
> > + break;
> > + if (wpc->iomap.type == IOMAP_HOLE)
> > + continue;
> > + iomap_add_to_ioend(inode, file_offset, page, iop, wpc, wbc,
> > + &submit_list);
> > + count++;
> > + }
> > +
> > + WARN_ON_ONCE(!wpc->ioend && !list_empty(&submit_list));
> > + WARN_ON_ONCE(!PageLocked(page));
> > + WARN_ON_ONCE(PageWriteback(page));
> > +
> > + /*
> > + * On error, we have to fail the ioend here because we may have set
> > + * pages under writeback, we have to make sure we run IO completion to
> > + * mark the error state of the IO appropriately, so we can't cancel the
> > + * ioend directly here.
>
> Few too many commas and run-ons here. Maybe reword it like this:
>
> /*
> * We cannot cancel the ioend directly here if there is a submission
> * error. We may have already set pages under writeback and hence we
> * have to run IO completion to mark the error state of the pages under
> * writeback appropriately.
>
> >
> >
> > That means we have to mark this page as under
> > + * writeback if we included any blocks from it in the ioend chain so
> > + * that completion treats it correctly.
> > + *
> > + * If we didn't include the page in the ioend, the on error we can
> then on error
>
> > + * simply discard and unlock it as there are no other users of the page
> > + * now. The caller will still need to trigger submission of outstanding
> > + * ioends on the writepage context so they are treated correctly on
> > + * error.
> > + */
>
> .....
>
> > +static int
> > +iomap_do_writepage(struct page *page, struct writeback_control *wbc, void *data)
> > +{
> > + struct iomap_writepage_ctx *wpc = data;
> > + struct inode *inode = page->mapping->host;
> > + pgoff_t end_index;
> > + u64 end_offset;
> > + loff_t offset;
> > +
> > + trace_iomap_writepage(inode, page, 0, 0);
> > +
> > + /*
> > + * Refuse to write the page out if we are called from reclaim context.
> > + *
> > + * This avoids stack overflows when called from deeply used stacks in
> > + * random callers for direct reclaim or memcg reclaim. We explicitly
> > + * allow reclaim from kswapd as the stack usage there is relatively low.
> > + *
> > + * This should never happen except in the case of a VM regression so
> > + * warn about it.
> > + */
> > + if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) ==
> > + PF_MEMALLOC))
> > + goto redirty;
> > +
> > + /*
> > + * Given that we do not allow direct reclaim to call us, we should
> > + * never be called while in a filesystem transaction.
> > + */
>
> never be called in a recursive filesystem reclaim context.
>
> > + if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS))
> > + goto redirty;
> > +
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
