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<[EMAIL PROTECTED]>
>
>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);
>
>
>
