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

[Dave Chinner <david@fromorbit.com>]

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?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs