2001-03-22 18:43:17

by Jan Harkes

[permalink] [raw]
Subject: 2.4.2 fs/inode.c


I found some code that seems wrong and didn't even match it's comment.
Patch is against 2.4.2, but should go cleanly against 2.4.3-pre6 as well.

Jan


--- linux/fs/inode.c.orig Thu Mar 22 13:20:55 2001
+++ linux/fs/inode.c Thu Mar 22 13:21:32 2001
@@ -133,7 +133,7 @@

if (sb) {
/* Don't do this for I_DIRTY_PAGES - that doesn't actually dirty the inode itself */
- if (flags & (I_DIRTY | I_DIRTY_SYNC)) {
+ if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
if (sb->s_op && sb->s_op->dirty_inode)
sb->s_op->dirty_inode(inode);
}


2001-03-22 19:07:27

by Stephen C. Tweedie

[permalink] [raw]
Subject: Re: 2.4.2 fs/inode.c

Hi,

On Thu, Mar 22, 2001 at 01:42:15PM -0500, Jan Harkes wrote:
>
> I found some code that seems wrong and didn't even match it's comment.
> Patch is against 2.4.2, but should go cleanly against 2.4.3-pre6 as well.

Patch looks fine to me. Have you tested it? If this goes wrong,
things break badly...

> /* Don't do this for I_DIRTY_PAGES - that doesn't actually dirty the inode itself */
> - if (flags & (I_DIRTY | I_DIRTY_SYNC)) {
> + if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {

--Stephen

2001-03-22 19:33:37

by Jan Harkes

[permalink] [raw]
Subject: Re: 2.4.2 fs/inode.c

On Thu, Mar 22, 2001 at 07:04:52PM +0000, Stephen C. Tweedie wrote:
> Hi,
>
> On Thu, Mar 22, 2001 at 01:42:15PM -0500, Jan Harkes wrote:
> >
> > I found some code that seems wrong and didn't even match it's comment.
> > Patch is against 2.4.2, but should go cleanly against 2.4.3-pre6 as well.
>
> Patch looks fine to me. Have you tested it? If this goes wrong,
> things break badly...

I've been running it for about a night and a morning now, nothing bad
has happened, my ext2 filesystem shows up clean when forcing a fsck.

If things actually break badly, it is a very serious bug in the
underlying FS. The FS should not 'happen to work' just because the VFS
inadvertedly marked unmodified inodes as being dirty.

Jan

2001-03-22 20:22:17

by Chris Mason

[permalink] [raw]
Subject: Re: 2.4.2 fs/inode.c



On Thursday, March 22, 2001 07:04:52 PM +0000 "Stephen C. Tweedie"
<[email protected]> wrote:

> Hi,
>
> On Thu, Mar 22, 2001 at 01:42:15PM -0500, Jan Harkes wrote:
>>
>> I found some code that seems wrong and didn't even match it's comment.
>> Patch is against 2.4.2, but should go cleanly against 2.4.3-pre6 as well.
>
> Patch looks fine to me. Have you tested it? If this goes wrong,
> things break badly...

This should only affect reiserfs, and it should be a good thing. I'll do
some tests, thanks for spotting.

>
>> /* Don't do this for I_DIRTY_PAGES - that doesn't actually dirty the
>> inode itself */ - if (flags & (I_DIRTY | I_DIRTY_SYNC)) {
>> + if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {

-chris

2001-03-26 17:53:13

by Chris Mason

[permalink] [raw]
Subject: Re: 2.4.2 fs/inode.c



On Thursday, March 22, 2001 01:42:15 PM -0500 Jan Harkes
<[email protected]> wrote:

>
> I found some code that seems wrong and didn't even match it's comment.
> Patch is against 2.4.2, but should go cleanly against 2.4.3-pre6 as well.
>

Ok, this looks correct, makes reiserfs faster, and survived under load. The
idea was to only call dirty_inode if sync_one might decide the inode needs
flushing to disk. So, the check in __mark_inode_dirty should look the same
as the check in sync_one.

> --- linux/fs/inode.c.orig Thu Mar 22 13:20:55 2001
> +++ linux/fs/inode.c Thu Mar 22 13:21:32 2001
> @@ -133,7 +133,7 @@
>
> if (sb) {
> /* Don't do this for I_DIRTY_PAGES - that doesn't actually dirty the
> inode itself */
> - if (flags & (I_DIRTY | I_DIRTY_SYNC)) {
> + if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
> if (sb->s_op && sb->s_op->dirty_inode)
> sb->s_op->dirty_inode(inode);
> }
> -

-chris