xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 6/8] xfs: don't chain ioends during writepage submission
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 10 Feb 2016 03:36:09 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1455094043-9694-7-git-send-email-david@xxxxxxxxxxxxx>
References: <1455094043-9694-1-git-send-email-david@xxxxxxxxxxxxx> <1455094043-9694-7-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
> -STATIC void
> +STATIC int
>  xfs_submit_ioend(
>       struct writeback_control *wbc,
>       xfs_ioend_t             *ioend,
>       int                     fail)

No that almost all of the function is rewritten can you rename
fail to error or status?  fail always suggests a boolean to me and
is rather confusing.

> + * Return the ioend we finished off so that the caller can submit it
> + * once it has finished processing the dirty page.
>   */
> -STATIC void
> +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        *prev = NULL;
> +
>       if (!wpc->ioend || wpc->io_type != wpc->ioend->io_type ||
>           bh->b_blocknr != wpc->last_block + 1) {
>               struct xfs_ioend        *new;
>  
> +             prev = wpc->ioend;

Looking at the new list_head based code it might be either to just
pass in a pointer to the submit_list and just add the previous ioend
here directly.

> +     LIST_HEAD(submit_list);
> +     struct xfs_ioend        *ioend, *next;
>       struct buffer_head      *bh, *head;
>       ssize_t                 len = 1 << inode->i_blkbits;
>       int                     error = 0;
>       int                     uptodate = 1;
>       int                     count = 0;

The count variable is pointless now - we only check for it being
non-zero, and we can do the same with a list_emptry on submit_list.

>  
> +
>       bh = head = page_buffers(page);
>       offset = page_offset(page);

nit: pointless second empty line added above

>       ret = xfs_do_writepage(page, wbc, &wpc);
> -     return xfs_writepage_submit(&wpc, wbc, ret);
> +     if (wpc.ioend)
> +             xfs_submit_ioend(wbc, wpc.ioend, ret);
> +     return ret;
>  }
>  
>  STATIC int
> @@ -1019,7 +1022,9 @@ xfs_vm_writepages(
>  
>       xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED);
>       ret = write_cache_pages(mapping, wbc, xfs_do_writepage, &wpc);
> -     return xfs_writepage_submit(&wpc, wbc, ret);
> +     if (wpc.ioend)
> +             xfs_submit_ioend(wbc, wpc.ioend, ret);
> +     return ret;

And this is where ignoreing the xfs_setfilesize_trans_alloc errors
reappears after a previous patch mostly fixed it up.

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