Re: [PATCH 2/2] ext4: journal superblock modifications in ext4_statfs()

[Andreas Dilger <adilger@sun.com>]

On 2009-11-06, at 15:33, Eric Sandeen wrote:
> commit a71ce8c6c9bf269b192f352ea555217815cf027e updated
> ext4_statfs() to update the on-disk superblock counters,
> but modified this buffer directly without any journaling of
> the change.  This is one of the accesses that was causing the crc  
> errors in journal replay as seen in kernel.org bugzilla #14354.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
>
> @@ -3650,20 +3652,33 @@ static int ext4_statfs(struct dentry  
> *dentry, struct kstatfs *buf)
> +	handle = ext4_journal_start_sb(sb, 1);
> +	if (IS_ERR(handle)) {
> +		err = PTR_ERR(handle);
> +		goto out;
> +	}
> +	err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh);
> +	if (err)
> +		goto journal_stop;
> +	es->s_free_inodes_count = cpu_to_le32(buf->f_ffree);
> +	ext4_free_blocks_count_set(es, buf->f_bfree);
> +	err = ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);


I admit to being the instigator of this change.

The intention is that we want to update the on-disk superblock block/ 
inode
counters from the per-cpu data periodically, since they are never  
updated
anymore (only the group summaries are updated, to avoid contention).   
However,
this isn't critical work, since it is only useful for read-only e2fsck  
not
reporting spurious errors on the filesystem and dumpe2fs/debugfs  
having some
chance at reporting a reasonable value for the filesystem space usage.

Starting a transaction as part of statfs is really counter-productive  
to making
that code efficient, which was the whole point of the original patch  
to remove
the per-call "overhead" calculation.

The intention was that the in-memory superblock would be updated  
whenever statfs
is called (this doesn't cost anything, since we've already computed  
the value
for statfs), and if the superblock is written to disk for some other  
reason they
will go along for the ride.

If the choice is between adding a proper transaction here, or not  
doing this at
all, I'd rather just not do it at all.  Of course, I'd like to work  
out some
kind of compromise, like only updating the superblock when there is  
already a shadow BH that is being used to write to the journal, or  
similar.

If there is a desire to keep a transaction here and update the  
superblock
counters, it _definitely_ doesn't need to be done on every statfs, but  
at most
once every 30 seconds or whatever.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.