posting POSIX draft ACL patches

posting POSIX draft ACL patches

[Rick Macklem]

So, I've finally figured out how to use git format-patch. It took
me a lot of tries before I discovered all you do is specify "master.."
to make it work. (I still haven't tried to email them via gmail, but
I've found the doc for that.)

At this point, the patches are still in need of testing (I have yet to
test the nfsd case where file object creation specifies a POSIX draft
ACL, since neither FreeBSD nor Linux clients do that.)

Is it time to post the patch sets for others to try or should I wait a while?

A couple of questions...
The patches currently have a lot of dprintk()s I used for testing.
Should those be removed before posting or left in for now?

They are currently based on linux-6.11.0-rc6 (linux-next of a few
weeks ago). What do you guys do w.r.t. rebasing them?

There are three sets: common, client and server (common is
needed by both the others.
The other two sets implement client and server side for handling
the new attributes proposed in
https://datatracker.ietf.org/doc/draft-rmacklem-nfsv4-posix-acls/

Thanks in advance for any help, rick
ps: The problem I thought I had w.r.t. server side large ACLs does
      not appear to be a problem. It also appears that the nfsd always
       sets up a scratch buffer, so the server code doesn't do that, either.

Re: posting POSIX draft ACL patches

[Chuck Lever III]

> On Oct 25, 2024, at 6:47 PM, Rick Macklem <rick.macklem@gmail.com> wrote:
> 
> So, I've finally figured out how to use git format-patch. It took
> me a lot of tries before I discovered all you do is specify "master.."
> to make it work. (I still haven't tried to email them via gmail, but
> I've found the doc for that.)
> 
> At this point, the patches are still in need of testing (I have yet to
> test the nfsd case where file object creation specifies a POSIX draft
> ACL, since neither FreeBSD nor Linux clients do that.)
> 
> Is it time to post the patch sets for others to try or should I wait a while?

My only hesitation is that the draft hasn't gone through WGLC
yet. The probability of changes to the wire protocol decrease
over time, but after WGLC, we can be pretty certain that
the protocol is stable and won't get any further changes
that might risk interoperability. The client folks might be
OK with just a personal draft, which we have now.

Maybe we should consider merging this work now and hide it
behind a Kconfig option that defaults N. The risk there is
that folks not directly involved might enable this anyway,
and if the protocol changes, we'd be stuck having to support
one or more pre-standard versions. Opening the floor for
comments.

For now, usually the developer can maintain a public git repo
that contains their patches so that others can pull them for
testing. You are free to continue posting new versions of
the series (with a cover letter that contains a change log).

We probably want to see some unit testing (ie, pynfs) but
that can be developed separately.


> A couple of questions...
> The patches currently have a lot of dprintk()s I used for testing.
> Should those be removed before posting or left in for now?

The usual policy on the server is that dprintk()s that were
used for development but have no diagnostic value for users
or administrators should be removed before merging. We also
don't like to keep dprintk() call sites in hot paths.
Observability in hot paths is done with tracepoints, but
if you don't want to add those, just leave a comment where
you think each tracepoint might be best, and someone can add
them later.


> They are currently based on linux-6.11.0-rc6 (linux-next of a few
> weeks ago). What do you guys do w.r.t. rebasing them?

When these are ready to merge, the series should be based
on nfsd-next (server side) or the -next branch for the
client side (Trond and Anna take every other release, so
ask which is preferred).

Pull the base branch into your repo and use "git rebase".


> There are three sets: common, client and server (common is
> needed by both the others.

We'll have to decide how these will get merged, as I
mentioned to you previously: Either one side goes first
and pulls the common branch, then the other side
merges later; or one side agrees to pull all three at
once.


> The other two sets implement client and server side for handling
> the new attributes proposed in
> https://datatracker.ietf.org/doc/draft-rmacklem-nfsv4-posix-acls/
> 
> Thanks in advance for any help, rick
> ps: The problem I thought I had w.r.t. server side large ACLs does
>      not appear to be a problem. It also appears that the nfsd always
>       sets up a scratch buffer, so the server code doesn't do that, either.
> 

--
Chuck Lever

Re: posting POSIX draft ACL patches

[Rick Macklem]

On Sun, Oct 27, 2024 at 8:02 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 25, 2024, at 6:47 PM, Rick Macklem <rick.macklem@gmail.com> wrote:
> >
> > So, I've finally figured out how to use git format-patch. It took
> > me a lot of tries before I discovered all you do is specify "master.."
> > to make it work. (I still haven't tried to email them via gmail, but
> > I've found the doc for that.)
> >
> > At this point, the patches are still in need of testing (I have yet to
> > test the nfsd case where file object creation specifies a POSIX draft
> > ACL, since neither FreeBSD nor Linux clients do that.)
> >
> > Is it time to post the patch sets for others to try or should I wait a while?
>
> My only hesitation is that the draft hasn't gone through WGLC
> yet. The probability of changes to the wire protocol decrease
> over time, but after WGLC, we can be pretty certain that
> the protocol is stable and won't get any further changes
> that might risk interoperability. The client folks might be
> OK with just a personal draft, which we have now.
>
> Maybe we should consider merging this work now and hide it
> behind a Kconfig option that defaults N. The risk there is
> that folks not directly involved might enable this anyway,
> and if the protocol changes, we'd be stuck having to support
> one or more pre-standard versions. Opening the floor for
> comments.
>
> For now, usually the developer can maintain a public git repo
> that contains their patches so that others can pull them for
> testing. You are free to continue posting new versions of
> the series (with a cover letter that contains a change log).
Ok. Reading between the lines, my interpretation of this is...
1) Don't post patches until they are ready to be considered
     for upstreaming.
     (I was really asking about patches for testing, but failed to
      mention that.)
2) If I put the patches somewhere others can "git pull", that
    is probably the best bet for patches for testing.

So, I just put the "common" part in a github repository forked
from linux-next.
https://github.com/rmacklem/linux.git
It has three branches (common, client and server), although I
have only populated "common" at this point.
(Btw, if someone is inspired to try a pull, just to see that it
works, that would be appreciated.)

I'm not a git guy, so maybe someone can explain how I can
move a bunch of patches from a local clone of linux-next to
the clone of the github one (so I can then push them to github).
- For common, I just did each patch by hand with a fresh commit,
  but there's enough of them in "client" and "server" that this
  will take a while.
  - Is there a better way via git?

Anyhow, once I get all them into github, I'll just leave them there
and try and coax others into trying them for the next testing event
(in the spring, maybe?).
I'll assume WGLC happens sometime between 6months->never,
so that should work out ok, I think?

>
> We probably want to see some unit testing (ie, pynfs) but
> that can be developed separately.
Yea, I know nothing about Python, so hopefully someone else
will be inspired to do this. I wonder what is out there for testing
the NFSv3 NFSACL protocol?

>
>
> > A couple of questions...
> > The patches currently have a lot of dprintk()s I used for testing.
> > Should those be removed before posting or left in for now?
>
> The usual policy on the server is that dprintk()s that were
> used for development but have no diagnostic value for users
> or administrators should be removed before merging. We also
> don't like to keep dprintk() call sites in hot paths.
Yea, I was really just asking w.r.t. test code. They are not indented,
so they are easy to remove at some point.

> Observability in hot paths is done with tracepoints, but
> if you don't want to add those, just leave a comment where
> you think each tracepoint might be best, and someone can add
> them later.
>
>
> > They are currently based on linux-6.11.0-rc6 (linux-next of a few
> > weeks ago). What do you guys do w.r.t. rebasing them?
>
> When these are ready to merge, the series should be based
> on nfsd-next (server side) or the -next branch for the
> client side (Trond and Anna take every other release, so
> ask which is preferred).
>
> Pull the base branch into your repo and use "git rebase".
I forked what was called linux-next in github, so hopefully that
should be it?

Thanks for the comments, rick

>
>
> > There are three sets: common, client and server (common is
> > needed by both the others.
>
> We'll have to decide how these will get merged, as I
> mentioned to you previously: Either one side goes first
> and pulls the common branch, then the other side
> merges later; or one side agrees to pull all three at
> once.
>
>
> > The other two sets implement client and server side for handling
> > the new attributes proposed in
> > https://datatracker.ietf.org/doc/draft-rmacklem-nfsv4-posix-acls/
> >
> > Thanks in advance for any help, rick
> > ps: The problem I thought I had w.r.t. server side large ACLs does
> >      not appear to be a problem. It also appears that the nfsd always
> >       sets up a scratch buffer, so the server code doesn't do that, either.
> >
>
> --
> Chuck Lever
>
>

Re: posting POSIX draft ACL patches

[Chuck Lever III]

> On Oct 28, 2024, at 4:17 PM, Rick Macklem <rick.macklem@gmail.com> wrote:
> 
> On Sun, Oct 27, 2024 at 8:02 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Oct 25, 2024, at 6:47 PM, Rick Macklem <rick.macklem@gmail.com> wrote:
>>> 
>>> So, I've finally figured out how to use git format-patch. It took
>>> me a lot of tries before I discovered all you do is specify "master.."
>>> to make it work. (I still haven't tried to email them via gmail, but
>>> I've found the doc for that.)
>>> 
>>> At this point, the patches are still in need of testing (I have yet to
>>> test the nfsd case where file object creation specifies a POSIX draft
>>> ACL, since neither FreeBSD nor Linux clients do that.)
>>> 
>>> Is it time to post the patch sets for others to try or should I wait a while?
>> 
>> My only hesitation is that the draft hasn't gone through WGLC
>> yet. The probability of changes to the wire protocol decrease
>> over time, but after WGLC, we can be pretty certain that
>> the protocol is stable and won't get any further changes
>> that might risk interoperability. The client folks might be
>> OK with just a personal draft, which we have now.
>> 
>> Maybe we should consider merging this work now and hide it
>> behind a Kconfig option that defaults N. The risk there is
>> that folks not directly involved might enable this anyway,
>> and if the protocol changes, we'd be stuck having to support
>> one or more pre-standard versions. Opening the floor for
>> comments.
>> 
>> For now, usually the developer can maintain a public git repo
>> that contains their patches so that others can pull them for
>> testing. You are free to continue posting new versions of
>> the series (with a cover letter that contains a change log).
> Ok. Reading between the lines, my interpretation of this is...
> 1) Don't post patches until they are ready to be considered
>     for upstreaming.
>     (I was really asking about patches for testing, but failed to
>      mention that.)

You can post such patches, but their Subject: should start
with "[PATCH RFC]" to show that you are not interested in
merging the work immediately.


> 2) If I put the patches somewhere others can "git pull", that
>    is probably the best bet for patches for testing.
> 
> So, I just put the "common" part in a github repository forked
> from linux-next.
> https://github.com/rmacklem/linux.git
> It has three branches (common, client and server), although I
> have only populated "common" at this point.
> (Btw, if someone is inspired to try a pull, just to see that it
> works, that would be appreciated.)
> 
> I'm not a git guy, so maybe someone can explain how I can
> move a bunch of patches from a local clone of linux-next to
> the clone of the github one (so I can then push them to github).
> - For common, I just did each patch by hand with a fresh commit,
>  but there's enough of them in "client" and "server" that this
>  will take a while.
>  - Is there a better way via git?

We can email privately about that.

I think the web UI allows you to create a branch based
on another branch in your tree. You should be able to
create a "client" and a "server" branch on top of
"common", for example, so that both branches can share
the same common parts.


> Anyhow, once I get all them into github, I'll just leave them there
> and try and coax others into trying them for the next testing event
> (in the spring, maybe?).

We're looking at May 12 - 16 in Ann Arbor for the next
BAT, but that isn't a solid date yet.


> I'll assume WGLC happens sometime between 6months->never,
> so that should work out ok, I think?
> 
>> 
>> We probably want to see some unit testing (ie, pynfs) but
>> that can be developed separately.
> Yea, I know nothing about Python, so hopefully someone else
> will be inspired to do this. I wonder what is out there for testing
> the NFSv3 NFSACL protocol?

There is almost nothing for NFSACL, as far as I know.


>>> A couple of questions...
>>> The patches currently have a lot of dprintk()s I used for testing.
>>> Should those be removed before posting or left in for now?
>> 
>> The usual policy on the server is that dprintk()s that were
>> used for development but have no diagnostic value for users
>> or administrators should be removed before merging. We also
>> don't like to keep dprintk() call sites in hot paths.
> Yea, I was really just asking w.r.t. test code. They are not indented,
> so they are easy to remove at some point.
> 
>> Observability in hot paths is done with tracepoints, but
>> if you don't want to add those, just leave a comment where
>> you think each tracepoint might be best, and someone can add
>> them later.
>> 
>> 
>>> They are currently based on linux-6.11.0-rc6 (linux-next of a few
>>> weeks ago). What do you guys do w.r.t. rebasing them?
>> 
>> When these are ready to merge, the series should be based
>> on nfsd-next (server side) or the -next branch for the
>> client side (Trond and Anna take every other release, so
>> ask which is preferred).
>> 
>> Pull the base branch into your repo and use "git rebase".
> I forked what was called linux-next in github, so hopefully that
> should be it?

I looked at your repo, it needs some adjustment. Let's be
in touch privately.

--
Chuck Lever