From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans Reiser Subject: Re: [PATCH][RESEND] ReiserFS file.c several bug-fix Date: Thu, 14 Jul 2005 01:34:21 -0700 Message-ID: <42D6238D.7040406@namesys.com> References: <1121363345.2818.7.camel@CoolQ> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: list-help: list-unsubscribe: list-post: Errors-To: flx@namesys.com In-Reply-To: <1121363345.2818.7.camel@CoolQ> List-Id: Content-Type: text/plain; charset="us-ascii" To: Vladimir Saveliev Cc: fs , reiserfs-list vs, if you don't reply, I am going to approve the patch without your comments.....;-) Please reply. Hans fs wrote: >Dear Hans Reiser, > This is the third time I send this patch. > Hope the bug can be handled ASAP 'cause it's really serious. > >Related FS: > ReiserFS > >Related Files: > fs/reiserfs/file.c > >Bug description: > Make a ReiserFS partition in USB storage HDD, create a test file >with enough size(dd if=/dev/zero of=testfile bs=4096 count=1024). > Write a program, do: > int fd; > char buf[4096]; > > fd = open("testfile", O_RDWRO | O_CREAT | O_SYNC, > (S_IRWXU | S_IRWXG | S_IRWXO) ); > or > open("testfile", O_RDWRO | O_CREAT | O_DSYNC, > (S_IRWXU | S_IRWXG | S_IRWXO) ); > > write(fd, buf, sizeof(buf)); > close(fd); > >After each operation, pause for a while, such as 3s. Between open and >write, unlug the USB wire. write returns no error instead of -EIO . > >Bug analysis: > reiserfs_file_write will claim some blocks, commit the I/O request, >if O_SYNC and O_DSYNC is used, it will > if ((file->f_flags & O_SYNC) || IS_SYNC(inode)) > res = generic_osync_inode(inode, file->f_mapping, > OSYNC_METADATA|OSYNC_DATA); >The question is, if I/O error occurs, > res = reiserfs_allocate_blocks_for_region fails with -EIO, so >it will exit the loop, no I/O request, no page marked as dirty. >If generic_osync_inode runs, it returns 0(no dirty page), res will be >overwritten from -EIO to 0, thus no error report. > >Also, reiserfs_file_write contains a serious bug, see here > blocks_to_allocate = reiserfs_prepare_file_region_for_write > (inode, pos, num_pages, write_bytes, prepared_pages); >Here blocks_to_allocate is defined as size_t, i.e. unsigned int, but >reiserfs_prepare_file_region_for_write is declared as int, so sometimes >it will return -EIO, -ENOENT, etc, take a look at this line > if ( blocks_to_allocate < 0 ) { <- This will never happen > res = blocks_to_allocate; > reiserfs_release_claimed_blocks(inode->i_sb, > num_pages << (PAGE_CACHE_SHIFT - inode->i_blkbits)); > break; > } > >Way around: >1) if already_written is zero, don't do generic_osync_inode >2) tell the result of reiserfs_prepare_file_region_for_write with IS_ERR > macro or cast it to size_t > >Signed-off-by: Qu Fuping > >Patch: >diff -uNp linux-2.6.12/fs/reiserfs/file.c >linux-2.6.12-new/fs/reiserfs/file.c > > > >------------------------------------------------------------------------ > >--- linux-2.6.12/fs/reiserfs/file.c 2005-06-23 14:59:27.000000000 -0400 >+++ linux-2.6.12-new/fs/reiserfs/file.c 2005-06-23 15:34:49.000000000 -0400 >@@ -1306,7 +1306,7 @@ static ssize_t reiserfs_file_write( stru > so that nobody else can access these until we are done. > We get number of actual blocks needed as a result.*/ > blocks_to_allocate = reiserfs_prepare_file_region_for_write(inode, pos, num_pages, write_bytes, prepared_pages); >- if ( blocks_to_allocate < 0 ) { >+ if ( IS_ERROR((const void *)blocks_to_allocate) ) { > res = blocks_to_allocate; > reiserfs_release_claimed_blocks(inode->i_sb, num_pages << (PAGE_CACHE_SHIFT - inode->i_blkbits)); > break; >@@ -1363,6 +1363,10 @@ static ssize_t reiserfs_file_write( stru > } > } > >+ /* If nothing is written, no need(actually, mustn't) to sync pages, just return res */ >+ if( already_written == 0 ) >+ goto out; >+ > if ((file->f_flags & O_SYNC) || IS_SYNC(inode)) > res = generic_osync_inode(inode, file->f_mapping, OSYNC_METADATA|OSYNC_DATA); > > >