[PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[Jan Kara]

When dumpe2fs is used on a filesystem mounted read-write, printed information
may be old and inconsistent because e.g. updates of some superblock fields
happen lazily or because we do not really try to handle various intermediate
states of filesystem operations. Document this in the manpage and warn user
when dumpe2fs is used for such filesystem.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 misc/dumpe2fs.8.in |    3 ++-
 misc/dumpe2fs.c    |   14 ++++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

  Ted, would you take this change? We actually had to explain to our users that
what they were doing isn't really going to work reliably...

diff --git a/misc/dumpe2fs.8.in b/misc/dumpe2fs.8.in
index c1dfcbd..e1336dc 100644
--- a/misc/dumpe2fs.8.in
+++ b/misc/dumpe2fs.8.in
@@ -71,7 +71,8 @@ print the version number of
 and exit.
 .SH BUGS
 You need to know the physical filesystem structure to understand the
-output.
+output. When used with a mounted filesystem, the printed information may be
+old or inconsistent.
 .SH AUTHOR
 .B dumpe2fs 
 was written by Remy Card <Remy.Card@linux.org>.  It is currently being
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index 9a0dd46..fa1362a 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -495,6 +495,7 @@ int main (int argc, char ** argv)
 	int		flags;
 	int		header_only = 0;
 	int		c;
+	int		mount_flags;
 
 #ifdef ENABLE_NLS
 	setlocale(LC_MESSAGES, "");
@@ -567,6 +568,13 @@ int main (int argc, char ** argv)
 		printf (_("Couldn't find valid filesystem superblock.\n"));
 		exit (1);
 	}
+	retval = ext2fs_check_if_mounted(device_name, &mount_flags);
+	if (retval) {
+		com_err("ext2fs_check_if_mount", retval,
+			_("while determining whether %s is mounted."),
+			device_name);
+		exit(1);
+	}
 	if (print_badblocks) {
 		list_bad_blocks(fs, 1);
 	} else {
@@ -595,6 +603,12 @@ int main (int argc, char ** argv)
 		}
 	}
 	ext2fs_close (fs);
+	if ((mount_flags & EXT2_MF_MOUNTED) &&
+	    !(mount_flags & EXT2_MF_READONLY)) {
+		fputs(_("The filesystem is read-write mounted so printed"
+			" information may be old or inconsistent.\n"), stderr);
+	}
+
 	remove_error_table(&et_ext2_error_table);
 	exit (0);
 }
-- 
1.7.1

Re: [PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[Andreas Dilger]

On 2011-07-28, at 9:17 AM, Jan Kara <jack@suse.cz> wrote:
> When dumpe2fs is used on a filesystem mounted read-write, printed information
> may be old and inconsistent because e.g. updates of some superblock fields
> happen lazily or because we do not really try to handle various intermediate
> states of filesystem operations. Document this in the manpage and warn user
> when dumpe2fs is used for such filesystem.

Jan, does this specifically relate to the free blocks or free inodes count?  My preference would actually be to have dumpe2fs print the correct values. I guess that means reading all of the group descriptors and adding up the free block/inode counts like the kernel does. 

I'm not against this patch in principle, since any operation on a mounted filesystem is inherently racy, but as it stands now the information printed by dumpe2fs can be completely useless. 

Cheers, Andreas

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> misc/dumpe2fs.8.in |    3 ++-
> misc/dumpe2fs.c    |   14 ++++++++++++++
> 2 files changed, 16 insertions(+), 1 deletions(-)
> 
>  Ted, would you take this change? We actually had to explain to our users that
> what they were doing isn't really going to work reliably...
> 
> diff --git a/misc/dumpe2fs.8.in b/misc/dumpe2fs.8.in
> index c1dfcbd..e1336dc 100644
> --- a/misc/dumpe2fs.8.in
> +++ b/misc/dumpe2fs.8.in
> @@ -71,7 +71,8 @@ print the version number of
> and exit.
> .SH BUGS
> You need to know the physical filesystem structure to understand the
> -output.
> +output. When used with a mounted filesystem, the printed information may be
> +old or inconsistent.
> .SH AUTHOR
> .B dumpe2fs 
> was written by Remy Card <Remy.Card@linux.org>.  It is currently being
> diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
> index 9a0dd46..fa1362a 100644
> --- a/misc/dumpe2fs.c
> +++ b/misc/dumpe2fs.c
> @@ -495,6 +495,7 @@ int main (int argc, char ** argv)
>    int        flags;
>    int        header_only = 0;
>    int        c;
> +    int        mount_flags;
> 
> #ifdef ENABLE_NLS
>    setlocale(LC_MESSAGES, "");
> @@ -567,6 +568,13 @@ int main (int argc, char ** argv)
>        printf (_("Couldn't find valid filesystem superblock.\n"));
>        exit (1);
>    }
> +    retval = ext2fs_check_if_mounted(device_name, &mount_flags);
> +    if (retval) {
> +        com_err("ext2fs_check_if_mount", retval,
> +            _("while determining whether %s is mounted."),
> +            device_name);
> +        exit(1);
> +    }
>    if (print_badblocks) {
>        list_bad_blocks(fs, 1);
>    } else {
> @@ -595,6 +603,12 @@ int main (int argc, char ** argv)
>        }
>    }
>    ext2fs_close (fs);
> +    if ((mount_flags & EXT2_MF_MOUNTED) &&
> +        !(mount_flags & EXT2_MF_READONLY)) {
> +        fputs(_("The filesystem is read-write mounted so printed"
> +            " information may be old or inconsistent.\n"), stderr);
> +    }
> +
>    remove_error_table(&et_ext2_error_table);
>    exit (0);
> }
> -- 
> 1.7.1
> 
> --
> 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

Re: [PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[Jan Kara]

On Thu 28-07-11 10:44:30, Andreas Dilger wrote:
> On 2011-07-28, at 9:17 AM, Jan Kara <jack@suse.cz> wrote:
> > When dumpe2fs is used on a filesystem mounted read-write, printed information
> > may be old and inconsistent because e.g. updates of some superblock fields
> > happen lazily or because we do not really try to handle various intermediate
> > states of filesystem operations. Document this in the manpage and warn user
> > when dumpe2fs is used for such filesystem.
> 
> Jan, does this specifically relate to the free blocks or free inodes
> count?  My preference would actually be to have dumpe2fs print the
> correct values. I guess that means reading all of the group descriptors
> and adding up the free block/inode counts like the kernel does. 
  Yes, the number of free blocks and inodes are the main offenders,
although there are other fields like s_wtime, or s_kbytes_written that are
updated only during umount.

> I'm not against this patch in principle, since any operation on a mounted
> filesystem is inherently racy, but as it stands now the information
> printed by dumpe2fs can be completely useless. 
  Well, but OTOH in some cases it might be desirable to actually see the
real numbers stored and since dumpe2fs is mainly for debugging purposes I'd
rather have it's output as close to what's on disk as possible. If people
are interested in amount of free blocks / inodes, they should use statfs(2)
after all.  So I'd be reluctant to add some computations like you
suggest... What dumpe2fs might do is sync the filesystem (and do statfs())
to force most of the information to disk when it sees the filesystem is
mounted. I can update my patch to do this when people think it's desirable.
  
									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[Andreas Dilger]

On 2011-07-28, at 1:51 PM, Jan Kara wrote:
> On Thu 28-07-11 10:44:30, Andreas Dilger wrote:
>> Jan, does this specifically relate to the free blocks or free inodes
>> count?  My preference would actually be to have dumpe2fs print the
>> correct values. I guess that means reading all of the group descriptors
>> and adding up the free block/inode counts like the kernel does. 
> 
>  Yes, the number of free blocks and inodes are the main offenders,
> although there are other fields like s_wtime, or s_kbytes_written that are
> updated only during umount.
> 
>> I'm not against this patch in principle, since any operation on a mounted
>> filesystem is inherently racy, but as it stands now the information
>> printed by dumpe2fs can be completely useless. 
> 
>  Well, but OTOH in some cases it might be desirable to actually see the
> real numbers stored and since dumpe2fs is mainly for debugging purposes I'd
> rather have it's output as close to what's on disk as possible. If people
> are interested in amount of free blocks / inodes, they should use statfs(2)
> after all.  So I'd be reluctant to add some computations like you
> suggest... What dumpe2fs might do is sync the filesystem (and do statfs())
> to force most of the information to disk when it sees the filesystem is
> mounted. I can update my patch to do this when people think it's desirable.

It would be nice if there was a simple way to update the superblock with this data.  Calling sync() wouldn't be an unreasonable time to update the superblock, or even just doing it once an hour automatically from the kernel, or at the next time the filesystem is modified.  In particular, it makes sense to update s_kbytes_written periodically instead of just at unmount time, or in some cases it may never updated if the filesystem is never unmounted cleanly.

Cheers, Andreas

Re: [PATCH] dumpe2fs: Warn when filesystem is mounted read-write

[Ted Ts'o]

On Thu, Jul 28, 2011 at 05:17:04PM +0200, Jan Kara wrote:
> When dumpe2fs is used on a filesystem mounted read-write, printed information
> may be old and inconsistent because e.g. updates of some superblock fields
> happen lazily or because we do not really try to handle various intermediate
> states of filesystem operations. Document this in the manpage and warn user
> when dumpe2fs is used for such filesystem.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

I decided to move the warning in the man page to the DESCRIPTION
section, since it's really not a bug, but something which should be
self-evident.

And I also decided for that reasn to not have dumpe2fs print it.  If
users complain, we can now just tell them to RTFM.

      		       	   	     	  - Ted