[PATCH 0/4] improve libxfs / xfs_repair I/O error handling

[PATCH 0/4] improve libxfs / xfs_repair I/O error handling

[Christoph Hellwig]

This series fixes various issues with handling of I/O errors in libxfs.
The most important one is patch 1, which fixes a regression introduced
in xfsprogs 3.0.0 that makes xfs_repair segfault for many read I/O errors.

I would recommend distributors to backport this patch to their releases.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/4] libxfs: handle read errors in libxfs_trans_read_buf

[Christoph Hellwig]

[-- Attachment #1: libxfs-do-not-ignore-read-errors --]
[-- Type: text/plain, Size: 1992 bytes --]

Libxfs_readbuf may return a NULL buffer to indicate that an error happend
during the read, but we currently ignore that if libxfs_trans_read_buf
is called with a NULL transaction pointer.  Fix this by copying the
relevant code from the kernel version of the routine, and also tidy
the code up a bit by using a common exit label.

This fixes a regression that was introduced in xfsprogs 3.0.0 by commit

	"Implement buffer and inode caching in libxfs, groundwork for a
	 parallel version of xfs_repair."

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

Index: xfsprogs-dev/libxfs/trans.c
===================================================================
--- xfsprogs-dev.orig/libxfs/trans.c	2011-09-20 20:33:48.000000000 +0000
+++ xfsprogs-dev/libxfs/trans.c	2011-09-20 20:35:24.000000000 +0000
@@ -478,9 +478,15 @@ libxfs_trans_read_buf(
 	xfs_buf_log_item_t	*bip;
 	xfs_buftarg_t		bdev;
 
+	*bpp = NULL;
+
 	if (tp == NULL) {
-		*bpp = libxfs_readbuf(dev, blkno, len, flags);
-		return 0;
+		bp = libxfs_readbuf(dev, blkno, len, flags);
+		if (!bp) {
+			return (flags & XBF_TRYLOCK) ?
+				EAGAIN : XFS_ERROR(ENOMEM);
+		}
+		goto done;
 	}
 
 	bdev.dev = dev;
@@ -490,15 +496,15 @@ libxfs_trans_read_buf(
 		ASSERT(XFS_BUF_FSPRIVATE(bp, void *) != NULL);
 		bip = XFS_BUF_FSPRIVATE(bp, xfs_buf_log_item_t*);
 		bip->bli_recur++;
-		*bpp = bp;
-		return 0;
+		goto done;
 	}
 
 	bp = libxfs_readbuf(dev, blkno, len, flags);
-       if (!bp){
-               *bpp = NULL;
-               return errno;
-       }
+	if (!bp) {
+		return (flags & XBF_TRYLOCK) ?
+			EAGAIN : XFS_ERROR(ENOMEM);
+	}
+
 #ifdef XACT_DEBUG
 	fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
 #endif
@@ -510,6 +516,7 @@ libxfs_trans_read_buf(
 
 	/* initialise b_fsprivate2 so we can find it incore */
 	XFS_BUF_SET_FSPRIVATE2(bp, tp);
+done:
 	*bpp = bp;
 	return 0;
 }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] libxfs: save errno before possibly overwriting it

[Christoph Hellwig]

[-- Attachment #1: libxfs-fix-errno --]
[-- Type: text/plain, Size: 1584 bytes --]

Save away errno for a later error return before possibly overwriting
it in fprintf.

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

Index: xfsprogs-dev/libxfs/rdwr.c
===================================================================
--- xfsprogs-dev.orig/libxfs/rdwr.c	2011-09-20 20:41:09.000000000 +0000
+++ xfsprogs-dev/libxfs/rdwr.c	2011-09-20 20:43:28.000000000 +0000
@@ -454,15 +454,17 @@ libxfs_readbufr(dev_t dev, xfs_daddr_t b
 {
 	int	fd = libxfs_device_to_fd(dev);
 	int	bytes = BBTOB(len);
+	int	error;
 
 	ASSERT(BBTOB(len) <= bp->b_bcount);
 
 	if (pread64(fd, bp->b_addr, bytes, LIBXFS_BBTOOFF64(blkno)) < 0) {
+		error = errno;
 		fprintf(stderr, _("%s: read failed: %s\n"),
-			progname, strerror(errno));
+			progname, strerror(error));
 		if (flags & LIBXFS_EXIT_ON_FAILURE)
 			exit(1);
-		return errno;
+		return error;
 	}
 #ifdef IO_DEBUG
 	printf("%lx: %s: read %u bytes, blkno=%llu(%llu), %p\n",
@@ -498,14 +500,16 @@ libxfs_writebufr(xfs_buf_t *bp)
 {
 	int	sts;
 	int	fd = libxfs_device_to_fd(bp->b_dev);
+	int	error;
 
 	sts = pwrite64(fd, bp->b_addr, bp->b_bcount, LIBXFS_BBTOOFF64(bp->b_blkno));
 	if (sts < 0) {
+		error = errno;
 		fprintf(stderr, _("%s: pwrite64 failed: %s\n"),
-			progname, strerror(errno));
+			progname, strerror(error));
 		if (bp->b_flags & LIBXFS_B_EXIT)
 			exit(1);
-		return errno;
+		return error;
 	}
 	else if (sts != bp->b_bcount) {
 		fprintf(stderr, _("%s: error - wrote only %d of %d bytes\n"),

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] libxfs: handle short reads in libxfs_readbufr

[Christoph Hellwig]

[-- Attachment #1: libxfs-handle-short-reads --]
[-- Type: text/plain, Size: 1645 bytes --]

Copy the code from libxfs_writebufr to handle short reads, and also
tidy up a formatting issue found in the libxfs_writebufr copy.

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

Index: xfsprogs-dev/libxfs/rdwr.c
===================================================================
--- xfsprogs-dev.orig/libxfs/rdwr.c	2011-09-20 20:43:28.000000000 +0000
+++ xfsprogs-dev/libxfs/rdwr.c	2011-09-20 20:43:49.000000000 +0000
@@ -455,16 +455,24 @@ libxfs_readbufr(dev_t dev, xfs_daddr_t b
 	int	fd = libxfs_device_to_fd(dev);
 	int	bytes = BBTOB(len);
 	int	error;
+	int	sts;
 
 	ASSERT(BBTOB(len) <= bp->b_bcount);
 
-	if (pread64(fd, bp->b_addr, bytes, LIBXFS_BBTOOFF64(blkno)) < 0) {
+	sts = pread64(fd, bp->b_addr, bytes, LIBXFS_BBTOOFF64(blkno));
+	if (sts < 0) {
 		error = errno;
 		fprintf(stderr, _("%s: read failed: %s\n"),
 			progname, strerror(error));
 		if (flags & LIBXFS_EXIT_ON_FAILURE)
 			exit(1);
 		return error;
+	} else if (sts != bytes) {
+		fprintf(stderr, _("%s: error - read only %d of %d bytes\n"),
+			progname, sts, bytes);
+		if (flags & LIBXFS_EXIT_ON_FAILURE)
+			exit(1);
+		return EIO;
 	}
 #ifdef IO_DEBUG
 	printf("%lx: %s: read %u bytes, blkno=%llu(%llu), %p\n",
@@ -510,8 +518,7 @@ libxfs_writebufr(xfs_buf_t *bp)
 		if (bp->b_flags & LIBXFS_B_EXIT)
 			exit(1);
 		return error;
-	}
-	else if (sts != bp->b_bcount) {
+	} else if (sts != bp->b_bcount) {
 		fprintf(stderr, _("%s: error - wrote only %d of %d bytes\n"),
 			progname, sts, bp->b_bcount);
 		if (bp->b_flags & LIBXFS_B_EXIT)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/4] libxfs: add b_error

[Christoph Hellwig]

[-- Attachment #1: libxfs-add-b_error --]
[-- Type: text/plain, Size: 2657 bytes --]

Add a b_error field to struct xfs_buf so that we can return the exact error
fro libxfs_readbuf.  And explicit error return would be nice, but this
requires large changes to common code that should be done on the kernel
side first.

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

Index: xfsprogs-dev/include/libxfs.h
===================================================================
--- xfsprogs-dev.orig/include/libxfs.h	2011-09-20 20:44:23.000000000 +0000
+++ xfsprogs-dev/include/libxfs.h	2011-09-20 20:44:32.000000000 +0000
@@ -230,6 +230,7 @@ typedef struct xfs_buf {
 	void			*b_fsprivate2;
 	void			*b_fsprivate3;
 	char			*b_addr;
+	int			b_error;
 #ifdef XFS_BUF_TRACING
 	struct list_head	b_lock_list;
 	const char		*b_func;
Index: xfsprogs-dev/libxfs/rdwr.c
===================================================================
--- xfsprogs-dev.orig/libxfs/rdwr.c	2011-09-20 20:44:37.000000000 +0000
+++ xfsprogs-dev/libxfs/rdwr.c	2011-09-20 20:45:22.000000000 +0000
@@ -314,6 +314,7 @@ libxfs_initbuf(xfs_buf_t *bp, dev_t devi
 	bp->b_blkno = bno;
 	bp->b_bcount = bytes;
 	bp->b_dev = device;
+	bp->b_error = 0;
 	if (!bp->b_addr)
 		bp->b_addr = memalign(libxfs_device_alignment(), bytes);
 	if (!bp->b_addr) {
@@ -495,10 +496,8 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
 	bp = libxfs_getbuf(dev, blkno, len);
 	if (bp && !(bp->b_flags & (LIBXFS_B_UPTODATE|LIBXFS_B_DIRTY))) {
 		error = libxfs_readbufr(dev, blkno, bp, len, flags);
-		if (error) {
-			libxfs_putbuf(bp);
-			return NULL;
-		}
+		if (error)
+			bp->b_error = error;
 	}
 	return bp;
 }
Index: xfsprogs-dev/libxfs/trans.c
===================================================================
--- xfsprogs-dev.orig/libxfs/trans.c	2011-09-20 20:45:29.000000000 +0000
+++ xfsprogs-dev/libxfs/trans.c	2011-09-20 20:46:34.000000000 +0000
@@ -477,6 +477,7 @@ libxfs_trans_read_buf(
 	xfs_buf_t		*bp;
 	xfs_buf_log_item_t	*bip;
 	xfs_buftarg_t		bdev;
+	int			error;
 
 	*bpp = NULL;
 
@@ -486,6 +487,8 @@ libxfs_trans_read_buf(
 			return (flags & XBF_TRYLOCK) ?
 				EAGAIN : XFS_ERROR(ENOMEM);
 		}
+		if (bp->b_error)
+			goto out_relse;
 		goto done;
 	}
 
@@ -504,6 +507,8 @@ libxfs_trans_read_buf(
 		return (flags & XBF_TRYLOCK) ?
 			EAGAIN : XFS_ERROR(ENOMEM);
 	}
+	if (bp->b_error)
+		goto out_relse;
 
 #ifdef XACT_DEBUG
 	fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
@@ -519,6 +524,10 @@ libxfs_trans_read_buf(
 done:
 	*bpp = bp;
 	return 0;
+out_relse:
+	error = bp->b_error;
+	xfs_buf_relse(bp);
+	return error;
 }
 
 /*

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] libxfs: handle read errors in libxfs_trans_read_buf

[Alex Elder]

On Tue, 2011-09-20 at 17:59 -0400, Christoph Hellwig wrote:
> Libxfs_readbuf may return a NULL buffer to indicate that an error happend
> during the read, but we currently ignore that if libxfs_trans_read_buf
> is called with a NULL transaction pointer.  Fix this by copying the
> relevant code from the kernel version of the routine, and also tidy
> the code up a bit by using a common exit label.
> 
> This fixes a regression that was introduced in xfsprogs 3.0.0 by commit
> 
> 	"Implement buffer and inode caching in libxfs, groundwork for a
> 	 parallel version of xfs_repair."
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Probably wouldn't hurt to initialize b_fsprivate2 even
if the transaction pointer is null.  Looks good though.

Reviewed-by: Alex Elder <aelder@sgi.com>

. . .
> @@ -510,6 +516,7 @@ libxfs_trans_read_buf(
>  
>  	/* initialise b_fsprivate2 so we can find it incore */
>  	XFS_BUF_SET_FSPRIVATE2(bp, tp);
> +done:
>  	*bpp = bp;
>  	return 0;
>  }


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] libxfs: save errno before possibly overwriting it

[Alex Elder]

On Tue, 2011-09-20 at 17:59 -0400, Christoph Hellwig wrote:
> Save away errno for a later error return before possibly overwriting
> it in fprintf.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] libxfs: handle short reads in libxfs_readbufr

[Alex Elder]

On Tue, 2011-09-20 at 17:59 -0400, Christoph Hellwig wrote:
> Copy the code from libxfs_writebufr to handle short reads, and also
> tidy up a formatting issue found in the libxfs_writebufr copy.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

That LIBXFS_B_EXIT == LIBXFS_EXIT_ON_FAILURE is a bad thing.
It might be nice if libxfs_readbuf() and libxfs_writebuf()
encoded that piece of information the same way (in b_flags).

Anyway, what you have here looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] libxfs: add b_error

[Alex Elder]

On Tue, 2011-09-20 at 17:59 -0400, Christoph Hellwig wrote:
> Add a b_error field to struct xfs_buf so that we can return the exact error
> fro libxfs_readbuf.  And explicit error return would be nice, but this
> requires large changes to common code that should be done on the kernel
> side first.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] libxfs: handle read errors in libxfs_trans_read_buf

[Christoph Hellwig]

On Wed, Sep 21, 2011 at 01:45:07PM -0500, Alex Elder wrote:
> Probably wouldn't hurt to initialize b_fsprivate2 even
> if the transaction pointer is null.  Looks good though.

It would - given that another transaction could be set in there.
Fairly unlikely in userspace, but quite possible in the kernel.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] libxfs: handle read errors in libxfs_trans_read_buf

[Alex Elder]

On Wed, 2011-09-21 at 15:49 -0400, Christoph Hellwig wrote:
> On Wed, Sep 21, 2011 at 01:45:07PM -0500, Alex Elder wrote:
> > Probably wouldn't hurt to initialize b_fsprivate2 even
> > if the transaction pointer is null.  Looks good though.
> 
> It would - given that another transaction could be set in there.
> Fairly unlikely in userspace, but quite possible in the kernel.
> 

You're right.	-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs