On Fri, Aug 25, 2017 at 11:05:50AM -0400, Brian Foster wrote:
> xfs_buf_item_unlock() historically checked the dirty state of the
> buffer by manually checking the buffer log formats for dirty
> segments. The introduction of ordered buffers invalidated this check
> because ordered buffers have dirty bli's but no dirty (logged)
> segments. The check was updated to accommodate ordered buffers by
> looking at the bli state first and considering the blf only if the
> bli is clean.
>
> This logic is safe but unnecessary. There is no valid case where the
> bli is clean yet the blf has dirty segments. The bli is set dirty
> whenever the blf is logged (via xfs_trans_log_buf()) and the blf is
> cleared in the only place BLI_DIRTY is cleared (xfs_trans_binval()).
>
> Remove the conditional blf dirty checks and replace with an assert
> that should catch any discrepencies between bli and blf dirty
> states. Refactor the old blf dirty check into a helper function to
> be used by the assert.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/xfs_buf_item.c | 46 +++++++++++++++++++++++++++++++---------------
> fs/xfs/xfs_buf_item.h | 1 +
> 2 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index cdae0ad5e0..9f4dbca 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -575,8 +575,9 @@ xfs_buf_item_unlock(
> {
> struct xfs_buf_log_item *bip = BUF_ITEM(lip);
> struct xfs_buf *bp = bip->bli_buf;
> - bool clean;
> + bool dirty;
> bool aborted;
> + bool ordered;
> int flags;
>
> /* Clear the buffer's association with this transaction. */
> @@ -620,20 +621,13 @@ xfs_buf_item_unlock(
> * regardless of whether it is dirty or not. A dirty abort implies a
> * shutdown, anyway.
> *
> - * Ordered buffers are dirty but may have no recorded changes, so ensure
> - * we only release clean items here.
> + * The bli dirty state should match whether the blf has logged segments
> + * except for ordered buffers, where only the bli should be dirty.
> */
> - clean = (flags & XFS_BLI_DIRTY) ? false : true;
> - if (clean) {
> - int i;
> - for (i = 0; i < bip->bli_format_count; i++) {
> - if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
> - bip->bli_formats[i].blf_map_size)) {
> - clean = false;
> - break;
> - }
> - }
> - }
> + dirty = (flags & XFS_BLI_DIRTY) ? true : false;
> + ordered = (flags & XFS_BLI_ORDERED) ? true : false;
> + ASSERT((!ordered && dirty == xfs_buf_item_dirty_format(bip)) ||
> + (ordered && dirty && !xfs_buf_item_dirty_format(bip)));
>
> /*
> * Clean buffers, by definition, cannot be in the AIL. However, aborted
> @@ -652,7 +646,7 @@ xfs_buf_item_unlock(
> ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
> xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> xfs_buf_item_relse(bp);
> - } else if (clean)
> + } else if (!dirty)
> xfs_buf_item_relse(bp);
> }
>
> @@ -945,6 +939,28 @@ xfs_buf_item_log(
> }
>
>
> +/*
> + * Return true if the buffer has any ranges logged/dirtied by a transaction,
> + * false otherwise.
> + */
> +bool
> +xfs_buf_item_dirty_format(
> + struct xfs_buf_log_item *bip)
> +{
> + int i;
> + bool dirty = false;
> +
> + for (i = 0; i < bip->bli_format_count; i++) {
> + if (!xfs_bitmap_empty(bip->bli_formats[i].blf_data_map,
> + bip->bli_formats[i].blf_map_size)) {
> + dirty = true;
> + break;
/me imagines this could be shortened to 'return true', but looks ok otherwise,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> + }
> + }
> +
> + return dirty;
> +}
> +
> STATIC void
> xfs_buf_item_free(
> xfs_buf_log_item_t *bip)
> diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
> index e0e744a..9690ce6 100644
> --- a/fs/xfs/xfs_buf_item.h
> +++ b/fs/xfs/xfs_buf_item.h
> @@ -64,6 +64,7 @@ typedef struct xfs_buf_log_item {
> int xfs_buf_item_init(struct xfs_buf *, struct xfs_mount *);
> void xfs_buf_item_relse(struct xfs_buf *);
> void xfs_buf_item_log(xfs_buf_log_item_t *, uint, uint);
> +bool xfs_buf_item_dirty_format(struct xfs_buf_log_item *);
> void xfs_buf_attach_iodone(struct xfs_buf *,
> void(*)(struct xfs_buf *, xfs_log_item_t *),
> xfs_log_item_t *);
> --
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
