[PATCH 2/6] xfs: consolidate superblock logging functions

[Brian Foster]

On Wed, Aug 06, 2014 at 05:59:06AM +1000, Dave Chinner wrote:
> On Tue, Aug 05, 2014 at 08:30:51AM -0400, Brian Foster wrote:
> > On Tue, Aug 05, 2014 at 10:34:40AM +1000, Dave Chinner wrote:
> > > On Mon, Aug 04, 2014 at 08:03:33PM -0400, Brian Foster wrote:
> > > > On Tue, Aug 05, 2014 at 08:15:26AM +1000, Dave Chinner wrote:
> > > > > e.g. did you know that the xfs_fs_writable() check in
> > > > > xfs_log_sbcount() is to prevent it from writing anything when
> > > > > unmounting a fully frozen filesystem? i.e. xfs_log_sbcount needs to
> > > > > succeed while a freeze is in progress, but fail when a freeze is
> > > > > fully complete?
> > > > > 
> > > > 
> > > > Hmm, so freeze_super() sets s_frozen to SB_FREEZE_FS before it calls
> > > > into the fs. Given the xfs_fs_writable() logic, how is that going to
> > > > differentiate a freezing fs from a frozen fs? It makes sense that this
> > > > would avoid blocking on umount of a frozen fs, but it seems like we'd
> > > > skip out just the same during the freeze sequence. Maybe I'm missing
> > > > something...
> > > 
> > > Hmmm - that means we broke it at some point. xfs_attr_quiesce is
> > > supposed to make the metadata uptodate on disk, so if it's not
> > > updating the superblock (i.e. syncing all the counters) then it's
> > > not doing the right thing - the sb counters on disk while the fs is
> > > frozen are not uptodate and hence correct behaviour if we crash with
> > > a frozen fs is dependent on log recovery finding a dirty log. That's
> > > a nasty little landmine and needs to be fixed, even though it's not
> > > causing issues at the moment (because we dirty the log after
> > > quiescing the filesystem).
> > > 
> > 
> > I'm wondering if that even helps in the case of a crash. It looks like
> > we would skip the counter sync and subsequent action of logging the sb
> > entirely.
> > 
> > Oh, according to the lazy sb counter commit log description we do some
> > kind of counter rebuild across the AGI/AGF structures and log the result
> > of that. So I take it that should a crash occur while in the frozen
> > state, the simple act of causing a log recovery to occur on subsequent
> > mount should rebuild everything correctly.
> 
> Right - it's log recovery that is hiding that little gem. We've been
> talking about whether we can change freeze to leave the log clean
> and so avoid the need for log recovery in snapshot images. If we
> did that, then we'd have exposed this bug....
> 
> > > Did I mention this code is not at all obvious? :/
> > > 
> > 
> > Heh. :P From what I can see, it looks like this has been the case since
> > commit 92821e2b, which introduced xfs_log_sbcount().
> 
> *nod*
> 
> > Perhaps xfs_log_sbcount() requires an open coded s_frozen check a la
> > the _xfs_trans_alloc() logic. E.g., skip out of SB_FREEZE_COMPLETE,
> > proceed otherwise..?
> 
> Possibly. But it still also needs the RO and shutdown checks.
> Perhaps passing xfs_fs_writable() a freeze level and checking
> against that?
> 

Right.. I was thinking of open coding the whole thing and modifying the
freeze check. Using a param to xfs_fs_writable() sounds generally nicer
though and we can prevent any future landmines over 'if
(...->s_writers.frozen)' logic. I'll give that a whirl.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david at fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs