Re: [PATCH 21/25] xfs: scrub extended attributes

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 21/25] xfs: scrub extended attributes
Date: Mon, 9 Oct 2017 14:14:19 -0700	[thread overview]
Message-ID: <20171009211419.GV7122@magnolia> (raw)
In-Reply-To: <20171009021312.GC3666@dastard>

On Mon, Oct 09, 2017 at 01:13:12PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 01:43:02PM -0700, Darrick J. Wong wrote:
> > +/* Extended Attributes */
> > +
> > +struct xfs_scrub_xattr {
> > +	struct xfs_attr_list_context	context;
> > +	struct xfs_scrub_context	*sc;
> > +};
> 
> A comment here explaining that we are using the listattr callback
> infrastructure to scrub the xattr?

New comments:

/*
 * Check that an extended attribute key can be looked up by hash.
 *
 * We use the XFS attribute list iterator (i.e. xfs_attr_list_int_ilocked)
 * to call this function for every attribute key in an inode.  Once
 * we're here, we load the attribute value to see if any errors happen,
 * or if we get more or less data than we expected.
 */

...comes before the definition of xfs_scrub_xattr_listent, and...

/*
 * Look up every xattr in this file by name.
 *
 * Use the backend implementation of xfs_attr_list to call
 * xfs_scrub_xattr_listent on every attribute key in this inode.
 * In other words, we use the same iterator/callback mechanism
 * that listattr uses to scrub extended attributes, though in our
 * _listent function, we check the value of the attribute.
 *
 * The VFS only locks i_rwsem when modifying attrs, so keep all
 * three locks held because that's the only way to ensure we're
 * the only thread poking into the da btree.  We traverse the da
 * btree while holding a leaf buffer locked for the xattr name
 * iteration, which doesn't really follow the usual buffer
 * locking order.
 */

...comes right before the call to xfs_attr_list_int_ilocked.

> And, now that I've got to the rest of the code, that we don't validate
> the pointers/values in the attribute records when doing dabtree record
> check because we are doing that indirectly afterwards by reading every
> attribute value.

Correct.

> And that this follows the pointers for remote attr blocks and reads
> them, hence checking the remote attr is valid via verifiers?

Correct.

> And, with that out of the way, what about attributes that listent
> skips?

Oops, I forgot those.

> i.e. those with the flag that says they are not valid?  We don't check
> they exist or are valid, and their existence would be a case for
> preening the xattr tree...

Good point, I'll add those.

> Otherwise this seems pretty straight forward...

Woot.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html