[3/4] xfsprogs: trivial changes to libxfs/trans.c
diff mbox series

Message ID 614554f7-2e1a-775e-0828-50b1307f1f09@redhat.com
State New
Headers show
Series
  • xfsprogs: inch libxfs/trans.c towards xfs_trans_buf.c
Related show

Commit Message

Eric Sandeen July 12, 2019, 9:38 p.m. UTC
Make some mostly trivial changes to libxfs/trans.c to more
closely match kernelspace xfs_trans.c, including:

- add tracepoint calls
- add comments
- add braces
- change tests for null
- reorder some tests and initializations

This /should/ be no functional changes.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Carlos Maiolino July 16, 2019, 11:31 a.m. UTC | #1
On Fri, Jul 12, 2019 at 04:38:42PM -0500, Eric Sandeen wrote:
> Make some mostly trivial changes to libxfs/trans.c to more
> closely match kernelspace xfs_trans.c, including:
> 
> - add tracepoint calls
> - add comments
> - add braces
> - change tests for null
> - reorder some tests and initializations
> 
> This /should/ be no functional changes.
> 

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/include/xfs_trace.h b/include/xfs_trace.h
> index 43720040..71a7466e 100644
> --- a/include/xfs_trace.h
> +++ b/include/xfs_trace.h
> @@ -168,6 +168,8 @@
>  #define trace_xfs_trans_bjoin(a)		((void) 0)
>  #define trace_xfs_trans_bhold(a)		((void) 0)
>  #define trace_xfs_trans_get_buf(a)		((void) 0)
> +#define trace_xfs_trans_get_buf_recur(a)	((void) 0)
> +#define trace_xfs_trans_log_buf(a)		((void) 0)
>  #define trace_xfs_trans_getsb_recur(a)		((void) 0)
>  #define trace_xfs_trans_getsb(a)		((void) 0)
>  #define trace_xfs_trans_read_buf_recur(a)	((void) 0)
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index fecefc7a..453e5476 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -389,6 +389,15 @@ libxfs_trans_bjoin(
>  	trace_xfs_trans_bjoin(bp->b_log_item);
>  }
>  
> +/*
> + * Get and lock the buffer for the caller if it is not already
> + * locked within the given transaction.  If it is already locked
> + * within the transaction, just increment its lock recursion count
> + * and return a pointer to it.
> + *
> + * If the transaction pointer is NULL, make this just a normal
> + * get_buf() call.
> + */
>  struct xfs_buf *
>  libxfs_trans_get_buf_map(
>  	struct xfs_trans	*tp,
> @@ -400,21 +409,31 @@ libxfs_trans_get_buf_map(
>  	xfs_buf_t		*bp;
>  	struct xfs_buf_log_item	*bip;
>  
> -	if (tp == NULL)
> +	if (!tp)
>  		return libxfs_getbuf_map(target, map, nmaps, 0);
>  
> +	/*
> +	 * If we find the buffer in the cache with this transaction
> +	 * pointer in its b_fsprivate2 field, then we know we already
> +	 * have it locked.  In this case we just increment the lock
> +	 * recursion count and return the buffer to the caller.
> +	 */
>  	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
>  	if (bp != NULL) {
>  		ASSERT(bp->b_transp == tp);
>  		bip = bp->b_log_item;
>  		ASSERT(bip != NULL);
>  		bip->bli_recur++;
> +		trace_xfs_trans_get_buf_recur(bip);
>  		return bp;
>  	}
>  
>  	bp = libxfs_getbuf_map(target, map, nmaps, 0);
> -	if (bp == NULL)
> +	if (bp == NULL) {
>  		return NULL;
> +	}
> +
> +	ASSERT(!bp->b_error);
>  
>  	_libxfs_trans_bjoin(tp, bp, 1);
>  	trace_xfs_trans_get_buf(bp->b_log_item);
> @@ -446,6 +465,8 @@ libxfs_trans_getsb(
>  	}
>  
>  	bp = libxfs_getsb(mp, flags);
> +	if (bp == NULL)
> +		return NULL;
>  
>  	_libxfs_trans_bjoin(tp, bp, 1);
>  	trace_xfs_trans_getsb(bp->b_log_item);
> @@ -480,7 +501,7 @@ libxfs_trans_read_buf_map(
>  	}
>  
>  	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
> -	if (bp != NULL) {
> +	if (bp) {
>  		ASSERT(bp->b_transp == tp);
>  		ASSERT(bp->b_log_item != NULL);
>  		bip = bp->b_log_item;
> @@ -507,38 +528,61 @@ out_relse:
>  	return error;
>  }
>  
> +/*
> + * Release a buffer previously joined to the transaction. If the buffer is
> + * modified within this transaction, decrement the recursion count but do not
> + * release the buffer even if the count goes to 0. If the buffer is not modified
> + * within the transaction, decrement the recursion count and release the buffer
> + * if the recursion count goes to 0.
> + *
> + * If the buffer is to be released and it was not already dirty before this
> + * transaction began, then also free the buf_log_item associated with it.
> + *
> + * If the transaction pointer is NULL, this is a normal xfs_buf_relse() call.
> + */
>  void
>  libxfs_trans_brelse(
>  	struct xfs_trans	*tp,
>  	struct xfs_buf		*bp)
>  {
> -	struct xfs_buf_log_item	*bip;
> +	struct xfs_buf_log_item	*bip = bp->b_log_item;
>  
> -	if (tp == NULL) {
> -		ASSERT(bp->b_transp == NULL);
> +	ASSERT(bp->b_transp == tp);
> +
> +	if (!tp) {
>  		libxfs_putbuf(bp);
>  		return;
>  	}
>  
>  	trace_xfs_trans_brelse(bip);
> -	ASSERT(bp->b_transp == tp);
> -	bip = bp->b_log_item;
>  	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
>  
> +	/*
> +	 * If the release is for a recursive lookup, then decrement the count
> +	 * and return.
> +	 */
>  	if (bip->bli_recur > 0) {
>  		bip->bli_recur--;
>  		return;
>  	}
>  
> -	/* If dirty/stale, can't release till transaction committed */
> -	if (bip->bli_flags & XFS_BLI_STALE)
> -		return;
> +	/*
> +	 * If the buffer is invalidated or dirty in this transaction, we can't
> +	 * release it until we commit.
> +	 */
>  	if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
>  		return;
> +	if (bip->bli_flags & XFS_BLI_STALE)
> +		return;
>  
> +	/*
> +	 * Unlink the log item from the transaction and clear the hold flag, if
> +	 * set. We wouldn't want the next user of the buffer to get confused.
> +	 */
>  	xfs_trans_del_item(&bip->bli_item);
> -	if (bip->bli_flags & XFS_BLI_HOLD)
> -		bip->bli_flags &= ~XFS_BLI_HOLD;
> +	bip->bli_flags &= ~XFS_BLI_HOLD;
> +
> +	/* drop the reference to the bli */
>  	xfs_buf_item_put(bip);
>  
>  	bp->b_transp = NULL;
> @@ -600,10 +644,11 @@ libxfs_trans_log_buf(
>  {
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
>  
> -	ASSERT((first <= last) && (last < bp->b_bcount));
> +	ASSERT(first <= last && last < BBTOB(bp->b_length));
>  
>  	xfs_trans_dirty_buf(tp, bp);
>  
> +	trace_xfs_trans_log_buf(bip);
>  	xfs_buf_item_log(bip, first, last);
>  }
>  
> @@ -632,6 +677,15 @@ libxfs_trans_binval(
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  }
>  
> +/*
> + * Mark the buffer as being one which contains newly allocated
> + * inodes.  We need to make sure that even if this buffer is
> + * relogged as an 'inode buf' we still recover all of the inode
> + * images in the face of a crash.  This works in coordination with
> + * xfs_buf_item_committed() to ensure that the buffer remains in the
> + * AIL at its original location even after it has been relogged.
> + */
> +/* ARGSUSED */
>  void
>  libxfs_trans_inode_alloc_buf(
>  	xfs_trans_t		*tp,
>

Patch
diff mbox series

diff --git a/include/xfs_trace.h b/include/xfs_trace.h
index 43720040..71a7466e 100644
--- a/include/xfs_trace.h
+++ b/include/xfs_trace.h
@@ -168,6 +168,8 @@ 
 #define trace_xfs_trans_bjoin(a)		((void) 0)
 #define trace_xfs_trans_bhold(a)		((void) 0)
 #define trace_xfs_trans_get_buf(a)		((void) 0)
+#define trace_xfs_trans_get_buf_recur(a)	((void) 0)
+#define trace_xfs_trans_log_buf(a)		((void) 0)
 #define trace_xfs_trans_getsb_recur(a)		((void) 0)
 #define trace_xfs_trans_getsb(a)		((void) 0)
 #define trace_xfs_trans_read_buf_recur(a)	((void) 0)
diff --git a/libxfs/trans.c b/libxfs/trans.c
index fecefc7a..453e5476 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -389,6 +389,15 @@  libxfs_trans_bjoin(
 	trace_xfs_trans_bjoin(bp->b_log_item);
 }
 
+/*
+ * Get and lock the buffer for the caller if it is not already
+ * locked within the given transaction.  If it is already locked
+ * within the transaction, just increment its lock recursion count
+ * and return a pointer to it.
+ *
+ * If the transaction pointer is NULL, make this just a normal
+ * get_buf() call.
+ */
 struct xfs_buf *
 libxfs_trans_get_buf_map(
 	struct xfs_trans	*tp,
@@ -400,21 +409,31 @@  libxfs_trans_get_buf_map(
 	xfs_buf_t		*bp;
 	struct xfs_buf_log_item	*bip;
 
-	if (tp == NULL)
+	if (!tp)
 		return libxfs_getbuf_map(target, map, nmaps, 0);
 
+	/*
+	 * If we find the buffer in the cache with this transaction
+	 * pointer in its b_fsprivate2 field, then we know we already
+	 * have it locked.  In this case we just increment the lock
+	 * recursion count and return the buffer to the caller.
+	 */
 	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
 	if (bp != NULL) {
 		ASSERT(bp->b_transp == tp);
 		bip = bp->b_log_item;
 		ASSERT(bip != NULL);
 		bip->bli_recur++;
+		trace_xfs_trans_get_buf_recur(bip);
 		return bp;
 	}
 
 	bp = libxfs_getbuf_map(target, map, nmaps, 0);
-	if (bp == NULL)
+	if (bp == NULL) {
 		return NULL;
+	}
+
+	ASSERT(!bp->b_error);
 
 	_libxfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_get_buf(bp->b_log_item);
@@ -446,6 +465,8 @@  libxfs_trans_getsb(
 	}
 
 	bp = libxfs_getsb(mp, flags);
+	if (bp == NULL)
+		return NULL;
 
 	_libxfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_getsb(bp->b_log_item);
@@ -480,7 +501,7 @@  libxfs_trans_read_buf_map(
 	}
 
 	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
-	if (bp != NULL) {
+	if (bp) {
 		ASSERT(bp->b_transp == tp);
 		ASSERT(bp->b_log_item != NULL);
 		bip = bp->b_log_item;
@@ -507,38 +528,61 @@  out_relse:
 	return error;
 }
 
+/*
+ * Release a buffer previously joined to the transaction. If the buffer is
+ * modified within this transaction, decrement the recursion count but do not
+ * release the buffer even if the count goes to 0. If the buffer is not modified
+ * within the transaction, decrement the recursion count and release the buffer
+ * if the recursion count goes to 0.
+ *
+ * If the buffer is to be released and it was not already dirty before this
+ * transaction began, then also free the buf_log_item associated with it.
+ *
+ * If the transaction pointer is NULL, this is a normal xfs_buf_relse() call.
+ */
 void
 libxfs_trans_brelse(
 	struct xfs_trans	*tp,
 	struct xfs_buf		*bp)
 {
-	struct xfs_buf_log_item	*bip;
+	struct xfs_buf_log_item	*bip = bp->b_log_item;
 
-	if (tp == NULL) {
-		ASSERT(bp->b_transp == NULL);
+	ASSERT(bp->b_transp == tp);
+
+	if (!tp) {
 		libxfs_putbuf(bp);
 		return;
 	}
 
 	trace_xfs_trans_brelse(bip);
-	ASSERT(bp->b_transp == tp);
-	bip = bp->b_log_item;
 	ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
 
+	/*
+	 * If the release is for a recursive lookup, then decrement the count
+	 * and return.
+	 */
 	if (bip->bli_recur > 0) {
 		bip->bli_recur--;
 		return;
 	}
 
-	/* If dirty/stale, can't release till transaction committed */
-	if (bip->bli_flags & XFS_BLI_STALE)
-		return;
+	/*
+	 * If the buffer is invalidated or dirty in this transaction, we can't
+	 * release it until we commit.
+	 */
 	if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
 		return;
+	if (bip->bli_flags & XFS_BLI_STALE)
+		return;
 
+	/*
+	 * Unlink the log item from the transaction and clear the hold flag, if
+	 * set. We wouldn't want the next user of the buffer to get confused.
+	 */
 	xfs_trans_del_item(&bip->bli_item);
-	if (bip->bli_flags & XFS_BLI_HOLD)
-		bip->bli_flags &= ~XFS_BLI_HOLD;
+	bip->bli_flags &= ~XFS_BLI_HOLD;
+
+	/* drop the reference to the bli */
 	xfs_buf_item_put(bip);
 
 	bp->b_transp = NULL;
@@ -600,10 +644,11 @@  libxfs_trans_log_buf(
 {
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
 
-	ASSERT((first <= last) && (last < bp->b_bcount));
+	ASSERT(first <= last && last < BBTOB(bp->b_length));
 
 	xfs_trans_dirty_buf(tp, bp);
 
+	trace_xfs_trans_log_buf(bip);
 	xfs_buf_item_log(bip, first, last);
 }
 
@@ -632,6 +677,15 @@  libxfs_trans_binval(
 	tp->t_flags |= XFS_TRANS_DIRTY;
 }
 
+/*
+ * Mark the buffer as being one which contains newly allocated
+ * inodes.  We need to make sure that even if this buffer is
+ * relogged as an 'inode buf' we still recover all of the inode
+ * images in the face of a crash.  This works in coordination with
+ * xfs_buf_item_committed() to ensure that the buffer remains in the
+ * AIL at its original location even after it has been relogged.
+ */
+/* ARGSUSED */
 void
 libxfs_trans_inode_alloc_buf(
 	xfs_trans_t		*tp,