xfs
[Top] [All Lists]

Re: [PATCH 08/11] xfs: don't emit v5 superblock warnings on write

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 08/11] xfs: don't emit v5 superblock warnings on write
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 24 May 2013 09:13:32 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130523152338.GS20028@xxxxxxx>
References: <1369123330-9579-1-git-send-email-david@xxxxxxxxxxxxx> <1369123330-9579-9-git-send-email-david@xxxxxxxxxxxxx> <20130522222608.GR20028@xxxxxxx> <20130523000327.GQ29466@dastard> <20130523152338.GS20028@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, May 23, 2013 at 10:23:38AM -0500, Ben Myers wrote:
> On Thu, May 23, 2013 at 10:03:27AM +1000, Dave Chinner wrote:
> > On Wed, May 22, 2013 at 05:26:08PM -0500, Ben Myers wrote:
> > > On Tue, May 21, 2013 at 06:02:07PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > > 
> > > > We write the superblock every 30s or so which results in the
> > > > verifier being called. Right now that results in this output
> > > > every 30s:
> > > > 
> > > > XFS (vda): Version 5 superblock detected. This kernel has EXPERIMENTAL 
> > > > support enabled!
> > > > Use of these features in this kernel is at your own risk!
> > > > 
> > > > And spamming the logs. Stop this output from occurring on superblock
> > > > writes.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > > > ---
> > > >  fs/xfs/xfs_mount.c |   18 +++++++++++-------
> > > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > > index f6bfbd7..e8e310c 100644
> > > > --- a/fs/xfs/xfs_mount.c
> > > > +++ b/fs/xfs/xfs_mount.c
> > > > @@ -314,7 +314,8 @@ STATIC int
> > > >  xfs_mount_validate_sb(
> > > >         xfs_mount_t     *mp,
> > > >         xfs_sb_t        *sbp,
> > > > -       bool            check_inprogress)
> > > > +       bool            check_inprogress,
> > > > +       bool            check_version)
> > > >  {
> > > >  
> > > >         /*
> > > > @@ -337,9 +338,10 @@ xfs_mount_validate_sb(
> > > >  
> > > >         /*
> > > >          * Version 5 superblock feature mask validation. Reject 
> > > > combinations the
> > > > -        * kernel cannot support up front before checking anything else.
> > > > +        * kernel cannot support up front before checking anything 
> > > > else. For
> > > > +        * write validation, we don't need to check feature masks.
> > > >          */
> > > > -       if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
> > > > +       if (check_version && XFS_SB_VERSION_NUM(sbp) == 
> > > > XFS_SB_VERSION_5) {
> > > 
> > > if (!quiet_version) {
> > > >                 xfs_alert(mp,
> > > >  "Version 5 superblock detected. This kernel has EXPERIMENTAL support 
> > > > enabled!\n"
> > > >  "Use of these features in this kernel is at your own risk!");
> > > 
> > > }
> > > 
> > > Since the stated goal of the patch is to be quieter and not to disable 
> > > useful
> > > tests in the verifier, I suggest you disable the print rather than 
> > > disable the
> > > test.
> > 
> > Checking the feature fields for whether the kernel supports the
> > features in them on write is not useful in any way.
> 
> Could it not detect corruption of the feature flags before they're written 
> out?
> My impression was that this was among the design goals of the verifiers.

It might, but we don't verify any of the existing feature flags,
either. For that matter, how do you verify them when the in-core
superblock can be modified asynchronously to the on-disk version in
the buffer without locking the incore superblock?

And if we do lock the incore superblock, how do we deal with the
lock inversion that this causes?

> > That's why the
> > variable is named "check_version" because it skips the v5
> > version field checking. This is stuff that is used by the mount
> > path (i.e. superblock read path), not the writeback path.
> 
> You were saying above that it's the write path that is spamming the logs.

The function is used by both the read and the write verifiers, but
the feature mask checks are only relevant to the read path which
occurs during mount. Doing the checks on write is what is spamming
the logs.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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