xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/5] xfs: don't chain ioends during writepage submission
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 9 Feb 2016 05:49:30 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1454910258-7578-6-git-send-email-david@xxxxxxxxxxxxx>
References: <1454910258-7578-1-git-send-email-david@xxxxxxxxxxxxx> <1454910258-7578-6-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
> +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;

<Prev in Thread] Current Thread [Next in Thread>