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@xxxxxxxxxxxxx > > _______________________________________________ > xfs mailing list > xfs@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
