Re: [PATCH 16/18] xfs: refactor the tail of xfs_writepage_map

Brian Foster
2018-05-31T13:49:38Z

On Wed, May 30, 2018 at 12:00:11PM +0200, Christoph Hellwig wrote:
> Rejuggle how we deal with the different error vs non-error and have
> ioends vs not have ioend cases to keep the fast path streamlined, and
> the duplicate code at a minimum.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> fs/xfs/xfs_aops.c | 65 +++++++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 38021023131e..ac417ef326a9 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -873,7 +873,14 @@ xfs_writepage_map(
> * submission of outstanding ioends on the writepage context so they are
> * treated correctly on error.
> */
> - if (count) {
> + if (unlikely(error)) {
> + if (!count) {
> + xfs_aops_discard_page(page);
> + ClearPageUptodate(page);
> + unlock_page(page);
> + goto done;
> + }
> +
> /*
> * If the page was not fully cleaned, we need to ensure that the
> * higher layers come back to it correctly. That means we need
> @@ -882,43 +889,35 @@ xfs_writepage_map(
> * so another attempt to write this page in this writeback sweep
> * will be made.
> */
> - if (error) {
> - set_page_writeback_keepwrite(page);
> - } else {
> - clear_page_dirty_for_io(page);
> - set_page_writeback(page);
> - }
> - unlock_page(page);
> -
> - /*
> - * Preserve the original error if there was one, otherwise catch
> - * submission errors here and propagate into subsequent ioend
> - * submissions.
> - */
> - list_for_each_entry_safe(ioend, next, &submit_list, io_list) {
> - int error2;
> -
> - list_del_init(&ioend->io_list);
> - error2 = xfs_submit_ioend(wbc, ioend, error);
> - if (error2 && !error)
> - error = error2;
> - }
> - } else if (error) {
> - xfs_aops_discard_page(page);
> - ClearPageUptodate(page);
> - unlock_page(page);
> + set_page_writeback_keepwrite(page);
> } else {
> - /*
> - * We can end up here with no error and nothing to write if we
> - * race with a partial page truncate on a sub-page block sized
> - * filesystem. In that case we need to mark the page clean.
> - */
> clear_page_dirty_for_io(page);
> set_page_writeback(page);
> - unlock_page(page);
> - end_page_writeback(page);
> }
>
> + unlock_page(page);
> +
> + /*
> + * Preserve the original error if there was one, otherwise catch
> + * submission errors here and propagate into subsequent ioend
> + * submissions.
> + */
> + list_for_each_entry_safe(ioend, next, &submit_list, io_list) {
> + int error2;
> +
> + list_del_init(&ioend->io_list);
> + error2 = xfs_submit_ioend(wbc, ioend, error);
> + if (error2 && !error)
> + error = error2;
> + }
> +
> + /*
> + * We can end up here with no error and nothing to write if we race with
> + * a partial page truncate on a sub-page block sized filesystem.
> + */
> + if (!count)
> + end_page_writeback(page);
> +done:
> mapping_set_error(page->mapping, error);
> return error;