Re: [PATCH 0/2] xfs: defrag support for v5 filesystems

Dave Chinner
2013-09-03T22:45:51Z

On Tue, Sep 03, 2013 at 02:12:01PM -0500, Ben Myers wrote:
> Hey Dave,
>
> On Fri, Aug 30, 2013 at 10:23:43AM +1000, Dave Chinner wrote:
> > Hi folks,
> >
> > The following 2 patches implement the BMBT owner change transaction
> > that is necessary to enable the XFS_IOC_SWAPEXT ioctl to operate on
> > v5 filesystems correctly. The first patch implements the
> > transactional runtime change, and the second patch implements the
> > recovery of that change.
> >
> > Both the run time and recovery code use the same mechanism for
> > changing the owner field in all the blocks in the BMBT on an inode,
> > and even though XFS_IOC_SWAPEXT only swaps the data fork, the code
> > has been written to be fork neutral so if we even need to swap
> > attribute forks it should just work for that, too.
> >
> > Further, because the BMBT code uses the generic btree
> > infrastructure, the btree modification is done as a generic function
> > as well and so should work for all types of btrees supported by the
> > generic code. Hence if the need arises we can easily change the
> > owner of any btree that uses the generic code.
> >
> > The testing carried out is documented in the description of the
> > second patch.
> >
> > AFAIA, this is the only remaining feature that the kernel v5
> > filesystem implementation didn't support. Hence, with this patchset,
> > there are no more feature checkboxes that need to be ticked that
> > would prevent us from removing the experimental tag from it. Testing
> > is the only remaining gate to removing the tag from the kernel
> > code...
>
> I believe there is still the discussion surrounding being able to use
> the self describing metadata without enabling crcs that needs to be
> resolved before removing the experimental tag.

There is no discussion to be had here - CRCs are not optional on v5
filesystems, nor is there any reason to make them optional. Please
stop bring this up over and over again - you're just wasting my time
by making me have to respond with the same answers over and over
again.

If people don't want CRCs, then we've still got a perfectly good v4
filesystem format that they can use.

> Some customers will want
> to use features such as t10-dif and won't want to calculate two separate
> crcs.

This is a straw man argument.

T10-dif is a completely different layer of protection that is only
useful for filesystem metadata if the CRC we calculate for the
metadata is written into the T10-DIF CRC fields. This is the only
way for us to get full end-to-end protection for metadata from
T10-dif. i.e. we have to supply the CRCs ourselves before we issue
the write IO, and verify it ourselves after a read IO.

Guess what we do right now with CRC support?

That's right: the existing CRC infrastructure is ready to support
integrated, end-to-end T10 CRCs for metadata in the filesystem. All
that is missing is the block layer interfaces and a few changes to
the CRC code to do iterative per-sector CRCs rather than
per-filesystem object CRCs. Surprise you, it may, but I've actually
considered how to implement T10-dif support as part of the overall
metadata CRC infrastructure architecture...

Given that with T10-dif we still need calculate and verify the
CRCs ourselves, why would we not also store it in the filesystem
metadata at the same time? That would mean that tools like
xfs_repair and xfs_db can also verify the metadata as being correct
without needing to explicitly support T10-dif. Of course, if you
want them to be able to repair or modify a filesystem with t10-dif
enabled, we need feature bits and explicit userspace support, too.

Hence, before making strawman arguments about how filesystem
metadata CRCs will need to be turned off for t10-dif support,
perhaps it would be better to first consider a design and prototype
support for end-to-end T10-dif CRCs for filesystem metadata and
share that with us?

As I've said before, we do not make on disk format changes for
strawmen or "potential" features that have not had design documents
or code published - we make the changes when code arrives to
implement the feature. So, until you have code for a feature that
*fundamentally requires* CRCs to be disabled to function correctly,
then the is not point even starting a discussion about making CRCs
optional on v5 superblocks.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com