Re: [BUG] Reiserfs: reiserfs_panic while running fs stress tests

[Suzuki <suzuki@in.ibm.com>]

Vladimir V. Saveliev wrote:
> Hello
>
> On Fri, 2006-05-12 at 12:15 +0530, Suzuki wrote:
>
> > Randy.Dunlap wrote:
> >
> > > On Fri, 05 May 2006 10:22:21 +0530 Suzuki wrote:
> > >
> > >
> > >
> > > > Hi,
> > > >
> > > >
> > > > I was working on a reiserfs panic with 2.6.17-rc3, while running fs
> > > > stress tests.
> > >
> > >
> > > Hi,
> > > What test(s) do you use?
> >
> > The problem was initially hit while running the following tests 
> > simultaneously..
> > IOZone, bonnie++, dbench, fs_inod, fs_maim, fsstress, fsx_linux, 
> > postmark, tiobench.
> >
> > As I had mentioned in my post, I have a simple testcase to trigger the 
> > panic which can hit the code path explained below.
> >
> > The root cause of the problem is (as mentioned in the earlier post):
> >
> >  Whenever there is an extending DIO write operation, the fs would
> > create a safe link so as to ensure the file size consistent, if there is
> > crash in between the DIO. This will be deleted once the write operation
> > finishes.
>
>
> I am not sure why safe link is needed for write. Maybe one who added
> that still remembers why that was done and can explain, please?
>
> Suzuki, would you please try the attached patch?

We had no luck in reproducing the issue until recently. This was again 
hit with 2.6.18-rc7. The patch (with  appropriate changes to match the 
current) has been verified to resolve the issue. Attached here is the 
patch which fits the current level.

Thanks,

Suzuki
> ------------------------------------------------------------------------
>
>
> diff -puN fs/reiserfs/file.c~reiserfs-dont-use-safelink-on-write fs/reiserfs/file.c
> --- linux-2.6.17-rc3/fs/reiserfs/file.c~reiserfs-dont-use-safelink-on-write	2006-05-12 11:28:16.000000000 +0400
> +++ linux-2.6.17-rc3-vs/fs/reiserfs/file.c	2006-05-12 11:29:16.000000000 +0400
> @@ -1305,54 +1305,7 @@ static ssize_t reiserfs_file_write(struc
>  	}
>  
>  	if (file->f_flags & O_DIRECT) {	// Direct IO needs treatment
> -		ssize_t result, after_file_end = 0;
> -		if ((*ppos + count >= inode->i_size)
> -		    || (file->f_flags & O_APPEND)) {
> -			/* If we are appending a file, we need to put this savelink in here.
> -			   If we will crash while doing direct io, finish_unfinished will
> -			   cut the garbage from the file end. */
> -			reiserfs_write_lock(inode->i_sb);
> -			err =
> -			    journal_begin(&th, inode->i_sb,
> -					  JOURNAL_PER_BALANCE_CNT);
> -			if (err) {
> -				reiserfs_write_unlock(inode->i_sb);
> -				return err;
> -			}
> -			reiserfs_update_inode_transaction(inode);
> -			add_save_link(&th, inode, 1 /* Truncate */ );
> -			after_file_end = 1;
> -			err =
> -			    journal_end(&th, inode->i_sb,
> -					JOURNAL_PER_BALANCE_CNT);
> -			reiserfs_write_unlock(inode->i_sb);
> -			if (err)
> -				return err;
> -		}
> -		result = generic_file_write(file, buf, count, ppos);
> -
> -		if (after_file_end) {	/* Now update i_size and remove the savelink */
> -			struct reiserfs_transaction_handle th;
> -			reiserfs_write_lock(inode->i_sb);
> -			err = journal_begin(&th, inode->i_sb, 1);
> -			if (err) {
> -				reiserfs_write_unlock(inode->i_sb);
> -				return err;
> -			}
> -			reiserfs_update_inode_transaction(inode);
> -			mark_inode_dirty(inode);
> -			err = journal_end(&th, inode->i_sb, 1);
> -			if (err) {
> -				reiserfs_write_unlock(inode->i_sb);
> -				return err;
> -			}
> -			err = remove_save_link(inode, 1 /* truncate */ );
> -			reiserfs_write_unlock(inode->i_sb);
> -			if (err)
> -				return err;
> -		}
> -
> -		return result;
> +	  	return generic_file_write(file, buf, count, ppos);
>  	}
>  
>  	if (unlikely((ssize_t) count < 0))
>
> _

Index: linux-2.6.19-rc1/fs/reiserfs/file.c
===================================================================
--- linux-2.6.19-rc1.orig/fs/reiserfs/file.c	2006-10-10 05:54:30.000000000 -0700
+++ linux-2.6.19-rc1/fs/reiserfs/file.c	2006-11-21 17:17:36.000000000 -0800
@@ -1306,56 +1306,8 @@
 			count = MAX_NON_LFS - (unsigned long)*ppos;
 	}
 
-	if (file->f_flags & O_DIRECT) {	// Direct IO needs treatment
-		ssize_t result, after_file_end = 0;
-		if ((*ppos + count >= inode->i_size)
-		    || (file->f_flags & O_APPEND)) {
-			/* If we are appending a file, we need to put this savelink in here.
-			   If we will crash while doing direct io, finish_unfinished will
-			   cut the garbage from the file end. */
-			reiserfs_write_lock(inode->i_sb);
-			err =
-			    journal_begin(&th, inode->i_sb,
-					  JOURNAL_PER_BALANCE_CNT);
-			if (err) {
-				reiserfs_write_unlock(inode->i_sb);
-				return err;
-			}
-			reiserfs_update_inode_transaction(inode);
-			add_save_link(&th, inode, 1 /* Truncate */ );
-			after_file_end = 1;
-			err =
-			    journal_end(&th, inode->i_sb,
-					JOURNAL_PER_BALANCE_CNT);
-			reiserfs_write_unlock(inode->i_sb);
-			if (err)
-				return err;
-		}
-		result = do_sync_write(file, buf, count, ppos);
-
-		if (after_file_end) {	/* Now update i_size and remove the savelink */
-			struct reiserfs_transaction_handle th;
-			reiserfs_write_lock(inode->i_sb);
-			err = journal_begin(&th, inode->i_sb, 1);
-			if (err) {
-				reiserfs_write_unlock(inode->i_sb);
-				return err;
-			}
-			reiserfs_update_inode_transaction(inode);
-			mark_inode_dirty(inode);
-			err = journal_end(&th, inode->i_sb, 1);
-			if (err) {
-				reiserfs_write_unlock(inode->i_sb);
-				return err;
-			}
-			err = remove_save_link(inode, 1 /* truncate */ );
-			reiserfs_write_unlock(inode->i_sb);
-			if (err)
-				return err;
-		}
-
-		return result;
-	}
+	if (file->f_flags & O_DIRECT) 
+		return do_sync_write(file, buf, count, ppos);
 
 	if (unlikely((ssize_t) count < 0))
 		return -EINVAL;