ReiserFS _get_block_create_0 wrong behavior when I/O fails

PATCH In review fs 3 participants 6 messages Jun 15, 2005 – Jun 16, 2005 VFS

lore

Share & tools

lore

fs

author

2005-06-15 09:08 UTC #0 lore raw

PATCH 1/1 ReiserFS _get_block_create_0 wrong behavior when I/O fails

Related FS:
ReiserFS

Related Files:
fs/reiserfs/inode.c

Bug description:
Make a ReiserFS partition in USB storage HDD, create a test file with
enough size.
Write a program, do: open(O_RDONLY) - read - close. After each
operation, pause for a while, such as 3s. Between open and read, unlug the USB wire. open returns zero-filled buffer, no error returns.

Bug analysis:
do_mpage_readpage will call FS-specific get_block to get buffer mapped
from disk. reiserfs_get_block doesn't return non-zero when I/O failure occurs.

    reiserfs_get_block -> _get_block_create_0 -> search_by_position_by_key

search_by_position_by_key returns IO_ERROR, but the original code just simply returns 0

research:

    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
	pathrelse (&path);
        if (p)
            kunmap(bh_result->b_page) ;
	// We do not return -ENOENT if there is a hole but page is uptodate, because it means
	// That there is some MMAPED data associated with it that is yet to be written to disk.
	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
	    return -ENOENT ;
	}
        return 0 ; <- 0 retuns for IO_ERROR
    }

Way around:
test result of search_for_position_by_key

Signed-off-by Qu Fuping<[email protected]>

Patch:

tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +5 -2

diff -uNp /tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c
@@ -254,6 +254,7 @@ static int _get_block_create_0 (struct i
     char * p = NULL;
     int chars;
     int ret ;
+    int result ;
     int done = 0 ;
     unsigned long offset ;

@@ -262,7 +263,8 @@ static int _get_block_create_0 (struct i
 		  (loff_t)block * inode->i_sb->s_blocksize + 1, TYPE_ANY, 3);
 
 research:
-    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
+    result = search_for_position_by_key (inode->i_sb, &key, &path) ;
+    if (result != POSITION_FOUND) {
 	pathrelse (&path);
         if (p)
             kunmap(bh_result->b_page) ;

@@ -270,7 +272,8 @@ research:
 	// That there is some MMAPED data associated with it that is yet to be written to disk.
 	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
 	    return -ENOENT ;
-	}
+	}else if (result == IO_ERROR)
+		return -EIO ;
         return 0 ;
     }

Vladimir Saveliev

2005-06-15 15:41 UTC #1 lore raw

Hello

Quoted 67 lines · fs

From: fs <[email protected]>
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: [iscas-linaccident 50] [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
Date: Wed, 15 Jun 2005 15:10:05 -0400
Related FS:
ReiserFS
Related Files:
fs/reiserfs/inode.c
Bug description:
Make a ReiserFS partition in USB storage HDD, create a test file with
enough size.
Write a program, do: open(O_RDONLY) - read - close. After each
operation, pause for a while, such as 3s. Between open and read, unlug the USB wire. open returns zero-filled buffer, no error returns.
Bug analysis:
do_mpage_readpage will call FS-specific get_block to get buffer mapped
from disk. reiserfs_get_block doesn't return non-zero when I/O failure occurs.
    reiserfs_get_block -> _get_block_create_0 -> search_by_position_by_key
search_by_position_by_key returns IO_ERROR, but the original code just simply returns 0
research:
    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
	pathrelse (&path);
        if (p)
            kunmap(bh_result->b_page) ;
	// We do not return -ENOENT if there is a hole but page is uptodate, because it means
	// That there is some MMAPED data associated with it that is yet to be written to disk.
	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
	    return -ENOENT ;
	}
        return 0 ; <- 0 retuns for IO_ERROR
    }
Way around:
test result of search_for_position_by_key
Signed-off-by Qu Fuping<[email protected]>
Patch:
diff -uNp /tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +3 -1
@@ -254,6 +254,7 @@ static int _get_block_create_0 (struct i
     char * p = NULL;
     int chars;
     int ret ;
+    int result ;
     int done = 0 ;
     unsigned long offset ;
 
@@ -262,7 +263,8 @@ static int _get_block_create_0 (struct i
 		  (loff_t)block * inode->i_sb->s_blocksize + 1, TYPE_ANY, 3);
 
 research:
-    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
+    result = search_for_position_by_key (inode->i_sb, &key, &path) ;
+    if (result != POSITION_FOUND) {
 	pathrelse (&path);
         if (p)
             kunmap(bh_result->b_page) ;

tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c research: · line 279

Quoted 9 lines

tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +2 -1

@@ -270,7 +272,8 @@ research:
 	// That there is some MMAPED data associated with it that is yet to be written to disk.
 	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
 	    return -ENOENT ;
-	}
+	}else if (result == IO_ERROR)
+		return -EIO ;
         return 0 ;
     }

Your patch is incomplete. There is one more search_for_position_by_key at the end of this function. You probably want to check its return value also.

Quoted 2 lines

fs

author

2005-06-16 06:05 UTC #2 lore raw

Dear Vladimir Saveliev,

Quoted 28 lines · Vladimir Saveliev

Quoted 28 lines

Patch:
diff -uNp /tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c

tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +4 -2

@@ -254,6 +254,7 @@ static int _get_block_create_0 (struct i
     char * p = NULL;
     int chars;
     int ret ;
+    int result ;
     int done = 0 ;
     unsigned long offset ;

@@ -262,7 +263,8 @@ static int _get_block_create_0 (struct i
 		  (loff_t)block * inode->i_sb->s_blocksize + 1, TYPE_ANY, 3);
 
 research:
-    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
+    result = search_for_position_by_key (inode->i_sb, &key, &path) ;
+    if (result != POSITION_FOUND) {
 	pathrelse (&path);
         if (p)
             kunmap(bh_result->b_page) ;

@@ -270,7 +272,8 @@ research:
 	// That there is some MMAPED data associated with it that is yet to be written to disk.
 	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
 	    return -ENOENT ;
-	}
+	}else if (result == IO_ERROR)

Quoted 8 lines

tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c research: · line 279
Quoted 4 lines
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +1 -0
+		return -EIO ;
         return 0 ;
     }
 
Your patch is incomplete. There is one more search_for_position_by_key at the end of this function. You probably want to check its return value also.

I notice there's a comment

	if (search_for_position_by_key (inode->i_sb, &key, &path) !=
POSITION_FOUND)
	    // we read something from tail, even if now we got IO_ERROR <-
Here, can you explain more ?
	    break;

Vladimir Saveliev

2005-06-16 10:41 UTC #3 lore raw

Hello

Quoted 54 lines · fs

Dear Vladimir Saveliev,
Quoted 28 lines · Vladimir Saveliev
Quoted 28 lines
Patch:
diff -uNp /tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +4 -2
@@ -254,6 +254,7 @@ static int _get_block_create_0 (struct i
     char * p = NULL;
     int chars;
     int ret ;
+    int result ;
     int done = 0 ;
     unsigned long offset ;
 
@@ -262,7 +263,8 @@ static int _get_block_create_0 (struct i
 		  (loff_t)block * inode->i_sb->s_blocksize + 1, TYPE_ANY, 3);
 
 research:
-    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
+    result = search_for_position_by_key (inode->i_sb, &key, &path) ;
+    if (result != POSITION_FOUND) {
 	pathrelse (&path);
         if (p)
             kunmap(bh_result->b_page) ;
@@ -270,7 +272,8 @@ research:
 	// That there is some MMAPED data associated with it that is yet to be written to disk.
 	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
 	    return -ENOENT ;
-	}
+	}else if (result == IO_ERROR)
Quoted 8 lines
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c research: · line 279
Quoted 4 lines
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +1 -0
+		return -EIO ;
         return 0 ;
     }
 
Your patch is incomplete. There is one more search_for_position_by_key at the end of this function. You probably want to check its return value also.
I notice there's a comment
	if (search_for_position_by_key (inode->i_sb, &key, &path) !=
POSITION_FOUND)
	    // we read something from tail, even if now we got IO_ERROR <-
Here, can you explain more ?
	    break;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Vladimir Saveliev

2005-06-16 10:46 UTC #4 lore raw

Hello

Quoted 47 lines · Vladimir Saveliev

Hello
Quoted 44 lines · fs
Dear Vladimir Saveliev,
Quoted 28 lines · Vladimir Saveliev
Quoted 28 lines
Patch:
diff -uNp /tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +4 -2
@@ -254,6 +254,7 @@ static int _get_block_create_0 (struct i
     char * p = NULL;
     int chars;
     int ret ;
+    int result ;
     int done = 0 ;
     unsigned long offset ;
 
@@ -262,7 +263,8 @@ static int _get_block_create_0 (struct i
 		  (loff_t)block * inode->i_sb->s_blocksize + 1, TYPE_ANY, 3);
 
 research:
-    if (search_for_position_by_key (inode->i_sb, &key, &path) != POSITION_FOUND) {
+    result = search_for_position_by_key (inode->i_sb, &key, &path) ;
+    if (result != POSITION_FOUND) {
 	pathrelse (&path);
         if (p)
             kunmap(bh_result->b_page) ;
@@ -270,7 +272,8 @@ research:
 	// That there is some MMAPED data associated with it that is yet to be written to disk.
 	if ((args & GET_BLOCK_NO_HOLE) && !PageUptodate(bh_result->b_page) ) {
 	    return -ENOENT ;
-	}
+	}else if (result == IO_ERROR)
Quoted 8 lines
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c research: · line 279
Quoted 4 lines
tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c renamed from tmp/linux-2.6.12-rc6/fs/reiserfs/inode.c +1 -0
+		return -EIO ;
         return 0 ;
     }
 
Your patch is incomplete. There is one more search_for_position_by_key at the end of this function. You probably want to check its return value also.
I notice there's a comment
	if (search_for_position_by_key (inode->i_sb, &key, &path) !=
POSITION_FOUND)
	    // we read something from tail, even if now we got IO_ERROR <-
Here, can you explain more ?

Yes. reiserfs may store file page in different blocks. In this do {} while loop it looks for different parts of page in the tree. If search_for_position_by_key fails - one part of page is not found and -EIO is to be returned.

Quoted 10 lines

Quoted 10 lines
	    break;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hans Reiser

2005-06-16 18:46 UTC #5 lore raw

In reply to fs

thanks fs