| Message ID | 16147.1568632167@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT PULL afs: Development for 5.4 | expand |
On Mon, Sep 16, 2019 at 4:09 AM David Howells <dhowells@redhat.com> wrote: > > Here's a set of patches for AFS. The first three are trivial, deleting > unused symbols and rolling out a wrapper function. Pulled. However, I was close to unpulling it again. It has a merge commit with this merge message: Merge remote-tracking branch 'net/master' into afs-next and that simply is not acceptable. Commit messages need to explain the commit. The same is even more true of merges! In a regular commit, you can at least look at the patch and say "ok, that change is obvious and self-explanatory". In a merge commit, the _only_ explanation you have is basically the commit message, and when the commit message is garbage, the merge is garbage. If you can't explain why you are doing a merge, then you shouldn't do the merge. It's that simple. And if you can't be bothered to write the explanation down, I'm not sure I can be bothered to then pull the end result. Linus
On Wed, Sep 18, 2019 at 5:22 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Commit messages need to explain the commit. The same is even more true > of merges! Side note: that wasn't actually the only problem with that merge. The other problem was that neither of the merge bases made any sense what-so-ever. Neither parent was any kind of "this is a good starting point" for anything. You literally merged two random trees. So even an explanation isn't really sufficient. You need to start looking at what you're doing, not doing random crazy stuff. Linus
The pull request you sent on Mon, 16 Sep 2019 12:09:27 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-next-20190915
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0bb73e42f027db64054fff4c3b3203c1626b9dc1
Thank you!
Linus Torvalds <torvalds@linux-foundation.org> wrote: > However, I was close to unpulling it again. It has a merge commit with > this merge message: > > Merge remote-tracking branch 'net/master' into afs-next > > and that simply is not acceptable. Apologies - I meant to rebase that away. There was a bug fix to rxrpc in net/master that didn't get pulled into your tree until Saturday. David
David Howells <dhowells@redhat.com> wrote: > > However, I was close to unpulling it again. It has a merge commit with > > this merge message: > > > > Merge remote-tracking branch 'net/master' into afs-next > > > > and that simply is not acceptable. > > Apologies - I meant to rebase that away. There was a bug fix to rxrpc in > net/master that didn't get pulled into your tree until Saturday. Actually, waiting for all outstanding fixes to get merged and then rebasing might not be the right thing here. The problem is that there are fixes in both trees: afs fixes go directly into yours whereas rxrpc fixes go via networking and I would prefer to base my patches on both of them for testing purposes. What's the preferred method for dealing with that? Base on a merge of the lastest of those fixes in each tree? David
On Thu, Sep 19, 2019 at 10:49:22AM +0100, David Howells wrote: > David Howells <dhowells@redhat.com> wrote: > > > > However, I was close to unpulling it again. It has a merge commit with > > > this merge message: > > > > > > Merge remote-tracking branch 'net/master' into afs-next > > > > > > and that simply is not acceptable. > > > > Apologies - I meant to rebase that away. There was a bug fix to rxrpc in > > net/master that didn't get pulled into your tree until Saturday. > > Actually, waiting for all outstanding fixes to get merged and then rebasing > might not be the right thing here. The problem is that there are fixes in > both trees: afs fixes go directly into yours whereas rxrpc fixes go via > networking and I would prefer to base my patches on both of them for testing > purposes. What's the preferred method for dealing with that? Base on a merge > of the lastest of those fixes in each tree? Why is it organised this way? I mean, yes, technically, rxrpc is a generic layer-6 protocol that any blah blah blah, but in practice no other user has come up in the last 37 years, so why bother pretending one is going to? Just git mv net/rxrpc fs/afs/ and merge everything through your tree. I feel similarly about net/9p, net/sunrpc and net/ceph. Every filesystem comes with its own presentation layer; nobody reuses an existing one. Just stop pretending they're separate components.
On Thu, Sep 19, 2019 at 3:55 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Sep 19, 2019 at 10:49:22AM +0100, David Howells wrote: > > David Howells <dhowells@redhat.com> wrote: > > > > > > However, I was close to unpulling it again. It has a merge commit with > > > > this merge message: > > > > > > > > Merge remote-tracking branch 'net/master' into afs-next > > > > > > > > and that simply is not acceptable. > > > > > > Apologies - I meant to rebase that away. There was a bug fix to rxrpc in > > > net/master that didn't get pulled into your tree until Saturday. > > > > Actually, waiting for all outstanding fixes to get merged and then rebasing > > might not be the right thing here. The problem is that there are fixes in > > both trees: afs fixes go directly into yours whereas rxrpc fixes go via > > networking and I would prefer to base my patches on both of them for testing > > purposes. What's the preferred method for dealing with that? Base on a merge > > of the lastest of those fixes in each tree? > > Why is it organised this way? I mean, yes, technically, rxrpc is a > generic layer-6 protocol that any blah blah blah, but in practice no > other user has come up in the last 37 years, so why bother pretending > one is going to? Just git mv net/rxrpc fs/afs/ and merge everything > through your tree. > > I feel similarly about net/9p, net/sunrpc and net/ceph. Every filesystem > comes with its own presentation layer; nobody reuses an existing one. > Just stop pretending they're separate components. net/ceph is also being used by drivers/block/rbd.c. net/ceph was split out of fs/ceph when rbd was introduced. We continued to manage them in a single ceph-client.git tree though. Thanks, Ilya
Matthew Wilcox <willy@infradead.org> wrote: > Why is it organised this way? I mean, yes, technically, rxrpc is a > generic layer-6 protocol that any blah blah blah, but in practice no > other user has come up in the last 37 years, so why bother pretending > one is going to? Just git mv net/rxrpc fs/afs/ and merge everything > through your tree. Note that, unlike 9p, sunrpc and ceph, rxrpc is exposed as a network protocol and can be used directly with socket(AF_RXRPC, ...). I have part of a userspace tool suite that uses this. David
On 9/19/2019 9:15 AM, Matthew Wilcox wrote: > Why is it organised this way? I mean, yes, technically, rxrpc is a > generic layer-6 protocol that any blah blah blah, but in practice no > other user has come up in the last 37 years, so why bother pretending > one is going to? Just git mv net/rxrpc fs/afs/ and merge everything > through your tree. > > I feel similarly about net/9p, net/sunrpc and net/ceph. Every filesystem > comes with its own presentation layer; nobody reuses an existing one. > Just stop pretending they're separate components. The IBM/OpenAFS rxrpc implementation has been used for many services other than afs3 over the past 37 years within institutions that had source code access. rxrpc provides a light-weight rpc layer capable of pluggable per-service security classes. It offers optional end-to-end authentication, integrity protection and wire privacy. The existence of Linux rxrpc as a socket family means that it can be used to implement not only clients for AFS and AuriStorFS services beyond the fileserver but also the location service, protection service, backup service, etc. The rxrpc socket family could also be used to implement the servers. The linux rxrpc implementation does not yet have all of the functionality of the AuriStor RX implementation but it is fast. Unlike every other RX implementation, the Linux rxrpc doesn't process packets through both UDP and RX. To achieve the same performance as Linux rxrpc the AuriStor userland RX must leverage DPDK. It is my hope that as Linux rxrpc continues to improve and is built into kernels by more distributions that it will see wider use. It is a perfect match for IoT. Jeffrey Altman
On Thu, Sep 19, 2019 at 2:49 AM David Howells <dhowells@redhat.com> wrote: > > Actually, waiting for all outstanding fixes to get merged and then rebasing > might not be the right thing here. The problem is that there are fixes in > both trees: afs fixes go directly into yours whereas rxrpc fixes go via > networking and I would prefer to base my patches on both of them for testing > purposes. What's the preferred method for dealing with that? Base on a merge > of the lastest of those fixes in each tree? If you absolutely *have* to have something from another development tree, that's generally a sign that something is screwed up with the model in the first place, but when it happens, you should make sure that you have a stable point in that development tree. You might ask the upstream developer (ie Davem, in the case of the network tree) what would be a good point, for example. Don't just pick a random "tree of the day". The same very much goes for my tree, btw. You should simply never just pick a random tree of the day as your base for work if you start with my tree. That's true whether you do a merge or just start new development on top of some point, or anything else, for that matter. Generally, you should never merge other peoples code without having them _tell_ you that some particular point is a good thing to merge. Releases are obviously implicitly such points, but generally cross-tree merges need communication (a pull request to upstream is the obvious such communication, but not necessarily the only one: we've had cross-tree development that has involved separate branches and just various synchronization emails between the two groups). Looking at rxrpc in particular - if that is what you were waiting for - it looks more like you should just had an rxrpc branch, and asked David to pull it for the 5.4 merge window. Then you could have used that branch itself, as a starting point, perhaps. Or - better yet, perhaps - merged it into your development tree based on a good AFS starting point, with a *big* merge message explaining what you are merging and why. Right now there is a merge with absolutely no explanation for why the merge exists at all, and with some very non-obvious bases that really look like they are just random points of development for both me and for Davem. Linus
Hi Linus, Here's a set of patches for AFS. The first three are trivial, deleting unused symbols and rolling out a wrapper function. The fourth and fifth patches make use of the previously added RCU-safe request_key facility to allow afs_permission() and afs_d_revalidate() to attempt to operate without dropping out of RCU-mode pathwalk. Under certain conditions, such as conflict with another client, we still have to drop out anyway, take a lock and consult the server. David --- The following changes since commit f16180739cd18a39a1a45516ac0e65d18a9f100e: Merge remote-tracking branch 'net/master' into afs-next (2019-09-02 11:43:44 +0100) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-next-20190915 for you to fetch changes up to a0753c29004f4983e303abce019f29e183b1ee48: afs: Support RCU pathwalk (2019-09-02 11:43:54 +0100) ---------------------------------------------------------------- AFS development Tested-by: Marc Dionne <marc.dionne@auristor.com> ---------------------------------------------------------------- David Howells (3): afs: Use afs_extract_discard() rather than iov_iter_discard() afs: Provide an RCU-capable key lookup afs: Support RCU pathwalk YueHaibing (2): afs: remove unused variable 'afs_voltypes' afs: remove unused variable 'afs_zero_fid' fs/afs/dir.c | 54 +++++++++++++++++++++++++- fs/afs/fsclient.c | 6 +-- fs/afs/internal.h | 1 + fs/afs/security.c | 108 +++++++++++++++++++++++++++++++++++++++++++--------- fs/afs/volume.c | 2 - fs/afs/yfsclient.c | 6 +-- include/linux/key.h | 14 ++++++- 7 files changed, 162 insertions(+), 29 deletions(-)