Re: [PATCH 48/49] xfs: Add read-only support for dirent filetype field

[Ric Wheeler <rwheeler@redhat.com>]

On 08/14/2013 07:47 PM, Geoffrey Wehrman wrote:
> On Wed, Aug 14, 2013 at 05:50:42PM +1000, Dave Chinner wrote:
> | On Tue, Aug 13, 2013 at 10:42:32AM -0500, Mark Tinguely wrote:
> | > On 08/12/13 19:50, Dave Chinner wrote:
> | > >On Mon, Aug 12, 2013 at 08:25:23AM -0500, Mark Tinguely wrote:
> | > >>On 08/11/13 19:59, Dave Chinner wrote:
> | > >>>On Tue, Jul 30, 2013 at 02:10:32PM -0500, Mark Tinguely wrote:
> | > >>>>On 07/19/13 01:25, Dave Chinner wrote:
> | > >>>>>From: Dave Chinner<dchinner@redhat.com>
> | > >>>>>
> | > >>>>>Add support for the file type field in directory entries so that
> | > >>>>>readdir can return the type of the inode the dirent points to to
> | > >>>>>userspace without first having to read the inode off disk.
> | .....
> | > >>>>>Signed-off-by: Dave Chinner<dchinner@redhat.com>
> | > >>>>>---
> | > >>>>>
> | > >>>>
> | > >>>>>+static inline int xfs_sb_version_hasftype(struct xfs_sb *sbp)
> | > >>>>>+{
> | > >>>>>+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5&&
> | > >>>>>+		xfs_sb_has_incompat_feature(sbp, XFS_SB_FEAT_INCOMPAT_FTYPE);
> | > >>>>>  }
> | > >>>>>
> | > >>>>
> | > >>>>This feature should support inode version 2 and 3.
> | > >>>
> | > >>>Has nothing to do with the inode version number - it has to do with
> | > >>>the directory structure being modified.
> | > >>>
> | > >>>We're changing the directory structure for CRCs, and this builds on
> | > >>>top of that. It is essentially part of the V3 directory format, and
> | > >>>should be treated as such. Suggesting that we retrofit and support a
> | > >>>modified v2 directory format is close to insane - instead of only
> | > >>>having to test v2 vs v3 directory formats, you're suggesting we
> | > >>>support v2 vs v2+dtype vs v3 vs v3+dtype. We simply do not have the
> | > >>>resources to adequately test and support such an explosion of
> | > >>>filesystem configurations.
> | > >>>
> | > >>>We've had this discussion before - new on-disk features go into the
> | > >>>v5 superblock format - the v4 superblock format from this point
> | > >>>onwards is essentially legacy support from an upstream development
> | > >>>perspective.
> | ....
> | > >>>That said, there's nothing to stop anyone from backporting such a
> | > >>>feature to an older kernel and maintaining it themselves - it's open
> | > >>>source software. But the idea that development should be constrained
> | > >>>by having to support both old and new formats is wrong - the old v4
> | > >>>format should be considered stable and we need think very hard about
> | > >>>changing it at all now, especially as much of the development focus
> | > >>>is now shifting to taking advantage of the additions to the v5
> | > >>>format....
> | > >>
> | > >>I guess we need more time to argue this out. It is not going into
> | > >>Linux 3.12 as a crc feature only.
> | > >
> | > >Seriously?
> | >
> | > yes seriously.
> |
> | Great, another random roadblock from the XFS maintainers to deal
> | with.
>
> The addition of the file type field to directory entries is a great
> new feature.  Your implementation of adding a "hidden" byte to the name
> field is especially clever.  This is a feature that can benefit both
> dir2 and dir3 format filesystems and is completely independent of your
> CRC and self describing metadata feature work.  I understand that you
> are not interested in porting the capability to dir2 format filesystems
> yourself and do not have the resources to provide the associated testing
> and support.  Myself and others within SGI have discussed these issues,
> and we are willing to take on the work ourselves rather than have this
> feature go only into v5 superblock filesystems where the feature is only
> accessible to those who are willing to risk using experimental code.
> Given that SGI will be doing the work to support the file type field
> in dir2 format filesystems, it doesn't make sense to add the code to
> v5 filesystems until all of the work is complete as there could be
> additional considerations for the on disk changes.
>
> We also noted that this feature should not be added to the kernel until
> userspace code is available to support this feature.  Specifically,
> xfs_repair needs to validate and if needed repair the the file type field.
> Also, tests are needed to validate the new functionality.  While I
> expect that you will provide this support for v5 superblock and dir3
> filesystems, one of us at SGI will extend the support to v4 superblock
> and dir2 filesystems.

Hi Geoffrey,

As much as the community admires your brave willingness to protect us from code 
that was entirely developed by Dave, that is not really how the upstream kernel 
works.

Dave is pretty much without equal in moving XFS along these days and this is a 
key feature that we are depending on.

Please don't try to create hurdles to other people getting work into upstream, 
that kind of thing will lead to a fork of the XFS code base and ultimately that 
will be harder for all of us.

It sounds like what you really should look at doing is to work inside of SGI to 
create a private, internal branch of the upstream code that you deliver in your 
product. Upstream code is all about innovation and new features, we don't let 
vendor specific, non-upstream branches become the place for hardening our code.

Best regards,

Ric


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs