From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Mason Subject: Re: [PATCH] 2.5.x write_super is not for syncing Date: 02 Dec 2002 20:10:24 -0500 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <1038877824.3040.133.camel@tiny> References: <3DEBD984.BACA3099@digeo.com> <1038872444.13527.104.camel@tiny> <3DEBF4C8.A68F4F8B@digeo.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: "linux-fsdevel@vger.kernel.org" Return-path: To: Andrew Morton In-Reply-To: <3DEBF4C8.A68F4F8B@digeo.com> List-Id: linux-fsdevel.vger.kernel.org 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