diff mbox series

[05/14] libxfs: make libxfs_buf_read_map return an error code

Message ID 158216310149.603628.17465705830434897306.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfsprogs: make buffer functions return error codes | expand

Commit Message

Darrick J. Wong Feb. 20, 2020, 1:45 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Make libxfs_buf_read_map() and libxfs_readbuf() return an error code
instead of making callers guess what happened based on whether or not
they got a buffer back.

Add a new SALVAGE flag so that certain utilities (xfs_db and xfs_repair)
can attempt salvage operations even if the verifiers failed, which was
the behavior before this change.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/io.c            |    4 +--
 libxfs/libxfs_io.h |   25 ++++++++++++------
 libxfs/rdwr.c      |   72 ++++++++++++++++++++++++++++++++++++++++++----------
 libxfs/trans.c     |   24 ++++-------------
 repair/da_util.c   |    3 +-
 5 files changed, 84 insertions(+), 44 deletions(-)

Comments

Christoph Hellwig Feb. 21, 2020, 3:03 p.m. UTC | #1 
On Wed, Feb 19, 2020 at 05:45:01PM -0800, Darrick J. Wong wrote:
> @@ -1050,15 +1083,26 @@ libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
>  				flags);
>  	else
>  		error = libxfs_readbufr_map(btp, bp, flags);
> +	if (error == -EIO && salvage)
> +		goto ok;

I understand the part about skipping the verifiers.  But how does ignoring
EIO in this case fir the scheme?
Darrick J. Wong Feb. 21, 2020, 4:15 p.m. UTC | #2 
On Fri, Feb 21, 2020 at 07:03:39AM -0800, Christoph Hellwig wrote:
> On Wed, Feb 19, 2020 at 05:45:01PM -0800, Darrick J. Wong wrote:
> > @@ -1050,15 +1083,26 @@ libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
> >  				flags);
> >  	else
> >  		error = libxfs_readbufr_map(btp, bp, flags);
> > +	if (error == -EIO && salvage)
> > +		goto ok;
> 
> I understand the part about skipping the verifiers.  But how does ignoring
> EIO in this case fir the scheme?

"Salvage" mode means that the caller wants an xfs_buf even if the
contents are invalid or missing due to EIO.  This is useful for db and
repair because we can fill the buffer with fixed or new metadata and
write it back to disk.

On a practical level this means I don't have to amend all callsites:

	err = libxfs_buf_read(...LIBXFS_READBUF_SALVAGE..., &bp);
	if (err == -EIO)
		err = libxfs_buf_get(..., &bp);
	if (err)
		goto barf;

...since EIO doesn't seem that much more special than EFSCORRUPTED.

--D
Christoph Hellwig Feb. 21, 2020, 4:28 p.m. UTC | #3 
On Fri, Feb 21, 2020 at 08:15:30AM -0800, Darrick J. Wong wrote:
> > I understand the part about skipping the verifiers.  But how does ignoring
> > EIO in this case fir the scheme?
> 
> "Salvage" mode means that the caller wants an xfs_buf even if the
> contents are invalid or missing due to EIO.  This is useful for db and
> repair because we can fill the buffer with fixed or new metadata and
> write it back to disk.

I thought the aim was to look at the buffer for the content even if
said content fails the verifiers, for which it makes sense to me.

> On a practical level this means I don't have to amend all callsites:
> 
> 	err = libxfs_buf_read(...LIBXFS_READBUF_SALVAGE..., &bp);
> 	if (err == -EIO)
> 		err = libxfs_buf_get(..., &bp);
> 	if (err)
> 		goto barf;
> 
> ...since EIO doesn't seem that much more special than EFSCORRUPTED.

I actually prefer that, as it really documents what is going on.  That
being said if reading the block fails with EIO chances are writing will
fail as well..
diff mbox series

Patch

diff --git a/db/io.c b/db/io.c
index b81e9969..5c9d72bb 100644
--- a/db/io.c
+++ b/db/io.c
@@ -542,8 +542,8 @@  set_cur(
 		if (!iocur_top->bbmap)
 			return;
 		memcpy(iocur_top->bbmap, bbmap, sizeof(struct bbmap));
-		bp = libxfs_buf_read_map(mp->m_ddev_targp, bbmap->b,
-					bbmap->nmaps, 0, ops);
+		libxfs_buf_read_map(mp->m_ddev_targp, bbmap->b, bbmap->nmaps,
+				LIBXFS_READBUF_SALVAGE, &bp, ops);
 	} else {
 		bp = libxfs_buf_read(mp->m_ddev_targp, blknum, len, 0, ops);
 		iocur_top->bbmap = NULL;
diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 189db8d8..06847ad5 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -126,6 +126,8 @@  extern struct cache_operations	libxfs_bcache_operations;
 
 /* Exit on buffer read error */
 #define LIBXFS_READBUF_FAIL_EXIT	(1 << 0)
+/* Return the buffer even if the IO or verifier fail. */
+#define LIBXFS_READBUF_SALVAGE		(1 << 1)
 
 /* Exit on buffer write error */
 #define LIBXFS_WRITEBUF_FAIL_EXIT	(1 << 0)
@@ -135,9 +137,9 @@  extern struct cache_operations	libxfs_bcache_operations;
 #define libxfs_buf_read(dev, daddr, len, flags, ops) \
 	libxfs_trace_readbuf(__FUNCTION__, __FILE__, __LINE__, \
 			    (dev), (daddr), (len), (flags), (ops))
-#define libxfs_buf_read_map(dev, map, nmaps, flags, ops) \
+#define libxfs_buf_read_map(dev, map, nmaps, flags, bpp, ops) \
 	libxfs_trace_readbuf_map(__FUNCTION__, __FILE__, __LINE__, \
-			    (dev), (map), (nmaps), (flags), (ops))
+			    (dev), (map), (nmaps), (flags), (bpp), (ops))
 #define libxfs_buf_dirty(buf, flags) \
 	libxfs_trace_dirtybuf(__FUNCTION__, __FILE__, __LINE__, \
 			      (buf), (flags))
@@ -153,9 +155,10 @@  extern struct cache_operations	libxfs_bcache_operations;
 struct xfs_buf *libxfs_trace_readbuf(const char *func, const char *file,
 			int line, struct xfs_buftarg *btp, xfs_daddr_t daddr,
 			size_t len, int flags, const struct xfs_buf_ops *ops);
-extern xfs_buf_t *libxfs_trace_readbuf_map(const char *, const char *, int,
-			struct xfs_buftarg *, struct xfs_buf_map *, int, int,
-			const struct xfs_buf_ops *);
+int libxfs_trace_readbuf_map(const char *func, const char *file, int line,
+			struct xfs_buftarg *btp, struct xfs_buf_map *maps,
+			int nmaps, int flags, struct xfs_buf **bpp,
+			const struct xfs_buf_ops *ops);
 void libxfs_trace_dirtybuf(const char *func, const char *file, int line,
 			struct xfs_buf *bp, int flags);
 struct xfs_buf *libxfs_trace_getbuf(const char *func, const char *file,
@@ -169,8 +172,9 @@  extern void	libxfs_trace_putbuf (const char *, const char *, int,
 
 #else
 
-extern xfs_buf_t *libxfs_buf_read_map(struct xfs_buftarg *, struct xfs_buf_map *,
-			int, int, const struct xfs_buf_ops *);
+int libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *maps,
+			int nmaps, int flags, struct xfs_buf **bpp,
+			const struct xfs_buf_ops *ops);
 void libxfs_buf_dirty(struct xfs_buf *bp, int flags);
 int libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *maps,
 			int nmaps, int flags, struct xfs_buf **bpp);
@@ -200,9 +204,14 @@  libxfs_buf_read(
 	xfs_buf_flags_t		flags,
 	const struct xfs_buf_ops *ops)
 {
+	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	return libxfs_buf_read_map(target, &map, 1, flags, ops);
+	error = libxfs_buf_read_map(target, &map, 1, flags, &bp, ops);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 #endif /* XFS_BUF_TRACING */
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 6eaa3e69..222ef441 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -389,8 +389,10 @@  libxfs_log_header(
 #undef libxfs_writebuf
 #undef libxfs_buf_get_map
 
-xfs_buf_t	*libxfs_buf_read_map(struct xfs_buftarg *, struct xfs_buf_map *,
-				int, int, const struct xfs_buf_ops *);
+int		libxfs_buf_read_map(struct xfs_buftarg *btp,
+				struct xfs_buf_map *maps, int nmaps, int flags,
+				struct xfs_buf **bpp,
+				const struct xfs_buf_ops *ops);
 int		libxfs_writebuf(xfs_buf_t *, int);
 int		libxfs_buf_get_map(struct xfs_buftarg *btp,
 				struct xfs_buf_map *maps, int nmaps, int flags,
@@ -420,11 +422,30 @@  libxfs_trace_readbuf(
 	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	bp = libxfs_buf_read_map(btp, &map, 1, flags, ops);
+	libxfs_buf_read_map(btp, &map, 1, flags, &bp, ops);
 	__add_trace(bp, func, file, line);
 	return bp;
 }
 
+int
+libxfs_trace_readbuf_map(
+	const char		*func,
+	const char		*file,
+	int			line,
+	struct xfs_buftarg	*btp,
+	struct xfs_buf_map	*map,
+	int			nmaps,
+	int			flags,
+	struct xfs_buf		**bpp,
+	const struct xfs_buf_ops *ops)
+{
+	int			error;
+
+	error = libxfs_buf_read_map(btp, map, nmaps, flags, bpp, ops);
+	__add_trace(*bpp, func, file, line);
+	return error;
+}
+
 void
 libxfs_trace_dirtybuf(
 	const char		*func,
@@ -1004,20 +1025,27 @@  libxfs_readbufr_map(struct xfs_buftarg *btp, struct xfs_buf *bp, int flags)
 	return error;
 }
 
-struct xfs_buf *
-libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
-		int flags, const struct xfs_buf_ops *ops)
+int
+libxfs_buf_read_map(
+	struct xfs_buftarg	*btp,
+	struct xfs_buf_map	*map,
+	int			nmaps,
+	int			flags,
+	struct xfs_buf		**bpp,
+	const struct xfs_buf_ops *ops)
 {
-	struct xfs_buf	*bp;
-	int		error = 0;
+	struct xfs_buf		*bp;
+	bool			salvage = flags & LIBXFS_READBUF_SALVAGE;
+	int			error = 0;
 
+	*bpp = NULL;
 	if (nmaps == 1)
 		error = libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
 				0, &bp);
 	else
 		error = __libxfs_buf_get_map(btp, map, nmaps, 0, &bp);
 	if (error)
-		return NULL;
+		return error;
 
 	/*
 	 * if the buffer was prefetched, it is likely that it was not validated.
@@ -1030,12 +1058,17 @@  libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
 	 * should not be dirtying unchecked buffers and therefore failing it
 	 * here because it's dirty and unchecked indicates we've screwed up
 	 * somewhere else.
+	 *
+	 * Note that if the caller passes in LIBXFS_READBUF_SALVAGE, that means
+	 * they want the buffer even if it contains error status.
 	 */
 	bp->b_error = 0;
 	if (bp->b_flags & (LIBXFS_B_UPTODATE | LIBXFS_B_DIRTY)) {
 		if (bp->b_flags & LIBXFS_B_UNCHECKED)
-			libxfs_readbuf_verify(bp, ops);
-		return bp;
+			error = libxfs_readbuf_verify(bp, ops);
+		if (error && !salvage)
+			goto err;
+		goto ok;
 	}
 
 	/*
@@ -1050,15 +1083,26 @@  libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
 				flags);
 	else
 		error = libxfs_readbufr_map(btp, bp, flags);
-	if (!error)
-		libxfs_readbuf_verify(bp, ops);
+	if (error == -EIO && salvage)
+		goto ok;
+	if (error)
+		goto err;
+
+	error = libxfs_readbuf_verify(bp, ops);
+	if (error && !salvage)
+		goto err;
 
+ok:
 #ifdef IO_DEBUGX
 	printf("%lx: %s: read %lu bytes, error %d, blkno=%llu(%llu), %p\n",
 		pthread_self(), __FUNCTION__, buf - (char *)bp->b_addr, error,
 		(long long)LIBXFS_BBTOOFF64(bp->b_bn), (long long)bp->b_bn, bp);
 #endif
-	return bp;
+	*bpp = bp;
+	return 0;
+err:
+	libxfs_buf_relse(bp);
+	return error;
 }
 
 /* Allocate a raw uncached buffer. */
diff --git a/libxfs/trans.c b/libxfs/trans.c
index a7407dc3..b833e369 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -510,15 +510,8 @@  libxfs_trans_read_buf_map(
 
 	*bpp = NULL;
 
-	if (tp == NULL) {
-		bp = libxfs_buf_read_map(target, map, nmaps, flags, ops);
-		if (!bp) {
-			return (flags & XBF_TRYLOCK) ?  -EAGAIN : -ENOMEM;
-		}
-		if (bp->b_error)
-			goto out_relse;
-		goto done;
-	}
+	if (tp == NULL)
+		return libxfs_buf_read_map(target, map, nmaps, flags, bpp, ops);
 
 	bp = xfs_trans_buf_item_match(tp, target, map, nmaps);
 	if (bp) {
@@ -530,22 +523,15 @@  libxfs_trans_read_buf_map(
 		goto done;
 	}
 
-	bp = libxfs_buf_read_map(target, map, nmaps, flags, ops);
-	if (!bp) {
-		return (flags & XBF_TRYLOCK) ?  -EAGAIN : -ENOMEM;
-	}
-	if (bp->b_error)
-		goto out_relse;
+	error = libxfs_buf_read_map(target, map, nmaps, flags, &bp, ops);
+	if (error)
+		return error;
 
 	_libxfs_trans_bjoin(tp, bp, 1);
 done:
 	trace_xfs_trans_read_buf(bp->b_log_item);
 	*bpp = bp;
 	return 0;
-out_relse:
-	error = bp->b_error;
-	xfs_buf_relse(bp);
-	return error;
 }
 
 /*
diff --git a/repair/da_util.c b/repair/da_util.c
index ed2ec3ba..add93ad8 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -64,7 +64,8 @@  da_read_buf(
 		map[i].bm_bn = XFS_FSB_TO_DADDR(mp, bmp[i].startblock);
 		map[i].bm_len = XFS_FSB_TO_BB(mp, bmp[i].blockcount);
 	}
-	bp = libxfs_buf_read_map(mp->m_dev, map, nex, 0, ops);
+	libxfs_buf_read_map(mp->m_dev, map, nex, LIBXFS_READBUF_SALVAGE,
+			&bp, ops);
 	if (map != map_array)
 		free(map);
 	return bp;