Re: [PATCH] ext4: fix overhead calculation in bigalloc filesystem (Re: ... )

[Zheng Liu <gnehzuil.liu@gmail.com>]

On Fri, Feb 22, 2013 at 10:20:42AM -0500, Theodore Ts'o wrote:
> On Fri, Feb 22, 2013 at 09:18:12PM +0800, Zheng Liu wrote:
> > > If there was a mode that I'd be tempted to get rid of, it would be the
> > > combined data=ordered/data=writeback modes.  The main reason why we
> > > keep it is because of a concern of buggy applications that depend on
> > > the implied fsync() of data=ordered mode at each commit.  However,
> > > ext4 has been around for long enough that I think most of the buggy
> > > applications have been fixed by now.  And of course, those buggy
> > > applications will lose in the same way when they are using btrfs and
> > > xfs.  Something to consider....
> > 
> > IMHO, the application shouldn't depend on a kernel feature.  So maybe it
> > is time to highlight this buggy usage.
> 
> Oh, agreed.  The question is how many people want us to keep the ext3
> semantics to support those buggy applications.  To the extent that
> distros are considering using ext4 to support ext3 file systems, there
> might be a desire to maintain (as closely as possible) ext3 semantics,
> even those that support buggy applications.  The primary problem is
> that the when comes down to application developers versus file system
> developers, the application developers vastly outnumber us.   :-)

Yes, as file system developers we always need to meet the application
developers' requirement.  So it seems that we still need to keep it in
ext4. :-)

> 
> > Just one minor comment below.  Otherwise the patch looks good to me, and
> > it can pass in xfstests with 'data=ordered' and 'data=writeback'.
> 
> I hadn't bothered testing it yet because I'm focused on testing
> and cleaning up the set of patches for the merge window --- and this
> change is clearly for the next merge window.  Thanks for testing it!

I guess you are busy testing patches for the merge window.  One thing I
need to let you know is this patch [1].  I really think it should be
applied for this merge window because it fixes a security hole due to my
fault.  As commit log describes, a non-privilege user could cause system
crash using a truncate(1) command.  So please check it.

1. http://www.spinics.net/lists/linux-ext4/msg36784.html

> 
> > >  	trace_ext4_ordered_write_end(inode, pos, len, copied);
> > 
> > Here this function needs to be renamed with trace_ext4_write_end().
> 
> Yes, agreed.
> 
> I also need to get rid of trace_ext4_writeback_write_end() in
> include/trace/events/ext4.h.
> 
> The other thing that needs to be done --- probably in a separate
> commit, just to make things easier to review for correctness, is now
> that we've folded ext4_writeback_write_end() and ext4_ordered_write_end()
> into a single function, we now have a single user of
> ext4_generic_write_end(), so we can now fold ext4_generic_write_end()
> into ext4_write_end().

Yes, we can take a close look at in next merge window.

Thanks,
                                                - Zheng