Re: xfs_repair segfaut in stage 6

Alex Elder
2011-09-20T20:25:48Z

On Wed, 2011-09-14 at 11:38 -0400, Christoph Hellwig wrote:
> On Wed, Sep 14, 2011 at 04:59:40PM +0200, Bartosz Cisek wrote:
> > Stack trace is pasted in bug issue [1] that is linked in first mail ;)
> > Compiled by hand from git: "DEBUG=-DDEBUG make". I don't know why some
> > of values are 'optimized out'.
> >
> > [1] http://oss.sgi.com/bugzilla/show_bug.cgi?id=914
>
> Looks like we do not handle read I/O errors very well (to say at all)
> in phase6. Can you see if the patch below makes a difference?

Christoph, I'm assuming you want this reviewed
as a submitted patch.

> ---
> From: Christoph Hellwig <hch@lst.de>
> Subject: repair: fix I/O error handling
>
> Currently libxfs_trans_read_buf never returns an error, even if
> libxfs_readbuf did not manage to complete the I/O. This is different
> from the kernel behaviour and can lead to segfaults in code that
> doesn't expect it. Add a new b_error member to xfs_buf (mirroring
> the kernel version) and use that to propagate proper error codes
> to the caller. Also fix libxfs_readbufr to handle short reads
> properly, and to not override errno values e.g. by a fprintf.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: xfsprogs-dev/include/libxfs.h
> ===================================================================
> --- xfsprogs-dev.orig/include/libxfs.h 2011-09-14 11:17:42.660738577 -0400
> +++ xfsprogs-dev/include/libxfs.h 2011-09-14 11:20:45.959738580 -0400
> @@ -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-14 11:12:08.807741720 -0400
> +++ xfsprogs-dev/libxfs/rdwr.c 2011-09-14 11:20:21.183238272 -0400
> @@ -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) {
> @@ -454,15 +455,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) {
> + if (pread64(fd, bp->b_addr, bytes, LIBXFS_BBTOOFF64(blkno)) != bytes) {

If we reach EOF this returns 0, but errno is I think
going to be 0. Do we want to print a "read failed"
message in that case? Is EOF a failure, or just
a somewhat normal condition?

Also, it may not matter in the calling code (I
did only a quick check) but maybe it would be
better to set bp->b_error here, where the error
really occurred, rather than in libxfs_readbuf().

Other than that, this change looks good to me.

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