Re: [PATCH 5/5] xfs: don't chain ioends during writepage submission

Christoph Hellwig
2016-02-09T13:49:34Z

> +STATIC struct xfs_ioend *
> xfs_add_to_ioend(
> struct inode *inode,
> struct buffer_head *bh,
> xfs_off_t offset,
> struct xfs_writepage_ctx *wpc)
> {
> + struct xfs_ioend *ioend_to_submit = NULL;

Maybe just

struct xfs_ioend *prev = NULL;

to be a little less verbose?

> @@ -738,29 +726,22 @@ xfs_writepage_submit(
> struct writeback_control *wbc,
> int status)
> {
> - struct blk_plug plug;
> -
> - /* Reserve log space if we might write beyond the on-disk inode size. */
> - if (!status && wpc->ioend && wpc->ioend->io_type != XFS_IO_UNWRITTEN &&
> - xfs_ioend_is_append(wpc->ioend))
> - status = xfs_setfilesize_trans_alloc(wpc->ioend);
> -
> - if (wpc->iohead) {
> - blk_start_plug(&plug);
> - xfs_submit_ioend(wbc, wpc->iohead, status);
> - blk_finish_plug(&plug);
> - }
> + if (wpc->ioend)
> + xfs_submit_ioend(wbc, wpc->ioend, status);
> return status;
> }

With this change xfs_writepage_submit is rather pointless, I'd
rather open code it in the callers.

> + ioend = xfs_add_to_ioend(inode, bh, offset, wpc);
> + if (ioend) {
> + ioend->io_list = NULL;
> + if (!ioend_to_submit)
> + ioend_to_submit = ioend;
> + else
> + ioend_tail->io_list = ioend;
> + ioend_tail = ioend;
> + }

Just using a list_head for this is a lot easier to read and less
error prone at the cost of a single additional pointer in the ioend.

> + while (ioend_to_submit) {
> + struct xfs_ioend *next = ioend_to_submit->io_list;
> +
> + ioend_to_submit->io_list = NULL;
> + xfs_submit_ioend(wbc, ioend_to_submit, 0);
> + ioend_to_submit = next;
> + }
> return 0;
>
> out_error:
> @@ -853,9 +848,16 @@ out_error:
> * ioend, then we can't touch it here and need to rely on IO submission
> * to unlock it.
> */
> - if (count)
> + if (count) {
> xfs_start_page_writeback(page, 0, count);
> - else {
> + while (ioend_to_submit) {
> + struct xfs_ioend *next = ioend_to_submit->io_list;
> +
> + ioend_to_submit->io_list = NULL;
> + xfs_submit_ioend(wbc, ioend_to_submit, 0);
> + ioend_to_submit = next;
> + }

I think this code cold be consolidated:

ASSERT(wpc->ioend || !count);
out:
if (count) {
xfs_start_page_writeback(page, !error, count);
while (ioend_to_submit) {
struct xfs_ioend *next = ioend_to_submit->io_list;

ioend_to_submit->io_list = NULL;
xfs_submit_ioend(wbc, ioend_to_submit, 0);
ioend_to_submit = next;
}
} else {
xfs_aops_discard_page(page);
ClearPageUptodate(page);
unlock_page(page);
}

return error;