Re: [PATCH] 2.5.x write_super is not for syncing

[Chris Mason <mason@suse.com>]

On Mon, 2002-12-02 at 19:03, Andrew Morton wrote:
> Chris Mason wrote:
> > 
> > On Mon, 2002-12-02 at 17:07, Andrew Morton wrote:
> > 
> > > Except for the s_dirt test in here.  If s_dirt is zero and we
> > > have dirty inodes, the filesystem _still_ is not told what to
> > > do.
> > 
> > Perhaps dirty inodes imply s_dirt should be true?
> 
> Well afaik (why doesn't anybody ever comment stuff?  Do they prefer
> buggy software?) s_dirt purely means "the superblock needs to be
> written out".

Sorry, neither of my comments were horribly clear.  Regardless of
s_dirt's current definition, s_dirt could mean 'something on the FS
needs to be written, and only the FS knows how to do it', or some other
wording that covers both the 'call me for periodic writes' case and the
'call me on sync()' case.  

I'm not saying we really need to muddy the s_dirt definition like that,
it's just an option in case someone really wants s_dirt checks
everywhere.

> > > That's a bit drastic.  At a minimum we need to remove that s_dirt
> > > test and make the commit_super() call unconditional.
> > >
> > > What would that break?
> > 
> > Not much, since foofs_commit_super could always check for s_dirt if the
> > local FS really cared.
> > 
> > The downside is that since we don't check for s_dirt in commit_supers(),
> > the FS might get sunk twice if the we need to restart at the head of the
> > supers list due to unmount.  But, a double sync is possible anyway under
> > FS load in the same situation.
> > 
> 
> But you do check:

Right, I meant that if we change the patch to remove the s_dirt check
from the commit_supers() function, a single FS might get sunk twice. 
That risk already exists when the FS is under load though, so I'm not
horribly worried about it.

> I'm thinking that for this particular problem, the solution isn't
> quite appropriate.  We don't really give a toss about the dirty
> state of the superblock.  We want to sync the darn filesystem.
> Sure, we can do that in ->commit_super(), but it really has nothing
> to do with the superblock.
> 
> What about something along these lines?

You'd still want sync_filesystem to get calls in the sys_sync case
though, right?  I'd be happy to rework the commit_super patch into a
sync_filesystem patch that doesn't bother with s_dirt except when
calling write_super().

The patch has two parts that matter in my mind:

1) In sync_supers (and other similar funcs), allow the FS to leave
s_dirt set.  This way we can setup per FS timings on when periodic
writes get done.

2) Give the FS a way to tell the difference between periodic writes that
don't need to force and wait for a commit, and a request from the user
that does require a full commit.

-chris