[PATCH RFC 0/2] xfs: refactor ordered buffer logging code

[PATCH RFC 0/2] xfs: refactor ordered buffer logging code

[Brian Foster]

Hi all,

Here's an initial refactoring based on the discussion on Alex's empty
xattr leaf buffer holding patch [1]. These patches split off some of
xfs_trans_log_buf() into a new helper and update xfs_trans_ordered_buf()
to use it and thus eliminate the need to log dirty ranges on ordered
buffers. The secondary purpose here is to create a function that we can
call from deferred ops processing to relog an already dirty/held buffer
across transaction rolls.

Note that this has only been very lightly tested and I might have one or
two more cleanups to tack on. Thoughts, reviews, flames appreciated...

Brian

[1] http://marc.info/?l=linux-xfs&m=150227679029089&w=2

Brian Foster (2):
  xfs: refactor buffer logging into buffer dirtying helper
  xfs: don't log dirty ranges for ordered buffers

 fs/xfs/libxfs/xfs_btree.c  |  3 +-
 fs/xfs/libxfs/xfs_ialloc.c |  2 --
 fs/xfs/xfs_trans.h         |  4 ++-
 fs/xfs/xfs_trans_buf.c     | 71 ++++++++++++++++++++++++++++------------------
 4 files changed, 48 insertions(+), 32 deletions(-)

-- 
2.9.4

[PATCH RFC 1/2] xfs: refactor buffer logging into buffer dirtying helper

[Brian Foster]

xfs_trans_log_buf() is responsible for logging the dirty segments of
a buffer along with setting all of the necessary state on the
transaction, buffer, bli, etc., to ensure that the associated items
are marked as dirty and prepared for I/O. We have a couple use cases
that need to to dirty a buffer in a transaction without actually
logging dirty ranges of the buffer.  One existing use case is
ordered buffers, which are currently logged with arbitrary ranges to
accomplish this even though the content of ordered buffers is never
written to the log. Another pending use case is to relog an already
dirty buffer across rolled transactions within the deferred
operations infrastructure. This is required to prevent a held
(XFS_BLI_HOLD) buffer from pinning the tail of the log.

Refactor xfs_trans_log_buf() into a new function that contains all
of the logic responsible to dirty the transaction, lidp, buffer and
bli. This new function can be used in the future for the use cases
outlined above. This patch does not introduce functional changes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_trans.h     |  4 +++-
 fs/xfs/xfs_trans_buf.c | 46 ++++++++++++++++++++++++++++++----------------
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 6bdad6f..a9c4404 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -213,7 +213,9 @@ void		xfs_trans_dquot_buf(xfs_trans_t *, struct xfs_buf *, uint);
 void		xfs_trans_inode_alloc_buf(xfs_trans_t *, struct xfs_buf *);
 void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
 void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
-void		xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
+void		xfs_trans_log_buf(struct xfs_trans *, struct xfs_buf *, uint,
+				  uint);
+void		xfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *);
 void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
 
 void		xfs_extent_free_init_defer_op(void);
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 86987d8..58818a0 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -493,25 +493,17 @@ xfs_trans_bhold_release(xfs_trans_t	*tp,
 }
 
 /*
- * This is called to mark bytes first through last inclusive of the given
- * buffer as needing to be logged when the transaction is committed.
- * The buffer must already be associated with the given transaction.
- *
- * First and last are numbers relative to the beginning of this buffer,
- * so the first byte in the buffer is numbered 0 regardless of the
- * value of b_blkno.
+ * Mark a buffer dirty in the transaction.
  */
 void
-xfs_trans_log_buf(xfs_trans_t	*tp,
-		  xfs_buf_t	*bp,
-		  uint		first,
-		  uint		last)
+xfs_trans_dirty_buf(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*bp)
 {
-	xfs_buf_log_item_t	*bip = bp->b_fspriv;
+	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
-	ASSERT(first <= last && last < BBTOB(bp->b_length));
 	ASSERT(bp->b_iodone == NULL ||
 	       bp->b_iodone == xfs_buf_iodone_callbacks);
 
@@ -531,8 +523,6 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
 	bp->b_iodone = xfs_buf_iodone_callbacks;
 	bip->bli_item.li_cb = xfs_buf_iodone;
 
-	trace_xfs_trans_log_buf(bip);
-
 	/*
 	 * If we invalidated the buffer within this transaction, then
 	 * cancel the invalidation now that we're dirtying the buffer
@@ -545,15 +535,39 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
 		bp->b_flags &= ~XBF_STALE;
 		bip->__bli_format.blf_flags &= ~XFS_BLF_CANCEL;
 	}
+	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
 
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+}
+
+/*
+ * This is called to mark bytes first through last inclusive of the given
+ * buffer as needing to be logged when the transaction is committed.
+ * The buffer must already be associated with the given transaction.
+ *
+ * First and last are numbers relative to the beginning of this buffer,
+ * so the first byte in the buffer is numbered 0 regardless of the
+ * value of b_blkno.
+ */
+void
+xfs_trans_log_buf(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*bp,
+	uint			first,
+	uint			last)
+{
+	struct xfs_buf_log_item	*bip = bp->b_fspriv;
+
+	ASSERT(first <= last && last < BBTOB(bp->b_length));
+
+	xfs_trans_dirty_buf(tp, bp);
 
 	/*
 	 * If we have an ordered buffer we are not logging any dirty range but
 	 * it still needs to be marked dirty and that it has been logged.
 	 */
-	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
+	trace_xfs_trans_log_buf(bip);
 	if (!(bip->bli_flags & XFS_BLI_ORDERED))
 		xfs_buf_item_log(bip, first, last);
 }
-- 
2.9.4

Re: [PATCH RFC 1/2] xfs: refactor buffer logging into buffer dirtying helper

[Darrick J. Wong]

On Mon, Aug 14, 2017 at 12:54:50PM -0400, Brian Foster wrote:
> xfs_trans_log_buf() is responsible for logging the dirty segments of
> a buffer along with setting all of the necessary state on the
> transaction, buffer, bli, etc., to ensure that the associated items
> are marked as dirty and prepared for I/O. We have a couple use cases
> that need to to dirty a buffer in a transaction without actually
> logging dirty ranges of the buffer.  One existing use case is
> ordered buffers, which are currently logged with arbitrary ranges to
> accomplish this even though the content of ordered buffers is never
> written to the log. Another pending use case is to relog an already
> dirty buffer across rolled transactions within the deferred
> operations infrastructure. This is required to prevent a held
> (XFS_BLI_HOLD) buffer from pinning the tail of the log.
> 
> Refactor xfs_trans_log_buf() into a new function that contains all
> of the logic responsible to dirty the transaction, lidp, buffer and
> bli. This new function can be used in the future for the use cases
> outlined above. This patch does not introduce functional changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_trans.h     |  4 +++-
>  fs/xfs/xfs_trans_buf.c | 46 ++++++++++++++++++++++++++++++----------------
>  2 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 6bdad6f..a9c4404 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -213,7 +213,9 @@ void		xfs_trans_dquot_buf(xfs_trans_t *, struct xfs_buf *, uint);
>  void		xfs_trans_inode_alloc_buf(xfs_trans_t *, struct xfs_buf *);
>  void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
>  void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
> -void		xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
> +void		xfs_trans_log_buf(struct xfs_trans *, struct xfs_buf *, uint,
> +				  uint);
> +void		xfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *);
>  void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>  
>  void		xfs_extent_free_init_defer_op(void);
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 86987d8..58818a0 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -493,25 +493,17 @@ xfs_trans_bhold_release(xfs_trans_t	*tp,
>  }
>  
>  /*
> - * This is called to mark bytes first through last inclusive of the given
> - * buffer as needing to be logged when the transaction is committed.
> - * The buffer must already be associated with the given transaction.
> - *
> - * First and last are numbers relative to the beginning of this buffer,
> - * so the first byte in the buffer is numbered 0 regardless of the
> - * value of b_blkno.
> + * Mark a buffer dirty in the transaction.
>   */
>  void
> -xfs_trans_log_buf(xfs_trans_t	*tp,
> -		  xfs_buf_t	*bp,
> -		  uint		first,
> -		  uint		last)
> +xfs_trans_dirty_buf(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*bp)
>  {
> -	xfs_buf_log_item_t	*bip = bp->b_fspriv;
> +	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	ASSERT(bp->b_transp == tp);
>  	ASSERT(bip != NULL);
> -	ASSERT(first <= last && last < BBTOB(bp->b_length));
>  	ASSERT(bp->b_iodone == NULL ||
>  	       bp->b_iodone == xfs_buf_iodone_callbacks);
>  
> @@ -531,8 +523,6 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
>  	bp->b_iodone = xfs_buf_iodone_callbacks;
>  	bip->bli_item.li_cb = xfs_buf_iodone;
>  
> -	trace_xfs_trans_log_buf(bip);
> -
>  	/*
>  	 * If we invalidated the buffer within this transaction, then
>  	 * cancel the invalidation now that we're dirtying the buffer
> @@ -545,15 +535,39 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
>  		bp->b_flags &= ~XBF_STALE;
>  		bip->__bli_format.blf_flags &= ~XFS_BLF_CANCEL;
>  	}
> +	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +}
> +
> +/*
> + * This is called to mark bytes first through last inclusive of the given
> + * buffer as needing to be logged when the transaction is committed.
> + * The buffer must already be associated with the given transaction.
> + *
> + * First and last are numbers relative to the beginning of this buffer,
> + * so the first byte in the buffer is numbered 0 regardless of the
> + * value of b_blkno.
> + */
> +void
> +xfs_trans_log_buf(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*bp,
> +	uint			first,
> +	uint			last)
> +{
> +	struct xfs_buf_log_item	*bip = bp->b_fspriv;
> +
> +	ASSERT(first <= last && last < BBTOB(bp->b_length));
> +
> +	xfs_trans_dirty_buf(tp, bp);
>  
>  	/*
>  	 * If we have an ordered buffer we are not logging any dirty range but
>  	 * it still needs to be marked dirty and that it has been logged.
>  	 */
> -	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
> +	trace_xfs_trans_log_buf(bip);
>  	if (!(bip->bli_flags & XFS_BLI_ORDERED))
>  		xfs_buf_item_log(bip, first, last);
>  }
> -- 
> 2.9.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/2] xfs: don't log dirty ranges for ordered buffers

[Brian Foster]

Ordered buffers are attached to transactions and pushed through the
logging infrastructure just like normal buffers with the exception
that they are not actually written to the log. Therefore, we don't
need to log dirty ranges of ordered buffers. xfs_trans_log_buf() is
called on ordered buffers to set up all of the dirty state on the
transaction, buffer and log item and prepare the buffer for I/O.

Now that xfs_trans_dirty_buf() is available, call it from
xfs_trans_ordered_buf() so the latter is now mutually exclusive with
xfs_trans_log_buf(). This reflects the implementation of ordered
buffers and helps eliminate confusion over the need to log ranges of
ordered buffers just to set up internal log state.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_btree.c  |  3 ++-
 fs/xfs/libxfs/xfs_ialloc.c |  2 --
 fs/xfs/xfs_trans_buf.c     | 25 +++++++++++++------------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index e0bcc4a..9c97896 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -4466,7 +4466,8 @@ xfs_btree_block_change_owner(
 	if (bp) {
 		if (cur->bc_tp) {
 			xfs_trans_ordered_buf(cur->bc_tp, bp);
-			xfs_btree_log_block(cur, bp, XFS_BB_OWNER);
+			/*xfs_trans_buf_set_type(cur->bc_tp, bp,
+					       XFS_BLFT_BTREE_BUF);*/
 		} else {
 			xfs_buf_delwri_queue(bp, bbcoi->buffer_list);
 		}
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index ffd5a15..12c0452 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -378,8 +378,6 @@ xfs_ialloc_inode_init(
 				 * transaction and pin the log appropriately.
 				 */
 				xfs_trans_ordered_buf(tp, fbuf);
-				xfs_trans_log_buf(tp, fbuf, 0,
-						  BBTOB(fbuf->b_length) - 1);
 			}
 		} else {
 			fbuf->b_flags |= XBF_DONE;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 58818a0..3a358cb 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -560,16 +560,12 @@ xfs_trans_log_buf(
 	struct xfs_buf_log_item	*bip = bp->b_fspriv;
 
 	ASSERT(first <= last && last < BBTOB(bp->b_length));
+	ASSERT(!(bip->bli_flags & XFS_BLI_ORDERED));
 
 	xfs_trans_dirty_buf(tp, bp);
 
-	/*
-	 * If we have an ordered buffer we are not logging any dirty range but
-	 * it still needs to be marked dirty and that it has been logged.
-	 */
 	trace_xfs_trans_log_buf(bip);
-	if (!(bip->bli_flags & XFS_BLI_ORDERED))
-		xfs_buf_item_log(bip, first, last);
+	xfs_buf_item_log(bip, first, last);
 }
 
 
@@ -722,12 +718,11 @@ xfs_trans_inode_alloc_buf(
 }
 
 /*
- * Mark the buffer as ordered for this transaction. This means
- * that the contents of the buffer are not recorded in the transaction
- * but it is tracked in the AIL as though it was. This allows us
- * to record logical changes in transactions rather than the physical
- * changes we make to the buffer without changing writeback ordering
- * constraints of metadata buffers.
+ * Mark the buffer as ordered for this transaction. This means that the contents
+ * of the buffer are not recorded in the transaction but it is tracked in the
+ * AIL as though it was. This allows us to record logical changes in
+ * transactions rather than the physical changes we make to the buffer without
+ * changing writeback ordering constraints of metadata buffers.
  */
 void
 xfs_trans_ordered_buf(
@@ -742,6 +737,12 @@ xfs_trans_ordered_buf(
 
 	bip->bli_flags |= XFS_BLI_ORDERED;
 	trace_xfs_buf_item_ordered(bip);
+
+	/*
+	 * We don't log a dirty range of an ordered buffer but it still needs
+	 * to be marked dirty and that it has been logged.
+	 */
+	xfs_trans_dirty_buf(tp, bp);
 }
 
 /*
-- 
2.9.4

Re: [PATCH RFC 2/2] xfs: don't log dirty ranges for ordered buffers

[Dave Chinner]

On Mon, Aug 14, 2017 at 12:54:51PM -0400, Brian Foster wrote:
> Ordered buffers are attached to transactions and pushed through the
> logging infrastructure just like normal buffers with the exception
> that they are not actually written to the log. Therefore, we don't
> need to log dirty ranges of ordered buffers. xfs_trans_log_buf() is
> called on ordered buffers to set up all of the dirty state on the
> transaction, buffer and log item and prepare the buffer for I/O.
> 
> Now that xfs_trans_dirty_buf() is available, call it from
> xfs_trans_ordered_buf() so the latter is now mutually exclusive with
> xfs_trans_log_buf(). This reflects the implementation of ordered
> buffers and helps eliminate confusion over the need to log ranges of
> ordered buffers just to set up internal log state.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_btree.c  |  3 ++-
>  fs/xfs/libxfs/xfs_ialloc.c |  2 --
>  fs/xfs/xfs_trans_buf.c     | 25 +++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index e0bcc4a..9c97896 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -4466,7 +4466,8 @@ xfs_btree_block_change_owner(
>  	if (bp) {
>  		if (cur->bc_tp) {
>  			xfs_trans_ordered_buf(cur->bc_tp, bp);
> -			xfs_btree_log_block(cur, bp, XFS_BB_OWNER);
> +			/*xfs_trans_buf_set_type(cur->bc_tp, bp,
> +					       XFS_BLFT_BTREE_BUF);*/

Not sure we need this - a owner change is a rare once-off operation
(e.g. defrag) and so typically we don't need to keep the btree
blocks cached for next reuse.

i.e. the logging of the btree block would do this to reset the age
of the buffer in the LRU as we're expecting it to be used again
soon. e.g. next allocation to that file, next allocation from an AG,
next allocation of an inode, etc.

xfs_btree_block_change_owner() just isn't one of those "likely to
use again shortly" type operations, so I think you can just get rid
of it...

Otherwise the patches look good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 2/2] xfs: don't log dirty ranges for ordered buffers

[Darrick J. Wong]

On Mon, Aug 14, 2017 at 12:54:51PM -0400, Brian Foster wrote:
> Ordered buffers are attached to transactions and pushed through the
> logging infrastructure just like normal buffers with the exception
> that they are not actually written to the log. Therefore, we don't
> need to log dirty ranges of ordered buffers. xfs_trans_log_buf() is
> called on ordered buffers to set up all of the dirty state on the
> transaction, buffer and log item and prepare the buffer for I/O.
> 
> Now that xfs_trans_dirty_buf() is available, call it from
> xfs_trans_ordered_buf() so the latter is now mutually exclusive with
> xfs_trans_log_buf(). This reflects the implementation of ordered
> buffers and helps eliminate confusion over the need to log ranges of
> ordered buffers just to set up internal log state.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_btree.c  |  3 ++-
>  fs/xfs/libxfs/xfs_ialloc.c |  2 --
>  fs/xfs/xfs_trans_buf.c     | 25 +++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index e0bcc4a..9c97896 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -4466,7 +4466,8 @@ xfs_btree_block_change_owner(
>  	if (bp) {
>  		if (cur->bc_tp) {
>  			xfs_trans_ordered_buf(cur->bc_tp, bp);
> -			xfs_btree_log_block(cur, bp, XFS_BB_OWNER);
> +			/*xfs_trans_buf_set_type(cur->bc_tp, bp,
> +					       XFS_BLFT_BTREE_BUF);*/

I don't see the point of this, but maybe you've already dropped it anyway. :)

>  		} else {
>  			xfs_buf_delwri_queue(bp, bbcoi->buffer_list);
>  		}
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index ffd5a15..12c0452 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -378,8 +378,6 @@ xfs_ialloc_inode_init(
>  				 * transaction and pin the log appropriately.
>  				 */
>  				xfs_trans_ordered_buf(tp, fbuf);
> -				xfs_trans_log_buf(tp, fbuf, 0,
> -						  BBTOB(fbuf->b_length) - 1);
>  			}
>  		} else {
>  			fbuf->b_flags |= XBF_DONE;
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 58818a0..3a358cb 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -560,16 +560,12 @@ xfs_trans_log_buf(
>  	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	ASSERT(first <= last && last < BBTOB(bp->b_length));
> +	ASSERT(!(bip->bli_flags & XFS_BLI_ORDERED));
>  
>  	xfs_trans_dirty_buf(tp, bp);
>  
> -	/*
> -	 * If we have an ordered buffer we are not logging any dirty range but
> -	 * it still needs to be marked dirty and that it has been logged.
> -	 */
>  	trace_xfs_trans_log_buf(bip);
> -	if (!(bip->bli_flags & XFS_BLI_ORDERED))
> -		xfs_buf_item_log(bip, first, last);
> +	xfs_buf_item_log(bip, first, last);
>  }
>  
>  
> @@ -722,12 +718,11 @@ xfs_trans_inode_alloc_buf(
>  }
>  
>  /*
> - * Mark the buffer as ordered for this transaction. This means
> - * that the contents of the buffer are not recorded in the transaction
> - * but it is tracked in the AIL as though it was. This allows us
> - * to record logical changes in transactions rather than the physical
> - * changes we make to the buffer without changing writeback ordering
> - * constraints of metadata buffers.
> + * Mark the buffer as ordered for this transaction. This means that the contents
> + * of the buffer are not recorded in the transaction but it is tracked in the
> + * AIL as though it was. This allows us to record logical changes in
> + * transactions rather than the physical changes we make to the buffer without
> + * changing writeback ordering constraints of metadata buffers.

Did the text of this comment change?  AFAICT the only difference is
where we line wrap?

(If that's the case, why bother?)

>   */
>  void
>  xfs_trans_ordered_buf(
> @@ -742,6 +737,12 @@ xfs_trans_ordered_buf(
>  
>  	bip->bli_flags |= XFS_BLI_ORDERED;
>  	trace_xfs_buf_item_ordered(bip);
> +
> +	/*
> +	 * We don't log a dirty range of an ordered buffer but it still needs
> +	 * to be marked dirty and that it has been logged.
> +	 */
> +	xfs_trans_dirty_buf(tp, bp);

Otherwise looks ok to me...

--D

>  }
>  
>  /*
> -- 
> 2.9.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] xfs: don't log dirty ranges for ordered buffers

[Brian Foster]

On Wed, Aug 16, 2017 at 10:15:01AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 14, 2017 at 12:54:51PM -0400, Brian Foster wrote:
> > Ordered buffers are attached to transactions and pushed through the
> > logging infrastructure just like normal buffers with the exception
> > that they are not actually written to the log. Therefore, we don't
> > need to log dirty ranges of ordered buffers. xfs_trans_log_buf() is
> > called on ordered buffers to set up all of the dirty state on the
> > transaction, buffer and log item and prepare the buffer for I/O.
> > 
> > Now that xfs_trans_dirty_buf() is available, call it from
> > xfs_trans_ordered_buf() so the latter is now mutually exclusive with
> > xfs_trans_log_buf(). This reflects the implementation of ordered
> > buffers and helps eliminate confusion over the need to log ranges of
> > ordered buffers just to set up internal log state.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_btree.c  |  3 ++-
> >  fs/xfs/libxfs/xfs_ialloc.c |  2 --
> >  fs/xfs/xfs_trans_buf.c     | 25 +++++++++++++------------
> >  3 files changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > index e0bcc4a..9c97896 100644
> > --- a/fs/xfs/libxfs/xfs_btree.c
> > +++ b/fs/xfs/libxfs/xfs_btree.c
> > @@ -4466,7 +4466,8 @@ xfs_btree_block_change_owner(
> >  	if (bp) {
> >  		if (cur->bc_tp) {
> >  			xfs_trans_ordered_buf(cur->bc_tp, bp);
> > -			xfs_btree_log_block(cur, bp, XFS_BB_OWNER);
> > +			/*xfs_trans_buf_set_type(cur->bc_tp, bp,
> > +					       XFS_BLFT_BTREE_BUF);*/
> 
> I don't see the point of this, but maybe you've already dropped it anyway. :)
> 

Yes, it's been dropped. I had it there to remind myself to look further
into it because it looked like the only part of xfs_btree_log_block()
that could have any effect on an ordered buffer.

> >  		} else {
> >  			xfs_buf_delwri_queue(bp, bbcoi->buffer_list);
> >  		}
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index ffd5a15..12c0452 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -378,8 +378,6 @@ xfs_ialloc_inode_init(
> >  				 * transaction and pin the log appropriately.
> >  				 */
> >  				xfs_trans_ordered_buf(tp, fbuf);
> > -				xfs_trans_log_buf(tp, fbuf, 0,
> > -						  BBTOB(fbuf->b_length) - 1);
> >  			}
> >  		} else {
> >  			fbuf->b_flags |= XBF_DONE;
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index 58818a0..3a358cb 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -560,16 +560,12 @@ xfs_trans_log_buf(
> >  	struct xfs_buf_log_item	*bip = bp->b_fspriv;
> >  
> >  	ASSERT(first <= last && last < BBTOB(bp->b_length));
> > +	ASSERT(!(bip->bli_flags & XFS_BLI_ORDERED));
> >  
> >  	xfs_trans_dirty_buf(tp, bp);
> >  
> > -	/*
> > -	 * If we have an ordered buffer we are not logging any dirty range but
> > -	 * it still needs to be marked dirty and that it has been logged.
> > -	 */
> >  	trace_xfs_trans_log_buf(bip);
> > -	if (!(bip->bli_flags & XFS_BLI_ORDERED))
> > -		xfs_buf_item_log(bip, first, last);
> > +	xfs_buf_item_log(bip, first, last);
> >  }
> >  
> >  
> > @@ -722,12 +718,11 @@ xfs_trans_inode_alloc_buf(
> >  }
> >  
> >  /*
> > - * Mark the buffer as ordered for this transaction. This means
> > - * that the contents of the buffer are not recorded in the transaction
> > - * but it is tracked in the AIL as though it was. This allows us
> > - * to record logical changes in transactions rather than the physical
> > - * changes we make to the buffer without changing writeback ordering
> > - * constraints of metadata buffers.
> > + * Mark the buffer as ordered for this transaction. This means that the contents
> > + * of the buffer are not recorded in the transaction but it is tracked in the
> > + * AIL as though it was. This allows us to record logical changes in
> > + * transactions rather than the physical changes we make to the buffer without
> > + * changing writeback ordering constraints of metadata buffers.
> 
> Did the text of this comment change?  AFAICT the only difference is
> where we line wrap?
> 
> (If that's the case, why bother?)
> 

The text hasn't changed. This just fixes the wrapping to 80 chars, which
is just one of those things I tend to fix up when already making changes
in a particular function (along with similar cleanups to code
indentation, eliminating the old typedef usages, etc.).

> >   */
> >  void
> >  xfs_trans_ordered_buf(
> > @@ -742,6 +737,12 @@ xfs_trans_ordered_buf(
> >  
> >  	bip->bli_flags |= XFS_BLI_ORDERED;
> >  	trace_xfs_buf_item_ordered(bip);
> > +
> > +	/*
> > +	 * We don't log a dirty range of an ordered buffer but it still needs
> > +	 * to be marked dirty and that it has been logged.
> > +	 */
> > +	xfs_trans_dirty_buf(tp, bp);
> 
> Otherwise looks ok to me...
> 

Thanks..

Brian

> --D
> 
> >  }
> >  
> >  /*
> > -- 
> > 2.9.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/2] xfs: refactor ordered buffer logging code

[Allison Henderson]

Based on previous discussions, these two look good to me.  You can add 
my review:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 08/14/2017 09:54 AM, Brian Foster wrote:
> Hi all,
>
> Here's an initial refactoring based on the discussion on Alex's empty
> xattr leaf buffer holding patch [1]. These patches split off some of
> xfs_trans_log_buf() into a new helper and update xfs_trans_ordered_buf()
> to use it and thus eliminate the need to log dirty ranges on ordered
> buffers. The secondary purpose here is to create a function that we can
> call from deferred ops processing to relog an already dirty/held buffer
> across transaction rolls.
>
> Note that this has only been very lightly tested and I might have one or
> two more cleanups to tack on. Thoughts, reviews, flames appreciated...
>
> Brian
>
> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__marc.info_-3Fl-3Dlinux-2Dxfs-26m-3D150227679029089-26w-3D2&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XFp4B05bcXkJ0dhYaFjd3F8telP01COkBp9cI7mKLb4&m=uWQD8gXmLnvThUaUiXNvMO_K_m7Ggg0h8HanYV0UWOY&s=Tw0p-QrZOdg2EYm3cUzm2ND1Fsoy98wwY9VG8I1viEQ&e=
>
> Brian Foster (2):
>    xfs: refactor buffer logging into buffer dirtying helper
>    xfs: don't log dirty ranges for ordered buffers
>
>   fs/xfs/libxfs/xfs_btree.c  |  3 +-
>   fs/xfs/libxfs/xfs_ialloc.c |  2 --
>   fs/xfs/xfs_trans.h         |  4 ++-
>   fs/xfs/xfs_trans_buf.c     | 71 ++++++++++++++++++++++++++++------------------
>   4 files changed, 48 insertions(+), 32 deletions(-)
>