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

[Dave Chinner]

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:
> > On Mon, Aug 04, 2014 at 08:48:36AM -0400, Brian Foster wrote:
> > > On Mon, Aug 04, 2014 at 06:09:30PM +1000, Dave Chinner wrote:
> > > > On Fri, Aug 01, 2014 at 10:39:29AM -0400, Brian Foster wrote:
> > > Fair point, an sb modification is something that should stand out. I
> > > think the characteristic of the new api is somewhat subtle, however.
> > 
> > Which is why there's a comment explaining it. Is there anything I
> > can add to that comment to make it clearer?
> > 
> 
> I would change this:
> 
> /*
>  * ...
>  *
>  * Note this code can be called during the process of freezing, so
>  * we may need to use the transaction allocator which does not
>  * block when the transaction subsystem is in its frozen state.
>  */
> 
> ... to something like:
> 
> /*
>  * ...
>  *
>  * Note that the caller is responsible for checking the frozen state of
>  * the filesystem. This procedure uses the non-blocking transaction
>  * allocator and thus will allow modifications to a frozen fs. This is
>  * required because this code can be called during the process of
>  * freezing where use of the high-level allocator would deadlock.
>  */

OK, I can do that.

> > > > > I'm not sure of the mechanics behind that, but I'm
> > > > > guessing some kind of reference remains on the sb of a frozen fs and a
> > > > > subsequent umount/mount is purely namespace magic. Point being... this
> > > > > appears to be implicit and confusing. IMO, using an _xfs_sync_sb()
> > > > > variant that allocates a nonblocking tp if one isn't provided as a
> > > > > parameter (for example) and using that only in the contexts we know it's
> > > > > Ok to avoid freeze interaction issues might be more clear.
> > > > 
> > > > Well, it was pretty clear to me that the code paths were free of
> > > > freeze interactions. Looking at this - especially the quota on/off
> > > > paths - I guess it's not as obvious as I thought it was... :/
> > > > 
> > > 
> > > My point was more geared towards future use. E.g., we have frozen fs
> > > management built into transaction management, which is nice and clean
> > > and easy to understand.
> > 
> > Actually, it's nowhere near as clean as you think. :/
> > 
> > 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).

Did I mention this code is not at all obvious? :/

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com