xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: Introduce writeback context for writepages
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 9 Feb 2016 05:39:41 -0800
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1454910258-7578-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1454910258-7578-1-git-send-email-david@xxxxxxxxxxxxx> <1454910258-7578-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
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.

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