| Message ID | 20081103201428.GB30565@ajones-laptop.nbttech.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, 3 Nov 2008 12:14:28 -0800 Arthur Jones <ajones@riverbed.com> wrote: > Hi Andrew, ... > > On Mon, Nov 03, 2008 at 11:33:18AM -0800, Andrew Morton wrote: > > [...] > > > --- a/fs/ext3/super.c > > > +++ b/fs/ext3/super.c > > > @@ -2392,7 +2392,13 @@ static int ext3_sync_fs(struct super_block *sb, int wait) > > > if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) { > > > if (wait) > > > log_wait_commit(EXT3_SB(sb)->s_journal, target); > > > - } > > > + } else if (wait) > > > + /* > > > + * We may have a commit in progress, clear it out > > > + * before we go on... > > > + */ > > > + ext3_force_commit(sb); > > > + > > > return 0; > > > } > > > > Can we do > > > > sb->s_dirt = 0; > > if (wait) > > ext3_force_commit(...); > > else > > journal_start_commit(...); > > I think this is what you had in mind: yup. > > diff --git a/fs/ext3/super.c b/fs/ext3/super.c > index 18eaa78..2b48b85 100644 > --- a/fs/ext3/super.c > +++ b/fs/ext3/super.c > @@ -2386,13 +2386,12 @@ static void ext3_write_super (struct super_block * sb) > > static int ext3_sync_fs(struct super_block *sb, int wait) > { > - tid_t target; > - > sb->s_dirt = 0; > - if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) { > - if (wait) > - log_wait_commit(EXT3_SB(sb)->s_journal, target); > - } > + if (wait) > + ext3_force_commit(sb); > + else > + journal_start_commit(EXT3_SB(sb)->s_journal, NULL); > + > return 0; > } > > I tried this and it too fixes the problem. FWIW I agree it > looks better... > OK. But still, questions remain. - should we clear s_dirt if log_wait_commit() didn't work? - ext3_force_commit() clears s_dirt also - should ext3_sync_fs() be dropping the ext3_force_commit() return value on the floor? This is all rather worrisome. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, ... On Mon, Nov 03, 2008 at 12:37:50PM -0800, Andrew Morton wrote: > [...] > OK. But still, questions remain. > > - should we clear s_dirt if log_wait_commit() didn't work? > > - ext3_force_commit() clears s_dirt also > > - should ext3_sync_fs() be dropping the ext3_force_commit() return > value on the floor? - does ext4 have a similar issue (ext4_sync_fs looks the same)? Arthur -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 3 Nov 2008 12:58:54 -0800 Arthur Jones <ajones@riverbed.com> wrote: > Hi Andrew, ... > > On Mon, Nov 03, 2008 at 12:37:50PM -0800, Andrew Morton wrote: > > [...] > > OK. But still, questions remain. > > > > - should we clear s_dirt if log_wait_commit() didn't work? > > > > - ext3_force_commit() clears s_dirt also > > > > - should ext3_sync_fs() be dropping the ext3_force_commit() return > > value on the floor? > > - does ext4 have a similar issue (ext4_sync_fs looks the same)? > I'm sure it does. Someone(tm) should prepare the ext4 patch once we've settled on the ext3 patch. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2008 at 01:13:13PM -0800, Andrew Morton wrote: > On Mon, 3 Nov 2008 12:58:54 -0800 > Arthur Jones <ajones@riverbed.com> wrote: > > > Hi Andrew, ... > > > > On Mon, Nov 03, 2008 at 12:37:50PM -0800, Andrew Morton wrote: > > > [...] > > > OK. But still, questions remain. > > > > > > - should we clear s_dirt if log_wait_commit() didn't work? > > > > > > - ext3_force_commit() clears s_dirt also > > > > > > - should ext3_sync_fs() be dropping the ext3_force_commit() return > > > value on the floor? Should all the callers of ->sync_fs also be dropping the error returns on the floor? > > > > - does ext4 have a similar issue (ext4_sync_fs looks the same)? > > > > I'm sure it does. Someone(tm) should prepare the ext4 patch once we've > settled on the ext3 patch. I'll take care of it. And I would reflect the error returns up to ext3_sync_fs's callers, although at the moment it's not clear it would make much of a difference. :-( - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 3 Nov 2008 16:19:29 -0500 Theodore Tso <tytso@mit.edu> wrote: > On Mon, Nov 03, 2008 at 01:13:13PM -0800, Andrew Morton wrote: > > On Mon, 3 Nov 2008 12:58:54 -0800 > > Arthur Jones <ajones@riverbed.com> wrote: > > > > > Hi Andrew, ... > > > > > > On Mon, Nov 03, 2008 at 12:37:50PM -0800, Andrew Morton wrote: > > > > [...] > > > > OK. But still, questions remain. > > > > > > > > - should we clear s_dirt if log_wait_commit() didn't work? > > > > > > > > - ext3_force_commit() clears s_dirt also > > > > > > > > - should ext3_sync_fs() be dropping the ext3_force_commit() return > > > > value on the floor? > > Should all the callers of ->sync_fs also be dropping the error returns > on the floor? Oh, this is sync_fs. How meaningful would it be to return an error from sys_umount()? Would there be any point in leaving the errored fs mounted? Dunno. But if we're going to drop this error on the floor, we should do that at a higher level, not on a per-fs basis ;) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2008 at 01:27:35PM -0800, Andrew Morton wrote: > Oh, this is sync_fs. How meaningful would it be to return an error > from sys_umount()? Would there be any point in leaving the errored fs > mounted? Dunno. > > But if we're going to drop this error on the floor, we should do > that at a higher level, not on a per-fs basis ;) At the very least we should log it at the VFS layer, IMHO. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext3/super.c b/fs/ext3/super.c index 18eaa78..2b48b85 100644 --- a/fs/ext3/super.c +++ b/fs/ext3/super.c @@ -2386,13 +2386,12 @@ static void ext3_write_super (struct super_block * sb) static int ext3_sync_fs(struct super_block *sb, int wait) { - tid_t target; - sb->s_dirt = 0; - if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) { - if (wait) - log_wait_commit(EXT3_SB(sb)->s_journal, target); - } + if (wait) + ext3_force_commit(sb); + else + journal_start_commit(EXT3_SB(sb)->s_journal, NULL); + return 0; }