Re: [PATCH 03/24] xfs: mark inode buffers in cache

From: Amir Goldstein <amir73il@gmail.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 03/24] xfs: mark inode buffers in cache
Date: Fri, 22 May 2020 10:45:16 +0300	[thread overview]
Message-ID: <CAOQ4uxiVMSHN8wmqpExZ9UvHEhopUAoYJTrbD_HwfSo9J39Mmw@mail.gmail.com> (raw)
In-Reply-To: <20200522035029.3022405-4-david@fromorbit.com>

On Fri, May 22, 2020 at 6:51 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Inode buffers always have write IO callbacks, so by marking them
> directly we can avoid needing to attach ->b_iodone functions to
> them. This avoids an indirect call, and makes future modifications
> much simpler.
>
> This is largely a rearrangement of the code at this point - no IO
> completion functionality changes at this point, just how the
> code is run is modified.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_buf.c       | 18 +++++++++++++-----
>  fs/xfs/xfs_buf.h       | 39 ++++++++++++++++++++++++++-------------
>  fs/xfs/xfs_buf_item.c  | 42 +++++++++++++++++++++++++++++++-----------
>  fs/xfs/xfs_buf_item.h  |  1 +
>  fs/xfs/xfs_inode.c     |  2 +-
>  fs/xfs/xfs_trans_buf.c |  3 +++
>  6 files changed, 75 insertions(+), 30 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 9c2fbb6bbf89d..6105b97028d6a 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -14,6 +14,8 @@
>  #include "xfs_mount.h"
>  #include "xfs_trace.h"
>  #include "xfs_log.h"
> +#include "xfs_trans.h"
> +#include "xfs_buf_item.h"
>  #include "xfs_errortag.h"
>  #include "xfs_error.h"
>
> @@ -1202,12 +1204,18 @@ xfs_buf_ioend(
>                 bp->b_flags |= XBF_DONE;
>         }
>
> -       if (bp->b_iodone)
> +       /* inodes always have a callback on write */
> +       if (!read && (bp->b_flags & _XBF_INODES)) {
> +               xfs_buf_inode_iodone(bp);
> +               return;
> +       }
> +
> +       if (bp->b_iodone) {
>                 (*(bp->b_iodone))(bp);
> -       else if (bp->b_flags & XBF_ASYNC)
> -               xfs_buf_relse(bp);
> -       else
> -               complete(&bp->b_iowait);
> +               return;
> +       }
> +
> +       xfs_buf_ioend_finish(bp);
>  }
>
>  static void
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 050c53b739e24..b3e5d653d09f1 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -30,15 +30,19 @@
>  #define XBF_STALE       (1 << 6) /* buffer has been staled, do not find it */
>  #define XBF_WRITE_FAIL  (1 << 7) /* async writes have failed on this buffer */
>
> -/* flags used only as arguments to access routines */
> -#define XBF_TRYLOCK     (1 << 16)/* lock requested, but do not wait */
> -#define XBF_UNMAPPED    (1 << 17)/* do not map the buffer */
> +/* buffer type flags for write callbacks */
> +#define _XBF_INODES     (1 << 16)/* inode buffer */

As I wrote on review of another type flag, best add a definition
of XBF_BUFFER_TYPE_MASK and document that buffer type
flags are mutually exclusive, maybe even ASSERT it in some
places.

For not changing logic by rearranging code:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.