* [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
@ 2005-06-15 20:09 fs
2005-06-15 15:41 ` Vladimir Saveliev
0 siblings, 1 reply; 6+ messages in thread
From: fs @ 2005-06-15 20:09 UTC (permalink / raw)
To: reiserfs-list; +Cc: linux-fsdevel, linux-kernel, reiser
[-- Attachment #1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #2: Forwarded message - [iscas-linaccident 50] [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails --]
[-- Type: message/rfc822, Size: 4038 bytes --]
From: fs <fs@ercist.iscas.ac.cn>
To: reiserfs-list@namesys.com
Cc: reiser@namesys.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, iscas-linaccident@intellilink.co.jp
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
Message-ID: <20050615191005.GA3188@CoolQ.6f.iscas.ac.cn>
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<fs@ercist.iscas.ac.cn>
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/fs/reiserfs/inode.c 2005-06-06 11:22:29.000000000 -0400
+++ /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c 2005-06-15 13:56:45.552564512 -0400
@@ -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 ;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
2005-06-15 20:09 [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails fs
@ 2005-06-15 15:41 ` Vladimir Saveliev
2005-06-16 17:04 ` fs
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Saveliev @ 2005-06-15 15:41 UTC (permalink / raw)
To: fs; +Cc: reiserfs-list, linux-fsdevel, linux-kernel, Hans Reiser
Hello
On Thu, 2005-06-16 at 00:09, fs wrote:
> From: fs <fs@ercist.iscas.ac.cn>
> To: reiserfs-list@namesys.com
> Cc: reiser@namesys.com, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, iscas-linaccident@intellilink.co.jp
> 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<fs@ercist.iscas.ac.cn>
>
> 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/fs/reiserfs/inode.c 2005-06-06 11:22:29.000000000 -0400
> +++ /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c 2005-06-15 13:56:45.552564512 -0400
> @@ -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 ;
> }
>
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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
2005-06-15 15:41 ` Vladimir Saveliev
@ 2005-06-16 17:04 ` fs
2005-06-16 10:41 ` Vladimir Saveliev
2005-06-16 18:46 ` Hans Reiser
0 siblings, 2 replies; 6+ messages in thread
From: fs @ 2005-06-16 17:04 UTC (permalink / raw)
To: Vladimir Saveliev; +Cc: reiserfs-list, linux-fsdevel, linux-kernel, Hans Reiser
Dear Vladimir Saveliev,
On Wed, 2005-06-15 at 11:41, Vladimir Saveliev wrote:
> > 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/fs/reiserfs/inode.c 2005-06-06 11:22:29.000000000 -0400
> > +++ /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c 2005-06-15 13:56:45.552564512 -0400
> > @@ -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 ;
> > }
> >
>
> 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
2005-06-16 17:04 ` fs
@ 2005-06-16 10:41 ` Vladimir Saveliev
2005-06-16 10:46 ` Vladimir Saveliev
2005-06-16 18:46 ` Hans Reiser
1 sibling, 1 reply; 6+ messages in thread
From: Vladimir Saveliev @ 2005-06-16 10:41 UTC (permalink / raw)
To: fs; +Cc: reiserfs-list, linux-fsdevel, linux-kernel, Hans Reiser
Hello
On Thu, 2005-06-16 at 21:04, fs wrote:
> Dear Vladimir Saveliev,
> On Wed, 2005-06-15 at 11:41, Vladimir Saveliev wrote:
>
> > > 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/fs/reiserfs/inode.c 2005-06-06 11:22:29.000000000 -0400
> > > +++ /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c 2005-06-15 13:56:45.552564512 -0400
> > > @@ -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 ;
> > > }
> > >
> >
> > 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
2005-06-16 10:41 ` Vladimir Saveliev
@ 2005-06-16 10:46 ` Vladimir Saveliev
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Saveliev @ 2005-06-16 10:46 UTC (permalink / raw)
To: fs; +Cc: reiserfs-list, linux-fsdevel, linux-kernel, Hans Reiser
Hello
On Thu, 2005-06-16 at 14:41, Vladimir Saveliev wrote:
> Hello
>
> On Thu, 2005-06-16 at 21:04, fs wrote:
> > Dear Vladimir Saveliev,
> > On Wed, 2005-06-15 at 11:41, Vladimir Saveliev wrote:
> >
> > > > 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/fs/reiserfs/inode.c 2005-06-06 11:22:29.000000000 -0400
> > > > +++ /tmp/linux-2.6.12-rc6.new/fs/reiserfs/inode.c 2005-06-15 13:56:45.552564512 -0400
> > > > @@ -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 ;
> > > > }
> > > >
> > >
> > > 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.
> > break;
> >
> >
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails
2005-06-16 17:04 ` fs
2005-06-16 10:41 ` Vladimir Saveliev
@ 2005-06-16 18:46 ` Hans Reiser
1 sibling, 0 replies; 6+ messages in thread
From: Hans Reiser @ 2005-06-16 18:46 UTC (permalink / raw)
To: fs; +Cc: Vladimir Saveliev, reiserfs-list, linux-fsdevel, linux-kernel
thanks fs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-06-16 18:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-06-15 20:09 [PATCH] ReiserFS _get_block_create_0 wrong behavior when I/O fails fs
2005-06-15 15:41 ` Vladimir Saveliev
2005-06-16 17:04 ` fs
2005-06-16 10:41 ` Vladimir Saveliev
2005-06-16 10:46 ` Vladimir Saveliev
2005-06-16 18:46 ` Hans Reiser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).