[PATCH] reiserfs:fix journaling issue regarding fsync()

[PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

Hi,

  When write() extends a file(i_size is increased) and fsync() is
called, change of inode must be written to journaling area
through fsync().
But,currently the i_trans_id is not correctly updated when i_size
is increased. So fsync() does not kick the journal writer.

Following patch fix this bug.

  Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

diff -Nru linux-2.6.17/fs/reiserfs/super.c linux-2.6.17_fix/fs/reiserfs/super.c
--- linux-2.6.17/fs/reiserfs/super.c	2006-06-18 10:49:35.000000000 +0900
+++ linux-2.6.17_fix/fs/reiserfs/super.c	2006-06-20 14:38:28.000000000 +0900
@@ -558,6 +558,7 @@
  		reiserfs_write_unlock(inode->i_sb);
  		return;
  	}
+	reiserfs_update_inode_transaction(inode);
  	reiserfs_update_sd(&th, inode);
  	journal_end(&th, inode->i_sb, 1);
  	reiserfs_write_unlock(inode->i_sb);


Thanks, 

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Vladimir V. Saveliev]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hello

Sorry, but afaics reiserfs' fsync works as it is supposed to.
I experiment with the attached program. After reboot I make sure that
file has "new file size".

On Tue, 2006-06-20 at 17:43 +0900, Hisashi Hifumi wrote:
> Hi,
> 
>   When write() extends a file(i_size is increased) and fsync() is
> called, change of inode must be written to journaling area
> through fsync().
> But,currently the i_trans_id is not correctly updated when i_size
> is increased. So fsync() does not kick the journal writer.
> 
> Following patch fix this bug.
> 
>   Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>
> 
> diff -Nru linux-2.6.17/fs/reiserfs/super.c linux-2.6.17_fix/fs/reiserfs/super.c
> --- linux-2.6.17/fs/reiserfs/super.c	2006-06-18 10:49:35.000000000 +0900
> +++ linux-2.6.17_fix/fs/reiserfs/super.c	2006-06-20 14:38:28.000000000 +0900
> @@ -558,6 +558,7 @@
>   		reiserfs_write_unlock(inode->i_sb);
>   		return;
>   	}
> +	reiserfs_update_inode_transaction(inode);
>   	reiserfs_update_sd(&th, inode);
>   	journal_end(&th, inode->i_sb, 1);
>   	reiserfs_write_unlock(inode->i_sb);
> 
> 
> Thanks, 
> 
> 

[-- Attachment #2: stest.c --]
[-- Type: text/x-csrc, Size: 695 bytes --]

#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>

int main(int argc, char **argv)
{
	int fd;
	struct stat st;

	fd = open(argv[1], O_WRONLY | O_APPEND);
	if (fd == -1) {
		perror("open failed");
		return 0;
	}
	if (fstat(fd, &st)) {
		perror("stat failed");
		return 0;
	}
	printf("old file size %d\n", st.st_size);
	if (write(fd, "hello", 5) != 5) {
		perror("write failed");
		return 0;
	}

        if (fstat(fd, &st)) {
                perror("stat failed");
                return 0;
        }
        printf("new file size %d\n", st.st_size);
	if (fsync(fd)) {
		perror("fsync failed");
		return 0;
	}

	printf("rebooting"); fflush(stdout);
	system("reboot -f -n");
	return 0;
}

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hans Reiser]

Vladimir V. Saveliev wrote:

>Hello
>
>Sorry, but afaics reiserfs' fsync works as it is supposed to.
>I experiment with the attached program. After reboot I make sure that
>file has "new file size".
>
>  
>
Can you and Hisashi discuss this in chat, and then send me the transcript?

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

At 19:15 06/06/23, Vladimir V. Saveliev wrote:
 >Hello
 >
 >Sorry, but afaics reiserfs' fsync works as it is supposed to.
 >I experiment with the attached program. After reboot I make sure that
 >file has "new file size".
 >

I modified your program shown as below.The only difference is that a for
loop is added. Write() and fsync() tests are repeated 100times.

#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>

int main(int argc, char **argv)
{
	int fd,i;
	struct stat st;

	fd = open(argv[1], O_WRONLY | O_APPEND);
	if (fd == -1) {
		perror("open failed");
		return 0;
	}
	if (fstat(fd, &st)) {
		perror("stat failed");
		return 0;
	}
	printf("old file size %d\n", st.st_size);
	for(i=0;i<100;i++) {
		if (write(fd, "hello", 5) != 5) {
			perror("write failed");
			return 0;
		}

		if (fstat(fd, &st)) {
			perror("stat failed");
			return 0;
		}
		printf("new file size %d\n", st.st_size);
		if (fsync(fd)) {
			perror("fsync failed");
			return 0;
		}
	}
	printf("rebooting"); fflush(stdout);
	system("reboot -f -n");
	return 0;
}

I tested with this program toward reiserfs (2.6.17 and 2.6.17 with my patch).
After I made an empty file, I run this program.
I performed this test 10times each.
Following results are obtained.

				success 	fail
2.6.17			5		5
2.6.17 with my patch	10		0

"Success" means that file size is 500bytes after reboot.
"Fail" means that file size is less than 500bytes after reboot.

So, this result showed that my patch is needed.

Thanks. 

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Chris Mason]

On Tuesday 20 June 2006 04:43, Hisashi Hifumi wrote:
> Hi,
>
>   When write() extends a file(i_size is increased) and fsync() is
> called, change of inode must be written to journaling area
> through fsync().
> But,currently the i_trans_id is not correctly updated when i_size
> is increased. So fsync() does not kick the journal writer.

Thanks for the patch.  One problem is this will bump the transaction marker 
for atime updates too.  I'd rather see the change done inside 
reiserfs_file_write.

reiserfs_file_write already updates the transaction when blocks are allocated, 
but you're right that to be 100% correct we should cover the case when i_size 
increases but new blocks are not added.  Was this the case you were trying to 
fix?

-chris

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

Hi,

At 09:47 06/06/30, Chris Mason wrote:
 >Thanks for the patch.  One problem is this will bump the transaction marker
 >for atime updates too.  I'd rather see the change done inside
 >reiserfs_file_write.

I did not realize that an atime updates is also influenced.

 >
 >reiserfs_file_write already updates the transaction when blocks are allocated,
 >but you're right that to be 100% correct we should cover the case when i_size
 >increases but new blocks are not added.  Was this the case you were trying to
 >fix?

Yes, that's right.

So, I remade my patch as follows.
I tested this patch and confirmed that the kernel with this patch work well.


Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>


diff -Nru linux-2.6.17/fs/reiserfs/file.c linux-2.6.17_fix/fs/reiserfs/file.c
--- linux-2.6.17/fs/reiserfs/file.c	2006-06-18 10:49:35.000000000 +0900
+++ linux-2.6.17_fix/fs/reiserfs/file.c	2006-06-30 10:09:35.000000000 +0900
@@ -860,8 +860,10 @@
  			// this sets the proper flags for O_SYNC to trigger a commit
  			mark_inode_dirty(inode);
  			reiserfs_write_unlock(inode->i_sb);
-		} else
+		} else {
+			reiserfs_update_inode_transaction(inode);
  			mark_inode_dirty(inode);
+		}

  		sd_update = 1;
  	}

Thanks.

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Chris Mason]

On Thursday 29 June 2006 21:36, Hisashi Hifumi wrote:
> Hi,
>
> At 09:47 06/06/30, Chris Mason wrote:
>  >Thanks for the patch.  One problem is this will bump the transaction
>  > marker for atime updates too.  I'd rather see the change done inside
>  >reiserfs_file_write.
>
> I did not realize that an atime updates is also influenced.
>
>  >reiserfs_file_write already updates the transaction when blocks are
>  > allocated, but you're right that to be 100% correct we should cover the
>  > case when i_size increases but new blocks are not added.  Was this the
>  > case you were trying to fix?
>
> Yes, that's right.
>
> So, I remade my patch as follows.
> I tested this patch and confirmed that the kernel with this patch work
> well.

This is correct, excpet you need to put the update_inode_transaction call 
inside reiserfs_write_lock/unlock.

-chris

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

Hi.

> This is correct, excpet you need to put the update_inode_transaction call
> inside reiserfs_write_lock/unlock.

Again, I remade my patch.
I have a question.
I understand update_inode_transaction must be inside reiserfs_write_lock.
Is update_inode_transaction needed to be inside journal_begin/end ?


Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

diff -Nru linux-2.6.17/fs/reiserfs/file.c linux-2.6.17_fix/fs/reiserfs/file.c
--- linux-2.6.17/fs/reiserfs/file.c     2006-06-18 10:49:35.000000000 +0900
+++ linux-2.6.17_fix/fs/reiserfs/file.c 2006-07-01 07:39:11.002239042 +0900
@@ -860,8 +860,12 @@
                        // this sets the proper flags for O_SYNC to
trigger a commit
                        mark_inode_dirty(inode);
                        reiserfs_write_unlock(inode->i_sb);
-               } else
+               } else {
+                       reiserfs_write_lock(inode->i_sb);
+                       reiserfs_update_inode_transaction(inode);
                        mark_inode_dirty(inode);
+                       reiserfs_write_unlock(inode->i_sb);
+               }

                sd_update = 1;
        }

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Chris Mason]

On Friday 30 June 2006 18:59, Hisashi Hifumi wrote:
> Hi.
>
> > This is correct, excpet you need to put the update_inode_transaction call
> > inside reiserfs_write_lock/unlock.
>
> Again, I remade my patch.

Thanks, this one looks good.

> I have a question.
> I understand update_inode_transaction must be inside reiserfs_write_lock.
> Is update_inode_transaction needed to be inside journal_begin/end ?

reiserfs_write_lock is enough, do_journal_end has reiserfs_write_lock while it 
is updating j_current_jl and j_trans_id.  We just need to make sure the 
transid and the jl pointer are consistent with each other.

-chris

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hans Reiser]

Chris Mason wrote:

>
>Thanks, this one looks good.
>  
>
Hisashi, please submit it to akpm.

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

At 00:34 06/07/03, Hans Reiser wrote:
 >Chris Mason wrote:
 >
 >>
 >>Thanks, this one looks good.
 >>
 >>
 >Hisashi, please submit it to akpm.

Hi, Andrew.

When write() extends a file(i_size is increased) and fsync() is called,
change of inode must be written to journaling area through fsync().
But,currently the i_trans_id is not correctly updated when i_size
is increased. So fsync() does not kick the journal writer.

Reiserfs_file_write() already updates the transaction when blocks are allocated,
but the case when i_size increases and new blocks are not added is not
correctly treated.

Following patch fix this bug.

Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

diff -Nru linux-2.6.17/fs/reiserfs/file.c linux-2.6.17_fix/fs/reiserfs/file.c
--- linux-2.6.17/fs/reiserfs/file.c	2006-06-18 10:49:35.000000000 +0900
+++ linux-2.6.17_fix/fs/reiserfs/file.c	2006-07-03 09:52:51.000000000 +0900
@@ -860,8 +860,12 @@
  			// this sets the proper flags for O_SYNC to trigger a commit
  			mark_inode_dirty(inode);
  			reiserfs_write_unlock(inode->i_sb);
-		} else
+		} else {
+			reiserfs_write_lock(inode->i_sb);
+			reiserfs_update_inode_transaction(inode);
  			mark_inode_dirty(inode);
+			reiserfs_write_unlock(inode->i_sb);
+		}

  		sd_update = 1;
  	}

Thanks. 

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Andrew Morton]

On Mon, 03 Jul 2006 10:31:49 +0900
Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp> wrote:

> When write() extends a file(i_size is increased) and fsync() is called,
> change of inode must be written to journaling area through fsync().
> But,currently the i_trans_id is not correctly updated when i_size
> is increased. So fsync() does not kick the journal writer.
> 
> Reiserfs_file_write() already updates the transaction when blocks are allocated,
> but the case when i_size increases and new blocks are not added is not
> correctly treated.

How can i_size be increased without adding blocks?  Are you referring to a
write which remains wholly within the final block of the file?  And/or to
an expanding lseek?

And what are the user-visible consequences of this bug?

Thanks.

Re: [PATCH] reiserfs:fix journaling issue regarding fsync()

[Hisashi Hifumi]

 >How can i_size be increased without adding blocks?  Are you referring to a
 >write which remains wholly within the final block of the file?

Yes, It is a case when a file extends within the final block.

 >
 >And what are the user-visible consequences of this bug?

Because the i_size is not written to the FS through fsync(),
an user can not see the whole content of a file when filesystem
crashes just after fsync() call.

thanks.