All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/14] xfsprogs: make buffer functions return error codes
@ 2020-02-25  0:14 Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 01/14] libxfs: make __cache_lookup return an error code Darrick J. Wong
                   ` (13 more replies)
  0 siblings, 14 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs

Hi all,

Let's fix all the xfs read/get buffer functions to return the usual
integer error codes and pass the buffer pointer as a out-argument.  This
makes it so that we can return useful error output instead of making
callers infer ENOMEM or EAGAIN or whatever other reality they crave from
the NULL pointer that they get when things don't go perfectly.

FWIW, all XBF_TRYLOCK callers must now trigger retries if they receive
EAGAIN.  This may lead to a slight behavioral change in that TRYLOCK
callers will no longer retry for things like ENOMEM, though I didn't see
any obvious changes in user-visible behavior when running fstests.

After finishing the error code conversion, we straighten out the TRYLOCK
callers to remove all this null pointer checks in favor of explicit
EAGAIN checks; and then we change the buffer IO/corruption error
reporting so that we report whoever called the buffer code even when
reading a buffer in transaction context.

The userspace port of the kernel patchset was very difficult, so I am
submitting this series to avoid dumping the work on Eric.  Note also
that this series applies after the first part of the 5.6 libxfs port.
For v2, the "read and salvage" flag only applies to corruption, so
callers must handle EIO separately.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=buf-return-errorcodes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 01/14] libxfs: make __cache_lookup return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 02/14] libxfs: make libxfs_getbuf_flags " Darrick J. Wong
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert __cache_lookup() to return numeric error codes like most
everywhere else in xfsprogs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/rdwr.c |   68 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 24 deletions(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index ce31e45d..955284d0 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -478,30 +478,40 @@ struct list_head	lock_buf_list = {&lock_buf_list, &lock_buf_list};
 int			lock_buf_count = 0;
 #endif
 
-static struct xfs_buf *
-__cache_lookup(struct xfs_bufkey *key, unsigned int flags)
+static int
+__cache_lookup(
+	struct xfs_bufkey	*key,
+	unsigned int		flags,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf	*bp;
+	struct cache_node	*cn = NULL;
+	struct xfs_buf		*bp;
 
-	cache_node_get(libxfs_bcache, key, (struct cache_node **)&bp);
-	if (!bp)
-		return NULL;
+	*bpp = NULL;
+
+	cache_node_get(libxfs_bcache, key, &cn);
+	if (!cn)
+		return -ENOMEM;
+	bp = container_of(cn, struct xfs_buf, b_node);
 
 	if (use_xfs_buf_lock) {
-		int ret;
+		int		ret;
 
 		ret = pthread_mutex_trylock(&bp->b_lock);
 		if (ret) {
 			ASSERT(ret == EAGAIN);
-			if (flags & LIBXFS_GETBUF_TRYLOCK)
-				goto out_put;
+			if (flags & LIBXFS_GETBUF_TRYLOCK) {
+				cache_node_put(libxfs_bcache, cn);
+				return -EAGAIN;
+			}
 
 			if (pthread_equal(bp->b_holder, pthread_self())) {
 				fprintf(stderr,
 	_("Warning: recursive buffer locking at block %" PRIu64 " detected\n"),
 					key->blkno);
 				bp->b_recur++;
-				return bp;
+				*bpp = bp;
+				return 0;
 			} else {
 				pthread_mutex_lock(&bp->b_lock);
 			}
@@ -510,9 +520,8 @@ __cache_lookup(struct xfs_bufkey *key, unsigned int flags)
 		bp->b_holder = pthread_self();
 	}
 
-	cache_node_set_priority(libxfs_bcache, &bp->b_node,
-			cache_node_get_priority(&bp->b_node) -
-						CACHE_PREFETCH_PRIORITY);
+	cache_node_set_priority(libxfs_bcache, cn,
+			cache_node_get_priority(cn) - CACHE_PREFETCH_PRIORITY);
 #ifdef XFS_BUF_TRACING
 	pthread_mutex_lock(&libxfs_bcache->c_mutex);
 	lock_buf_count++;
@@ -525,10 +534,8 @@ __cache_lookup(struct xfs_bufkey *key, unsigned int flags)
 		bp, bp->b_bn, (long long)LIBXFS_BBTOOFF64(key->blkno));
 #endif
 
-	return bp;
-out_put:
-	cache_node_put(libxfs_bcache, &bp->b_node);
-	return NULL;
+	*bpp = bp;
+	return 0;
 }
 
 static struct xfs_buf *
@@ -538,13 +545,18 @@ libxfs_getbuf_flags(
 	int			len,
 	unsigned int		flags)
 {
-	struct xfs_bufkey key = {NULL};
+	struct xfs_bufkey	key = {NULL};
+	struct xfs_buf		*bp;
+	int			error;
 
 	key.buftarg = btp;
 	key.blkno = blkno;
 	key.bblen = len;
 
-	return __cache_lookup(&key, flags);
+	error = __cache_lookup(&key, flags, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -568,11 +580,16 @@ reset_buf_state(
 }
 
 static struct xfs_buf *
-__libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *map,
-		    int nmaps, int flags)
+__libxfs_buf_get_map(
+	struct xfs_buftarg	*btp,
+	struct xfs_buf_map	*map,
+	int			nmaps,
+	int			flags)
 {
-	struct xfs_bufkey key = {NULL};
-	int i;
+	struct xfs_bufkey	key = {NULL};
+	struct xfs_buf		*bp;
+	int			i;
+	int			error;
 
 	if (nmaps == 1)
 		return libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
@@ -586,7 +603,10 @@ __libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *map,
 	key.map = map;
 	key.nmaps = nmaps;
 
-	return __cache_lookup(&key, flags);
+	error = __cache_lookup(&key, flags, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 struct xfs_buf *


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 02/14] libxfs: make libxfs_getbuf_flags return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 01/14] libxfs: make __cache_lookup return an error code Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 03/14] libxfs: make libxfs_buf_get_map " Darrick J. Wong
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert libxfs_getbuf_flags() to return numeric error codes like most
everywhere else in xfsprogs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/rdwr.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 955284d0..ea9979a2 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -538,25 +538,21 @@ __cache_lookup(
 	return 0;
 }
 
-static struct xfs_buf *
+static int
 libxfs_getbuf_flags(
 	struct xfs_buftarg	*btp,
 	xfs_daddr_t		blkno,
 	int			len,
-	unsigned int		flags)
+	unsigned int		flags,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_bufkey	key = {NULL};
-	struct xfs_buf		*bp;
-	int			error;
 
 	key.buftarg = btp;
 	key.blkno = blkno;
 	key.bblen = len;
 
-	error = __cache_lookup(&key, flags, &bp);
-	if (error)
-		return NULL;
-	return bp;
+	return __cache_lookup(&key, flags, bpp);
 }
 
 /*
@@ -591,9 +587,13 @@ __libxfs_buf_get_map(
 	int			i;
 	int			error;
 
-	if (nmaps == 1)
-		return libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
-					   flags);
+	if (nmaps == 1) {
+		error = libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
+				flags, &bp);
+		if (error)
+			return NULL;
+		return bp;
+	}
 
 	key.buftarg = btp;
 	key.blkno = map[0].bm_bn;
@@ -741,8 +741,8 @@ libxfs_readbuf(
 	struct xfs_buf		*bp;
 	int			error;
 
-	bp = libxfs_getbuf_flags(btp, blkno, len, 0);
-	if (!bp)
+	error = libxfs_getbuf_flags(btp, blkno, len, 0, &bp);
+	if (error)
 		return NULL;
 
 	/*


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 03/14] libxfs: make libxfs_buf_get_map return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 01/14] libxfs: make __cache_lookup return an error code Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 02/14] libxfs: make libxfs_getbuf_flags " Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 04/14] libxfs: refactor libxfs_readbuf out of existence Darrick J. Wong
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert libxfs_buf_get_map() to return numeric error codes like most
everywhere else in xfsprogs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_io.h |   20 +++++++++----
 libxfs/rdwr.c      |   78 ++++++++++++++++++++++++++++------------------------
 libxfs/trans.c     |   16 +++++++----
 repair/prefetch.c  |    6 +++-
 4 files changed, 69 insertions(+), 51 deletions(-)


diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 957f0396..3cad7ef5 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -141,9 +141,9 @@ extern struct cache_operations	libxfs_bcache_operations;
 #define libxfs_buf_get(dev, daddr, len) \
 	libxfs_trace_getbuf(__FUNCTION__, __FILE__, __LINE__, \
 			    (dev), (daddr), (len))
-#define libxfs_buf_get_map(dev, map, nmaps, flags) \
+#define libxfs_buf_get_map(dev, map, nmaps, flags, bpp) \
 	libxfs_trace_getbuf_map(__FUNCTION__, __FILE__, __LINE__, \
-			    (dev), (map), (nmaps), (flags))
+			    (dev), (map), (nmaps), (flags), (bpp))
 #define libxfs_buf_relse(buf) \
 	libxfs_trace_putbuf(__FUNCTION__, __FILE__, __LINE__, (buf))
 
@@ -158,8 +158,9 @@ void libxfs_trace_dirtybuf(const char *func, const char *file, int line,
 struct xfs_buf *libxfs_trace_getbuf(const char *func, const char *file,
 			int line, struct xfs_buftarg *btp, xfs_daddr_t daddr,
 			size_t len);
-extern xfs_buf_t *libxfs_trace_getbuf_map(const char *, const char *, int,
-			struct xfs_buftarg *, struct xfs_buf_map *, int, int);
+int libxfs_trace_getbuf_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);
 extern void	libxfs_trace_putbuf (const char *, const char *, int,
 			xfs_buf_t *);
 
@@ -169,8 +170,8 @@ 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);
 void libxfs_buf_mark_dirty(struct xfs_buf *bp);
-struct xfs_buf *libxfs_buf_get_map(struct xfs_buftarg *btp,
-			struct xfs_buf_map *map, int nmaps, int flags);
+int libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *maps,
+			int nmaps, int flags, struct xfs_buf **bpp);
 void	libxfs_buf_relse(struct xfs_buf *bp);
 
 static inline struct xfs_buf*
@@ -179,9 +180,14 @@ libxfs_buf_get(
 	xfs_daddr_t		blkno,
 	size_t			numblks)
 {
+	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	return libxfs_buf_get_map(target, &map, 1, 0);
+	error = libxfs_buf_get_map(target, &map, 1, 0, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 static inline struct xfs_buf*
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index ea9979a2..31fc49b5 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -155,10 +155,9 @@ 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_writebuf(xfs_buf_t *, int);
-struct xfs_buf *libxfs_buf_get(struct xfs_buftarg *btp, xfs_daddr_t daddr,
-				size_t len);
-struct xfs_buf	*libxfs_buf_get_map(struct xfs_buftarg *btp,
-			struct xfs_buf_map *map, int nmaps, int flags);
+int		libxfs_buf_get_map(struct xfs_buftarg *btp,
+				struct xfs_buf_map *maps, int nmaps, int flags,
+				struct xfs_buf **bpp);
 void		libxfs_buf_relse(struct xfs_buf *bp);
 
 #define	__add_trace(bp, func, file, line)	\
@@ -212,19 +211,27 @@ libxfs_trace_getbuf(
 	struct xfs_buf		*bp;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	bp = libxfs_buf_get_map(target, &map, 1, 0);
+	libxfs_buf_get_map(target, &map, 1, 0, &bp);
 	__add_trace(bp, func, file, line);
 	return bp;
 }
 
-xfs_buf_t *
-libxfs_trace_getbuf_map(const char *func, const char *file, int line,
-		struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
-		int flags)
+int
+libxfs_trace_getbuf_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)
 {
-	xfs_buf_t	*bp = libxfs_buf_get_map(btp, map, nmaps, flags);
-	__add_trace(bp, func, file, line);
-	return bp;
+	int			error;
+
+	error = libxfs_buf_get_map(btp, map, nmaps, flags, bpp);
+	__add_trace(*bpp, func, file, line);
+	return error;
 }
 
 void
@@ -575,25 +582,20 @@ reset_buf_state(
 				LIBXFS_B_UPTODATE);
 }
 
-static struct xfs_buf *
+static int
 __libxfs_buf_get_map(
 	struct xfs_buftarg	*btp,
 	struct xfs_buf_map	*map,
 	int			nmaps,
-	int			flags)
+	int			flags,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_bufkey	key = {NULL};
-	struct xfs_buf		*bp;
 	int			i;
-	int			error;
 
-	if (nmaps == 1) {
-		error = libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
-				flags, &bp);
-		if (error)
-			return NULL;
-		return bp;
-	}
+	if (nmaps == 1)
+		return libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
+				flags, bpp);
 
 	key.buftarg = btp;
 	key.blkno = map[0].bm_bn;
@@ -603,21 +605,25 @@ __libxfs_buf_get_map(
 	key.map = map;
 	key.nmaps = nmaps;
 
-	error = __cache_lookup(&key, flags, &bp);
-	if (error)
-		return NULL;
-	return bp;
+	return __cache_lookup(&key, flags, bpp);
 }
 
-struct xfs_buf *
-libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *map,
-		  int nmaps, int flags)
+int
+libxfs_buf_get_map(
+	struct xfs_buftarg	*btp,
+	struct xfs_buf_map	*map,
+	int			nmaps,
+	int			flags,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf	*bp;
+	int			error;
 
-	bp = __libxfs_buf_get_map(btp, map, nmaps, flags);
-	reset_buf_state(bp);
-	return bp;
+	error = __libxfs_buf_get_map(btp, map, nmaps, flags, bpp);
+	if (error)
+		return error;
+
+	reset_buf_state(*bpp);
+	return 0;
 }
 
 void
@@ -820,8 +826,8 @@ libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
 		return libxfs_readbuf(btp, map[0].bm_bn, map[0].bm_len,
 					flags, ops);
 
-	bp = __libxfs_buf_get_map(btp, map, nmaps, 0);
-	if (!bp)
+	error = __libxfs_buf_get_map(btp, map, nmaps, 0, &bp);
+	if (error)
 		return NULL;
 
 	bp->b_error = 0;
diff --git a/libxfs/trans.c b/libxfs/trans.c
index b78bca86..21dd66cb 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -423,11 +423,16 @@ libxfs_trans_get_buf_map(
 	int			nmaps,
 	xfs_buf_flags_t		flags)
 {
-	xfs_buf_t		*bp;
+	struct xfs_buf		*bp;
 	struct xfs_buf_log_item	*bip;
+	int			error;
 
-	if (!tp)
-		return libxfs_buf_get_map(target, map, nmaps, 0);
+	if (!tp) {
+		error = libxfs_buf_get_map(target, map, nmaps, 0, &bp);
+		if (error)
+			return NULL;
+		return bp;
+	}
 
 	/*
 	 * If we find the buffer in the cache with this transaction
@@ -445,10 +450,9 @@ libxfs_trans_get_buf_map(
 		return bp;
 	}
 
-	bp = libxfs_buf_get_map(target, map, nmaps, 0);
-	if (bp == NULL) {
+	error = libxfs_buf_get_map(target, map, nmaps, 0, &bp);
+	if (error)
 		return NULL;
-	}
 
 	ASSERT(!bp->b_error);
 
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 7f705cc0..3cc85501 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -113,6 +113,7 @@ pf_queue_io(
 {
 	struct xfs_buf		*bp;
 	xfs_fsblock_t		fsbno = XFS_DADDR_TO_FSB(mp, map[0].bm_bn);
+	int			error;
 
 	/*
 	 * Never block on a buffer lock here, given that the actual repair
@@ -120,8 +121,9 @@ pf_queue_io(
 	 * the lock holder is either reading it from disk himself or
 	 * completely overwriting it this behaviour is perfectly fine.
 	 */
-	bp = libxfs_buf_get_map(mp->m_dev, map, nmaps, LIBXFS_GETBUF_TRYLOCK);
-	if (!bp)
+	error = -libxfs_buf_get_map(mp->m_dev, map, nmaps,
+			LIBXFS_GETBUF_TRYLOCK, &bp);
+	if (error)
 		return;
 
 	if (bp->b_flags & LIBXFS_B_UPTODATE) {


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 04/14] libxfs: refactor libxfs_readbuf out of existence
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (2 preceding siblings ...)
  2020-02-25  0:14 ` [PATCH 03/14] libxfs: make libxfs_buf_get_map " Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig

From: Darrick J. Wong <darrick.wong@oracle.com>

The two libxfs_readbuf* functions are awfully similar, so refactor one
into the other to reduce duplicated code.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/rdwr.c |   83 +++++++++++++++++++++------------------------------------
 1 file changed, 30 insertions(+), 53 deletions(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 31fc49b5..11307f34 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -736,53 +736,6 @@ libxfs_readbuf_verify(
 	return bp->b_error;
 }
 
-static struct xfs_buf *
-libxfs_readbuf(
-	struct xfs_buftarg	*btp,
-	xfs_daddr_t		blkno,
-	size_t			len,
-	int			flags,
-	const struct xfs_buf_ops *ops)
-{
-	struct xfs_buf		*bp;
-	int			error;
-
-	error = libxfs_getbuf_flags(btp, blkno, len, 0, &bp);
-	if (error)
-		return NULL;
-
-	/*
-	 * if the buffer was prefetched, it is likely that it was not validated.
-	 * Hence if we are supplied an ops function and the buffer is marked as
-	 * unchecked, we need to validate it now.
-	 *
-	 * We do this verification even if the buffer is dirty - the
-	 * verification is almost certainly going to fail the CRC check in this
-	 * case as a dirty buffer has not had the CRC recalculated. However, we
-	 * should not be dirtying unchecked buffers and therefore failing it
-	 * here because it's dirty and unchecked indicates we've screwed up
-	 * somewhere else.
-	 */
-	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;
-	}
-
-	/*
-	 * Set the ops on a cache miss (i.e. first physical read) as the
-	 * verifier may change the ops to match the type of buffer it contains.
-	 * A cache hit might reset the verifier to the original type if we set
-	 * it again, but it won't get called again and set to match the buffer
-	 * contents. *cough* xfs_da_node_buf_ops *cough*.
-	 */
-	error = libxfs_readbufr(btp, blkno, bp, len, flags);
-	if (!error)
-		libxfs_readbuf_verify(bp, ops);
-	return bp;
-}
-
 int
 libxfs_readbufr_map(struct xfs_buftarg *btp, struct xfs_buf *bp, int flags)
 {
@@ -823,20 +776,44 @@ libxfs_buf_read_map(struct xfs_buftarg *btp, struct xfs_buf_map *map, int nmaps,
 	int		error = 0;
 
 	if (nmaps == 1)
-		return libxfs_readbuf(btp, map[0].bm_bn, map[0].bm_len,
-					flags, ops);
-
-	error = __libxfs_buf_get_map(btp, map, nmaps, 0, &bp);
+		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;
 
+	/*
+	 * If the buffer was prefetched, it is likely that it was not validated.
+	 * Hence if we are supplied an ops function and the buffer is marked as
+	 * unchecked, we need to validate it now.
+	 *
+	 * We do this verification even if the buffer is dirty - the
+	 * verification is almost certainly going to fail the CRC check in this
+	 * case as a dirty buffer has not had the CRC recalculated. However, we
+	 * should not be dirtying unchecked buffers and therefore failing it
+	 * here because it's dirty and unchecked indicates we've screwed up
+	 * somewhere else.
+	 */
 	bp->b_error = 0;
-	if ((bp->b_flags & (LIBXFS_B_UPTODATE|LIBXFS_B_DIRTY))) {
+	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_readbufr_map(btp, bp, flags);
+
+	/*
+	 * Set the ops on a cache miss (i.e. first physical read) as the
+	 * verifier may change the ops to match the type of buffer it contains.
+	 * A cache hit might reset the verifier to the original type if we set
+	 * it again, but it won't get called again and set to match the buffer
+	 * contents. *cough* xfs_da_node_buf_ops *cough*.
+	 */
+	if (nmaps == 1)
+		error = libxfs_readbufr(btp, map[0].bm_bn, bp, map[0].bm_len,
+				flags);
+	else
+		error = libxfs_readbufr_map(btp, bp, flags);
 	if (!error)
 		libxfs_readbuf_verify(bp, ops);
 


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (3 preceding siblings ...)
  2020-02-25  0:14 ` [PATCH 04/14] libxfs: refactor libxfs_readbuf out of existence Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25 17:55   ` Christoph Hellwig
  2020-02-25  0:14 ` [PATCH 06/14] xfs: make xfs_buf_read_map " Darrick J. Wong
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs

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      |   71 +++++++++++++++++++++++++++++++++++++++++-----------
 libxfs/trans.c     |   24 ++++--------------
 repair/da_util.c   |    3 +-
 5 files changed, 82 insertions(+), 45 deletions(-)


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 3cad7ef5..ca65e35e 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -127,14 +127,17 @@ extern struct cache_operations	libxfs_bcache_operations;
 
 #define LIBXFS_GETBUF_TRYLOCK	(1 << 0)
 
+/* Return the buffer even if the verifiers fail. */
+#define LIBXFS_READBUF_SALVAGE		(1 << 1)
+
 #ifdef XFS_BUF_TRACING
 
 #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_mark_dirty(buf) \
 	libxfs_trace_dirtybuf(__FUNCTION__, __FILE__, __LINE__, \
 			      (buf))
@@ -150,9 +153,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);
 struct xfs_buf *libxfs_trace_getbuf(const char *func, const char *file,
@@ -166,8 +170,8 @@ extern void	libxfs_trace_putbuf (const char *, const char *, int,
 
 #else
 
-struct xfs_buf *libxfs_buf_read_map(struct xfs_buftarg *btp,
-			struct xfs_buf_map *map, int nmaps, int flags,
+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_mark_dirty(struct xfs_buf *bp);
 int libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *maps,
@@ -198,9 +202,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 11307f34..a24d5912 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -151,9 +151,10 @@ static char *next(
 #undef libxfs_writebuf
 #undef libxfs_buf_get_map
 
-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 *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,
@@ -183,11 +184,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,
@@ -768,20 +788,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.
@@ -794,12 +821,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 fails verification.
 	 */
 	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;
 	}
 
 	/*
@@ -814,15 +846,24 @@ 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)
+		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 21dd66cb..73f69bce 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 e639ecda..5061880f 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;


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 06/14] xfs: make xfs_buf_read_map return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (4 preceding siblings ...)
  2020-02-25  0:14 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:14 ` [PATCH 07/14] xfs: make xfs_buf_get " Darrick J. Wong
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 4ed8e27b4f755f50d78dc3d9f9760b60e891f97b

Convert xfs_buf_read_map() to return numeric error codes like most
everywhere else in xfs.  This involves moving the open-coded logic that
reports metadata IO read / corruption errors and stales the buffer into
xfs_buf_read_map so that the logic is all in one place.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/xfs_alloc.c       |   11 +++++++----
 libxfs/xfs_attr_remote.c |   10 ----------
 2 files changed, 7 insertions(+), 14 deletions(-)


diff --git a/libxfs/xfs_alloc.c b/libxfs/xfs_alloc.c
index ec233d0d..1bf813e3 100644
--- a/libxfs/xfs_alloc.c
+++ b/libxfs/xfs_alloc.c
@@ -2952,14 +2952,17 @@ xfs_read_agf(
 	trace_xfs_read_agf(mp, agno);
 
 	ASSERT(agno != NULLAGNUMBER);
-	error = xfs_trans_read_buf(
-			mp, tp, mp->m_ddev_targp,
+	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
 			XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops);
+	/*
+	 * Callers of xfs_read_agf() currently interpret a NULL bpp as EAGAIN
+	 * and need to be converted to check for EAGAIN specifically.
+	 */
+	if (error == -EAGAIN)
+		return 0;
 	if (error)
 		return error;
-	if (!*bpp)
-		return 0;
 
 	ASSERT(!(*bpp)->b_error);
 	xfs_buf_set_ref(*bpp, XFS_AGF_REF);
diff --git a/libxfs/xfs_attr_remote.c b/libxfs/xfs_attr_remote.c
index c7a4be35..00371bdc 100644
--- a/libxfs/xfs_attr_remote.c
+++ b/libxfs/xfs_attr_remote.c
@@ -421,16 +421,6 @@ xfs_attr_rmtval_get(
 					&xfs_attr3_rmt_buf_ops);
 			if (!bp)
 				return -ENOMEM;
-			error = bp->b_error;
-			if (error) {
-				xfs_buf_ioerror_alert(bp, __func__);
-				xfs_buf_relse(bp);
-
-				/* bad CRC means corrupted metadata */
-				if (error == -EFSBADCRC)
-					error = -EFSCORRUPTED;
-				return error;
-			}
 
 			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
 							&offset, &valuelen,


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 07/14] xfs: make xfs_buf_get return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (5 preceding siblings ...)
  2020-02-25  0:14 ` [PATCH 06/14] xfs: make xfs_buf_read_map " Darrick J. Wong
@ 2020-02-25  0:14 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 08/14] xfs: make xfs_buf_get_uncached " Darrick J. Wong
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:14 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 841263e93310595c30653a9f530b2d7bbeed5aae

Convert xfs_buf_get() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_io.h       |   22 +++----
 libxfs/rdwr.c            |   11 ++--
 libxfs/xfs_attr_remote.c |    6 +-
 libxfs/xfs_sb.c          |    8 +--
 repair/phase5.c          |  138 +++++++++++++++++++++++++++++++++-------------
 5 files changed, 122 insertions(+), 63 deletions(-)


diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index ca65e35e..cd38ec09 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -141,9 +141,9 @@ extern struct cache_operations	libxfs_bcache_operations;
 #define libxfs_buf_mark_dirty(buf) \
 	libxfs_trace_dirtybuf(__FUNCTION__, __FILE__, __LINE__, \
 			      (buf))
-#define libxfs_buf_get(dev, daddr, len) \
+#define libxfs_buf_get(dev, daddr, len, bpp) \
 	libxfs_trace_getbuf(__FUNCTION__, __FILE__, __LINE__, \
-			    (dev), (daddr), (len))
+			    (dev), (daddr), (len), (bpp))
 #define libxfs_buf_get_map(dev, map, nmaps, flags, bpp) \
 	libxfs_trace_getbuf_map(__FUNCTION__, __FILE__, __LINE__, \
 			    (dev), (map), (nmaps), (flags), (bpp))
@@ -159,9 +159,9 @@ int libxfs_trace_readbuf_map(const char *func, const char *file, int line,
 			const struct xfs_buf_ops *ops);
 void libxfs_trace_dirtybuf(const char *func, const char *file, int line,
 			struct xfs_buf *bp);
-struct xfs_buf *libxfs_trace_getbuf(const char *func, const char *file,
-			int line, struct xfs_buftarg *btp, xfs_daddr_t daddr,
-			size_t len);
+int libxfs_trace_getbuf(const char *func, const char *file, int line,
+			struct xfs_buftarg *btp, xfs_daddr_t daddr,
+			size_t len, struct xfs_buf **bpp);
 int libxfs_trace_getbuf_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);
@@ -178,20 +178,16 @@ int libxfs_buf_get_map(struct xfs_buftarg *btp, struct xfs_buf_map *maps,
 			int nmaps, int flags, struct xfs_buf **bpp);
 void	libxfs_buf_relse(struct xfs_buf *bp);
 
-static inline struct xfs_buf*
+static inline int
 libxfs_buf_get(
 	struct xfs_buftarg	*target,
 	xfs_daddr_t		blkno,
-	size_t			numblks)
+	size_t			numblks,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
-	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	error = libxfs_buf_get_map(target, &map, 1, 0, &bp);
-	if (error)
-		return NULL;
-	return bp;
+	return libxfs_buf_get_map(target, &map, 1, 0, bpp);
 }
 
 static inline struct xfs_buf*
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index a24d5912..0778a15f 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -219,21 +219,22 @@ libxfs_trace_dirtybuf(
 	libxfs_buf_mark_dirty(bp);
 }
 
-struct xfs_buf *
+int
 libxfs_trace_getbuf(
 	const char		*func,
 	const char		*file,
 	int			line,
 	struct xfs_buftarg	*btp,
 	xfs_daddr_t		blkno,
-	size_t			len)
+	size_t			len,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	libxfs_buf_get_map(target, &map, 1, 0, &bp);
+	error = libxfs_buf_get_map(target, &map, 1, 0, bpp);
 	__add_trace(bp, func, file, line);
-	return bp;
+	return error;
 }
 
 int
diff --git a/libxfs/xfs_attr_remote.c b/libxfs/xfs_attr_remote.c
index 00371bdc..88163ea8 100644
--- a/libxfs/xfs_attr_remote.c
+++ b/libxfs/xfs_attr_remote.c
@@ -544,9 +544,9 @@ xfs_attr_rmtval_set(
 		dblkno = XFS_FSB_TO_DADDR(mp, map.br_startblock),
 		dblkcnt = XFS_FSB_TO_BB(mp, map.br_blockcount);
 
-		bp = xfs_buf_get(mp->m_ddev_targp, dblkno, dblkcnt);
-		if (!bp)
-			return -ENOMEM;
+		error = xfs_buf_get(mp->m_ddev_targp, dblkno, dblkcnt, &bp);
+		if (error)
+			return error;
 		bp->b_ops = &xfs_attr3_rmt_buf_ops;
 
 		xfs_attr_rmtval_copyin(mp, bp, args->dp->i_ino, &offset,
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index 4f750d19..d3d5e11d 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -962,9 +962,9 @@ xfs_update_secondary_sbs(
 	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
 		struct xfs_buf		*bp;
 
-		bp = xfs_buf_get(mp->m_ddev_targp,
+		error = xfs_buf_get(mp->m_ddev_targp,
 				 XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
-				 XFS_FSS_TO_BB(mp, 1));
+				 XFS_FSS_TO_BB(mp, 1), &bp);
 		/*
 		 * If we get an error reading or writing alternate superblocks,
 		 * continue.  xfs_repair chooses the "best" superblock based
@@ -972,12 +972,12 @@ xfs_update_secondary_sbs(
 		 * superblocks un-updated than updated, and xfs_repair may
 		 * pick them over the properly-updated primary.
 		 */
-		if (!bp) {
+		if (error) {
 			xfs_warn(mp,
 		"error allocating secondary superblock for ag %d",
 				agno);
 			if (!saved_error)
-				saved_error = -ENOMEM;
+				saved_error = error;
 			continue;
 		}
 
diff --git a/repair/phase5.c b/repair/phase5.c
index 7ec58f88..abae8a08 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -652,6 +652,7 @@ prop_freespace_cursor(xfs_mount_t *mp, xfs_agnumber_t agno,
 	xfs_agblock_t		agbno;
 	bt_stat_level_t		*lptr;
 	const struct xfs_buf_ops *ops = btnum_to_ops(btnum);
+	int			error;
 
 	ASSERT(btnum == XFS_BTNUM_BNO || btnum == XFS_BTNUM_CNT);
 
@@ -692,9 +693,13 @@ prop_freespace_cursor(xfs_mount_t *mp, xfs_agnumber_t agno,
 
 		bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(agbno);
 
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(
+	_("Cannot grab free space btree buffer, err=%d"),
+					error);
 		lptr->agbno = agbno;
 
 		if (lptr->modulo)
@@ -752,6 +757,7 @@ build_freespace_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 	bt_stat_level_t		*lptr;
 	xfs_extlen_t		freeblks;
 	const struct xfs_buf_ops *ops = btnum_to_ops(btnum);
+	int			error;
 
 	ASSERT(btnum == XFS_BTNUM_BNO || btnum == XFS_BTNUM_CNT);
 
@@ -770,9 +776,13 @@ build_freespace_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 		lptr = &btree_curs->level[i];
 
 		agbno = get_next_blockaddr(agno, i, btree_curs);
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(
+	_("Cannot grab free space btree buffer, err=%d"),
+					error);
 
 		if (i == btree_curs->num_levels - 1)
 			btree_curs->root = agbno;
@@ -881,9 +891,14 @@ build_freespace_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 			lptr->agbno = get_next_blockaddr(agno, 0, btree_curs);
 			bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(lptr->agbno);
 
-			lptr->buf_p = libxfs_buf_get(mp->m_dev,
+			error = -libxfs_buf_get(mp->m_dev,
 					XFS_AGB_TO_DADDR(mp, agno, lptr->agbno),
-					XFS_FSB_TO_BB(mp, 1));
+					XFS_FSB_TO_BB(mp, 1),
+					&lptr->buf_p);
+			if (error)
+				do_error(
+	_("Cannot grab free space btree buffer, err=%d"),
+						error);
 		}
 	}
 
@@ -1021,6 +1036,7 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
 	xfs_agblock_t		agbno;
 	bt_stat_level_t		*lptr;
 	const struct xfs_buf_ops *ops = btnum_to_ops(btnum);
+	int			error;
 
 	level++;
 
@@ -1059,9 +1075,12 @@ prop_ino_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
 
 		bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(agbno);
 
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab inode btree buffer, err=%d"),
+					error);
 		lptr->agbno = agbno;
 
 		if (lptr->modulo)
@@ -1108,10 +1127,14 @@ build_agi(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *btree_curs,
 	xfs_buf_t	*agi_buf;
 	xfs_agi_t	*agi;
 	int		i;
+	int		error;
 
-	agi_buf = libxfs_buf_get(mp->m_dev,
+	error = -libxfs_buf_get(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp)),
-			mp->m_sb.sb_sectsize/BBSIZE);
+			mp->m_sb.sb_sectsize / BBSIZE, &agi_buf);
+	if (error)
+		do_error(_("Cannot grab AG %u AGI buffer, err=%d"),
+				agno, error);
 	agi_buf->b_ops = &xfs_agi_buf_ops;
 	agi = XFS_BUF_TO_AGI(agi_buf);
 	memset(agi, 0, mp->m_sb.sb_sectsize);
@@ -1173,6 +1196,7 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 	int			spmask;
 	uint64_t		sparse;
 	uint16_t		holemask;
+	int			error;
 
 	ASSERT(btnum == XFS_BTNUM_INO || btnum == XFS_BTNUM_FINO);
 
@@ -1180,9 +1204,12 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 		lptr = &btree_curs->level[i];
 
 		agbno = get_next_blockaddr(agno, i, btree_curs);
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab inode btree buffer, err=%d"),
+					error);
 
 		if (i == btree_curs->num_levels - 1)
 			btree_curs->root = agbno;
@@ -1313,9 +1340,14 @@ build_ino_tree(xfs_mount_t *mp, xfs_agnumber_t agno,
 			lptr->agbno = get_next_blockaddr(agno, 0, btree_curs);
 			bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(lptr->agbno);
 
-			lptr->buf_p = libxfs_buf_get(mp->m_dev,
+			error = -libxfs_buf_get(mp->m_dev,
 					XFS_AGB_TO_DADDR(mp, agno, lptr->agbno),
-					XFS_FSB_TO_BB(mp, 1));
+					XFS_FSB_TO_BB(mp, 1),
+					&lptr->buf_p);
+			if (error)
+				do_error(
+	_("Cannot grab inode btree buffer, err=%d"),
+						error);
 		}
 	}
 
@@ -1429,6 +1461,7 @@ prop_rmap_cursor(
 	xfs_agblock_t		agbno;
 	struct bt_stat_level	*lptr;
 	const struct xfs_buf_ops *ops = btnum_to_ops(XFS_BTNUM_RMAP);
+	int			error;
 
 	level++;
 
@@ -1467,9 +1500,12 @@ prop_rmap_cursor(
 
 		bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(agbno);
 
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab rmapbt buffer, err=%d"),
+					error);
 		lptr->agbno = agbno;
 
 		if (lptr->modulo)
@@ -1577,9 +1613,12 @@ build_rmap_tree(
 		lptr = &btree_curs->level[i];
 
 		agbno = get_next_blockaddr(agno, i, btree_curs);
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab rmapbt buffer, err=%d"),
+					error);
 
 		if (i == btree_curs->num_levels - 1)
 			btree_curs->root = agbno;
@@ -1677,9 +1716,14 @@ _("Insufficient memory to construct reverse-map cursor."));
 			lptr->agbno = get_next_blockaddr(agno, 0, btree_curs);
 			bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(lptr->agbno);
 
-			lptr->buf_p = libxfs_buf_get(mp->m_dev,
+			error = -libxfs_buf_get(mp->m_dev,
 					XFS_AGB_TO_DADDR(mp, agno, lptr->agbno),
-					XFS_FSB_TO_BB(mp, 1));
+					XFS_FSB_TO_BB(mp, 1),
+					&lptr->buf_p);
+			if (error)
+				do_error(
+	_("Cannot grab rmapbt buffer, err=%d"),
+						error);
 		}
 	}
 	free_slab_cursor(&rmap_cur);
@@ -1781,6 +1825,7 @@ prop_refc_cursor(
 	xfs_agblock_t		agbno;
 	struct bt_stat_level	*lptr;
 	const struct xfs_buf_ops *ops = btnum_to_ops(XFS_BTNUM_REFC);
+	int			error;
 
 	level++;
 
@@ -1819,9 +1864,12 @@ prop_refc_cursor(
 
 		bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(agbno);
 
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab refcountbt buffer, err=%d"),
+					error);
 		lptr->agbno = agbno;
 
 		if (lptr->modulo)
@@ -1884,9 +1932,12 @@ build_refcount_tree(
 		lptr = &btree_curs->level[i];
 
 		agbno = get_next_blockaddr(agno, i, btree_curs);
-		lptr->buf_p = libxfs_buf_get(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, 1));
+		error = -libxfs_buf_get(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp, 1), &lptr->buf_p);
+		if (error)
+			do_error(_("Cannot grab refcountbt buffer, err=%d"),
+					error);
 
 		if (i == btree_curs->num_levels - 1)
 			btree_curs->root = agbno;
@@ -1972,9 +2023,14 @@ _("Insufficient memory to construct refcount cursor."));
 			lptr->agbno = get_next_blockaddr(agno, 0, btree_curs);
 			bt_hdr->bb_u.s.bb_rightsib = cpu_to_be32(lptr->agbno);
 
-			lptr->buf_p = libxfs_buf_get(mp->m_dev,
+			error = -libxfs_buf_get(mp->m_dev,
 					XFS_AGB_TO_DADDR(mp, agno, lptr->agbno),
-					XFS_FSB_TO_BB(mp, 1));
+					XFS_FSB_TO_BB(mp, 1),
+					&lptr->buf_p);
+			if (error)
+				do_error(
+	_("Cannot grab refcountbt buffer, err=%d"),
+						error);
 		}
 	}
 	free_slab_cursor(&refc_cur);
@@ -2007,9 +2063,12 @@ build_agf_agfl(
 	__be32			*freelist;
 	int			error;
 
-	agf_buf = libxfs_buf_get(mp->m_dev,
+	error = -libxfs_buf_get(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
-			mp->m_sb.sb_sectsize/BBSIZE);
+			mp->m_sb.sb_sectsize / BBSIZE, &agf_buf);
+	if (error)
+		do_error(_("Cannot grab AG %u AGF buffer, err=%d"),
+				agno, error);
 	agf_buf->b_ops = &xfs_agf_buf_ops;
 	agf = XFS_BUF_TO_AGF(agf_buf);
 	memset(agf, 0, mp->m_sb.sb_sectsize);
@@ -2079,9 +2138,12 @@ build_agf_agfl(
 		platform_uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
 
 	/* initialise the AGFL, then fill it if there are blocks left over. */
-	agfl_buf = libxfs_buf_get(mp->m_dev,
+	error = -libxfs_buf_get(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGFL_DADDR(mp)),
-			mp->m_sb.sb_sectsize/BBSIZE);
+			mp->m_sb.sb_sectsize / BBSIZE, &agfl_buf);
+	if (error)
+		do_error(_("Cannot grab AG %u AGFL buffer, err=%d"),
+				agno, error);
 	agfl_buf->b_ops = &xfs_agfl_buf_ops;
 	agfl = XFS_BUF_TO_AGFL(agfl_buf);
 


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 08/14] xfs: make xfs_buf_get_uncached return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (6 preceding siblings ...)
  2020-02-25  0:14 ` [PATCH 07/14] xfs: make xfs_buf_get " Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 09/14] xfs: make xfs_buf_read " Darrick J. Wong
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 2842b6db3d539bec08d080b22635b6e8acaa30ec

Convert xfs_buf_get_uncached() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_io.h        |    4 ++--
 libxfs/rdwr.c             |    8 +++++---
 libxfs/xfs_ag.c           |   21 ++++++++++++---------
 libxlog/xfs_log_recover.c |    7 +++++--
 mkfs/xfs_mkfs.c           |    8 +++++++-
 5 files changed, 31 insertions(+), 17 deletions(-)


diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index cd38ec09..703a2168 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -254,8 +254,8 @@ xfs_buf_associate_memory(struct xfs_buf *bp, void *mem, size_t len)
 	return 0;
 }
 
-struct xfs_buf *libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen,
-		int flags);
+int libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen, int flags,
+		struct xfs_buf **bpp);
 int libxfs_buf_read_uncached(struct xfs_buftarg *targ, xfs_daddr_t daddr,
 		size_t bblen, int flags, struct xfs_buf **bpp,
 		const struct xfs_buf_ops *ops);
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 0778a15f..2d056009 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -889,13 +889,15 @@ libxfs_getbufr_uncached(
  * Allocate an uncached buffer that points nowhere.  The refcount will be 1,
  * and the cache node hash list will be empty to indicate that it's uncached.
  */
-struct xfs_buf *
+int
 libxfs_buf_get_uncached(
 	struct xfs_buftarg	*targ,
 	size_t			bblen,
-	int			flags)
+	int			flags,
+	struct xfs_buf		**bpp)
 {
-	return libxfs_getbufr_uncached(targ, XFS_BUF_DADDR_NULL, bblen);
+	*bpp = libxfs_getbufr_uncached(targ, XFS_BUF_DADDR_NULL, bblen);
+	return *bpp != NULL ? 0 : -ENOMEM;
 }
 
 /*
diff --git a/libxfs/xfs_ag.c b/libxfs/xfs_ag.c
index 5bf6975d..73fb30cb 100644
--- a/libxfs/xfs_ag.c
+++ b/libxfs/xfs_ag.c
@@ -23,25 +23,28 @@
 #include "xfs_ag_resv.h"
 #include "xfs_health.h"
 
-static struct xfs_buf *
+static int
 xfs_get_aghdr_buf(
 	struct xfs_mount	*mp,
 	xfs_daddr_t		blkno,
 	size_t			numblks,
+	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
-	bp = xfs_buf_get_uncached(mp->m_ddev_targp, numblks, 0);
-	if (!bp)
-		return NULL;
+	error = xfs_buf_get_uncached(mp->m_ddev_targp, numblks, 0, &bp);
+	if (error)
+		return error;
 
 	xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
 	bp->b_bn = blkno;
 	bp->b_maps[0].bm_bn = blkno;
 	bp->b_ops = ops;
 
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 static inline bool is_log_ag(struct xfs_mount *mp, struct aghdr_init_data *id)
@@ -340,13 +343,13 @@ xfs_ag_init_hdr(
 	struct aghdr_init_data	*id,
 	aghdr_init_work_f	work,
 	const struct xfs_buf_ops *ops)
-
 {
 	struct xfs_buf		*bp;
+	int			error;
 
-	bp = xfs_get_aghdr_buf(mp, id->daddr, id->numblks, ops);
-	if (!bp)
-		return -ENOMEM;
+	error = xfs_get_aghdr_buf(mp, id->daddr, id->numblks, &bp, ops);
+	if (error)
+		return error;
 
 	(*work)(mp, bp, id);
 
diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index e7e57bd2..78fbafab 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -35,11 +35,13 @@ xlog_buf_bbcount_valid(
  * to map to a range of nbblks basic blocks at any valid (basic
  * block) offset within the log.
  */
-xfs_buf_t *
+struct xfs_buf *
 xlog_get_bp(
 	struct xlog	*log,
 	int		nbblks)
 {
+	struct xfs_buf	*bp;
+
 	if (!xlog_buf_bbcount_valid(log, nbblks)) {
 		xfs_warn(log->l_mp, "Invalid block length (0x%x) for buffer",
 			nbblks);
@@ -67,7 +69,8 @@ xlog_get_bp(
 		nbblks += log->l_sectBBsize;
 	nbblks = round_up(nbblks, log->l_sectBBsize);
 
-	return libxfs_buf_get_uncached(log->l_dev, nbblks, 0);
+	libxfs_buf_get_uncached(log->l_dev, nbblks, 0, &bp);
+	return bp;
 }
 
 /*
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 295deb86..efe4feaa 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3404,8 +3404,14 @@ alloc_write_buf(
 	int			bblen)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
-	bp = libxfs_buf_get_uncached(btp, bblen, 0);
+	error = -libxfs_buf_get_uncached(btp, bblen, 0, &bp);
+	if (error) {
+		fprintf(stderr, _("Could not get memory for buffer, err=%d\n"),
+				error);
+		exit(1);
+	}
 	bp->b_bn = daddr;
 	bp->b_maps[0].bm_bn = daddr;
 	return bp;


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 09/14] xfs: make xfs_buf_read return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (7 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 08/14] xfs: make xfs_buf_get_uncached " Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 10/14] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 0e3eccce5e0e438bc1aa3c2913221d3d43a1bef4

Convert xfs_buf_read() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 copy/xfs_copy.c          |   11 +++++--
 db/io.c                  |    3 +-
 libxfs/init.c            |   35 +++++++++++----------
 libxfs/libxfs_io.h       |   21 +++++--------
 libxfs/rdwr.c            |   20 +++++++-----
 libxfs/xfs_attr_remote.c |    8 ++---
 mkfs/xfs_mkfs.c          |   13 ++++----
 repair/attr_repair.c     |   29 ++++++++++--------
 repair/dino_chunks.c     |   23 ++++++++------
 repair/dinode.c          |   25 ++++++++++-----
 repair/phase3.c          |    8 +++--
 repair/prefetch.c        |    8 +++--
 repair/rt.c              |   12 ++++---
 repair/scan.c            |   76 ++++++++++++++++++++++++++++++++++------------
 14 files changed, 178 insertions(+), 114 deletions(-)


diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 5cab1a5f..91c2ae01 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -725,9 +725,14 @@ main(int argc, char **argv)
 	/* Do it again, now with proper length and verifier */
 	libxfs_buf_relse(sbp);
 
-	sbp = libxfs_buf_read(mbuf.m_ddev_targp, XFS_SB_DADDR,
-			     1 << (sb->sb_sectlog - BBSHIFT),
-			     0, &xfs_sb_buf_ops);
+	error = -libxfs_buf_read(mbuf.m_ddev_targp, XFS_SB_DADDR,
+			1 << (sb->sb_sectlog - BBSHIFT), 0, &sbp,
+			&xfs_sb_buf_ops);
+	if (error) {
+		do_log(_("%s: couldn't read superblock, error=%d\n"),
+				progname, error);
+		exit(1);
+	}
 	libxfs_buf_relse(sbp);
 
 	mp = libxfs_mount(&mbuf, sb, xargs.ddev, xargs.logdev, xargs.rtdev, 0);
diff --git a/db/io.c b/db/io.c
index 5c9d72bb..384e4c0f 100644
--- a/db/io.c
+++ b/db/io.c
@@ -545,7 +545,8 @@ set_cur(
 		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);
+		libxfs_buf_read(mp->m_ddev_targp, blknum, len,
+				LIBXFS_READBUF_SALVAGE, &bp, ops);
 		iocur_top->bbmap = NULL;
 	}
 
diff --git a/libxfs/init.c b/libxfs/init.c
index 328f46ac..dfddb0d5 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -415,9 +415,10 @@ rtmount_init(
 	xfs_mount_t	*mp,	/* file system mount structure */
 	int		flags)
 {
-	xfs_buf_t	*bp;	/* buffer for last block of subvolume */
+	struct xfs_buf	*bp;	/* buffer for last block of subvolume */
+	struct xfs_sb	*sbp;	/* filesystem superblock copy in mount */
 	xfs_daddr_t	d;	/* address of last block of subvolume */
-	xfs_sb_t	*sbp;	/* filesystem superblock copy in mount */
+	int		error;
 
 	sbp = &mp->m_sb;
 	if (sbp->sb_rblocks == 0)
@@ -450,9 +451,9 @@ rtmount_init(
 			(unsigned long long) mp->m_sb.sb_rblocks);
 		return -1;
 	}
-	bp = libxfs_buf_read(mp->m_rtdev,
-			d - XFS_FSB_TO_BB(mp, 1), XFS_FSB_TO_BB(mp, 1), 0, NULL);
-	if (bp == NULL) {
+	error = libxfs_buf_read(mp->m_rtdev, d - XFS_FSB_TO_BB(mp, 1),
+			XFS_FSB_TO_BB(mp, 1), 0, &bp, NULL);
+	if (error) {
 		fprintf(stderr, _("%s: realtime size check failed\n"),
 			progname);
 		return -1;
@@ -730,9 +731,9 @@ libxfs_mount(
 		return mp;
 
 	/* device size checks must pass unless we're a debugger. */
-	bp = libxfs_buf_read(mp->m_dev, d - XFS_FSS_TO_BB(mp, 1),
-			XFS_FSS_TO_BB(mp, 1), 0, NULL);
-	if (!bp) {
+	error = libxfs_buf_read(mp->m_dev, d - XFS_FSS_TO_BB(mp, 1),
+			XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
+	if (error) {
 		fprintf(stderr, _("%s: data size check failed\n"), progname);
 		if (!debugger)
 			return NULL;
@@ -742,10 +743,10 @@ libxfs_mount(
 	if (mp->m_logdev_targp->dev &&
 	    mp->m_logdev_targp->dev != mp->m_ddev_targp->dev) {
 		d = (xfs_daddr_t) XFS_FSB_TO_BB(mp, mp->m_sb.sb_logblocks);
-		if ( (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_logblocks) ||
-		     (!(bp = libxfs_buf_read(mp->m_logdev_targp,
-					d - XFS_FSB_TO_BB(mp, 1),
-					XFS_FSB_TO_BB(mp, 1), 0, NULL)))) {
+		if (XFS_BB_TO_FSB(mp, d) != mp->m_sb.sb_logblocks ||
+		    libxfs_buf_read(mp->m_logdev_targp,
+				d - XFS_FSB_TO_BB(mp, 1), XFS_FSB_TO_BB(mp, 1),
+				0, &bp, NULL)) {
 			fprintf(stderr, _("%s: log size checks failed\n"),
 					progname);
 			if (!debugger)
@@ -770,10 +771,10 @@ libxfs_mount(
 	 * read the first one and let the user know to check the geometry.
 	 */
 	if (sbp->sb_agcount > 1000000) {
-		bp = libxfs_buf_read(mp->m_dev,
+		error = libxfs_buf_read(mp->m_dev,
 				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
-				0, NULL);
-		if (bp->b_error) {
+				0, &bp, NULL);
+		if (error) {
 			fprintf(stderr, _("%s: read of AG %u failed\n"),
 						progname, sbp->sb_agcount);
 			if (!debugger)
@@ -781,8 +782,8 @@ libxfs_mount(
 			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
 								progname);
 			sbp->sb_agcount = 1;
-		}
-		libxfs_buf_relse(bp);
+		} else
+			libxfs_buf_relse(bp);
 	}
 
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
index 703a2168..0e5df33d 100644
--- a/libxfs/libxfs_io.h
+++ b/libxfs/libxfs_io.h
@@ -132,9 +132,9 @@ extern struct cache_operations	libxfs_bcache_operations;
 
 #ifdef XFS_BUF_TRACING
 
-#define libxfs_buf_read(dev, daddr, len, flags, ops) \
+#define libxfs_buf_read(dev, daddr, len, flags, bpp, ops) \
 	libxfs_trace_readbuf(__FUNCTION__, __FILE__, __LINE__, \
-			    (dev), (daddr), (len), (flags), (ops))
+			    (dev), (daddr), (len), (flags), (bpp), (ops))
 #define libxfs_buf_read_map(dev, map, nmaps, flags, bpp, ops) \
 	libxfs_trace_readbuf_map(__FUNCTION__, __FILE__, __LINE__, \
 			    (dev), (map), (nmaps), (flags), (bpp), (ops))
@@ -150,9 +150,10 @@ extern struct cache_operations	libxfs_bcache_operations;
 #define libxfs_buf_relse(buf) \
 	libxfs_trace_putbuf(__FUNCTION__, __FILE__, __LINE__, (buf))
 
-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);
+int 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,
+			struct xfs_buf **bpp);
 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,
@@ -190,22 +191,18 @@ libxfs_buf_get(
 	return libxfs_buf_get_map(target, &map, 1, 0, bpp);
 }
 
-static inline struct xfs_buf*
+static inline int
 libxfs_buf_read(
 	struct xfs_buftarg	*target,
 	xfs_daddr_t		blkno,
 	size_t			numblks,
 	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	struct xfs_buf		*bp;
-	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	error = libxfs_buf_read_map(target, &map, 1, flags, &bp, ops);
-	if (error)
-		return NULL;
-	return bp;
+	return libxfs_buf_read_map(target, &map, 1, flags, bpp, ops);
 }
 
 #endif /* XFS_BUF_TRACING */
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 2d056009..5918566c 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -170,7 +170,7 @@ do {						\
 	}					\
 } while (0)
 
-struct xfs_buf *
+int
 libxfs_trace_readbuf(
 	const char		*func,
 	const char		*file,
@@ -179,14 +179,15 @@ libxfs_trace_readbuf(
 	xfs_daddr_t		blkno,
 	size_t			len,
 	int			flags,
-	const struct xfs_buf_ops *ops)
+	const struct xfs_buf_ops *ops,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	libxfs_buf_read_map(btp, &map, 1, flags, &bp, ops);
-	__add_trace(bp, func, file, line);
-	return bp;
+	error = libxfs_buf_read_map(btp, &map, 1, flags, bpp, ops);
+	__add_trace(*bpp, func, file, line);
+	return error;
 }
 
 int
@@ -270,8 +271,11 @@ struct xfs_buf *
 libxfs_getsb(
 	struct xfs_mount	*mp)
 {
-	return libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR,
-			XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);
+	struct xfs_buf		*bp;
+
+	libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR, XFS_FSS_TO_BB(mp, 1),
+			0, &bp, &xfs_sb_buf_ops);
+	return bp;
 }
 
 kmem_zone_t			*xfs_buf_zone;
diff --git a/libxfs/xfs_attr_remote.c b/libxfs/xfs_attr_remote.c
index 88163ea8..b2a01567 100644
--- a/libxfs/xfs_attr_remote.c
+++ b/libxfs/xfs_attr_remote.c
@@ -417,10 +417,10 @@ xfs_attr_rmtval_get(
 			       (map[i].br_startblock != HOLESTARTBLOCK));
 			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
 			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
-			bp = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt, 0,
-					&xfs_attr3_rmt_buf_ops);
-			if (!bp)
-				return -ENOMEM;
+			error = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt,
+					0, &bp, &xfs_attr3_rmt_buf_ops);
+			if (error)
+				return error;
 
 			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
 							&offset, &valuelen,
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index efe4feaa..8a6534f3 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3593,13 +3593,14 @@ rewrite_secondary_superblocks(
 	struct xfs_mount	*mp)
 {
 	struct xfs_buf		*buf;
+	int			error;
 
 	/* rewrite the last superblock */
-	buf = libxfs_buf_read(mp->m_dev,
+	error = -libxfs_buf_read(mp->m_dev,
 			XFS_AGB_TO_DADDR(mp, mp->m_sb.sb_agcount - 1,
 				XFS_SB_DADDR),
-			XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);
-	if (!buf) {
+			XFS_FSS_TO_BB(mp, 1), 0, &buf, &xfs_sb_buf_ops);
+	if (error) {
 		fprintf(stderr, _("%s: could not re-read AG %u superblock\n"),
 				progname, mp->m_sb.sb_agcount - 1);
 		exit(1);
@@ -3612,11 +3613,11 @@ rewrite_secondary_superblocks(
 	if (mp->m_sb.sb_agcount <= 2)
 		return;
 
-	buf = libxfs_buf_read(mp->m_dev,
+	error = -libxfs_buf_read(mp->m_dev,
 			XFS_AGB_TO_DADDR(mp, (mp->m_sb.sb_agcount - 1) / 2,
 				XFS_SB_DADDR),
-			XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);
-	if (!buf) {
+			XFS_FSS_TO_BB(mp, 1), 0, &buf, &xfs_sb_buf_ops);
+	if (error) {
 		fprintf(stderr, _("%s: could not re-read AG %u superblock\n"),
 				progname, (mp->m_sb.sb_agcount - 1) / 2);
 		exit(1);
diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 3cef3004..9d68f61b 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -391,6 +391,7 @@ rmtval_get(xfs_mount_t *mp, xfs_ino_t ino, blkmap_t *blkmap,
 	xfs_buf_t	*bp;
 	int		clearit = 0, i = 0, length = 0, amountdone = 0;
 	int		hdrsize = 0;
+	int		error;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb))
 		hdrsize = sizeof(struct xfs_attr3_rmt_hdr);
@@ -405,10 +406,10 @@ rmtval_get(xfs_mount_t *mp, xfs_ino_t ino, blkmap_t *blkmap,
 			clearit = 1;
 			break;
 		}
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
-				    XFS_FSB_TO_BB(mp, 1), 0,
-				    &xfs_attr3_rmt_buf_ops);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
+				XFS_FSB_TO_BB(mp, 1), LIBXFS_READBUF_SALVAGE,
+				&bp, &xfs_attr3_rmt_buf_ops);
+		if (error) {
 			do_warn(
 	_("can't read remote block for attributes of inode %" PRIu64 "\n"), ino);
 			clearit = 1;
@@ -737,6 +738,7 @@ process_leaf_attr_level(xfs_mount_t	*mp,
 	xfs_dahash_t		current_hashval = 0;
 	xfs_dahash_t		greatest_hashval;
 	struct xfs_attr3_icleaf_hdr leafhdr;
+	int			error;
 
 	da_bno = da_cursor->level[0].bno;
 	ino = da_cursor->ino;
@@ -763,10 +765,11 @@ process_leaf_attr_level(xfs_mount_t	*mp,
 			goto error_out;
 		}
 
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, dev_bno),
-				    XFS_FSB_TO_BB(mp, 1), 0,
-				    &xfs_attr3_leaf_buf_ops);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev,
+				XFS_FSB_TO_DADDR(mp, dev_bno),
+				XFS_FSB_TO_BB(mp, 1), LIBXFS_READBUF_SALVAGE,
+				&bp, &xfs_attr3_leaf_buf_ops);
+		if (error) {
 			do_warn(
 	_("can't read file block %u (fsbno %" PRIu64 ") for attribute fork of inode %" PRIu64 "\n"),
 				da_bno, dev_bno, ino);
@@ -1073,6 +1076,7 @@ process_longform_attr(
 	xfs_fsblock_t		bno;
 	struct xfs_buf		*bp;
 	struct xfs_da_blkinfo	*info;
+	int			error;
 
 	*repair = 0;
 
@@ -1094,13 +1098,14 @@ process_longform_attr(
 		return 1;
 	}
 
-	bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
-				XFS_FSB_TO_BB(mp, 1), 0, &xfs_da3_node_buf_ops);
-	if (!bp) {
+	error = -libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
+			XFS_FSB_TO_BB(mp, 1), LIBXFS_READBUF_SALVAGE, &bp,
+			&xfs_da3_node_buf_ops);
+	if (error) {
 		do_warn(
 	_("can't read block 0 of inode %" PRIu64 " attribute fork\n"),
 			ino);
-		return(1);
+		return 1;
 	}
 	if (bp->b_error == -EFSBADCRC)
 		(*repair)++;
diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index b2ed1112..6685a4d2 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -31,6 +31,7 @@ check_aginode_block(xfs_mount_t	*mp,
 	int		i;
 	int		cnt = 0;
 	xfs_buf_t	*bp;
+	int		error;
 
 	/*
 	 * it's ok to read these possible inode blocks in one at
@@ -39,9 +40,10 @@ check_aginode_block(xfs_mount_t	*mp,
 	 * tree and we wouldn't be here and we stale the buffers out
 	 * so no one else will overlap them.
 	 */
-	bp = libxfs_buf_read(mp->m_dev, XFS_AGB_TO_DADDR(mp, agno, agbno),
-			XFS_FSB_TO_BB(mp, 1), 0, NULL);
-	if (!bp) {
+	error = -libxfs_buf_read(mp->m_dev, XFS_AGB_TO_DADDR(mp, agno, agbno),
+			XFS_FSB_TO_BB(mp, 1), LIBXFS_READBUF_SALVAGE, &bp,
+			NULL);
+	if (error) {
 		do_warn(_("cannot read agbno (%u/%u), disk block %" PRId64 "\n"),
 			agno, agbno, XFS_AGB_TO_DADDR(mp, agno, agbno));
 		return(0);
@@ -612,6 +614,7 @@ process_inode_chunk(
 	int			bp_index;
 	int			cluster_offset;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	int			error;
 
 	ASSERT(first_irec != NULL);
 	ASSERT(XFS_AGINO_TO_OFFSET(mp, first_irec->ino_startnum) == 0);
@@ -656,13 +659,13 @@ process_inode_chunk(
 		pftrace("about to read off %llu in AG %d",
 			XFS_AGB_TO_DADDR(mp, agno, agbno), agno);
 
-		bplist[bp_index] = libxfs_buf_read(mp->m_dev,
-					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp,
-						M_IGEO(mp)->blocks_per_cluster),
-					0,
-					&xfs_inode_buf_ops);
-		if (!bplist[bp_index]) {
+		error = -libxfs_buf_read(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, agbno),
+				XFS_FSB_TO_BB(mp,
+					M_IGEO(mp)->blocks_per_cluster),
+				LIBXFS_READBUF_SALVAGE, &bplist[bp_index],
+				&xfs_inode_buf_ops);
+		if (error) {
 			do_warn(_("cannot read inode %" PRIu64 ", disk block %" PRId64 ", cnt %d\n"),
 				XFS_AGINO_TO_INO(mp, agno, first_irec->ino_startnum),
 				XFS_AGB_TO_DADDR(mp, agno, agbno),
diff --git a/repair/dinode.c b/repair/dinode.c
index 08521ac8..e771558d 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -730,6 +730,7 @@ get_agino_buf(
 	xfs_daddr_t		cluster_daddr;
 	xfs_daddr_t		cluster_blks;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	int			error;
 
 	/*
 	 * Inode buffers have been read into memory in inode_cluster_size
@@ -748,9 +749,9 @@ get_agino_buf(
 		cluster_agino, cluster_daddr, cluster_blks);
 #endif
 
-	bp = libxfs_buf_read(mp->m_dev, cluster_daddr, cluster_blks,
-			0, &xfs_inode_buf_ops);
-	if (!bp) {
+	error = -libxfs_buf_read(mp->m_dev, cluster_daddr, cluster_blks, 0,
+			&bp, &xfs_inode_buf_ops);
+	if (error) {
 		do_warn(_("cannot read inode (%u/%u), disk block %" PRIu64 "\n"),
 			agno, cluster_agino, cluster_daddr);
 		return NULL;
@@ -1149,6 +1150,7 @@ process_quota_inode(
 	xfs_fileoff_t		qbno;
 	int			i;
 	int			t = 0;
+	int			error;
 
 	switch (ino_type) {
 		case XR_INO_UQUOTA:
@@ -1179,9 +1181,11 @@ process_quota_inode(
 		fsbno = blkmap_get(blkmap, qbno);
 		dqid = (xfs_dqid_t)qbno * dqperchunk;
 
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
-				    dqchunklen, 0, &xfs_dquot_buf_ops);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev,
+				XFS_FSB_TO_DADDR(mp, fsbno), dqchunklen,
+				LIBXFS_READBUF_SALVAGE, &bp,
+				&xfs_dquot_buf_ops);
+		if (error) {
 			do_warn(
 _("cannot read inode %" PRIu64 ", file block %" PRIu64 ", disk block %" PRIu64 "\n"),
 				lino, qbno, fsbno);
@@ -1255,6 +1259,7 @@ process_symlink_remote(
 	int			pathlen;
 	int			offset;
 	int			i;
+	int			error;
 
 	offset = 0;
 	pathlen = be64_to_cpu(dino->di_size);
@@ -1286,9 +1291,11 @@ _("cannot read inode %" PRIu64 ", file block %d, NULL disk block\n"),
 
 		byte_cnt = XFS_FSB_TO_B(mp, blk_cnt);
 
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
-				    BTOBB(byte_cnt), 0, &xfs_symlink_buf_ops);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev,
+				XFS_FSB_TO_DADDR(mp, fsbno), BTOBB(byte_cnt),
+				LIBXFS_READBUF_SALVAGE, &bp,
+				&xfs_symlink_buf_ops);
+		if (error) {
 			do_warn(
 _("cannot read inode %" PRIu64 ", file block %d, disk block %" PRIu64 "\n"),
 				lino, i, fsbno);
diff --git a/repair/phase3.c b/repair/phase3.c
index 79dc65f8..d30a698e 100644
--- a/repair/phase3.c
+++ b/repair/phase3.c
@@ -27,11 +27,13 @@ process_agi_unlinked(
 	struct xfs_agi		*agip;
 	xfs_agnumber_t		i;
 	int			agi_dirty = 0;
+	int			error;
 
-	bp = libxfs_buf_read(mp->m_dev,
+	error = -libxfs_buf_read(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp)),
-			mp->m_sb.sb_sectsize/BBSIZE, 0, &xfs_agi_buf_ops);
-	if (!bp)
+			mp->m_sb.sb_sectsize / BBSIZE, LIBXFS_READBUF_SALVAGE,
+			&bp, &xfs_agi_buf_ops);
+	if (error)
 		do_error(_("cannot read agi block %" PRId64 " for ag %u\n"),
 			XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp)), agno);
 
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 3cc85501..e9be2c71 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -272,10 +272,12 @@ pf_scan_lbtree(
 {
 	xfs_buf_t		*bp;
 	int			rc;
+	int			error;
 
-	bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, dbno),
-			XFS_FSB_TO_BB(mp, 1), 0, &xfs_bmbt_buf_ops);
-	if (!bp)
+	error = -libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, dbno),
+			XFS_FSB_TO_BB(mp, 1), LIBXFS_READBUF_SALVAGE, &bp,
+			&xfs_bmbt_buf_ops);
+	if (error)
 		return 0;
 
 	XFS_BUF_SET_PRIORITY(bp, isadir ? B_DIR_BMAP : B_BMAP);
diff --git a/repair/rt.c b/repair/rt.c
index b514998d..d901e751 100644
--- a/repair/rt.c
+++ b/repair/rt.c
@@ -193,9 +193,9 @@ process_rtbitmap(xfs_mount_t	*mp,
 			error = 1;
 			continue;
 		}
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
-				XFS_FSB_TO_BB(mp, 1), NULL);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
+				XFS_FSB_TO_BB(mp, 1), 0, NULL, &bp);
+		if (error) {
 			do_warn(_("can't read block %d for rtbitmap inode\n"),
 					bmbno);
 			error = 1;
@@ -255,9 +255,9 @@ process_rtsummary(xfs_mount_t	*mp,
 			error++;
 			continue;
 		}
-		bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
-				XFS_FSB_TO_BB(mp, 1), NULL);
-		if (!bp) {
+		error = -libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, bno),
+				XFS_FSB_TO_BB(mp, 1), 0, NULL, &bp);
+		if (error) {
 			do_warn(_("can't read block %d for rtsummary inode\n"),
 					sumbno);
 			error++;
diff --git a/repair/scan.c b/repair/scan.c
index 66b596db..99f65cff 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -46,6 +46,39 @@ set_mp(xfs_mount_t *mpp)
 	mp = mpp;
 }
 
+/*
+ * Read a buffer into memory, even if it fails verifier checks.
+ * If an IO error happens, return a zeroed buffer.
+ */
+static inline int
+salvage_buffer(
+	struct xfs_buftarg	*target,
+	xfs_daddr_t		blkno,
+	size_t			numblks,
+	struct xfs_buf		**bpp,
+	const struct xfs_buf_ops *ops)
+{
+	int			error;
+
+	error = -libxfs_buf_read(target, blkno, numblks,
+			LIBXFS_READBUF_SALVAGE, bpp, ops);
+	if (error != EIO)
+		return error;
+
+	/*
+	 * If the read produced an IO error, grab the buffer (which will now
+	 * be full of zeroes) and make it look like we read the data from the
+	 * disk but it failed verification.
+	 */
+	error = -libxfs_buf_get(target, blkno, numblks, bpp);
+	if (error)
+		return error;
+
+	(*bpp)->b_error = -EFSCORRUPTED;
+	(*bpp)->b_ops = ops;
+	return 0;
+}
+
 static void
 scan_sbtree(
 	xfs_agblock_t	root,
@@ -66,11 +99,12 @@ scan_sbtree(
 	void		*priv,
 	const struct xfs_buf_ops *ops)
 {
-	xfs_buf_t	*bp;
+	struct xfs_buf	*bp;
+	int		error;
 
-	bp = libxfs_buf_read(mp->m_dev, XFS_AGB_TO_DADDR(mp, agno, root),
-			XFS_FSB_TO_BB(mp, 1), 0, ops);
-	if (!bp) {
+	error = salvage_buffer(mp->m_dev, XFS_AGB_TO_DADDR(mp, agno, root),
+			XFS_FSB_TO_BB(mp, 1), &bp, ops);
+	if (error) {
 		do_error(_("can't read btree block %d/%d\n"), agno, root);
 		return;
 	}
@@ -123,9 +157,9 @@ scan_lbtree(
 	int		dirty = 0;
 	bool		badcrc = false;
 
-	bp = libxfs_buf_read(mp->m_dev, XFS_FSB_TO_DADDR(mp, root),
-		      XFS_FSB_TO_BB(mp, 1), 0, ops);
-	if (!bp)  {
+	err = salvage_buffer(mp->m_dev, XFS_FSB_TO_DADDR(mp, root),
+			XFS_FSB_TO_BB(mp, 1), &bp, ops);
+	if (err) {
 		do_error(_("can't read btree block %d/%d\n"),
 			XFS_FSB_TO_AGNO(mp, root),
 			XFS_FSB_TO_AGBNO(mp, root));
@@ -2102,6 +2136,7 @@ scan_freelist(
 	xfs_buf_t		*agflbuf;
 	xfs_agnumber_t		agno;
 	struct agfl_state	state;
+	int			error;
 
 	agno = be32_to_cpu(agf->agf_seqno);
 
@@ -2113,10 +2148,10 @@ scan_freelist(
 	if (be32_to_cpu(agf->agf_flcount) == 0)
 		return;
 
-	agflbuf = libxfs_buf_read(mp->m_dev,
-				 XFS_AG_DADDR(mp, agno, XFS_AGFL_DADDR(mp)),
-				 XFS_FSS_TO_BB(mp, 1), 0, &xfs_agfl_buf_ops);
-	if (!agflbuf)  {
+	error = salvage_buffer(mp->m_dev,
+			XFS_AG_DADDR(mp, agno, XFS_AGFL_DADDR(mp)),
+			XFS_FSS_TO_BB(mp, 1), &agflbuf, &xfs_agfl_buf_ops);
+	if (error) {
 		do_abort(_("can't read agfl block for ag %d\n"), agno);
 		return;
 	}
@@ -2330,6 +2365,7 @@ scan_ag(
 	int		sb_dirty = 0;
 	int		status;
 	char		*objname = NULL;
+	int		error;
 
 	sb = (struct xfs_sb *)calloc(BBTOB(XFS_FSS_TO_BB(mp, 1)), 1);
 	if (!sb) {
@@ -2337,27 +2373,27 @@ scan_ag(
 		return;
 	}
 
-	sbbuf = libxfs_buf_read(mp->m_dev, XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
-				XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);
-	if (!sbbuf)  {
+	error = salvage_buffer(mp->m_dev, XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
+			XFS_FSS_TO_BB(mp, 1), &sbbuf, &xfs_sb_buf_ops);
+	if (error) {
 		objname = _("root superblock");
 		goto out_free_sb;
 	}
 	libxfs_sb_from_disk(sb, XFS_BUF_TO_SBP(sbbuf));
 
-	agfbuf = libxfs_buf_read(mp->m_dev,
+	error = salvage_buffer(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
-			XFS_FSS_TO_BB(mp, 1), 0, &xfs_agf_buf_ops);
-	if (!agfbuf)  {
+			XFS_FSS_TO_BB(mp, 1), &agfbuf, &xfs_agf_buf_ops);
+	if (error) {
 		objname = _("agf block");
 		goto out_free_sbbuf;
 	}
 	agf = XFS_BUF_TO_AGF(agfbuf);
 
-	agibuf = libxfs_buf_read(mp->m_dev,
+	error = salvage_buffer(mp->m_dev,
 			XFS_AG_DADDR(mp, agno, XFS_AGI_DADDR(mp)),
-			XFS_FSS_TO_BB(mp, 1), 0, &xfs_agi_buf_ops);
-	if (!agibuf)  {
+			XFS_FSS_TO_BB(mp, 1), &agibuf, &xfs_agi_buf_ops);
+	if (error) {
 		objname = _("agi block");
 		goto out_free_agfbuf;
 	}


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 10/14] xfs: make xfs_trans_get_buf_map return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (8 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 09/14] xfs: make xfs_buf_read " Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 11/14] xfs: make xfs_trans_get_buf " Darrick J. Wong
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 9676b54e6e28689af1b4247569f14466bdfc5390

Convert xfs_trans_get_buf_map() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/xfs_trans.h   |   15 ++++++++++-----
 libxfs/trans.c        |   22 +++++++++++-----------
 libxfs/xfs_da_btree.c |    8 ++------
 3 files changed, 23 insertions(+), 22 deletions(-)


diff --git a/include/xfs_trans.h b/include/xfs_trans.h
index cff27546..33ab5d73 100644
--- a/include/xfs_trans.h
+++ b/include/xfs_trans.h
@@ -105,10 +105,9 @@ void	libxfs_trans_log_buf(struct xfs_trans *, struct xfs_buf *,
 				uint, uint);
 bool	libxfs_trans_ordered_buf(xfs_trans_t *, struct xfs_buf *);
 
-struct xfs_buf	*libxfs_trans_get_buf_map(struct xfs_trans *tp,
-					struct xfs_buftarg *btp,
-					struct xfs_buf_map *map, int nmaps,
-					xfs_buf_flags_t flags);
+int	libxfs_trans_get_buf_map(struct xfs_trans *tp, struct xfs_buftarg *btp,
+		struct xfs_buf_map *map, int nmaps, xfs_buf_flags_t flags,
+		struct xfs_buf **bpp);
 
 int	libxfs_trans_read_buf_map(struct xfs_mount *mp, struct xfs_trans *tp,
 				  struct xfs_buftarg *btp,
@@ -123,8 +122,14 @@ libxfs_trans_get_buf(
 	int			numblks,
 	uint			flags)
 {
+	struct xfs_buf		*bp;
+	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
-	return libxfs_trans_get_buf_map(tp, btp, &map, 1, flags);
+
+	error = libxfs_trans_get_buf_map(tp, btp, &map, 1, flags, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 static inline int
diff --git a/libxfs/trans.c b/libxfs/trans.c
index 73f69bce..ddd07cff 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -415,24 +415,22 @@ libxfs_trans_bhold_release(
  * If the transaction pointer is NULL, make this just a normal
  * get_buf() call.
  */
-struct xfs_buf *
+int
 libxfs_trans_get_buf_map(
 	struct xfs_trans	*tp,
 	struct xfs_buftarg	*target,
 	struct xfs_buf_map	*map,
 	int			nmaps,
-	xfs_buf_flags_t		flags)
+	xfs_buf_flags_t		flags,
+	struct xfs_buf		**bpp)
 {
 	struct xfs_buf		*bp;
 	struct xfs_buf_log_item	*bip;
 	int			error;
 
-	if (!tp) {
-		error = libxfs_buf_get_map(target, map, nmaps, 0, &bp);
-		if (error)
-			return NULL;
-		return bp;
-	}
+	*bpp = NULL;
+	if (!tp)
+		return libxfs_buf_get_map(target, map, nmaps, 0, bpp);
 
 	/*
 	 * If we find the buffer in the cache with this transaction
@@ -447,18 +445,20 @@ libxfs_trans_get_buf_map(
 		ASSERT(bip != NULL);
 		bip->bli_recur++;
 		trace_xfs_trans_get_buf_recur(bip);
-		return bp;
+		*bpp = bp;
+		return 0;
 	}
 
 	error = libxfs_buf_get_map(target, map, nmaps, 0, &bp);
 	if (error)
-		return NULL;
+		return error;
 
 	ASSERT(!bp->b_error);
 
 	_libxfs_trans_bjoin(tp, bp, 1);
 	trace_xfs_trans_get_buf(bp->b_log_item);
-	return bp;
+	*bpp = bp;
+	return 0;
 }
 
 xfs_buf_t *
diff --git a/libxfs/xfs_da_btree.c b/libxfs/xfs_da_btree.c
index 5102c6e0..3f40e99e 100644
--- a/libxfs/xfs_da_btree.c
+++ b/libxfs/xfs_da_btree.c
@@ -2588,13 +2588,9 @@ xfs_da_get_buf(
 	if (error || nmap == 0)
 		goto out_free;
 
-	bp = xfs_trans_get_buf_map(tp, mp->m_ddev_targp, mapp, nmap, 0);
-	error = bp ? bp->b_error : -EIO;
-	if (error) {
-		if (bp)
-			xfs_trans_brelse(tp, bp);
+	error = xfs_trans_get_buf_map(tp, mp->m_ddev_targp, mapp, nmap, 0, &bp);
+	if (error)
 		goto out_free;
-	}
 
 	*bpp = bp;
 


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 11/14] xfs: make xfs_trans_get_buf return an error code
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (9 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 10/14] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 12/14] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: ce92464c180b60e79022bdf1175b7737a11f59b7

Convert xfs_trans_get_buf() to return numeric error codes like most
everywhere else in xfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/xfs_trans.h |   12 ++++--------
 libxfs/xfs_btree.c  |   23 ++++++++++++++++-------
 libxfs/xfs_ialloc.c |   12 ++++++------
 libxfs/xfs_sb.c     |    9 +++++----
 mkfs/proto.c        |   10 ++++++++--
 5 files changed, 39 insertions(+), 27 deletions(-)


diff --git a/include/xfs_trans.h b/include/xfs_trans.h
index 33ab5d73..16e2a468 100644
--- a/include/xfs_trans.h
+++ b/include/xfs_trans.h
@@ -114,22 +114,18 @@ int	libxfs_trans_read_buf_map(struct xfs_mount *mp, struct xfs_trans *tp,
 				  struct xfs_buf_map *map, int nmaps,
 				  xfs_buf_flags_t flags, struct xfs_buf **bpp,
 				  const struct xfs_buf_ops *ops);
-static inline struct xfs_buf *
+static inline int
 libxfs_trans_get_buf(
 	struct xfs_trans	*tp,
 	struct xfs_buftarg	*btp,
 	xfs_daddr_t		blkno,
 	int			numblks,
-	uint			flags)
+	uint			flags,
+	struct xfs_buf		**bpp)
 {
-	struct xfs_buf		*bp;
-	int			error;
 	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
 
-	error = libxfs_trans_get_buf_map(tp, btp, &map, 1, flags, &bp);
-	if (error)
-		return NULL;
-	return bp;
+	return libxfs_trans_get_buf_map(tp, btp, &map, 1, flags, bpp);
 }
 
 static inline int
diff --git a/libxfs/xfs_btree.c b/libxfs/xfs_btree.c
index ead05a46..aeaa9623 100644
--- a/libxfs/xfs_btree.c
+++ b/libxfs/xfs_btree.c
@@ -685,11 +685,16 @@ xfs_btree_get_bufl(
 	xfs_trans_t	*tp,		/* transaction pointer */
 	xfs_fsblock_t	fsbno)		/* file system block number */
 {
+	struct xfs_buf		*bp;
 	xfs_daddr_t		d;		/* real disk block address */
+	int			error;
 
 	ASSERT(fsbno != NULLFSBLOCK);
 	d = XFS_FSB_TO_DADDR(mp, fsbno);
-	return xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0);
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -703,12 +708,17 @@ xfs_btree_get_bufs(
 	xfs_agnumber_t	agno,		/* allocation group number */
 	xfs_agblock_t	agbno)		/* allocation group block number */
 {
+	struct xfs_buf		*bp;
 	xfs_daddr_t		d;		/* real disk block address */
+	int			error;
 
 	ASSERT(agno != NULLAGNUMBER);
 	ASSERT(agbno != NULLAGBLOCK);
 	d = XFS_AGB_TO_DADDR(mp, agno, agbno);
-	return xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0);
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
+	if (error)
+		return NULL;
+	return bp;
 }
 
 /*
@@ -1267,11 +1277,10 @@ xfs_btree_get_buf_block(
 	error = xfs_btree_ptr_to_daddr(cur, ptr, &d);
 	if (error)
 		return error;
-	*bpp = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d,
-				 mp->m_bsize, 0);
-
-	if (!*bpp)
-		return -ENOMEM;
+	error = xfs_trans_get_buf(cur->bc_tp, mp->m_ddev_targp, d, mp->m_bsize,
+			0, bpp);
+	if (error)
+		return error;
 
 	(*bpp)->b_ops = cur->bc_ops->buf_ops;
 	*block = XFS_BUF_TO_BLOCK(*bpp);
diff --git a/libxfs/xfs_ialloc.c b/libxfs/xfs_ialloc.c
index baa99551..00b33263 100644
--- a/libxfs/xfs_ialloc.c
+++ b/libxfs/xfs_ialloc.c
@@ -271,6 +271,7 @@ xfs_ialloc_inode_init(
 	int			i, j;
 	xfs_daddr_t		d;
 	xfs_ino_t		ino = 0;
+	int			error;
 
 	/*
 	 * Loop over the new block(s), filling in the inodes.  For small block
@@ -322,12 +323,11 @@ xfs_ialloc_inode_init(
 		 */
 		d = XFS_AGB_TO_DADDR(mp, agno, agbno +
 				(j * M_IGEO(mp)->blocks_per_cluster));
-		fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
-					 mp->m_bsize *
-					 M_IGEO(mp)->blocks_per_cluster,
-					 XBF_UNMAPPED);
-		if (!fbuf)
-			return -ENOMEM;
+		error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
+				mp->m_bsize * M_IGEO(mp)->blocks_per_cluster,
+				XBF_UNMAPPED, &fbuf);
+		if (error)
+			return error;
 
 		/* Initialize the inode buffers and log them appropriately. */
 		fbuf->b_ops = &xfs_inode_buf_ops;
diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index d3d5e11d..687e33d8 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -1162,13 +1162,14 @@ xfs_sb_get_secondary(
 	struct xfs_buf		**bpp)
 {
 	struct xfs_buf		*bp;
+	int			error;
 
 	ASSERT(agno != 0 && agno != NULLAGNUMBER);
-	bp = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-			XFS_FSS_TO_BB(mp, 1), 0);
-	if (!bp)
-		return -ENOMEM;
+			XFS_FSS_TO_BB(mp, 1), 0, &bp);
+	if (error)
+		return error;
 	bp->b_ops = &xfs_sb_buf_ops;
 	xfs_buf_oneshot(bp);
 	*bpp = bp;
diff --git a/mkfs/proto.c b/mkfs/proto.c
index 26a613fe..ab01c8b0 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -254,8 +254,14 @@ newfile(
 			exit(1);
 		}
 		d = XFS_FSB_TO_DADDR(mp, map.br_startblock);
-		bp = libxfs_trans_get_buf(logit ? tp : NULL, mp->m_dev, d,
-			nb << mp->m_blkbb_log, 0);
+		error = -libxfs_trans_get_buf(logit ? tp : NULL, mp->m_dev, d,
+				nb << mp->m_blkbb_log, 0, &bp);
+		if (error) {
+			fprintf(stderr,
+				_("%s: cannot allocate buffer for file\n"),
+				progname);
+			exit(1);
+		}
 		memmove(bp->b_addr, buf, len);
 		if (len < bp->b_bcount)
 			memset((char *)bp->b_addr + len, 0, bp->b_bcount - len);


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 12/14] xfs: remove the xfs_btree_get_buf[ls] functions
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (10 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 11/14] xfs: make xfs_trans_get_buf " Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 13/14] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 14/14] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: ee647f85cb81b09bbfa2886954828ed03fa3ec38

Remove the xfs_btree_get_bufs and xfs_btree_get_bufl functions, since
they're pretty trivial oneliners.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/xfs_alloc.c |   16 +++++++++-------
 libxfs/xfs_bmap.c  |   14 +++++++++-----
 libxfs/xfs_btree.c |   46 ----------------------------------------------
 libxfs/xfs_btree.h |   21 ---------------------
 4 files changed, 18 insertions(+), 79 deletions(-)


diff --git a/libxfs/xfs_alloc.c b/libxfs/xfs_alloc.c
index 1bf813e3..be1bf736 100644
--- a/libxfs/xfs_alloc.c
+++ b/libxfs/xfs_alloc.c
@@ -1066,11 +1066,11 @@ xfs_alloc_ag_vextent_small(
 	if (args->datatype & XFS_ALLOC_USERDATA) {
 		struct xfs_buf	*bp;
 
-		bp = xfs_btree_get_bufs(args->mp, args->tp, args->agno, fbno);
-		if (XFS_IS_CORRUPT(args->mp, !bp)) {
-			error = -EFSCORRUPTED;
+		error = xfs_trans_get_buf(args->tp, args->mp->m_ddev_targp,
+				XFS_AGB_TO_DADDR(args->mp, args->agno, fbno),
+				args->mp->m_bsize, 0, &bp);
+		if (error)
 			goto error;
-		}
 		xfs_trans_binval(args->tp, bp);
 	}
 	*fbnop = args->agbno = fbno;
@@ -2343,9 +2343,11 @@ xfs_free_agfl_block(
 	if (error)
 		return error;
 
-	bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno);
-	if (XFS_IS_CORRUPT(tp->t_mountp, !bp))
-		return -EFSCORRUPTED;
+	error = xfs_trans_get_buf(tp, tp->t_mountp->m_ddev_targp,
+			XFS_AGB_TO_DADDR(tp->t_mountp, agno, agbno),
+			tp->t_mountp->m_bsize, 0, &bp);
+	if (error)
+		return error;
 	xfs_trans_binval(tp, bp);
 
 	return 0;
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index 5efa2f0c..f917a6eb 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -723,11 +723,11 @@ xfs_bmap_extents_to_btree(
 	cur->bc_private.b.allocated++;
 	ip->i_d.di_nblocks++;
 	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
-	abp = xfs_btree_get_bufl(mp, tp, args.fsbno);
-	if (XFS_IS_CORRUPT(mp, !abp)) {
-		error = -EFSCORRUPTED;
+	error = xfs_trans_get_buf(tp, mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(mp, args.fsbno),
+			mp->m_bsize, 0, &abp);
+	if (error)
 		goto out_unreserve_dquot;
-	}
 
 	/*
 	 * Fill in the child block.
@@ -871,7 +871,11 @@ xfs_bmap_local_to_extents(
 	ASSERT(args.fsbno != NULLFSBLOCK);
 	ASSERT(args.len == 1);
 	tp->t_firstblock = args.fsbno;
-	bp = xfs_btree_get_bufl(args.mp, tp, args.fsbno);
+	error = xfs_trans_get_buf(tp, args.mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(args.mp, args.fsbno),
+			args.mp->m_bsize, 0, &bp);
+	if (error)
+		goto done;
 
 	/*
 	 * Initialize the block, copy the data and log the remote buffer.
diff --git a/libxfs/xfs_btree.c b/libxfs/xfs_btree.c
index aeaa9623..57862dfa 100644
--- a/libxfs/xfs_btree.c
+++ b/libxfs/xfs_btree.c
@@ -675,52 +675,6 @@ xfs_btree_get_block(
 	return XFS_BUF_TO_BLOCK(*bpp);
 }
 
-/*
- * Get a buffer for the block, return it with no data read.
- * Long-form addressing.
- */
-xfs_buf_t *				/* buffer for fsbno */
-xfs_btree_get_bufl(
-	xfs_mount_t	*mp,		/* file system mount point */
-	xfs_trans_t	*tp,		/* transaction pointer */
-	xfs_fsblock_t	fsbno)		/* file system block number */
-{
-	struct xfs_buf		*bp;
-	xfs_daddr_t		d;		/* real disk block address */
-	int			error;
-
-	ASSERT(fsbno != NULLFSBLOCK);
-	d = XFS_FSB_TO_DADDR(mp, fsbno);
-	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
-	if (error)
-		return NULL;
-	return bp;
-}
-
-/*
- * Get a buffer for the block, return it with no data read.
- * Short-form addressing.
- */
-xfs_buf_t *				/* buffer for agno/agbno */
-xfs_btree_get_bufs(
-	xfs_mount_t	*mp,		/* file system mount point */
-	xfs_trans_t	*tp,		/* transaction pointer */
-	xfs_agnumber_t	agno,		/* allocation group number */
-	xfs_agblock_t	agbno)		/* allocation group block number */
-{
-	struct xfs_buf		*bp;
-	xfs_daddr_t		d;		/* real disk block address */
-	int			error;
-
-	ASSERT(agno != NULLAGNUMBER);
-	ASSERT(agbno != NULLAGBLOCK);
-	d = XFS_AGB_TO_DADDR(mp, agno, agbno);
-	error = xfs_trans_get_buf(tp, mp->m_ddev_targp, d, mp->m_bsize, 0, &bp);
-	if (error)
-		return NULL;
-	return bp;
-}
-
 /*
  * Change the cursor to point to the first record at the given level.
  * Other levels are unaffected.
diff --git a/libxfs/xfs_btree.h b/libxfs/xfs_btree.h
index 2e727f23..8bead747 100644
--- a/libxfs/xfs_btree.h
+++ b/libxfs/xfs_btree.h
@@ -296,27 +296,6 @@ xfs_btree_dup_cursor(
 	xfs_btree_cur_t		*cur,	/* input cursor */
 	xfs_btree_cur_t		**ncur);/* output cursor */
 
-/*
- * Get a buffer for the block, return it with no data read.
- * Long-form addressing.
- */
-struct xfs_buf *				/* buffer for fsbno */
-xfs_btree_get_bufl(
-	struct xfs_mount	*mp,	/* file system mount point */
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_fsblock_t		fsbno);	/* file system block number */
-
-/*
- * Get a buffer for the block, return it with no data read.
- * Short-form addressing.
- */
-struct xfs_buf *				/* buffer for agno/agbno */
-xfs_btree_get_bufs(
-	struct xfs_mount	*mp,	/* file system mount point */
-	struct xfs_trans	*tp,	/* transaction pointer */
-	xfs_agnumber_t		agno,	/* allocation group number */
-	xfs_agblock_t		agbno);	/* allocation group block number */
-
 /*
  * Compute first and last byte offsets for the fields given.
  * Interprets the offsets table, which contains struct field offsets.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 13/14] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (11 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 12/14] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  2020-02-25  0:15 ` [PATCH 14/14] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: f48e2df8a877ca1c19d92cfd7e74cc5956fa84cb

Refactor xfs_read_agf and xfs_alloc_read_agf to return EAGAIN if the
caller passed TRYLOCK and we weren't able to get the lock; and change
the callers to recognize this.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/xfs_alloc.c |   36 ++++++++++++++----------------------
 libxfs/xfs_bmap.c  |   11 ++++++-----
 2 files changed, 20 insertions(+), 27 deletions(-)


diff --git a/libxfs/xfs_alloc.c b/libxfs/xfs_alloc.c
index be1bf736..a92ca524 100644
--- a/libxfs/xfs_alloc.c
+++ b/libxfs/xfs_alloc.c
@@ -2498,12 +2498,11 @@ xfs_alloc_fix_freelist(
 
 	if (!pag->pagf_init) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
-		if (!pag->pagf_init) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
-			goto out_agbp_relse;
 		}
 	}
 
@@ -2529,11 +2528,10 @@ xfs_alloc_fix_freelist(
 	 */
 	if (!agbp) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
-		if (error)
-			goto out_no_agbp;
-		if (!agbp) {
-			ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
-			ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
+		if (error) {
+			/* Couldn't lock the AGF so skip this AG. */
+			if (error == -EAGAIN)
+				error = 0;
 			goto out_no_agbp;
 		}
 	}
@@ -2764,11 +2762,10 @@ xfs_alloc_pagf_init(
 	xfs_buf_t		*bp;
 	int			error;
 
-	if ((error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp)))
-		return error;
-	if (bp)
+	error = xfs_alloc_read_agf(mp, tp, agno, flags, &bp);
+	if (!error)
 		xfs_trans_brelse(tp, bp);
-	return 0;
+	return error;
 }
 
 /*
@@ -2957,12 +2954,6 @@ xfs_read_agf(
 	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
 			XFS_AG_DADDR(mp, agno, XFS_AGF_DADDR(mp)),
 			XFS_FSS_TO_BB(mp, 1), flags, bpp, &xfs_agf_buf_ops);
-	/*
-	 * Callers of xfs_read_agf() currently interpret a NULL bpp as EAGAIN
-	 * and need to be converted to check for EAGAIN specifically.
-	 */
-	if (error == -EAGAIN)
-		return 0;
 	if (error)
 		return error;
 
@@ -2988,14 +2979,15 @@ xfs_alloc_read_agf(
 
 	trace_xfs_alloc_read_agf(mp, agno);
 
+	/* We don't support trylock when freeing. */
+	ASSERT((flags & (XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK)) !=
+			(XFS_ALLOC_FLAG_FREEING | XFS_ALLOC_FLAG_TRYLOCK));
 	ASSERT(agno != NULLAGNUMBER);
 	error = xfs_read_agf(mp, tp, agno,
 			(flags & XFS_ALLOC_FLAG_TRYLOCK) ? XBF_TRYLOCK : 0,
 			bpp);
 	if (error)
 		return error;
-	if (!*bpp)
-		return 0;
 	ASSERT(!(*bpp)->b_error);
 
 	agf = XFS_BUF_TO_AGF(*bpp);
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index f917a6eb..d43155d0 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -3304,11 +3304,12 @@ xfs_bmap_longest_free_extent(
 	pag = xfs_perag_get(mp, ag);
 	if (!pag->pagf_init) {
 		error = xfs_alloc_pagf_init(mp, tp, ag, XFS_ALLOC_FLAG_TRYLOCK);
-		if (error)
-			goto out;
-
-		if (!pag->pagf_init) {
-			*notinit = 1;
+		if (error) {
+			/* Couldn't lock the AGF, so skip this AG. */
+			if (error == -EAGAIN) {
+				*notinit = 1;
+				error = 0;
+			}
 			goto out;
 		}
 	}


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 14/14] xfs: remove unnecessary null pointer checks from _read_agf callers
  2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
                   ` (12 preceding siblings ...)
  2020-02-25  0:15 ` [PATCH 13/14] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
@ 2020-02-25  0:15 ` Darrick J. Wong
  13 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25  0:15 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs, Christoph Hellwig, Dave Chinner

From: Darrick J. Wong <darrick.wong@oracle.com>

Source kernel commit: 706b8c5bc70391be510a5454f307db90b622b279

Drop the null buffer pointer checks in all code that calls
xfs_alloc_read_agf and doesn't pass XFS_ALLOC_FLAG_TRYLOCK because
they're no longer necessary.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/xfs_refcount.c |    6 ------
 1 file changed, 6 deletions(-)


diff --git a/libxfs/xfs_refcount.c b/libxfs/xfs_refcount.c
index 71d29486..b8a45b15 100644
--- a/libxfs/xfs_refcount.c
+++ b/libxfs/xfs_refcount.c
@@ -1176,8 +1176,6 @@ xfs_refcount_finish_one(
 				XFS_ALLOC_FLAG_FREEING, &agbp);
 		if (error)
 			return error;
-		if (XFS_IS_CORRUPT(tp->t_mountp, !agbp))
-			return -EFSCORRUPTED;
 
 		rcur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno);
 		if (!rcur) {
@@ -1717,10 +1715,6 @@ xfs_refcount_recover_cow_leftovers(
 	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
 	if (error)
 		goto out_trans;
-	if (!agbp) {
-		error = -ENOMEM;
-		goto out_trans;
-	}
 	cur = xfs_refcountbt_init_cursor(mp, tp, agbp, agno);
 
 	/* Find all the leftover CoW staging extents. */


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-25  0:14 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
@ 2020-02-25 17:55   ` Christoph Hellwig
  2020-02-25 18:59     ` Darrick J. Wong
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-02-25 17:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: sandeen, linux-xfs

On Mon, Feb 24, 2020 at 04:14:40PM -0800, Darrick J. Wong wrote:
> 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      |   71 +++++++++++++++++++++++++++++++++++++++++-----------
>  libxfs/trans.c     |   24 ++++--------------
>  repair/da_util.c   |    3 +-
>  5 files changed, 82 insertions(+), 45 deletions(-)
> 
> 
> 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;

I think instead of ignorining the error and checkig b_error further down
that should be moved to work based on the return value.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-25 17:55   ` Christoph Hellwig
@ 2020-02-25 18:59     ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-25 18:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

On Tue, Feb 25, 2020 at 09:55:24AM -0800, Christoph Hellwig wrote:
> On Mon, Feb 24, 2020 at 04:14:40PM -0800, Darrick J. Wong wrote:
> > 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      |   71 +++++++++++++++++++++++++++++++++++++++++-----------
> >  libxfs/trans.c     |   24 ++++--------------
> >  repair/da_util.c   |    3 +-
> >  5 files changed, 82 insertions(+), 45 deletions(-)
> > 
> > 
> > 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;
> 
> I think instead of ignorining the error and checkig b_error further down
> that should be moved to work based on the return value.

Yeah, I'll add a cleanup patch for that on the end, after we convert
libxfs_buf_read.  Thanks for reviewing!

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-21 16:15     ` Darrick J. Wong
@ 2020-02-21 16:28       ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-02-21 16:28 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, sandeen, linux-xfs

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..

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-21 15:03   ` Christoph Hellwig
@ 2020-02-21 16:15     ` Darrick J. Wong
  2020-02-21 16:28       ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-21 16:15 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-20  1:45 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
@ 2020-02-21 15:03   ` Christoph Hellwig
  2020-02-21 16:15     ` Darrick J. Wong
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-02-21 15:03 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: sandeen, linux-xfs

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code
  2020-02-20  1:44 [PATCH 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
@ 2020-02-20  1:45 ` Darrick J. Wong
  2020-02-21 15:03   ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2020-02-20  1:45 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs

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(-)


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;


^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-02-25 18:59 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25  0:14 [PATCH v2 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
2020-02-25  0:14 ` [PATCH 01/14] libxfs: make __cache_lookup return an error code Darrick J. Wong
2020-02-25  0:14 ` [PATCH 02/14] libxfs: make libxfs_getbuf_flags " Darrick J. Wong
2020-02-25  0:14 ` [PATCH 03/14] libxfs: make libxfs_buf_get_map " Darrick J. Wong
2020-02-25  0:14 ` [PATCH 04/14] libxfs: refactor libxfs_readbuf out of existence Darrick J. Wong
2020-02-25  0:14 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
2020-02-25 17:55   ` Christoph Hellwig
2020-02-25 18:59     ` Darrick J. Wong
2020-02-25  0:14 ` [PATCH 06/14] xfs: make xfs_buf_read_map " Darrick J. Wong
2020-02-25  0:14 ` [PATCH 07/14] xfs: make xfs_buf_get " Darrick J. Wong
2020-02-25  0:15 ` [PATCH 08/14] xfs: make xfs_buf_get_uncached " Darrick J. Wong
2020-02-25  0:15 ` [PATCH 09/14] xfs: make xfs_buf_read " Darrick J. Wong
2020-02-25  0:15 ` [PATCH 10/14] xfs: make xfs_trans_get_buf_map " Darrick J. Wong
2020-02-25  0:15 ` [PATCH 11/14] xfs: make xfs_trans_get_buf " Darrick J. Wong
2020-02-25  0:15 ` [PATCH 12/14] xfs: remove the xfs_btree_get_buf[ls] functions Darrick J. Wong
2020-02-25  0:15 ` [PATCH 13/14] xfs: make xfs_*read_agf return EAGAIN to ALLOC_FLAG_TRYLOCK callers Darrick J. Wong
2020-02-25  0:15 ` [PATCH 14/14] xfs: remove unnecessary null pointer checks from _read_agf callers Darrick J. Wong
  -- strict thread matches above, loose matches on Subject: below --
2020-02-20  1:44 [PATCH 00/14] xfsprogs: make buffer functions return error codes Darrick J. Wong
2020-02-20  1:45 ` [PATCH 05/14] libxfs: make libxfs_buf_read_map return an error code Darrick J. Wong
2020-02-21 15:03   ` Christoph Hellwig
2020-02-21 16:15     ` Darrick J. Wong
2020-02-21 16:28       ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.