xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 1/3] xfs: remove bitfield based superblock updates
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 10 Jan 2015 10:36:51 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150109203501.GA2233@xxxxxxxxxxxxxx>
References: <1420667465-7453-1-git-send-email-david@xxxxxxxxxxxxx> <1420667465-7453-2-git-send-email-david@xxxxxxxxxxxxx> <20150109203501.GA2233@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>