Skip to contentSkip to conversation

ReiserFS file.c several bug-fix

PATCH Superseded fs 3 participants 3 messages Jun 23, 2005

lore
Share & tools
lore

fs

author
#0 lore raw

PATCH 1/1 ReiserFS file.c several bug-fix

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.
Write a program, do: open(O_SYNC/O_DSYNC) - 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:
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 run generic_osync_inode, 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

fs/reiserfs/file.c +5 -7
@@ -225,12 +225,6 @@ static int reiserfs_allocate_blocks_for_
     hint.preallocate = will_prealloc;
 
     /* Call block allocator to allocate blocks */
-    res = reiserfs_allocate_blocknrs(&hint, allocated_blocks, blocks_to_allocate, blocks_to_allocate);
-    if ( res != CARRY_ON ) {
-	if ( res == NO_DISK_SPACE ) {
-	    /* We flush the transaction in case of no space. This way some
-	       blocks might become free */
-	    SB_JOURNAL(inode->i_sb)->j_must_wait = 1;
 	    res = restart_transaction(th, inode, &path);
             if (res)
                 goto error_exit;

@@ -1306,7 +1300,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 +1357,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);
#1 lore raw

Hello

Quoted 6 lines · fs

Related FS:
ReiserFS

Related Files:
fs/reiserfs/file.c

Quoted 7 lines

Bug description:
Make a ReiserFS partition in USB storage HDD, create a test file
with enough size.
Write a program, do: open(O_SYNC/O_DSYNC) - 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.

Open returns not buffer buf opened file descriptor or -1. Please describe your test more carefully.

Quoted 2 lines

Bug analysis:
reiserfs_file_write will claim some blocks, commit the I/O request,

So, you said: open(O_SYNC/O_DSYNC) - read - close. Where does write come from?

Quoted 35 lines

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 run generic_osync_inode, 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

#2 lore raw

----- Original Message -----
From: "Vladimir Saveliev" <[email protected]>
To: "fs" <[email protected]>
Cc: "reiserfs-list" <[email protected]>; "Hans Reiser" <[email protected]>; "iscas-linaccident" <[email protected]>
Sent: Thursday, June 23, 2005 5:16 PM
Subject: [iscas-linaccident 55] Re: [PATCH] ReiserFS file.c several bug-fix

Quoted 14 lines

Hello

Quoted 10 lines · fs

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.
Write a program, do: open(O_SYNC/O_DSYNC) - read - close. After each

Quoted 8 lines
Quoted 4 lines

operation, pause for a while, such as 3s. Between open and read, unlug

the USB wire. open returns zero-filled buffer, no error returns.

Open returns not buffer buf opened file descriptor or -1. Please describe your test more carefully.

Sorry to use the wrong template. :(
Bug description:
Make a ReiserFS partition in USB storage HDD, create a test file,
for example, 64K.
Write a program, do: open(O_SYNC or O_DSYNC) - write(no need to use lseek)
- close. After each operation, pause for a while, such as 3s. Between open
and write, unplug the USB wire. write returns no error.

Quoted 43 lines
Quoted 2 lines

Bug analysis:
reiserfs_file_write will claim some blocks, commit the I/O request,

So, you said: open(O_SYNC/O_DSYNC) - read - close. Where does write come from?

Quoted 35 lines

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 run generic_osync_inode, 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