Re: [PATCH 2/5] xfs: Introduce writeback context for writepages

Christoph Hellwig
2016-02-09T13:39:48Z

This looks good in general and now passes testing for me. A couple
comments below:

>
> /*
> - * Cancel submission of all buffer_heads so far in this endio.
> - * Toss the endio too. Only ever called for the initial page
> - * in a writepage request, so only ever one page.
> - */
> -STATIC void
> -xfs_cancel_ioend(
> - xfs_ioend_t *ioend)
> -{
> - xfs_ioend_t *next;
> - struct buffer_head *bh, *next_bh;
> -
> - do {
> - next = ioend->io_list;
> - bh = ioend->io_buffer_head;
> - do {
> - next_bh = bh->b_private;
> - clear_buffer_async_write(bh);
> - /*
> - * The unwritten flag is cleared when added to the
> - * ioend. We're not submitting for I/O so mark the
> - * buffer unwritten again for next time around.
> - */
> - if (ioend->io_type == XFS_IO_UNWRITTEN)
> - set_buffer_unwritten(bh);
> - unlock_buffer(bh);
> - } while ((bh = next_bh) != NULL);
> -
> - mempool_free(ioend, xfs_ioend_pool);
> - } while ((ioend = next) != NULL);
> -}

Removing xfs_cancel_ioend and replacing it with the start and cancel
writeback scheme that we currently only use for
xfs_setfilesize_trans_alloc failures actually seems to be the biggest
change in this patch and is entirely undocumented. Any chance you
could split this into a prep patch and properly document it?

> -
> - if (!ioend || need_ioend || type != ioend->io_type) {
> - xfs_ioend_t *previous = *result;
> -
> - ioend = xfs_alloc_ioend(inode, type);
> - ioend->io_offset = offset;
> - ioend->io_buffer_head = bh;
> - ioend->io_buffer_tail = bh;
> - if (previous)
> - previous->io_list = ioend;
> - *result = ioend;
> + if (!wpc->ioend || wpc->io_type != wpc->ioend->io_type ||
> + bh->b_blocknr != wpc->last_block + 1) {

We now start a new ioend if the blocks aren't contiguous, which seems
reasonable. But this also means the similar check in xfs_submit_ioend
should be removed at the same time.