Skip to contentSkip to conversation

- set b_error from bio error in xfs_buf_bio_end_io

RFC PATCH In review Eric Sandeen 2 participants 4 messages Dec 5, 2008 XFS filesystem

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

RFC PATCH 1/1 - set b_error from bio error in xfs_buf_bio_end_io

Tim mentioned something about the code in xfs_buf_iodone_work() which detects barrier failures post-mount, as added in commit 0bfefc46dc028df60120acdb92062169c9328769,
[XFS] Barriers need to be dynamically checked and switched off

        if ((bp->b_error == EOPNOTSUPP) &&
            (bp->b_flags & (XBF_ORDERED|XBF_ASYNC)) == (XBF_ORDERED|XBF_ASYNC)) {
                XB_TRACE(bp, "ordered_retry", bp->b_iodone);
                bp->b_flags &= ~XBF_ORDERED;
                bp->b_flags |= _XFS_BARRIER_FAILED;

...

but it seems that nothing ever sets EOPNOTSUPP on b_error, so this path would never be hit.

I think that we need to do something like below, totally untested, to ensure that bio errors get set on b_error, if we're looking for them by name, no?

(I'm not sure if we still need the BIO_UPTODATE test, or if we can just look at the error we're given and be done?)

Does this seem about right?

Thanks,
-Eric

fs/xfs/linux-2.6/xfs_buf.c +4 -2
Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
@@ -1114,8 +1114,10 @@ xfs_buf_bio_end_io(
 	unsigned int		blocksize = bp->b_target->bt_bsize;
 	struct bio_vec		*bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
 
-	if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
-		bp->b_error = EIO;
+	if (error)
+		bp->b_error = XFS_ERROR(-error);
+	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
+		bp->b_error = XFS_ERROR(EIO);
 
 	do {
 		struct page	*page = bvec->bv_page;

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

author
#1 lore raw
Quoted 10 lines · Eric Sandeen

Tim mentioned something about the code in xfs_buf_iodone_work() which detects barrier failures post-mount, as added in commit 0bfefc46dc028df60120acdb92062169c9328769,
[XFS] Barriers need to be dynamically checked and switched off

        if ((bp->b_error == EOPNOTSUPP) &&
            (bp->b_flags & (XBF_ORDERED|XBF_ASYNC)) == (XBF_ORDERED|XBF_ASYNC)) {
                XB_TRACE(bp, "ordered_retry", bp->b_iodone);
                bp->b_flags &= ~XBF_ORDERED;
                bp->b_flags |= _XFS_BARRIER_FAILED;
Quoted 8 lines

...

but it seems that nothing ever sets EOPNOTSUPP on b_error, so this path would never be hit.

I think that we need to do something like below, totally untested, to ensure that bio errors get set on b_error, if we're looking for them by name, no?

oh, heh, Tim had mentioned that Lachlan suggested this same thing on irc yesterday, but somehow I had forgotten or totally missed that... didn't mean to usurp Lachlan :)

-Eric

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

#2 lore raw
In reply to Eric Sandeen
Quoted 32 lines · Eric Sandeen

Tim mentioned something about the code in xfs_buf_iodone_work() which detects barrier failures post-mount, as added in commit 0bfefc46dc028df60120acdb92062169c9328769,
[XFS] Barriers need to be dynamically checked and switched off

        if ((bp->b_error == EOPNOTSUPP) &&
            (bp->b_flags & (XBF_ORDERED|XBF_ASYNC)) == (XBF_ORDERED|XBF_ASYNC)) {
                XB_TRACE(bp, "ordered_retry", bp->b_iodone);
                bp->b_flags &= ~XBF_ORDERED;
                bp->b_flags |= _XFS_BARRIER_FAILED;

...

but it seems that nothing ever sets EOPNOTSUPP on b_error, so this path would never be hit.

I think that we need to do something like below, totally untested, to ensure that bio errors get set on b_error, if we're looking for them by name, no?

(I'm not sure if we still need the BIO_UPTODATE test, or if we can just look at the error we're given and be done?)

Does this seem about right?

Thanks,
-Eric

Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c xfs_buf_bio_end_io( · line 1120

Quoted 9 lines
fs/xfs/linux-2.6/xfs_buf.c +4 -2
@@ -1114,8 +1114,10 @@ xfs_buf_bio_end_io(
 	unsigned int		blocksize = bp->b_target->bt_bsize;
 	struct bio_vec		*bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
 
-	if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
-		bp->b_error = EIO;
+	if (error)
+		bp->b_error = XFS_ERROR(-error);
+	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
+		bp->b_error = XFS_ERROR(EIO);

I would suggest this:

@@ -1114,8 +1140,7 @@ xfs_buf_bio_end_io(
unsigned int blocksize = bp->b_target->bt_bsize;
struct bio_vec *bvec = bio->bi_io_vec + bio->bi_vcnt - 1;

- if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
- bp->b_error = EIO;
+ xfs_buf_ioerror(bp, -error);

  	do {
  		struct page	*page = bvec->bv_page;

The BIO_UPTODATE checks have already been done before calling this function and error has been set appropriately so we can just use it.

Quoted 10 lines
fs/xfs/linux-2.6/xfs_buf.c +0 -0
 	do {
 		struct page	*page = bvec->bv_page;

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

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

author
#3 lore raw

fs/xfs/linux-2.6/xfs_buf.c xfs_buf_bio_end_io( · line 1143

Quoted 9 lines · Lachlan McIlroy

I would suggest this:

fs/xfs/linux-2.6/xfs_buf.c +1 -2
@@ -1114,8 +1140,7 @@ xfs_buf_bio_end_io(
  	unsigned int		blocksize = bp->b_target->bt_bsize;
  	struct bio_vec		*bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
 
-	if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
-		bp->b_error = EIO;
+	xfs_buf_ioerror(bp, -error);

oh, sure. I forgot we had the wrapper to set the error ... :)

Quoted 2 lines

The BIO_UPTODATE checks have already been done before calling this function and error has been set appropriately so we can just use it.

ah so it is. yeah, makes perfect sense.

-Eric

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