xfs
[Top] [All Lists]

Re: [PATCH 10/21] xfs: add CRC checks to remote symlinks

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 10/21] xfs: add CRC checks to remote symlinks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 21 Mar 2013 12:32:43 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130320220334.GN22182@xxxxxxx>
References: <1363091454-8852-1-git-send-email-david@xxxxxxxxxxxxx> <1363091454-8852-11-git-send-email-david@xxxxxxxxxxxxx> <20130320220334.GN22182@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 20, 2013 at 05:03:34PM -0500, Ben Myers wrote:
> On Tue, Mar 12, 2013 at 11:30:43PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Add a header to the remote symlink block, containing location and
> > owner information, as well as CRCs and LSN fields. This requires
> > verifiers to be added to the remote symlink buffers for CRC enabled
> > filesystems.
> > 
> > This also fixes a bug reading multiple block symlinks, where the second 
> > block
> > overwrites the first block when copying out the link name.
> 
> More comments below.
....
> 
> >   */
> >  STATIC void
> >  xfs_bmap_local_to_extents_init_fn(
> > @@ -1332,23 +1334,12 @@ xfs_bmap_local_to_extents_init_fn(
> >     struct xfs_inode        *ip,
> >     struct xfs_ifork        *ifp)
> >  {
> > +   ASSERT(0);
> 
> That has nothing to do with crcs.

It most definitely does. This path is completely invalid to take now
that CRCs are enabled for remote symlink blocks.

Every caller through this function requires a transformation of
opaque data to block format. For CRC enable blocks, that means a
type specific handler *must* be used for header initialisation and
copying the data. Hence for debug kernels we want to know
immediately if this path is *ever* taken, because it is indicative
of an unhandled local-to-extent format transition out of
xfs_bmapi_write().

This is *exactly* the path that caused the bmbt verifier failures on
remote symlink buffers in 3.8.0. Once I've had time to study the
code more thoroughly, I'll probably remove the entire
local-to-extents path from xfs_bmapi_write() because there should be
no existing users of the path and callers should do the format
transformation themselves as is done right now....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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