Re: [PATCH 1/3] xfs: remove bitfield based superblock updates

[Dave Chinner <david@fromorbit.com>]

On Fri, Jan 09, 2015 at 03:35:01PM -0500, Brian Foster wrote:
> On Thu, Jan 08, 2015 at 08:51:03AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When we log changes to the superblock, we first have to write them
> > to the on-disk buffer, and then log that. Right now we have a
> > complex bitfield based arrangement to only write the modified field
> > to the buffer before we log it.
> > 
> > This used to be necessary as a performance optimisation because we
> > logged the superblock buffer in every extent or inode allocation or
> > freeing, and so performance was extremely important. We haven't done
> > this for years, however, ever since the lazy superblock counters
> > pulled the superblock logging out of the transaction commit
> > fast path.
> > 
> > Hence we have a bunch of complexity that is not necessary that makes
> > writing the in-core superblock to disk much more complex than it
> > needs to be. We only need to log the superblock now during
> > management operations (e.g. during mount, unmount or quota control
> > operations) so it is not a performance critical path anymore.
> > 
> > As such, remove the complex field based logging mechanism and
> > replace it with a simple conversion function similar to what we use
> > for all other on-disk structures.
> > 
> > This means we always log the entirity of the superblock, but again
> > because we rarely modify the superblock this is not an issue for log
> > bandwidth or CPU time. Indeed, if we do log the superblock
> > frequently, delayed logging will minimise the impact of this
> > overhead.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
....
> >  	/*
> > -	 * GQUOTINO and PQUOTINO cannot be used together in versions of
> > -	 * superblock that do not have pquotino. from->sb_flags tells us which
> > -	 * quota is active and should be copied to disk. If neither are active,
> > -	 * make sure we write NULLFSINO to the sb_gquotino field as a quota
> > -	 * inode value of "0" is invalid when the XFS_SB_VERSION_QUOTA feature
> > -	 * bit is set.
> > +	 * GQUOTINO and PQUOTINO cannot be used together in versions
> > +	 * of superblock that do not have pquotino. from->sb_flags
> > +	 * tells us which quota is active and should be copied to
> > +	 * disk. If neither are active, we should NULL the inode.
> >  	 *
> > -	 * Note that we don't need to handle the sb_uquotino or sb_pquotino here
> > -	 * as they do not require any translation. Hence the main sb field loop
> > -	 * will write them appropriately from the in-core superblock.
> > +	 * In all cases, the separate pquotino must remain 0 because it
> > +	 * it beyond the "end" of the valid non-pquotino superblock.
> >  	 */
> > -	if ((*fields & XFS_SB_GQUOTINO) &&
> > -				(from->sb_qflags & XFS_GQUOTA_ACCT))
> > +	if (from->sb_qflags & XFS_GQUOTA_ACCT)
> >  		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
> > -	else if ((*fields & XFS_SB_PQUOTINO) &&
> > -				(from->sb_qflags & XFS_PQUOTA_ACCT))
> > +	else if (from->sb_qflags & XFS_PQUOTA_ACCT)
> >  		to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
> > -	else {
> > -		/*
> > -		 * We can't rely on just the fields being logged to tell us
> > -		 * that it is safe to write NULLFSINO - we should only do that
> > -		 * if quotas are not actually enabled. Hence only write
> > -		 * NULLFSINO if both in-core quota inodes are NULL.
> > -		 */
> > -		if (from->sb_gquotino == NULLFSINO &&
> > -		    from->sb_pquotino == NULLFSINO)
> > -			to->sb_gquotino = cpu_to_be64(NULLFSINO);
> > -	}
> > +	else
> > +		to->sb_gquotino = cpu_to_be64(NULLFSINO);
> 
> FYI... it looks like the above hunk causes a regression due to resetting
> sb_gquotaino when one of the in-core inodes is set. I'm seeing
> disconnected quota inode messages on some xfstests (e.g., xfs/108) on v4
> filesystems. I'm about to put a patch on the list to go back to the
> original logic...

Interesting, I'm not seeing that in any of my v4 testing. hmmm - I
wonder if that's a result of using config sections and the test
device is not being remade with the changing format. I'll look into
it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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