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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
....
> > /*
> > - * 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
