Skip to contentSkip to conversation

xfs: lobotomise xfs_trans_read_buf_map()

PATCH Superseded Dave Chinner 3 participants 8 messages Dec 1, 2014 – Dec 3, 2014 XFS filesystem

lore
Share & tools
lore

Dave Chinner

author
#0 lore raw

PATCH 1/1 xfs: lobotomise xfs_trans_read_buf_map()

From: Dave Chinner <[email protected]>

There's a case in that code where it checks for a buffer match in a transaction where the buffer is not marked done. i.e. trying to catch a buffer we have locked in the transaction but have not completed IO on.

The only way we can find a buffer that has not had IO completed on it is if it had readahead issued on it, but we never do readahead on buffers that we have already joined into a transaction. Hence this condition cannot occur, and buffers locked and joined into a transaction should always be marked done and not under IO.

Remove this code and re-order xfs_trans_read_buf_map() to remove duplicated IO dispatch and error handling code.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/xfs_trans_buf.c | 113 ++++++++++++-------------------------------------
 1 file changed, 26 insertions(+), 87 deletions(-)
fs/xfs/xfs_trans_buf.c +26 -87
index d3d80be..0136b4f 100644
@@ -255,46 +255,11 @@ xfs_trans_read_buf_map(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	xfs_buf_t		*bp;
-	xfs_buf_log_item_t	*bip;
+	struct xfs_buf		*bp = NULL;
+	struct xfs_buf_log_item	*bip;
 	int			error;
 
 	*bpp = NULL;
-	if (!tp) {
-		bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-		if (!bp)
-			return (flags & XBF_TRYLOCK) ?
-					-EAGAIN : -ENOMEM;
-
-		if (bp->b_error) {
-			error = bp->b_error;
-			xfs_buf_ioerror_alert(bp, __func__);
-			XFS_BUF_UNDONE(bp);
-			xfs_buf_stale(bp);
-			xfs_buf_relse(bp);
-
-			/* bad CRC means corrupted metadata */
-			if (error == -EFSBADCRC)
-				error = -EFSCORRUPTED;
-			return error;
-		}
-#ifdef DEBUG
-		if (xfs_do_error) {
-			if (xfs_error_target == target) {
-				if (((xfs_req_num++) % xfs_error_mod) == 0) {
-					xfs_buf_relse(bp);
-					xfs_debug(mp, "Returning error!");
-					return -EIO;
-				}
-			}
-		}
-#endif
-		if (XFS_FORCED_SHUTDOWN(mp))
-			goto shutdown_abort;
-		*bpp = bp;
-		return 0;
-	}
-
 	/*
 	 * If we find the buffer in the cache with this transaction
 	 * pointer in its b_fsprivate2 field, then we know we already

@@ -303,49 +268,24 @@ xfs_trans_read_buf_map(
 	 * If the buffer is not yet read in, then we read it in, increment
 	 * the lock recursion count, and return it to the caller.
 	 */
-	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
-	if (bp != NULL) {
+	if (tp)
+		bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
+	if (bp) {
 		ASSERT(xfs_buf_islocked(bp));
 		ASSERT(bp->b_transp == tp);
 		ASSERT(bp->b_fspriv != NULL);
 		ASSERT(!bp->b_error);
-		if (!(XFS_BUF_ISDONE(bp))) {
-			trace_xfs_trans_read_buf_io(bp, _RET_IP_);
-			ASSERT(!XFS_BUF_ISASYNC(bp));
-			ASSERT(bp->b_iodone == NULL);
-			XFS_BUF_READ(bp);
-			bp->b_ops = ops;
-
-			error = xfs_buf_submit_wait(bp);
-			if (error) {
-				if (!XFS_FORCED_SHUTDOWN(mp))
-					xfs_buf_ioerror_alert(bp, __func__);
-				xfs_buf_relse(bp);
-				/*
-				 * We can gracefully recover from most read
-				 * errors. Ones we can't are those that happen
-				 * after the transaction's already dirty.
-				 */
-				if (tp->t_flags & XFS_TRANS_DIRTY)
-					xfs_force_shutdown(tp->t_mountp,
-							SHUTDOWN_META_IO_ERROR);
-				/* bad CRC means corrupted metadata */
-				if (error == -EFSBADCRC)
-					error = -EFSCORRUPTED;
-				return error;
-			}
-		}
+		ASSERT(bp->b_flags & XBF_DONE);
+
 		/*
 		 * We never locked this buf ourselves, so we shouldn't
 		 * brelse it either. Just get out.
 		 */
 		if (XFS_FORCED_SHUTDOWN(mp)) {
 			trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
-			*bpp = NULL;
 			return -EIO;
 		}
 
-
 		bip = bp->b_fspriv;
 		bip->bli_recur++;
 

@@ -356,17 +296,20 @@ xfs_trans_read_buf_map(
 	}
 
 	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (bp == NULL) {
-		*bpp = NULL;
-		return (flags & XBF_TRYLOCK) ?
-					0 : -ENOMEM;
+	if (!bp) {
+		if (!(flags & XBF_TRYLOCK))
+			return -ENOMEM;
+		return tp ? 0 : -EAGAIN;
 	}
+
 	if (bp->b_error) {
 		error = bp->b_error;
+		if (!XFS_FORCED_SHUTDOWN(mp))
+			xfs_buf_ioerror_alert(bp, __func__);
+		bp->b_flags &= ~XBF_DONE;
 		xfs_buf_stale(bp);
-		XFS_BUF_DONE(bp);
-		xfs_buf_ioerror_alert(bp, __func__);
-		if (tp->t_flags & XFS_TRANS_DIRTY)
+
+		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
 			xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
 		xfs_buf_relse(bp);
 

@@ -376,32 +319,28 @@ xfs_trans_read_buf_map(
 		return error;
 	}
 #ifdef DEBUG
-	if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
+	if (xfs_do_error && (!tp || !(tp->t_flags & XFS_TRANS_DIRTY))) {
 		if (xfs_error_target == target) {
 			if (((xfs_req_num++) % xfs_error_mod) == 0) {
-				xfs_force_shutdown(tp->t_mountp,
-						   SHUTDOWN_META_IO_ERROR);
 				xfs_buf_relse(bp);
-				xfs_debug(mp, "Returning trans error!");
+				xfs_debug(mp, "Returning error!");
 				return -EIO;
 			}
 		}
 	}
 #endif
-	if (XFS_FORCED_SHUTDOWN(mp))
-		goto shutdown_abort;
+	if (XFS_FORCED_SHUTDOWN(mp)) {
+		xfs_buf_relse(bp);
+		trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
+		return -EIO;
+	}
 
-	_xfs_trans_bjoin(tp, bp, 1);
+	if (tp)
+		_xfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_read_buf(bp->b_fspriv);
-
 	*bpp = bp;
 	return 0;
 
-shutdown_abort:
-	trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
-	xfs_buf_relse(bp);
-	*bpp = NULL;
-	return -EIO;
 }
 
 /*
Signature · 6 lines
2.0.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#1 lore raw
Quoted 5 lines · Dave Chinner

The only way we can find a buffer that has not had IO completed on it is if it had readahead issued on it, but we never do readahead on buffers that we have already joined into a transaction. Hence this condition cannot occur, and buffers locked and joined into a transaction should always be marked done and not under IO.

Should we add an ASSERT that would trigger when someone tries to issue readahead on a buffer with b_transp set?

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 302

Quoted 9 lines
fs/xfs/xfs_trans_buf.c +4 -4
 	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (bp == NULL) {
-		*bpp = NULL;
-		return (flags & XBF_TRYLOCK) ?
-					0 : -ENOMEM;
+	if (!bp) {
+		if (!(flags & XBF_TRYLOCK))
+			return -ENOMEM;
+		return tp ? 0 : -EAGAIN;

Can you fix the inconsistent return for the trylock case in a follow on patch? This difference doesn't look intentional to me, and I would be surprised if it's correctly handled in the callers.

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 367

Quoted 9 lines
fs/xfs/xfs_trans_buf.c +4 -1
 	}
+
 	if (bp->b_error) {
 		error = bp->b_error;
+		if (!XFS_FORCED_SHUTDOWN(mp))
+			xfs_buf_ioerror_alert(bp, __func__);
+		bp->b_flags &= ~XBF_DONE;
 		xfs_buf_stale(bp);
-		XFS_BUF_DONE(bp);

The old non-tp case did a XFS_BUF_UNDONE, which you open code here, while the with-tp case did a XFS_BUF_DONE. I think this change needs a little explanation.

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 329

Quoted 13 lines

#ifdef DEBUG

fs/xfs/xfs_trans_buf.c +2 -4
-	if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
+	if (xfs_do_error && (!tp || !(tp->t_flags & XFS_TRANS_DIRTY))) {
 		if (xfs_error_target == target) {
 			if (((xfs_req_num++) % xfs_error_mod) == 0) {
-				xfs_force_shutdown(tp->t_mountp,
-						   SHUTDOWN_META_IO_ERROR);
 				xfs_buf_relse(bp);
-				xfs_debug(mp, "Returning trans error!");
+				xfs_debug(mp, "Returning error!");
 				return -EIO;
 			}
 		}

I would suggest to kill this xfs_do_error error code, it's the last use of the never initialized xfs_do_error and xfs_error_target variables.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#2 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner

The only way we can find a buffer that has not had IO completed on it is if it had readahead issued on it, but we never do readahead on buffers that we have already joined into a transaction. Hence this condition cannot occur, and buffers locked and joined into a transaction should always be marked done and not under IO.

Should we add an ASSERT that would trigger when someone tries to issue readahead on a buffer with b_transp set?

Perhaps so.

Quoted 5 lines
Quoted 5 lines
fs/xfs/xfs_trans_buf.c +0 -4
 	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (bp == NULL) {
-		*bpp = NULL;
-		return (flags & XBF_TRYLOCK) ?
-					0 : -ENOMEM;
Quoted 8 lines

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 302

Quoted 4 lines
fs/xfs/xfs_trans_buf.c +4 -0
+	if (!bp) {
+		if (!(flags & XBF_TRYLOCK))
+			return -ENOMEM;
+		return tp ? 0 : -EAGAIN;

Can you fix the inconsistent return for the trylock case in a follow on patch? This difference doesn't look intentional to me, and I would be surprised if it's correctly handled in the callers.

Ok, I'll do an audit and make this common in a follow up patch. Just to confirm:

		if (!(flags & XBF_TRYLOCK))
			return -ENOMEM;
		return -EAGAIN;

is what you want to see, right?

Quoted 5 lines
Quoted 5 lines
fs/xfs/xfs_trans_buf.c +2 -0
 	}
+
 	if (bp->b_error) {
 		error = bp->b_error;
+		if (!XFS_FORCED_SHUTDOWN(mp))
Quoted 8 lines

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 367

Quoted 4 lines
fs/xfs/xfs_trans_buf.c +2 -1
+			xfs_buf_ioerror_alert(bp, __func__);
+		bp->b_flags &= ~XBF_DONE;
 		xfs_buf_stale(bp);
-		XFS_BUF_DONE(bp);

The old non-tp case did a XFS_BUF_UNDONE, which you open code here, while the with-tp case did a XFS_BUF_DONE. I think this change needs a little explanation.

Consistency. A read failed on the buffer, so the contents are undefined. XBF_DONE implies the contents of the buffer are valid and so setting XBF_DONE is wrong. Further, the buffer is marked stale, again indicating that the contents are invalid and that it should never be written. This makes the XBF_DONE value redundant.

Hence it doesn't matter whether it is transaction context or not, XBF_DONE should not be set on a stale buffer that failed a read....

I'll add a comment explaining this.

Quoted 10 lines
Quoted 10 lines

#ifdef DEBUG

fs/xfs/xfs_trans_buf.c +1 -4
-	if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
+	if (xfs_do_error && (!tp || !(tp->t_flags & XFS_TRANS_DIRTY))) {
 		if (xfs_error_target == target) {
 			if (((xfs_req_num++) % xfs_error_mod) == 0) {
-				xfs_force_shutdown(tp->t_mountp,
-						   SHUTDOWN_META_IO_ERROR);
 				xfs_buf_relse(bp);
-				xfs_debug(mp, "Returning trans error!");
Quoted 8 lines

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 329

Quoted 4 lines
fs/xfs/xfs_trans_buf.c +1 -0
+				xfs_debug(mp, "Returning error!");
 				return -EIO;
 			}
 		}

I would suggest to kill this xfs_do_error error code, it's the last use of the never initialized xfs_do_error and xfs_error_target variables.

I was in two minds w.r.t. killing that code. My initial patch did kill it, but I didn't in this rework. I'll update the patch to kill it.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#3 lore raw

PATCH v2 1/1 xfs: lobotomise xfs_trans_read_buf_map()

From: Dave Chinner <[email protected]>

There's a case in that code where it checks for a buffer match in a transaction where the buffer is not marked done. i.e. trying to catch a buffer we have locked in the transaction but have not completed IO on.

The only way we can find a buffer that has not had IO completed on it is if it had readahead issued on it, but we never do readahead on buffers that we have already joined into a transaction. Hence this condition cannot occur, and buffers locked and joined into a transaction should always be marked done and not under IO.

Remove this code and re-order xfs_trans_read_buf_map() to remove duplicated IO dispatch and error handling code.

Signed-off-by Dave Chinner <[email protected]>

---

Version 2:
- kill unused xfs_do_error debug code
- add comment explaining clearing of XBF_DONE and marking the buffer stale on read error.

 fs/xfs/xfs_trans_buf.c | 135 ++++++++++++-------------------------------------
 1 file changed, 33 insertions(+), 102 deletions(-)
fs/xfs/xfs_trans_buf.c +33 -102
index d3d80be..df14070 100644
@@ -227,13 +227,6 @@ xfs_trans_getsb(xfs_trans_t	*tp,
 	return bp;
 }
 
-#ifdef DEBUG
-xfs_buftarg_t *xfs_error_target;
-int	xfs_do_error;
-int	xfs_req_num;
-int	xfs_error_mod = 33;
-#endif
-
 /*
  * Get and lock the buffer for the caller if it is not already
  * locked within the given transaction.  If it has not yet been

@@ -255,46 +248,11 @@ xfs_trans_read_buf_map(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	xfs_buf_t		*bp;
-	xfs_buf_log_item_t	*bip;
+	struct xfs_buf		*bp = NULL;
+	struct xfs_buf_log_item	*bip;
 	int			error;
 
 	*bpp = NULL;
-	if (!tp) {
-		bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-		if (!bp)
-			return (flags & XBF_TRYLOCK) ?
-					-EAGAIN : -ENOMEM;
-
-		if (bp->b_error) {
-			error = bp->b_error;
-			xfs_buf_ioerror_alert(bp, __func__);
-			XFS_BUF_UNDONE(bp);
-			xfs_buf_stale(bp);
-			xfs_buf_relse(bp);
-
-			/* bad CRC means corrupted metadata */
-			if (error == -EFSBADCRC)
-				error = -EFSCORRUPTED;
-			return error;
-		}
-#ifdef DEBUG
-		if (xfs_do_error) {
-			if (xfs_error_target == target) {
-				if (((xfs_req_num++) % xfs_error_mod) == 0) {
-					xfs_buf_relse(bp);
-					xfs_debug(mp, "Returning error!");
-					return -EIO;
-				}
-			}
-		}
-#endif
-		if (XFS_FORCED_SHUTDOWN(mp))
-			goto shutdown_abort;
-		*bpp = bp;
-		return 0;
-	}
-
 	/*
 	 * If we find the buffer in the cache with this transaction
 	 * pointer in its b_fsprivate2 field, then we know we already

@@ -303,49 +261,24 @@ xfs_trans_read_buf_map(
 	 * If the buffer is not yet read in, then we read it in, increment
 	 * the lock recursion count, and return it to the caller.
 	 */
-	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
-	if (bp != NULL) {
+	if (tp)
+		bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
+	if (bp) {
 		ASSERT(xfs_buf_islocked(bp));
 		ASSERT(bp->b_transp == tp);
 		ASSERT(bp->b_fspriv != NULL);
 		ASSERT(!bp->b_error);
-		if (!(XFS_BUF_ISDONE(bp))) {
-			trace_xfs_trans_read_buf_io(bp, _RET_IP_);
-			ASSERT(!XFS_BUF_ISASYNC(bp));
-			ASSERT(bp->b_iodone == NULL);
-			XFS_BUF_READ(bp);
-			bp->b_ops = ops;
-
-			error = xfs_buf_submit_wait(bp);
-			if (error) {
-				if (!XFS_FORCED_SHUTDOWN(mp))
-					xfs_buf_ioerror_alert(bp, __func__);
-				xfs_buf_relse(bp);
-				/*
-				 * We can gracefully recover from most read
-				 * errors. Ones we can't are those that happen
-				 * after the transaction's already dirty.
-				 */
-				if (tp->t_flags & XFS_TRANS_DIRTY)
-					xfs_force_shutdown(tp->t_mountp,
-							SHUTDOWN_META_IO_ERROR);
-				/* bad CRC means corrupted metadata */
-				if (error == -EFSBADCRC)
-					error = -EFSCORRUPTED;
-				return error;
-			}
-		}
+		ASSERT(bp->b_flags & XBF_DONE);
+
 		/*
 		 * We never locked this buf ourselves, so we shouldn't
 		 * brelse it either. Just get out.
 		 */
 		if (XFS_FORCED_SHUTDOWN(mp)) {
 			trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
-			*bpp = NULL;
 			return -EIO;
 		}
 
-
 		bip = bp->b_fspriv;
 		bip->bli_recur++;
 

@@ -356,17 +289,29 @@ xfs_trans_read_buf_map(
 	}
 
 	bp = xfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (bp == NULL) {
-		*bpp = NULL;
-		return (flags & XBF_TRYLOCK) ?
-					0 : -ENOMEM;
+	if (!bp) {
+		if (!(flags & XBF_TRYLOCK))
+			return -ENOMEM;
+		return tp ? 0 : -EAGAIN;
 	}
+
+	/*
+	 * If we've had a read error, then the contents of the buffer are
+	 * invalid and shoul dnot be used. To ensure that a followup read tries
+	 * to pull the buffer from disk again, we clear the XBF_DONE flag and
+	 * mark the buffer stale. This ensures that anyone who has a current
+	 * reference to the buffer will interpret it's contents correctly and
+	 * future cache lookups will also treat it as an empty, uninitialised
+	 * buffer.
+	 */
 	if (bp->b_error) {
 		error = bp->b_error;
+		if (!XFS_FORCED_SHUTDOWN(mp))
+			xfs_buf_ioerror_alert(bp, __func__);
+		bp->b_flags &= ~XBF_DONE;
 		xfs_buf_stale(bp);
-		XFS_BUF_DONE(bp);
-		xfs_buf_ioerror_alert(bp, __func__);
-		if (tp->t_flags & XFS_TRANS_DIRTY)
+
+		if (tp && (tp->t_flags & XFS_TRANS_DIRTY))
 			xfs_force_shutdown(tp->t_mountp, SHUTDOWN_META_IO_ERROR);
 		xfs_buf_relse(bp);
 

@@ -375,33 +320,19 @@ xfs_trans_read_buf_map(
 			error = -EFSCORRUPTED;
 		return error;
 	}
-#ifdef DEBUG
-	if (xfs_do_error && !(tp->t_flags & XFS_TRANS_DIRTY)) {
-		if (xfs_error_target == target) {
-			if (((xfs_req_num++) % xfs_error_mod) == 0) {
-				xfs_force_shutdown(tp->t_mountp,
-						   SHUTDOWN_META_IO_ERROR);
-				xfs_buf_relse(bp);
-				xfs_debug(mp, "Returning trans error!");
-				return -EIO;
-			}
-		}
+
+	if (XFS_FORCED_SHUTDOWN(mp)) {
+		xfs_buf_relse(bp);
+		trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
+		return -EIO;
 	}
-#endif
-	if (XFS_FORCED_SHUTDOWN(mp))
-		goto shutdown_abort;
 
-	_xfs_trans_bjoin(tp, bp, 1);
+	if (tp)
+		_xfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_read_buf(bp->b_fspriv);
-
 	*bpp = bp;
 	return 0;
 
-shutdown_abort:
-	trace_xfs_trans_read_buf_shut(bp, _RET_IP_);
-	xfs_buf_relse(bp);
-	*bpp = NULL;
-	return -EIO;
 }
 
 /*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v2 1/1
#4 lore raw
Quoted 7 lines

/*
* Get and lock the buffer for the caller if it is not already
* locked within the given transaction. If it has not yet been

fs/xfs/xfs_trans_buf.c +0 -0
@@ -255,46 +248,11 @@ xfs_trans_read_buf_map(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {

fs/xfs/xfs_trans_buf.c xfs_trans_read_buf_map( · line 258

Quoted 10 lines
fs/xfs/xfs_trans_buf.c +5 -2
-	xfs_buf_t		*bp;
-	xfs_buf_log_item_t	*bip;
+	struct xfs_buf		*bp = NULL;
+	struct xfs_buf_log_item	*bip;
 	int			error;
 
 	*bpp = NULL;
+	/*
+	 * If we've had a read error, then the contents of the buffer are
+	 * invalid and shoul dnot be used. To ensure that a followup read tries

should not

Otherwise looks fine,

Reviewed-by Christoph Hellwig <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw
In reply to Dave Chinner
Quoted 4 lines · Dave Chinner
Quoted 3 lines

Can you fix the inconsistent return for the trylock case in a follow on patch? This difference doesn't look intentional to me, and I would be surprised if it's correctly handled in the callers.

Quoted 8 lines

Ok, I'll do an audit and make this common in a follow up patch. Just to confirm:

		if (!(flags & XBF_TRYLOCK))
			return -ENOMEM;
		return -EAGAIN;

is what you want to see, right?

Yes.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#6 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
Quoted 3 lines

Can you fix the inconsistent return for the trylock case in a follow on patch? This difference doesn't look intentional to me, and I would be surprised if it's correctly handled in the callers.

Ok, I'll do an audit and make this common in a follow up patch. Just to confirm:

Quoted 7 lines
Quoted 5 lines
		if (!(flags & XBF_TRYLOCK))
			return -ENOMEM;
		return -EAGAIN;

is what you want to see, right?

Yes.

Even ENOMEM / EAGAIN could be wrong if _xfs_buf_find() was given an illegal block number - then it would be EFSCORRUPT.

I think we need to push the error message from _xfs_buf_find(). I played with it once and seemed to have lost it and can do it again if no one else has the time.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#7 lore raw
Quoted 13 lines · Mark Tinguely
Quoted 12 lines · Christoph Hellwig
Quoted 11 lines · Dave Chinner
Quoted 3 lines

Can you fix the inconsistent return for the trylock case in a follow on patch? This difference doesn't look intentional to me, and I would be surprised if it's correctly handled in the callers.

Ok, I'll do an audit and make this common in a follow up patch. Just to confirm:

		if (!(flags & XBF_TRYLOCK))
			return -ENOMEM;
		return -EAGAIN;
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

is what you want to see, right?

Yes.

Even ENOMEM / EAGAIN could be wrong if _xfs_buf_find() was given an illegal block number - then it would be EFSCORRUPT.

Well, in theory. Yes, the comment I wrote in xfs_buf_find() at the time of adding the first check says we should return EFSCORRUPTED, but that's only indicative of the fact that we've been handed something that is bad. The code won't do anything different if it is handed ENOMEM or EFSCORRUPTED - the same error handling will occur, and the only difference will be a warning in the log.

Quoted 3 lines

I think we need to push the error message from _xfs_buf_find(). I played with it once and seemed to have lost it and can do it again if no one else has the time.

The check in _xfs_buf_find() is a loud warning because it indicates we failed to validate a block number at a higher layer. IOWs, if you see that warning fire in _xfs_buf_find(), we need to track down where that bad block number came from and why it wasn't correctly validated before we tried to actually use it. Those are the bugs we need to fix; reworking the buffer cache API so that _xfs_buf_find() can return an error won't fix those bugs, nor change the error handling behaviour of the callers of xfs_trans*buf[_map].

Hence I don't think we need to change _Xfs_buf_find() or the way it reports errors at all. Put the work into fixing the block number validation bugs it exposes...

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs