| Message ID | 1311941612-9108-1-git-send-email-lczerner@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, 29 Jul 2011, Lukas Czerner wrote: > We are doing ext2fs_flush() twice right now at the end of the mke2fs. > First by directly calling ext2fs_flush() which is intended to write > superblock and fs accounting information. And then it is invoked again > when we are calling ext2fs_close(), only this time, because the fs is > not dirty, we are writing out only superblock. > > I think it is bad to call it twice because even when writing only super > block it takes some time on bigger file systems and moreover > ext2fs_close() can fail without any reasonable explanation for the user. > Also ext2fs_flush() is printing out progress and it is confusing for the > users. > > Fix all this by removing the ext2fs_flush() and leaving it all to > ext2fs_close(). However we need to introduce new variables to store > check interval and max mount count, because fs structure is freed on > ext2fs_close() and we really want to print those information as the last > info for the user. Ping > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > misc/mke2fs.c | 15 ++++++++------- > misc/tune2fs.c | 3 ++- > misc/util.c | 5 ++--- > misc/util.h | 2 +- > tests/m_extent_journal/expect.1 | 2 +- > tests/m_uninit/expect.1 | 2 +- > 6 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/misc/mke2fs.c b/misc/mke2fs.c > index e062bda..481e550 100644 > --- a/misc/mke2fs.c > +++ b/misc/mke2fs.c > @@ -2143,7 +2143,7 @@ int main (int argc, char *argv[]) > ext2_filsys fs; > badblocks_list bb_list = 0; > unsigned int journal_blocks; > - unsigned int i; > + unsigned int i, max_mnt_count, checkinterval; > int val, hash_alg; > int flags; > int old_bitmaps; > @@ -2469,22 +2469,23 @@ no_journal: > if (!quiet) > printf(_("Writing superblocks and " > "filesystem accounting information: ")); > - retval = ext2fs_flush(fs); > + checkinterval = fs->super->s_checkinterval; > + max_mnt_count = fs->super->s_max_mnt_count; > + retval = ext2fs_close(fs); > if (retval) { > fprintf(stderr, > _("\nWarning, had trouble writing out superblocks.")); > - } > - if (!quiet) { > + } else if (!quiet) { > printf(_("done\n\n")); > if (!getenv("MKE2FS_SKIP_CHECK_MSG")) > - print_check_message(fs); > + print_check_message(max_mnt_count, checkinterval); > } > - val = ext2fs_close(fs); > + > remove_error_table(&et_ext2_error_table); > remove_error_table(&et_prof_error_table); > profile_release(profile); > for (i=0; fs_types[i]; i++) > free(fs_types[i]); > free(fs_types); > - return (retval || val) ? 1 : 0; > + return retval; > } > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index 5bf5187..0e1e1cf 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -568,7 +568,8 @@ static void add_journal(ext2_filsys fs) > if ((mount_flags & EXT2_MF_MOUNTED) == 0) > fs->flags &= ~EXT2_FLAG_SUPER_ONLY; > } > - print_check_message(fs); > + print_check_message(fs->super->s_max_mnt_count, > + fs->super->s_checkinterval); > return; > > err: > diff --git a/misc/util.c b/misc/util.c > index 51bdb60..18329ee 100644 > --- a/misc/util.c > +++ b/misc/util.c > @@ -282,12 +282,11 @@ unsigned int figure_journal_size(int size, ext2_filsys fs) > return j_blocks; > } > > -void print_check_message(ext2_filsys fs) > +void print_check_message(unsigned int mnt, unsigned int check) > { > printf(_("This filesystem will be automatically " > "checked every %d mounts or\n" > "%g days, whichever comes first. " > "Use tune2fs -c or -i to override.\n"), > - fs->super->s_max_mnt_count, > - (double)fs->super->s_checkinterval / (3600 * 24)); > + mnt, check / (3600 * 24)); > } > diff --git a/misc/util.h b/misc/util.h > index e0c99f6..d05f17e 100644 > --- a/misc/util.h > +++ b/misc/util.h > @@ -23,4 +23,4 @@ extern void check_plausibility(const char *device); > extern void parse_journal_opts(const char *opts); > extern void check_mount(const char *device, int force, const char *type); > extern unsigned int figure_journal_size(int size, ext2_filsys fs); > -extern void print_check_message(ext2_filsys fs); > +extern void print_check_message(unsigned int, unsigned int); > diff --git a/tests/m_extent_journal/expect.1 b/tests/m_extent_journal/expect.1 > index 88ea2d9..a8c5b63 100644 > --- a/tests/m_extent_journal/expect.1 > +++ b/tests/m_extent_journal/expect.1 > @@ -18,7 +18,7 @@ Writing inode tables: 0/81/82/83/84/85/86/87/8 done > Creating journal (4096 blocks): done > Writing superblocks and filesystem accounting information: 0/81/82/83/84/85/86/87/8 done > > -0/81/82/83/84/85/86/87/8 Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super > +Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super > > Pass 1: Checking inodes, blocks, and sizes > Pass 2: Checking directory structure > diff --git a/tests/m_uninit/expect.1 b/tests/m_uninit/expect.1 > index 173c072..a3c059d 100644 > --- a/tests/m_uninit/expect.1 > +++ b/tests/m_uninit/expect.1 > @@ -17,7 +17,7 @@ Allocating group tables: 0/16 1/16 2/16 3/16 4/16 5/16 > Writing inode tables: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done > Writing superblocks and filesystem accounting information: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done > > - 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg > +Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg > > Pass 1: Checking inodes, blocks, and sizes > Pass 2: Checking directory structure >
On Thu, 11 Aug 2011, Lukas Czerner wrote: > On Fri, 29 Jul 2011, Lukas Czerner wrote: > > > We are doing ext2fs_flush() twice right now at the end of the mke2fs. > > First by directly calling ext2fs_flush() which is intended to write > > superblock and fs accounting information. And then it is invoked again > > when we are calling ext2fs_close(), only this time, because the fs is > > not dirty, we are writing out only superblock. > > > > I think it is bad to call it twice because even when writing only super > > block it takes some time on bigger file systems and moreover > > ext2fs_close() can fail without any reasonable explanation for the user. > > Also ext2fs_flush() is printing out progress and it is confusing for the > > users. > > > > Fix all this by removing the ext2fs_flush() and leaving it all to > > ext2fs_close(). However we need to introduce new variables to store > > check interval and max mount count, because fs structure is freed on > > ext2fs_close() and we really want to print those information as the last > > info for the user. > > Ping Ping^2 > > > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > > --- > > misc/mke2fs.c | 15 ++++++++------- > > misc/tune2fs.c | 3 ++- > > misc/util.c | 5 ++--- > > misc/util.h | 2 +- > > tests/m_extent_journal/expect.1 | 2 +- > > tests/m_uninit/expect.1 | 2 +- > > 6 files changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/misc/mke2fs.c b/misc/mke2fs.c > > index e062bda..481e550 100644 > > --- a/misc/mke2fs.c > > +++ b/misc/mke2fs.c > > @@ -2143,7 +2143,7 @@ int main (int argc, char *argv[]) > > ext2_filsys fs; > > badblocks_list bb_list = 0; > > unsigned int journal_blocks; > > - unsigned int i; > > + unsigned int i, max_mnt_count, checkinterval; > > int val, hash_alg; > > int flags; > > int old_bitmaps; > > @@ -2469,22 +2469,23 @@ no_journal: > > if (!quiet) > > printf(_("Writing superblocks and " > > "filesystem accounting information: ")); > > - retval = ext2fs_flush(fs); > > + checkinterval = fs->super->s_checkinterval; > > + max_mnt_count = fs->super->s_max_mnt_count; > > + retval = ext2fs_close(fs); > > if (retval) { > > fprintf(stderr, > > _("\nWarning, had trouble writing out superblocks.")); > > - } > > - if (!quiet) { > > + } else if (!quiet) { > > printf(_("done\n\n")); > > if (!getenv("MKE2FS_SKIP_CHECK_MSG")) > > - print_check_message(fs); > > + print_check_message(max_mnt_count, checkinterval); > > } > > - val = ext2fs_close(fs); > > + > > remove_error_table(&et_ext2_error_table); > > remove_error_table(&et_prof_error_table); > > profile_release(profile); > > for (i=0; fs_types[i]; i++) > > free(fs_types[i]); > > free(fs_types); > > - return (retval || val) ? 1 : 0; > > + return retval; > > } > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > > index 5bf5187..0e1e1cf 100644 > > --- a/misc/tune2fs.c > > +++ b/misc/tune2fs.c > > @@ -568,7 +568,8 @@ static void add_journal(ext2_filsys fs) > > if ((mount_flags & EXT2_MF_MOUNTED) == 0) > > fs->flags &= ~EXT2_FLAG_SUPER_ONLY; > > } > > - print_check_message(fs); > > + print_check_message(fs->super->s_max_mnt_count, > > + fs->super->s_checkinterval); > > return; > > > > err: > > diff --git a/misc/util.c b/misc/util.c > > index 51bdb60..18329ee 100644 > > --- a/misc/util.c > > +++ b/misc/util.c > > @@ -282,12 +282,11 @@ unsigned int figure_journal_size(int size, ext2_filsys fs) > > return j_blocks; > > } > > > > -void print_check_message(ext2_filsys fs) > > +void print_check_message(unsigned int mnt, unsigned int check) > > { > > printf(_("This filesystem will be automatically " > > "checked every %d mounts or\n" > > "%g days, whichever comes first. " > > "Use tune2fs -c or -i to override.\n"), > > - fs->super->s_max_mnt_count, > > - (double)fs->super->s_checkinterval / (3600 * 24)); > > + mnt, check / (3600 * 24)); > > } > > diff --git a/misc/util.h b/misc/util.h > > index e0c99f6..d05f17e 100644 > > --- a/misc/util.h > > +++ b/misc/util.h > > @@ -23,4 +23,4 @@ extern void check_plausibility(const char *device); > > extern void parse_journal_opts(const char *opts); > > extern void check_mount(const char *device, int force, const char *type); > > extern unsigned int figure_journal_size(int size, ext2_filsys fs); > > -extern void print_check_message(ext2_filsys fs); > > +extern void print_check_message(unsigned int, unsigned int); > > diff --git a/tests/m_extent_journal/expect.1 b/tests/m_extent_journal/expect.1 > > index 88ea2d9..a8c5b63 100644 > > --- a/tests/m_extent_journal/expect.1 > > +++ b/tests/m_extent_journal/expect.1 > > @@ -18,7 +18,7 @@ Writing inode tables: 0/81/82/83/84/85/86/87/8 done > > Creating journal (4096 blocks): done > > Writing superblocks and filesystem accounting information: 0/81/82/83/84/85/86/87/8 done > > > > -0/81/82/83/84/85/86/87/8 Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super > > +Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super > > > > Pass 1: Checking inodes, blocks, and sizes > > Pass 2: Checking directory structure > > diff --git a/tests/m_uninit/expect.1 b/tests/m_uninit/expect.1 > > index 173c072..a3c059d 100644 > > --- a/tests/m_uninit/expect.1 > > +++ b/tests/m_uninit/expect.1 > > @@ -17,7 +17,7 @@ Allocating group tables: 0/16 1/16 2/16 3/16 4/16 5/16 > > Writing inode tables: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done > > Writing superblocks and filesystem accounting information: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done > > > > - 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg > > +Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg > > > > Pass 1: Checking inodes, blocks, and sizes > > Pass 2: Checking directory structure > > > >
Thanks, applied, with an obvious bugfix: > -void print_check_message(ext2_filsys fs) > +void print_check_message(unsigned int mnt, unsigned int check) > { > printf(_("This filesystem will be automatically " > "checked every %d mounts or\n" > "%g days, whichever comes first. " > "Use tune2fs -c or -i to override.\n"), > - fs->super->s_max_mnt_count, > - (double)fs->super->s_checkinterval / (3600 * 24)); > + mnt, check / (3600 * 24)); > } ../../misc/util.c: In function ‘print_check_message’: ../../misc/util.c:291: warning: format ‘%g’ expects type ‘double’, but argument 3 has type ‘unsigned int’ the (double) cast was dropped in your patch. The message should be made a bit better in the case that s_max_mnt_count or s_checkinterval is zero, but that's a separate patch. - 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/misc/mke2fs.c b/misc/mke2fs.c index e062bda..481e550 100644 --- a/misc/mke2fs.c +++ b/misc/mke2fs.c @@ -2143,7 +2143,7 @@ int main (int argc, char *argv[]) ext2_filsys fs; badblocks_list bb_list = 0; unsigned int journal_blocks; - unsigned int i; + unsigned int i, max_mnt_count, checkinterval; int val, hash_alg; int flags; int old_bitmaps; @@ -2469,22 +2469,23 @@ no_journal: if (!quiet) printf(_("Writing superblocks and " "filesystem accounting information: ")); - retval = ext2fs_flush(fs); + checkinterval = fs->super->s_checkinterval; + max_mnt_count = fs->super->s_max_mnt_count; + retval = ext2fs_close(fs); if (retval) { fprintf(stderr, _("\nWarning, had trouble writing out superblocks.")); - } - if (!quiet) { + } else if (!quiet) { printf(_("done\n\n")); if (!getenv("MKE2FS_SKIP_CHECK_MSG")) - print_check_message(fs); + print_check_message(max_mnt_count, checkinterval); } - val = ext2fs_close(fs); + remove_error_table(&et_ext2_error_table); remove_error_table(&et_prof_error_table); profile_release(profile); for (i=0; fs_types[i]; i++) free(fs_types[i]); free(fs_types); - return (retval || val) ? 1 : 0; + return retval; } diff --git a/misc/tune2fs.c b/misc/tune2fs.c index 5bf5187..0e1e1cf 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -568,7 +568,8 @@ static void add_journal(ext2_filsys fs) if ((mount_flags & EXT2_MF_MOUNTED) == 0) fs->flags &= ~EXT2_FLAG_SUPER_ONLY; } - print_check_message(fs); + print_check_message(fs->super->s_max_mnt_count, + fs->super->s_checkinterval); return; err: diff --git a/misc/util.c b/misc/util.c index 51bdb60..18329ee 100644 --- a/misc/util.c +++ b/misc/util.c @@ -282,12 +282,11 @@ unsigned int figure_journal_size(int size, ext2_filsys fs) return j_blocks; } -void print_check_message(ext2_filsys fs) +void print_check_message(unsigned int mnt, unsigned int check) { printf(_("This filesystem will be automatically " "checked every %d mounts or\n" "%g days, whichever comes first. " "Use tune2fs -c or -i to override.\n"), - fs->super->s_max_mnt_count, - (double)fs->super->s_checkinterval / (3600 * 24)); + mnt, check / (3600 * 24)); } diff --git a/misc/util.h b/misc/util.h index e0c99f6..d05f17e 100644 --- a/misc/util.h +++ b/misc/util.h @@ -23,4 +23,4 @@ extern void check_plausibility(const char *device); extern void parse_journal_opts(const char *opts); extern void check_mount(const char *device, int force, const char *type); extern unsigned int figure_journal_size(int size, ext2_filsys fs); -extern void print_check_message(ext2_filsys fs); +extern void print_check_message(unsigned int, unsigned int); diff --git a/tests/m_extent_journal/expect.1 b/tests/m_extent_journal/expect.1 index 88ea2d9..a8c5b63 100644 --- a/tests/m_extent_journal/expect.1 +++ b/tests/m_extent_journal/expect.1 @@ -18,7 +18,7 @@ Writing inode tables: 0/81/82/83/84/85/86/87/8 done Creating journal (4096 blocks): done Writing superblocks and filesystem accounting information: 0/81/82/83/84/85/86/87/8 done -0/81/82/83/84/85/86/87/8 Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super +Filesystem features: has_journal ext_attr resize_inode dir_index filetype extent sparse_super Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure diff --git a/tests/m_uninit/expect.1 b/tests/m_uninit/expect.1 index 173c072..a3c059d 100644 --- a/tests/m_uninit/expect.1 +++ b/tests/m_uninit/expect.1 @@ -17,7 +17,7 @@ Allocating group tables: 0/16 1/16 2/16 3/16 4/16 5/16 Writing inode tables: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done Writing superblocks and filesystem accounting information: 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 done - 0/16 1/16 2/16 3/16 4/16 5/16 6/16 7/16 8/16 9/1610/1611/1612/1613/1614/1615/16 Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg +Filesystem features: ext_attr resize_inode dir_index filetype sparse_super uninit_bg Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure
We are doing ext2fs_flush() twice right now at the end of the mke2fs. First by directly calling ext2fs_flush() which is intended to write superblock and fs accounting information. And then it is invoked again when we are calling ext2fs_close(), only this time, because the fs is not dirty, we are writing out only superblock. I think it is bad to call it twice because even when writing only super block it takes some time on bigger file systems and moreover ext2fs_close() can fail without any reasonable explanation for the user. Also ext2fs_flush() is printing out progress and it is confusing for the users. Fix all this by removing the ext2fs_flush() and leaving it all to ext2fs_close(). However we need to introduce new variables to store check interval and max mount count, because fs structure is freed on ext2fs_close() and we really want to print those information as the last info for the user. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- misc/mke2fs.c | 15 ++++++++------- misc/tune2fs.c | 3 ++- misc/util.c | 5 ++--- misc/util.h | 2 +- tests/m_extent_journal/expect.1 | 2 +- tests/m_uninit/expect.1 | 2 +- 6 files changed, 15 insertions(+), 14 deletions(-)