[GIT PULL afs: Development for 5.4
mbox series

Message ID 16147.1568632167@warthog.procyon.org.uk
State New
Headers show
Series
  • [GIT PULL afs: Development for 5.4
Related show

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/afs-next-20190915

Message

David Howells Sept. 16, 2019, 11:09 a.m. UTC
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(-)

Comments

Linus Torvalds Sept. 19, 2019, 12:22 a.m. UTC | #1
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
Linus Torvalds Sept. 19, 2019, 12:24 a.m. UTC | #2
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
pr-tracker-bot@kernel.org Sept. 19, 2019, 1:55 a.m. UTC | #3
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!
David Howells Sept. 19, 2019, 6:40 a.m. UTC | #4
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 Sept. 19, 2019, 9:49 a.m. UTC | #5
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
Matthew Wilcox Sept. 19, 2019, 1:15 p.m. UTC | #6
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.
Ilya Dryomov Sept. 19, 2019, 2:03 p.m. UTC | #7
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
David Howells Sept. 19, 2019, 2:24 p.m. UTC | #8
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
Jeffrey E Altman Sept. 19, 2019, 3:05 p.m. UTC | #9
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
Linus Torvalds Sept. 19, 2019, 4:29 p.m. UTC | #10
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