From: Eric Sandeen <sandeen@sandeen.net>
To: Geoffrey Wehrman <gwehrman@sgi.com>
Cc: Ric Wheeler <rwheeler@redhat.com>,
Mark Tinguely <tinguely@sgi.com>,
xfs@oss.sgi.com
Subject: Re: [PATCH 48/49] xfs: Add read-only support for dirent filetype field
Date: Tue, 20 Aug 2013 14:47:36 -0500 [thread overview]
Message-ID: <5213C7D8.5020603@sandeen.net> (raw)
In-Reply-To: <20130816140811.GA30422@sgi.com>
On 8/16/13 9:08 AM, Geoffrey Wehrman wrote:
> I would like to take this opportunity to point out that SGI's
> attempts to contribute code to XFS are frequently blocked by Red
> Hat without technical merit. Most recently we tried to submit
> code for the agskip mount option which SGI has been shipping for
> years. [http://oss.sgi.com/archives/xfs/2013-01/msg00561.html]
Honestly, shipping it for years was SGI's first mistake. At that
point you can't respond to upstream reviews, because you are committed
to the patch as-is - it's already in the field. You can't
argue from technical points; you need it merged because you shipped it.
So all you can ask for is thumbs up or thumbs down; you got thumbs down.
Not going upstream first will almost always burn you; now you have
extra maintenance burden forever. Which takes time away from your
upstream presence, etc, etc.
> We asked for fields to be reserved in the new v3 inodes for parent
> pointers and allocation policies. That request was soundly rejected
> despite the existence of unreserved padding in the new inode format.
> [http://oss.sgi.com/archives/xfs/2013-04/msg00214.html] The response
> to code submitted by SGI has been so negative
The response, IIRC, was that we shouldn't reserve fields for
unreviewed designs from the future. That's sound practice, IMHO,
and I agree.
extN has weird, old, unused structure fields, because this principle
wasn't followed. I've seen the counterexample, and it sucks.
TBH, if I went to btrfs or ext4, and said hey, please add these fields
because I have a plan, they'd have told me no as well, and to come
back with a patch which implements the plan, for review.
> , we don't even bother
> submitting most of our code to the list anymore as long as it does not
> affect the on-disk format.
That really is unfortunate. See above about how that is only going to
hurt you, I'm afraid.
> The list of features and capabilities that we
> carry in our internally maintained source trees is significant and long:
> DMAPI, behavior chains, agskip mount option, ibound mount option, etc.
> These are all features that have been rejected by the external community
> but are of value to SGI customers.
Sure, but their mere existence in your tree doesn't mean that it's the
best solution. Upstream review & discussion can be arduous, but it
finds & fixes things. Code written & shipped w/o 3rd party review
tends to have more rough edges than if it had been wrung through
8 versions.
"Note: The ibound mount option is not compatible with the inode64
mount option. If you specify both options, the mount(8) command will
ignore the first option specified. "
Review might have cleaned up that behavior. :)
> A reiterate my appreciation of Red Hat's contributions to XFS. However, I
> hope that you and others at Red Hat recognize that Red Hat is not the sole
> source for innovation and contributions to XFS. The playing field must be
> kept level and everyone in the community must be allowed to participate.
Asking for justification, testing, adherence to best practices, patch
modification cycles, hard questioning, design review, etc is the standard
to which everyone should be held. It's how xfs has been developed upstream
for years, and what has made it the premier Linux filesystem. We owe
each other courtesy during the process, but hard questions and a high bar
should not be perceived as discourtesy. If it really does get personal,
and sometimes it does despite all our efforts, then point it out, and people
should reset the discussion to the technical side.
Look, I'm trying to be master of 3 or more different Linux filesystems -
and probably doing a poor job of it. But the one thing that has impressed
me about XFS apart from the others is that for the most part, bad code doesn't
get through. On-list review is rigorous, and code is held to very high
standards. You have to be willing to get behind your proposal and make good
technical arguments, and see it through to the end - or sometimes, lose the
argument and regroup.
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-08-20 19:47 UTC|newest]
Thread overview: 158+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-19 6:24 [PATCH 00/49] current patch queue for 3.12 Dave Chinner
2013-07-19 6:24 ` [PATCH 01/49] xfs: separate out log format definitions Dave Chinner
2013-07-23 14:00 ` Brian Foster
2013-07-19 6:24 ` [PATCH 02/49] xfs: split out inode log item format definition Dave Chinner
2013-07-23 14:00 ` Brian Foster
2013-07-30 16:20 ` Mark Tinguely
2013-08-01 8:50 ` Christoph Hellwig
2013-07-19 6:24 ` [PATCH 03/49] xfs: split out buf log item format definitions Dave Chinner
2013-07-23 14:01 ` Brian Foster
2013-07-19 6:24 ` [PATCH 04/49] xfs: split out inode log item format definition Dave Chinner
2013-07-23 14:01 ` Brian Foster
2013-07-19 6:24 ` [PATCH 05/49] xfs: separate dquot on disk format definitions out of xfs_quota.h Dave Chinner
2013-07-24 12:09 ` Brian Foster
2013-08-01 8:51 ` Christoph Hellwig
2013-08-02 1:44 ` Dave Chinner
2013-07-19 6:24 ` [PATCH 06/49] xfs: separate icreate log format definitions from xfs_icreate_item.h Dave Chinner
2013-07-24 12:09 ` Brian Foster
2013-07-19 6:24 ` [PATCH 07/49] xfs: split out on-disk transaction definitions Dave Chinner
2013-07-24 12:09 ` Brian Foster
2013-07-19 6:24 ` [PATCH 08/49] xfs: introduce xfs_rtalloc_defs.h Dave Chinner
2013-07-24 12:09 ` Brian Foster
2013-07-19 6:24 ` [PATCH 09/49] xfs: introduce xfs_quota_defs.h Dave Chinner
2013-07-25 12:54 ` Brian Foster
2013-07-19 6:24 ` [PATCH 10/49] xfs: sync minor header differences needed by userspace Dave Chinner
2013-07-25 12:54 ` Brian Foster
2013-07-19 6:24 ` [PATCH 11/49] xfs: split out transaction reservation code Dave Chinner
2013-07-25 12:55 ` Brian Foster
2013-07-19 6:24 ` [PATCH 12/49] xfs: move inode fork definitions to a new header file Dave Chinner
2013-07-25 20:40 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 13/49] xfs: move unrealted definitions out of xfs_inode.h Dave Chinner
2013-07-25 19:24 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 14/49] xfs: introduce xfs_inode_buf.c for inode buffer operations Dave Chinner
2013-07-25 19:17 ` Mark Tinguely
2013-07-26 0:24 ` Dave Chinner
2013-07-26 12:41 ` Brian Foster
2013-07-19 6:24 ` [PATCH 15/49] xfs: move getdents code into it's own file Dave Chinner
2013-07-26 13:00 ` Brian Foster
2013-07-19 6:24 ` [PATCH 16/49] xfs: reshuffle dir2 definitions around for userspace Dave Chinner
2013-07-26 13:18 ` Brian Foster
2013-07-19 6:24 ` [PATCH 17/49] xfs: split out attribute listing code into separate file Dave Chinner
2013-07-27 20:23 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 18/49] xfs: split out attribute fork truncation " Dave Chinner
2013-07-27 19:25 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 19/49] xfs: split out the remote symlink handling Dave Chinner
2013-07-27 19:48 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 20/49] xfs: introduce xfs_sb.c for sharing with libxfs Dave Chinner
2013-07-27 17:54 ` Mark Tinguely
2013-07-28 1:08 ` Dave Chinner
2013-07-19 6:24 ` [PATCH 21/49] xfs: create xfs_bmap_util.[ch] Dave Chinner
2013-07-27 17:57 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 22/49] xfs: minor cleanups Dave Chinner
2013-07-27 18:00 ` Mark Tinguely
2013-07-28 1:07 ` Dave Chinner
2013-07-19 6:24 ` [PATCH 23/49] xfs: fix issues that cause userspace warnings Dave Chinner
2013-07-27 18:02 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 24/49] xfs: kill xfs_vnodeops.[ch] Dave Chinner
2013-07-26 19:18 ` Mark Tinguely
2013-07-27 1:55 ` Dave Chinner
2013-07-27 18:58 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 25/49] xfs: consolidate xfs_rename.c Dave Chinner
2013-07-26 19:33 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 26/49] xfs: consolidate xfs_utils.c Dave Chinner
2013-07-26 20:16 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 27/49] xfs: consolidate extent swap code Dave Chinner
2013-07-26 21:16 ` Mark Tinguely
2013-07-19 6:24 ` [PATCH 28/49] xfs: don't special case shared superblock mounts Dave Chinner
2013-07-26 15:32 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 29/49] xfs: kill __KERNEL__ check for debug code in allocation code Dave Chinner
2013-07-26 15:07 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 30/49] xfs: remove __KERNEL__ from debug code Dave Chinner
2013-07-26 15:03 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 31/49] xfs: remove __KERNEL__ check from xfs_dir2_leaf.c Dave Chinner
2013-07-26 14:16 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 32/49] xfs: xfs_filestreams.h doesn't need __KERNEL__ Dave Chinner
2013-07-26 14:10 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 33/49] xfs: move kernel specific type definitions to xfs.h Dave Chinner
2013-07-26 13:51 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 34/49] xfs: make struct xfs_perag kernel only Dave Chinner
2013-07-26 13:32 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 35/49] xfs: Introduce a new structure to hold transaction reservation items Dave Chinner
2013-07-22 13:05 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 36/49] xfs: Introduce tr_fsyncts to m_reservation Dave Chinner
2013-07-22 13:22 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 37/49] xfs: Make writeid transaction use tr_writeid Dave Chinner
2013-07-22 13:23 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 38/49] xfs: refactor xfs_trans_reserve() interface Dave Chinner
2013-07-22 13:27 ` Mark Tinguely
2013-07-22 23:37 ` Dave Chinner
2013-07-19 6:25 ` [PATCH 39/49] xfs: Get rid of all XFS_XXX_LOG_RES() macro Dave Chinner
2013-07-22 13:31 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 40/49] xfs: Refactor xfs_ticket_alloc() to extract a new helper Dave Chinner
2013-07-22 13:49 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 41/49] xfs: Add xfs_log_rlimit.c Dave Chinner
2013-07-23 15:15 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 42/49] xfs: Validate log space at mount time Dave Chinner
2013-07-22 13:55 ` Mark Tinguely
2013-07-25 4:11 ` Dave Chinner
2013-07-19 6:25 ` [PATCH 43/49] xfs: return log item size in IOP_SIZE Dave Chinner
2013-07-23 18:22 ` Mark Tinguely
2013-08-01 8:18 ` Christoph Hellwig
2013-07-19 6:25 ` [PATCH 44/49] xfs: Reduce allocations during CIL insertion Dave Chinner
2013-07-23 21:15 ` Mark Tinguely
2013-07-23 21:44 ` Michael L. Semon
2013-07-24 13:28 ` Mark Tinguely
2013-07-24 19:20 ` Michael L. Semon
2013-07-25 0:21 ` Dave Chinner
2013-07-25 15:02 ` Mark Tinguely
2013-07-26 0:32 ` Dave Chinner
2013-07-26 20:46 ` Michael L. Semon
2013-07-26 21:06 ` Mark Tinguely
2013-07-26 22:19 ` Michael L. Semon
2013-07-27 1:58 ` Dave Chinner
2013-07-27 18:32 ` Mark Tinguely
2013-07-28 1:12 ` Dave Chinner
2013-07-29 14:15 ` Mark Tinguely
2013-07-30 0:30 ` Dave Chinner
2013-07-30 13:31 ` Mark Tinguely
2013-07-30 22:19 ` Dave Chinner
2013-07-19 6:25 ` [PATCH 45/49] xfs: avoid CIL allocation during insert Dave Chinner
2013-07-29 18:13 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 46/49] xfs: Combine CIL insert and prepare passes Dave Chinner
2013-07-23 21:21 ` Mark Tinguely
2013-07-25 0:23 ` Dave Chinner
2013-07-29 21:07 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 47/49] xfs: split the CIL lock Dave Chinner
2013-07-29 22:24 ` Mark Tinguely
2013-07-19 6:25 ` [PATCH 48/49] xfs: Add read-only support for dirent filetype field Dave Chinner
2013-07-30 19:10 ` Mark Tinguely
2013-08-12 0:59 ` ***** SUSPECTED SPAM ***** " Dave Chinner
2013-08-12 13:25 ` Mark Tinguely
2013-08-13 0:50 ` Dave Chinner
2013-08-13 15:42 ` Mark Tinguely
2013-08-13 15:57 ` Christoph Hellwig
2013-08-14 7:50 ` Dave Chinner
2013-08-14 18:47 ` Geoffrey Wehrman
2013-08-15 4:22 ` Dave Chinner
2013-08-15 17:39 ` Geoffrey Wehrman
2013-08-15 5:59 ` Ric Wheeler
2013-08-15 18:04 ` Geoffrey Wehrman
2013-08-15 21:41 ` Ric Wheeler
2013-08-16 14:08 ` Geoffrey Wehrman
2013-08-19 5:28 ` Dave Chinner
2013-08-19 18:48 ` Geoffrey Wehrman
2013-08-20 2:23 ` Dave Chinner
2013-08-20 14:30 ` Geoffrey Wehrman
2013-08-20 18:27 ` Ric Wheeler
2013-08-20 19:47 ` Eric Sandeen [this message]
2013-08-15 16:50 ` Eric Sandeen
2013-08-15 18:32 ` Geoffrey Wehrman
2013-08-15 18:41 ` Eric Sandeen
2013-08-15 19:06 ` Geoffrey Wehrman
2013-08-15 18:54 ` Ben Myers
2013-08-15 22:40 ` Ben Myers
2013-07-19 6:25 ` [PATCH 49/49] xfs: Add write " Dave Chinner
2013-07-21 6:23 ` [PATCH 00/49] current patch queue for 3.12 Michael L. Semon
2013-07-22 23:43 ` Dave Chinner
2013-07-23 1:00 ` Michael L. Semon
2013-08-01 21:21 ` Ben Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5213C7D8.5020603@sandeen.net \
--to=sandeen@sandeen.net \
--cc=gwehrman@sgi.com \
--cc=rwheeler@redhat.com \
--cc=tinguely@sgi.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox