[v2,2/2] manual/llio.texi: update manual to document file-private locks

  Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 ChangeLog        |   1 +
 manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 182 insertions(+), 2 deletions(-)
  

On 04/11/2014 08:55 AM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  ChangeLog        |   1 +
>  manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 182 insertions(+), 2 deletions(-)

You are a great developer, submitting documentation along
with new interfaces! You have all of my attention :-)

Same comments about ChangeLog and BZ apply.

The ChangeLog here should be:

2014-04-11  Jeff Layton  <jlayton@redhat.com>

	* manual/llio.texi: add section about file-private locks

> diff --git a/ChangeLog b/ChangeLog
> index 55a84e598e46..622315629a44 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -2,6 +2,7 @@
>  
>  	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
>  	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> +	* manual/llio.texi: add section about file-private locks

Other way around.

If you are batching your ChangeLog's you'd write:

2014-04-11  Jeff Layton  <jlayton@redhat.com>

	* manual/llio.texi: add section about file-private locks

	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
	(F_GETLKP, F_SETLKP, F_SETLKPW): New macros.

With a space between them to indicate they were distinct
commits but on the same date.
  
>  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
>  
> diff --git a/manual/llio.texi b/manual/llio.texi
> index 6f8adfc607d7..bcaf0e419a90 100644
> --- a/manual/llio.texi
> +++ b/manual/llio.texi
> @@ -57,6 +57,8 @@ directly.)
>                                           flags associated with open files.
>  * File Locks::                          Fcntl commands for implementing
>                                           file locking.
> +* File-private Locks::                  Fcntl commands for implementing
> +                                         file-private locking.

OK.

>  * Interrupt Input::                     Getting an asynchronous signal when
>                                           input arrives.
>  * IOCTLs::                              Generic I/O Control operations.
> @@ -2890,7 +2892,7 @@ Get flags associated with the open file.  @xref{File Status Flags}.
>  Set flags associated with the open file.  @xref{File Status Flags}.
>  
>  @item F_GETLK
> -Get a file lock.  @xref{File Locks}.
> +Get (test) a file lock.  @xref{File Locks}.

Why the parenthetical comment? Please write out what you'd like to say.
  
>  @item F_SETLK
>  Set or clear a file lock.  @xref{File Locks}.
> @@ -2898,6 +2900,15 @@ Set or clear a file lock.  @xref{File Locks}.
>  @item F_SETLKW
>  Like @code{F_SETLK}, but wait for completion.  @xref{File Locks}.
>  
> +@item F_GETLKP
> +Get (test) a file-private lock.  @xref{File Locks}.
> +
> +@item F_SETLKP
> +Set or clear a file lock.  @xref{File Locks}.
> +
> +@item F_SETLKPW
> +Like @code{F_SETLKP}, but wait for completion.  @xref{File Locks}.

If these three are Linux specific please say so at the end of each
e.g. `Specific to Linux.'

We have kFreeBSD and Hurd using glibc and I would like to know which
constants apply to which OS. Although kFreeBSD is not upstream we
still have Hurd.

> +
>  @item F_GETOWN
>  Get process or process group ID to receive @code{SIGIO} signals.
>  @xref{Interrupt Input}.
> @@ -3576,6 +3587,10 @@ set_nonblock_flag (int desc, int value)
>  
>  @cindex file locks
>  @cindex record locking
> +This section describes "classic" record locks.  There is also a newer type
> +of record lock that is associated with the open file table entry instead

You should make explicit mention of the distinction between file descriptor
and file table entry here since it impacts the semantics of file-private
locks on dup (as you note below).

> +of the process.  @xref{File-private Locks}

The distinction has nothing to do with classical versus new, it has to do
with the association of the lock and how it behaves. Please reword this
to avoid quote "classic" distinction and instead talk about where and how
the locks are applied.

Also note that documentation exists forever, and in 5 years the file table
entry locks won't be "newer type", instead they will just be another type
of lock. Thus avoid saying things like "newer type" and just say "open
file table entry locks."

e.g.

This section describe record locks whose locks are associated with the
process. There is also support for record locks associated with open file
table entries instead of the process.  @xref{File-private Locks}.

Note the period after the @xref is expected style.

> +
>  The remaining @code{fcntl} commands are used to support @dfn{record
>  locking}, which permits multiple cooperating programs to prevent each
>  other from simultaneously accessing parts of a file in error-prone
> @@ -3641,7 +3656,9 @@ the file.
>  @item pid_t l_pid
>  This field is the process ID (@pxref{Process Creation Concepts}) of the
>  process holding the lock.  It is filled in by calling @code{fcntl} with
> -the @code{F_GETLK} command, but is ignored when making a lock.
> +the @code{F_GETLK} command, but is ignored when making a lock.  If the
> +conflicting lock is a File-private lock (@pxref{File-private Locks}),
> +then this field will be set to @math{-1}.
>  @end table
>  @end deftp
>  
> @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
>  controlling access to a file.  There is still potential for access to
>  the file by programs that don't use the lock protocol.
>  
> +@node File-private Locks
> +@section File-private Locks
> +
> +In contrast to classic record locks (@pxref{File Locks}), file-private

See comments above about "classic" or "new type."

Please reword like "In contrast to process-associated record locks..."

> +locks are associated with an open file table entry rather than a
> +process.  File-private locks set on an open file descriptor will never
> +conflict with existing file-private locks set on that file descriptor.
> +
> +File-private locks are also inherited by child processes across
> +@code{fork} (@pxref{Creating a Process}), along with the file

What about clone with/without CLONE_FILES?

> +descriptor.  For this reason, the @code{l_pid} field in @code{struct
> +flock} is meaningless for file private locks.  For the @code{F_GETLK} and

It can't be meaningless, it has to have some meaning. Setting it to -1 means
"the locks is associated with an open file descriptor and not a process."
Please reword e.g. For this reason, the @code{l_pid} field in @code {struct
flock} is set to @math{-1} for file private locks to indicate that the lock
is associated with an open file table entry instead of a process... 

> +@code{F_GETLKP} commands, the @code{l_pid} value is always set to
> +@math{-1} if the blocking lock is a file-private one.

OK.

> +
> +Note that using @code{dup} (@pxref{Duplicating Descriptors}) to clone a

s/Note that using/Using/g

s/to clone/to copy/g -- We are creating a "copy" and that's the language
we use throughout for dup, not "clone".

> +file descriptor does not give you a new instance of the open file, but
> +instead just clones a reference to an existing open file.  Thus,

Suggest:
... but instead copies the reference to the existing file table entry
for the open file.

> +file-private locks set on a file descriptor cloned by @code{dup} will
> +never conflict with file-private locks set on the original descriptor.

OK. The dup'd fd acts as if it was the original fd since it never came
from a distinct call to open.

> +
> +File-private locks always conflict with classic record locks, even if
> +acquired by the same process or on the same open file descriptor.

OK.

> +
> +File-private locks use the same @code{struct flock} as classic POSIX
> +locks as an argument (@pxref{File Locks}) and the macros for the
> +cmd values are also declared in the header file @file{fcntl.h}. To
> +use them, the macro @code{_GNU_SOURCE} must be defined prior to
> +including @file{fcntl.h}.

OK.

> +
> +In contrast to the @code{F_SETLK} or @code{F_SETLKW} commands, any
> +@code{struct flock} used as an argument to the @code{F_SETLKP} or
> +@code{F_SETLKPW} commands must have the @code{l_pid} value set to
> +@math{0}.

Why not -1? This means that between F_GETLK and F_SETLK I need
to explicitly zero l_pid otherwise the F_SETLK fails?

> +
> +@pindex fcntl.h.
> +
> +@deftypevr Macro int F_GETLKP
> +This macro is used as the @var{command} argument to @code{fcntl}, to
> +specify that it should get information about a lock.  This command
> +requires a third argument of type @w{@code{struct flock *}} to be passed
> +to @code{fcntl}, so that the form of the call is:

Missing sentence?

> +
> +If there is a lock already in place that would block the lock described
> +by the @var{lockp} argument, information about that lock overwrites
> +@code{*@var{lockp}}.  Existing locks are not reported if they are
> +compatible with making a new lock as specified.  Thus, you should
> +specify a lock type of @code{F_WRLCK} if you want to find out about both
> +read and write locks, or @code{F_RDLCK} if you want to find out about
> +write locks only.

OK.

> +
> +There might be more than one lock affecting the region specified by the
> +@var{lockp} argument, but @code{fcntl} only returns information about
> +one of them. 

Suggest adding more weasel words:

Which lock information is returned when multiple locks are present is
unspecified.

Feel free to merge that with the sentence above into something more
compact. I only want to say that you should be explicit that it is
unspecified which lock information is returned if there are multiple
locks overlapping the region of interest.

> ... The @code{l_whence} member of the @var{lockp} structure is
> +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> +set to identify the locked region.
> +
> +If no lock applies, the only change to the @var{lockp} structure is to
> +update the @code{l_type} to a value of @code{F_UNLCK}.
> +
> +The normal return value from @code{fcntl} with this command is an
> +unspecified value other than @math{-1}, which is reserved to indicate an
> +error.  The following @code{errno} error conditions are defined for
> +this command:
> +
> +@table @code
> +@item EBADF
> +The @var{filedes} argument is invalid.
> +
> +@item EINVAL
> +Either the @var{lockp} argument doesn't specify valid lock information,
> +or the file associated with @var{filedes} doesn't support locks.
> +@end table
> +@end deftypevr

OK.

> +
> +@comment fcntl.h
> +@comment POSIX.1
> +@deftypevr Macro int F_SETLKP
> +This macro is used as the @var{command} argument to @code{fcntl}, to
> +specify that it should set or clear a lock.  This command requires a
> +third argument of type @w{@code{struct flock *}} to be passed to
> +@code{fcntl}, so that the form of the call is:
> +
> +@smallexample
> +fcntl (@var{filedes}, F_SETLKP, @var{lockp})
> +@end smallexample
> +
> +If the opened file already has a lock on any part of the
> +region, the old lock on that part is replaced with the new lock.  You
> +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> +
> +If the lock cannot be set, @code{fcntl} returns immediately with a value
> +of @math{-1}.  This function does not block waiting for other tasks
> +to release locks.  If @code{fcntl} succeeds, it returns a value other
> +than @math{-1}.
> +
> +The following @code{errno} error conditions are defined for this
> +function:
> +
> +@table @code
> +@item EAGAIN
> +@itemx EACCES
> +The lock cannot be set because it is blocked by an existing lock on the
> +file.  Some systems use @code{EAGAIN} in this case, and other systems
> +use @code{EACCES}; your program should treat them alike, after
> +@code{F_SETLKP}.  (@gnulinuxhurdsystems{} always use @code{EAGAIN}.)
> +
> +@item EBADF
> +Either: the @var{filedes} argument is invalid; you requested a read lock
> +but the @var{filedes} is not open for read access; or, you requested a
> +write lock but the @var{filedes} is not open for write access.
> +
> +@item EINVAL
> +Either the @var{lockp} argument doesn't specify valid lock information,
> +or the file associated with @var{filedes} doesn't support locks.
> +
> +@item ENOLCK
> +The system has run out of file lock resources; there are already too
> +many file locks in place.
> +
> +Well-designed file systems never report this error, because they have no
> +limitation on the number of locks.  However, you must still take account
> +of the possibility of this error, as it could result from network access
> +to a file system on another machine.
> +@end table
> +@end deftypevr

OK.

> +
> +@comment fcntl.h
> +@comment POSIX.1
> +@deftypevr Macro int F_SETLKPW
> +This macro is used as the @var{command} argument to @code{fcntl}, to
> +specify that it should set or clear a lock.  It is just like the
> +@code{F_SETLKP} command, but causes the process to block (or wait)

s/block (or wait)/wait/g

> +until the request can be specified.

s/specified/completed/g

> +
> +This command requires a third argument of type @code{struct flock *}, as
> +for the @code{F_SETLKP} command.
> +
> +The @code{fcntl} return values and errors are the same as for the
> +@code{F_SETLKP} command, but these additional @code{errno} error conditions
> +are defined for this command:
> +
> +@table @code
> +@item EINTR
> +The function was interrupted by a signal while it was waiting.
> +@xref{Interrupted Primitives}.
> +
> +@end table
> +@end deftypevr

OK.

> +
> +File-private locks are useful in the same sorts of situations as classic
> +record locks.  They can also be used to synchronize file access between
> +threads within the same process by giving each thread its own open file
> +instance.
> +
> +Because they are only released automatically when the last reference to
> +an open file is destroyed, file-private locks allow more assurance that
> +the locks will not be released due to a library routine opening and
> +closing a file without the application being aware.
> +
> +As with classic record locks, file-private locks are also voluntary.

Is it too much to ask for an example?

Like the threaded example in https://lwn.net/Articles/586904/ ? :-)

> +
>  @node Interrupt Input
>  @section Interrupt-Driven Input
  
Please repost a new v2 when you get a chance.

Cheers,
Carlos.
  

On Fri, 11 Apr 2014 20:29:32 -0400
"Carlos O'Donell" <carlos@redhat.com> wrote:

> On 04/11/2014 08:55 AM, Jeff Layton wrote:
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  ChangeLog        |   1 +
> >  manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 182 insertions(+), 2 deletions(-)
> 
> You are a great developer, submitting documentation along
> with new interfaces! You have all of my attention :-)
> 
> Same comments about ChangeLog and BZ apply.
> 
> The ChangeLog here should be:
> 
> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> 
> 	* manual/llio.texi: add section about file-private locks
> 
> > diff --git a/ChangeLog b/ChangeLog
> > index 55a84e598e46..622315629a44 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -2,6 +2,7 @@
> >  
> >  	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> >  	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> > +	* manual/llio.texi: add section about file-private locks
> 
> Other way around.
> 
> If you are batching your ChangeLog's you'd write:
> 
> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> 
> 	* manual/llio.texi: add section about file-private locks
> 
> 	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> 	(F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> 
> With a space between them to indicate they were distinct
> commits but on the same date.
>   
> >  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
> >  
> > diff --git a/manual/llio.texi b/manual/llio.texi
> > index 6f8adfc607d7..bcaf0e419a90 100644
> > --- a/manual/llio.texi
> > +++ b/manual/llio.texi
> > @@ -57,6 +57,8 @@ directly.)
> >                                           flags associated with open files.
> >  * File Locks::                          Fcntl commands for implementing
> >                                           file locking.
> > +* File-private Locks::                  Fcntl commands for implementing
> > +                                         file-private locking.
> 
> OK.
> 
> >  * Interrupt Input::                     Getting an asynchronous signal when
> >                                           input arrives.
> >  * IOCTLs::                              Generic I/O Control operations.
> > @@ -2890,7 +2892,7 @@ Get flags associated with the open file.  @xref{File Status Flags}.
> >  Set flags associated with the open file.  @xref{File Status Flags}.
> >  
> >  @item F_GETLK
> > -Get a file lock.  @xref{File Locks}.
> > +Get (test) a file lock.  @xref{File Locks}.
> 
> Why the parenthetical comment? Please write out what you'd like to say.
>   

Ok.

> >  @item F_SETLK
> >  Set or clear a file lock.  @xref{File Locks}.
> > @@ -2898,6 +2900,15 @@ Set or clear a file lock.  @xref{File Locks}.
> >  @item F_SETLKW
> >  Like @code{F_SETLK}, but wait for completion.  @xref{File Locks}.
> >  
> > +@item F_GETLKP
> > +Get (test) a file-private lock.  @xref{File Locks}.
> > +
> > +@item F_SETLKP
> > +Set or clear a file lock.  @xref{File Locks}.
> > +
> > +@item F_SETLKPW
> > +Like @code{F_SETLKP}, but wait for completion.  @xref{File Locks}.
> 
> If these three are Linux specific please say so at the end of each
> e.g. `Specific to Linux.'
> 
> We have kFreeBSD and Hurd using glibc and I would like to know which
> constants apply to which OS. Although kFreeBSD is not upstream we
> still have Hurd.
> 

These are currently linux-specific, but eventually I'd like to see
other OS' adopt this as well. I'm attempting to get this incorporated
as part of the POSIX spec, but that will probably take a while.

> > +
> >  @item F_GETOWN
> >  Get process or process group ID to receive @code{SIGIO} signals.
> >  @xref{Interrupt Input}.
> > @@ -3576,6 +3587,10 @@ set_nonblock_flag (int desc, int value)
> >  
> >  @cindex file locks
> >  @cindex record locking
> > +This section describes "classic" record locks.  There is also a newer type
> > +of record lock that is associated with the open file table entry instead
> 
> You should make explicit mention of the distinction between file descriptor
> and file table entry here since it impacts the semantics of file-private
> locks on dup (as you note below).
> 

I'll disagree here. This section is about process-associated locks, so
I don't think it's really appropriate to make that distinction here.
The descriptor/file-table entry distinction doesn't have any bearing on
how process-associated locks work. I think we *do* need to make that
distinction in the file-private lock section.

> > +of the process.  @xref{File-private Locks}
> 
> The distinction has nothing to do with classical versus new, it has to do
> with the association of the lock and how it behaves. Please reword this
> to avoid quote "classic" distinction and instead talk about where and how
> the locks are applied.
> 
> Also note that documentation exists forever, and in 5 years the file table
> entry locks won't be "newer type", instead they will just be another type
> of lock. Thus avoid saying things like "newer type" and just say "open
> file table entry locks."
> 
> e.g.
> 
> This section describe record locks whose locks are associated with the
> process. There is also support for record locks associated with open file
> table entries instead of the process.  @xref{File-private Locks}.
> 
> Note the period after the @xref is expected style.
> 

Ok.

> > +
> >  The remaining @code{fcntl} commands are used to support @dfn{record
> >  locking}, which permits multiple cooperating programs to prevent each
> >  other from simultaneously accessing parts of a file in error-prone
> > @@ -3641,7 +3656,9 @@ the file.
> >  @item pid_t l_pid
> >  This field is the process ID (@pxref{Process Creation Concepts}) of the
> >  process holding the lock.  It is filled in by calling @code{fcntl} with
> > -the @code{F_GETLK} command, but is ignored when making a lock.
> > +the @code{F_GETLK} command, but is ignored when making a lock.  If the
> > +conflicting lock is a File-private lock (@pxref{File-private Locks}),
> > +then this field will be set to @math{-1}.
> >  @end table
> >  @end deftp
> >  
> > @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
> >  controlling access to a file.  There is still potential for access to
> >  the file by programs that don't use the lock protocol.
> >  
> > +@node File-private Locks
> > +@section File-private Locks
> > +
> > +In contrast to classic record locks (@pxref{File Locks}), file-private
> 
> See comments above about "classic" or "new type."
> 
> Please reword like "In contrast to process-associated record locks..."
> 
> > +locks are associated with an open file table entry rather than a
> > +process.  File-private locks set on an open file descriptor will never
> > +conflict with existing file-private locks set on that file descriptor.
> > +
> > +File-private locks are also inherited by child processes across
> > +@code{fork} (@pxref{Creating a Process}), along with the file
> 
> What about clone with/without CLONE_FILES?
> 

Ok, I'll add that in as well.

> > +descriptor.  For this reason, the @code{l_pid} field in @code{struct
> > +flock} is meaningless for file private locks.  For the @code{F_GETLK} and
> 
> It can't be meaningless, it has to have some meaning. Setting it to -1 means
> "the locks is associated with an open file descriptor and not a process."
> Please reword e.g. For this reason, the @code{l_pid} field in @code {struct
> flock} is set to @math{-1} for file private locks to indicate that the lock
> is associated with an open file table entry instead of a process... 
> 

Hmm ok...

> > +@code{F_GETLKP} commands, the @code{l_pid} value is always set to
> > +@math{-1} if the blocking lock is a file-private one.
> 
> OK.
> 
> > +
> > +Note that using @code{dup} (@pxref{Duplicating Descriptors}) to clone a
> 
> s/Note that using/Using/g
> 
> s/to clone/to copy/g -- We are creating a "copy" and that's the language
> we use throughout for dup, not "clone".
> 
> > +file descriptor does not give you a new instance of the open file, but
> > +instead just clones a reference to an existing open file.  Thus,
> 
> Suggest:
> ... but instead copies the reference to the existing file table entry
> for the open file.
> 

Ok.

> > +file-private locks set on a file descriptor cloned by @code{dup} will
> > +never conflict with file-private locks set on the original descriptor.
> 
> OK. The dup'd fd acts as if it was the original fd since it never came
> from a distinct call to open.
> 

Right.

> > +
> > +File-private locks always conflict with classic record locks, even if
> > +acquired by the same process or on the same open file descriptor.
> 
> OK.
> 
> > +
> > +File-private locks use the same @code{struct flock} as classic POSIX
> > +locks as an argument (@pxref{File Locks}) and the macros for the
> > +cmd values are also declared in the header file @file{fcntl.h}. To
> > +use them, the macro @code{_GNU_SOURCE} must be defined prior to
> > +including @file{fcntl.h}.
> 
> OK.
> 
> > +
> > +In contrast to the @code{F_SETLK} or @code{F_SETLKW} commands, any
> > +@code{struct flock} used as an argument to the @code{F_SETLKP} or
> > +@code{F_SETLKPW} commands must have the @code{l_pid} value set to
> > +@math{0}.
> 
> Why not -1? This means that between F_GETLK and F_SETLK I need
> to explicitly zero l_pid otherwise the F_SETLK fails?
> 

This change was suggested by Neil Brown in the comments on the LWN
article. My intent is to leave as little undefined behavior as
possible. If we ensure that this is set to something distinct then we
have the possibility to extend this interface in the future by overloading
the l_pid field.

As to why zero and not -1...I don't think it really matters. You'd need
to reset that value anyway since there's no guarantee that you'd get -1
back in that field on a F_GETLK or F_GETLKP call. In the event that
you're not reusing a struct flock, using 0 here makes it easier to use
a C99 initializer to set it up.

> > +
> > +@pindex fcntl.h.
> > +
> > +@deftypevr Macro int F_GETLKP
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should get information about a lock.  This command
> > +requires a third argument of type @w{@code{struct flock *}} to be passed
> > +to @code{fcntl}, so that the form of the call is:
> 
> Missing sentence?
> 

Oops, good catch.

> > +
> > +If there is a lock already in place that would block the lock described
> > +by the @var{lockp} argument, information about that lock overwrites
> > +@code{*@var{lockp}}.  Existing locks are not reported if they are
> > +compatible with making a new lock as specified.  Thus, you should
> > +specify a lock type of @code{F_WRLCK} if you want to find out about both
> > +read and write locks, or @code{F_RDLCK} if you want to find out about
> > +write locks only.
> 
> OK.
> 
> > +
> > +There might be more than one lock affecting the region specified by the
> > +@var{lockp} argument, but @code{fcntl} only returns information about
> > +one of them. 
> 
> Suggest adding more weasel words:
> 
> Which lock information is returned when multiple locks are present is
> unspecified.
> 
> Feel free to merge that with the sentence above into something more
> compact. I only want to say that you should be explicit that it is
> unspecified which lock information is returned if there are multiple
> locks overlapping the region of interest.
> 

Ok. Note that I copied a lot of this verbiage from the original File
Locking manual entry. It may be appropriate to update it with similar
weasel-wordage.

> > ... The @code{l_whence} member of the @var{lockp} structure is
> > +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> > +set to identify the locked region.
> > +
> > +If no lock applies, the only change to the @var{lockp} structure is to
> > +update the @code{l_type} to a value of @code{F_UNLCK}.
> > +
> > +The normal return value from @code{fcntl} with this command is an
> > +unspecified value other than @math{-1}, which is reserved to indicate an
> > +error.  The following @code{errno} error conditions are defined for
> > +this command:
> > +
> > +@table @code
> > +@item EBADF
> > +The @var{filedes} argument is invalid.
> > +
> > +@item EINVAL
> > +Either the @var{lockp} argument doesn't specify valid lock information,
> > +or the file associated with @var{filedes} doesn't support locks.
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +@comment fcntl.h
> > +@comment POSIX.1
> > +@deftypevr Macro int F_SETLKP
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should set or clear a lock.  This command requires a
> > +third argument of type @w{@code{struct flock *}} to be passed to
> > +@code{fcntl}, so that the form of the call is:
> > +
> > +@smallexample
> > +fcntl (@var{filedes}, F_SETLKP, @var{lockp})
> > +@end smallexample
> > +
> > +If the opened file already has a lock on any part of the
> > +region, the old lock on that part is replaced with the new lock.  You
> > +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> > +
> > +If the lock cannot be set, @code{fcntl} returns immediately with a value
> > +of @math{-1}.  This function does not block waiting for other tasks
> > +to release locks.  If @code{fcntl} succeeds, it returns a value other
> > +than @math{-1}.
> > +
> > +The following @code{errno} error conditions are defined for this
> > +function:
> > +
> > +@table @code
> > +@item EAGAIN
> > +@itemx EACCES
> > +The lock cannot be set because it is blocked by an existing lock on the
> > +file.  Some systems use @code{EAGAIN} in this case, and other systems
> > +use @code{EACCES}; your program should treat them alike, after
> > +@code{F_SETLKP}.  (@gnulinuxhurdsystems{} always use @code{EAGAIN}.)
> > +
> > +@item EBADF
> > +Either: the @var{filedes} argument is invalid; you requested a read lock
> > +but the @var{filedes} is not open for read access; or, you requested a
> > +write lock but the @var{filedes} is not open for write access.
> > +
> > +@item EINVAL
> > +Either the @var{lockp} argument doesn't specify valid lock information,
> > +or the file associated with @var{filedes} doesn't support locks.
> > +
> > +@item ENOLCK
> > +The system has run out of file lock resources; there are already too
> > +many file locks in place.
> > +
> > +Well-designed file systems never report this error, because they have no
> > +limitation on the number of locks.  However, you must still take account
> > +of the possibility of this error, as it could result from network access
> > +to a file system on another machine.
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +@comment fcntl.h
> > +@comment POSIX.1
> > +@deftypevr Macro int F_SETLKPW
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should set or clear a lock.  It is just like the
> > +@code{F_SETLKP} command, but causes the process to block (or wait)
> 
> s/block (or wait)/wait/g
> 
> > +until the request can be specified.
> 
> s/specified/completed/g
> 

Ok.

> > +
> > +This command requires a third argument of type @code{struct flock *}, as
> > +for the @code{F_SETLKP} command.
> > +
> > +The @code{fcntl} return values and errors are the same as for the
> > +@code{F_SETLKP} command, but these additional @code{errno} error conditions
> > +are defined for this command:
> > +
> > +@table @code
> > +@item EINTR
> > +The function was interrupted by a signal while it was waiting.
> > +@xref{Interrupted Primitives}.
> > +
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +File-private locks are useful in the same sorts of situations as classic
> > +record locks.  They can also be used to synchronize file access between
> > +threads within the same process by giving each thread its own open file
> > +instance.
> > +
> > +Because they are only released automatically when the last reference to
> > +an open file is destroyed, file-private locks allow more assurance that
> > +the locks will not be released due to a library routine opening and
> > +closing a file without the application being aware.
> > +
> > +As with classic record locks, file-private locks are also voluntary.
> 
> Is it too much to ask for an example?
> 
> Like the threaded example in https://lwn.net/Articles/586904/ ? :-)
> 

Sure, sounds like a good idea.

> > +
> >  @node Interrupt Input
> >  @section Interrupt-Driven Input
>   
> Please repost a new v2 when you get a chance.
> 

Will do. Thanks for the review!
  

On 04/12/2014 07:03 AM, Jeff Layton wrote:
> On Fri, 11 Apr 2014 20:29:32 -0400
> "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
>> On 04/11/2014 08:55 AM, Jeff Layton wrote:
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>>  ChangeLog        |   1 +
>>>  manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 182 insertions(+), 2 deletions(-)
>>
>> You are a great developer, submitting documentation along
>> with new interfaces! You have all of my attention :-)
>>
>> Same comments about ChangeLog and BZ apply.
>>
>> The ChangeLog here should be:
>>
>> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
>>
>> 	* manual/llio.texi: add section about file-private locks
>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> index 55a84e598e46..622315629a44 100644
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -2,6 +2,7 @@
>>>  
>>>  	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
>>>  	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
>>> +	* manual/llio.texi: add section about file-private locks
>>
>> Other way around.
>>
>> If you are batching your ChangeLog's you'd write:
>>
>> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
>>
>> 	* manual/llio.texi: add section about file-private locks
>>
>> 	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
>> 	(F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
>>
>> With a space between them to indicate they were distinct
>> commits but on the same date.
>>   
>>>  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
>>>  
>>> diff --git a/manual/llio.texi b/manual/llio.texi
>>> index 6f8adfc607d7..bcaf0e419a90 100644
>>> --- a/manual/llio.texi
>>> +++ b/manual/llio.texi
>>> @@ -57,6 +57,8 @@ directly.)
>>>                                           flags associated with open files.
>>>  * File Locks::                          Fcntl commands for implementing
>>>                                           file locking.
>>> +* File-private Locks::                  Fcntl commands for implementing
>>> +                                         file-private locking.
>>
>> OK.
>>
>>>  * Interrupt Input::                     Getting an asynchronous signal when
>>>                                           input arrives.
>>>  * IOCTLs::                              Generic I/O Control operations.
>>> @@ -2890,7 +2892,7 @@ Get flags associated with the open file.  @xref{File Status Flags}.
>>>  Set flags associated with the open file.  @xref{File Status Flags}.
>>>  
>>>  @item F_GETLK
>>> -Get a file lock.  @xref{File Locks}.
>>> +Get (test) a file lock.  @xref{File Locks}.
>>
>> Why the parenthetical comment? Please write out what you'd like to say.
>>   
> 
> Ok.
> 
>>>  @item F_SETLK
>>>  Set or clear a file lock.  @xref{File Locks}.
>>> @@ -2898,6 +2900,15 @@ Set or clear a file lock.  @xref{File Locks}.
>>>  @item F_SETLKW
>>>  Like @code{F_SETLK}, but wait for completion.  @xref{File Locks}.
>>>  
>>> +@item F_GETLKP
>>> +Get (test) a file-private lock.  @xref{File Locks}.
>>> +
>>> +@item F_SETLKP
>>> +Set or clear a file lock.  @xref{File Locks}.
>>> +
>>> +@item F_SETLKPW
>>> +Like @code{F_SETLKP}, but wait for completion.  @xref{File Locks}.
>>
>> If these three are Linux specific please say so at the end of each
>> e.g. `Specific to Linux.'
>>
>> We have kFreeBSD and Hurd using glibc and I would like to know which
>> constants apply to which OS. Although kFreeBSD is not upstream we
>> still have Hurd.
>>
> 
> These are currently linux-specific, but eventually I'd like to see
> other OS' adopt this as well. I'm attempting to get this incorporated
> as part of the POSIX spec, but that will probably take a while.

Until they are in POSIX please annotate them as being specific to Linux.
We can then revise the documentation once they are generic.

>>> +
>>>  @item F_GETOWN
>>>  Get process or process group ID to receive @code{SIGIO} signals.
>>>  @xref{Interrupt Input}.
>>> @@ -3576,6 +3587,10 @@ set_nonblock_flag (int desc, int value)
>>>  
>>>  @cindex file locks
>>>  @cindex record locking
>>> +This section describes "classic" record locks.  There is also a newer type
>>> +of record lock that is associated with the open file table entry instead
>>
>> You should make explicit mention of the distinction between file descriptor
>> and file table entry here since it impacts the semantics of file-private
>> locks on dup (as you note below).
>>
> 
> I'll disagree here. This section is about process-associated locks, so
> I don't think it's really appropriate to make that distinction here.
> The descriptor/file-table entry distinction doesn't have any bearing on
> how process-associated locks work. I think we *do* need to make that
> distinction in the file-private lock section.

I agree, I'd lost track that we were in the process-associated locks section.

>>> +of the process.  @xref{File-private Locks}
>>
>> The distinction has nothing to do with classical versus new, it has to do
>> with the association of the lock and how it behaves. Please reword this
>> to avoid quote "classic" distinction and instead talk about where and how
>> the locks are applied.
>>
>> Also note that documentation exists forever, and in 5 years the file table
>> entry locks won't be "newer type", instead they will just be another type
>> of lock. Thus avoid saying things like "newer type" and just say "open
>> file table entry locks."
>>
>> e.g.
>>
>> This section describe record locks whose locks are associated with the
>> process. There is also support for record locks associated with open file
>> table entries instead of the process.  @xref{File-private Locks}.
>>
>> Note the period after the @xref is expected style.
>>
> 
> Ok.
> 
>>> +
>>>  The remaining @code{fcntl} commands are used to support @dfn{record
>>>  locking}, which permits multiple cooperating programs to prevent each
>>>  other from simultaneously accessing parts of a file in error-prone
>>> @@ -3641,7 +3656,9 @@ the file.
>>>  @item pid_t l_pid
>>>  This field is the process ID (@pxref{Process Creation Concepts}) of the
>>>  process holding the lock.  It is filled in by calling @code{fcntl} with
>>> -the @code{F_GETLK} command, but is ignored when making a lock.
>>> +the @code{F_GETLK} command, but is ignored when making a lock.  If the
>>> +conflicting lock is a File-private lock (@pxref{File-private Locks}),
>>> +then this field will be set to @math{-1}.
>>>  @end table
>>>  @end deftp
>>>  
>>> @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
>>>  controlling access to a file.  There is still potential for access to
>>>  the file by programs that don't use the lock protocol.
>>>  
>>> +@node File-private Locks
>>> +@section File-private Locks
>>> +
>>> +In contrast to classic record locks (@pxref{File Locks}), file-private
>>
>> See comments above about "classic" or "new type."
>>
>> Please reword like "In contrast to process-associated record locks..."
>>
>>> +locks are associated with an open file table entry rather than a
>>> +process.  File-private locks set on an open file descriptor will never
>>> +conflict with existing file-private locks set on that file descriptor.
>>> +
>>> +File-private locks are also inherited by child processes across
>>> +@code{fork} (@pxref{Creating a Process}), along with the file
>>
>> What about clone with/without CLONE_FILES?
>>
> 
> Ok, I'll add that in as well.

Thanks.

>>> +descriptor.  For this reason, the @code{l_pid} field in @code{struct
>>> +flock} is meaningless for file private locks.  For the @code{F_GETLK} and
>>
>> It can't be meaningless, it has to have some meaning. Setting it to -1 means
>> "the locks is associated with an open file descriptor and not a process."
>> Please reword e.g. For this reason, the @code{l_pid} field in @code {struct
>> flock} is set to @math{-1} for file private locks to indicate that the lock
>> is associated with an open file table entry instead of a process... 
>>
> 
> Hmm ok...

On second thought we should not be that proscriptive. We should just say that
if l_pid is -1 that the lock is not associated with a process, but leave
it at that. We may eventually come up with other lock types that are not
associated with a process and those locks can also set l_pid to -1.

>>> +@code{F_GETLKP} commands, the @code{l_pid} value is always set to
>>> +@math{-1} if the blocking lock is a file-private one.
>>
>> OK.
>>
>>> +
>>> +Note that using @code{dup} (@pxref{Duplicating Descriptors}) to clone a
>>
>> s/Note that using/Using/g
>>
>> s/to clone/to copy/g -- We are creating a "copy" and that's the language
>> we use throughout for dup, not "clone".
>>
>>> +file descriptor does not give you a new instance of the open file, but
>>> +instead just clones a reference to an existing open file.  Thus,
>>
>> Suggest:
>> ... but instead copies the reference to the existing file table entry
>> for the open file.
>>
> 
> Ok.
> 
>>> +file-private locks set on a file descriptor cloned by @code{dup} will
>>> +never conflict with file-private locks set on the original descriptor.
>>
>> OK. The dup'd fd acts as if it was the original fd since it never came
>> from a distinct call to open.
>>
> 
> Right.
> 
>>> +
>>> +File-private locks always conflict with classic record locks, even if
>>> +acquired by the same process or on the same open file descriptor.
>>
>> OK.
>>
>>> +
>>> +File-private locks use the same @code{struct flock} as classic POSIX
>>> +locks as an argument (@pxref{File Locks}) and the macros for the
>>> +cmd values are also declared in the header file @file{fcntl.h}. To
>>> +use them, the macro @code{_GNU_SOURCE} must be defined prior to
>>> +including @file{fcntl.h}.
>>
>> OK.
>>
>>> +
>>> +In contrast to the @code{F_SETLK} or @code{F_SETLKW} commands, any
>>> +@code{struct flock} used as an argument to the @code{F_SETLKP} or
>>> +@code{F_SETLKPW} commands must have the @code{l_pid} value set to
>>> +@math{0}.
>>
>> Why not -1? This means that between F_GETLK and F_SETLK I need
>> to explicitly zero l_pid otherwise the F_SETLK fails?
>>
> 
> This change was suggested by Neil Brown in the comments on the LWN
> article. My intent is to leave as little undefined behavior as
> possible. If we ensure that this is set to something distinct then we
> have the possibility to extend this interface in the future by overloading
> the l_pid field.
> 
> As to why zero and not -1...I don't think it really matters. You'd need
> to reset that value anyway since there's no guarantee that you'd get -1
> back in that field on a F_GETLK or F_GETLKP call. In the event that
> you're not reusing a struct flock, using 0 here makes it easier to use
> a C99 initializer to set it up.

In that case I think we should explicitly call out that l_pid needs to
be checked and cleared before subsequent calls to F_GETLK/F_GETLKP.

OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?

>>> +
>>> +@pindex fcntl.h.
>>> +
>>> +@deftypevr Macro int F_GETLKP
>>> +This macro is used as the @var{command} argument to @code{fcntl}, to
>>> +specify that it should get information about a lock.  This command
>>> +requires a third argument of type @w{@code{struct flock *}} to be passed
>>> +to @code{fcntl}, so that the form of the call is:
>>
>> Missing sentence?
>>
> 
> Oops, good catch.
> 
>>> +
>>> +If there is a lock already in place that would block the lock described
>>> +by the @var{lockp} argument, information about that lock overwrites
>>> +@code{*@var{lockp}}.  Existing locks are not reported if they are
>>> +compatible with making a new lock as specified.  Thus, you should
>>> +specify a lock type of @code{F_WRLCK} if you want to find out about both
>>> +read and write locks, or @code{F_RDLCK} if you want to find out about
>>> +write locks only.
>>
>> OK.
>>
>>> +
>>> +There might be more than one lock affecting the region specified by the
>>> +@var{lockp} argument, but @code{fcntl} only returns information about
>>> +one of them. 
>>
>> Suggest adding more weasel words:
>>
>> Which lock information is returned when multiple locks are present is
>> unspecified.
>>
>> Feel free to merge that with the sentence above into something more
>> compact. I only want to say that you should be explicit that it is
>> unspecified which lock information is returned if there are multiple
>> locks overlapping the region of interest.
>>
> 
> Ok. Note that I copied a lot of this verbiage from the original File
> Locking manual entry. It may be appropriate to update it with similar
> weasel-wordage.

Sure. I hope that when you say "manual entry" you mean the glibc manual,
otherwise there are copyright issues if you copied this from somewhere
else.

>>> ... The @code{l_whence} member of the @var{lockp} structure is
>>> +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
>>> +set to identify the locked region.
>>> +
>>> +If no lock applies, the only change to the @var{lockp} structure is to
>>> +update the @code{l_type} to a value of @code{F_UNLCK}.
>>> +
>>> +The normal return value from @code{fcntl} with this command is an
>>> +unspecified value other than @math{-1}, which is reserved to indicate an
>>> +error.  The following @code{errno} error conditions are defined for
>>> +this command:
>>> +
>>> +@table @code
>>> +@item EBADF
>>> +The @var{filedes} argument is invalid.
>>> +
>>> +@item EINVAL
>>> +Either the @var{lockp} argument doesn't specify valid lock information,
>>> +or the file associated with @var{filedes} doesn't support locks.
>>> +@end table
>>> +@end deftypevr
>>
>> OK.
>>
>>> +
>>> +@comment fcntl.h
>>> +@comment POSIX.1
>>> +@deftypevr Macro int F_SETLKP
>>> +This macro is used as the @var{command} argument to @code{fcntl}, to
>>> +specify that it should set or clear a lock.  This command requires a
>>> +third argument of type @w{@code{struct flock *}} to be passed to
>>> +@code{fcntl}, so that the form of the call is:
>>> +
>>> +@smallexample
>>> +fcntl (@var{filedes}, F_SETLKP, @var{lockp})
>>> +@end smallexample
>>> +
>>> +If the opened file already has a lock on any part of the
>>> +region, the old lock on that part is replaced with the new lock.  You
>>> +can remove a lock by specifying a lock type of @code{F_UNLCK}.
>>> +
>>> +If the lock cannot be set, @code{fcntl} returns immediately with a value
>>> +of @math{-1}.  This function does not block waiting for other tasks
>>> +to release locks.  If @code{fcntl} succeeds, it returns a value other
>>> +than @math{-1}.
>>> +
>>> +The following @code{errno} error conditions are defined for this
>>> +function:
>>> +
>>> +@table @code
>>> +@item EAGAIN
>>> +@itemx EACCES
>>> +The lock cannot be set because it is blocked by an existing lock on the
>>> +file.  Some systems use @code{EAGAIN} in this case, and other systems
>>> +use @code{EACCES}; your program should treat them alike, after
>>> +@code{F_SETLKP}.  (@gnulinuxhurdsystems{} always use @code{EAGAIN}.)
>>> +
>>> +@item EBADF
>>> +Either: the @var{filedes} argument is invalid; you requested a read lock
>>> +but the @var{filedes} is not open for read access; or, you requested a
>>> +write lock but the @var{filedes} is not open for write access.
>>> +
>>> +@item EINVAL
>>> +Either the @var{lockp} argument doesn't specify valid lock information,
>>> +or the file associated with @var{filedes} doesn't support locks.
>>> +
>>> +@item ENOLCK
>>> +The system has run out of file lock resources; there are already too
>>> +many file locks in place.
>>> +
>>> +Well-designed file systems never report this error, because they have no
>>> +limitation on the number of locks.  However, you must still take account
>>> +of the possibility of this error, as it could result from network access
>>> +to a file system on another machine.
>>> +@end table
>>> +@end deftypevr
>>
>> OK.
>>
>>> +
>>> +@comment fcntl.h
>>> +@comment POSIX.1
>>> +@deftypevr Macro int F_SETLKPW
>>> +This macro is used as the @var{command} argument to @code{fcntl}, to
>>> +specify that it should set or clear a lock.  It is just like the
>>> +@code{F_SETLKP} command, but causes the process to block (or wait)
>>
>> s/block (or wait)/wait/g
>>
>>> +until the request can be specified.
>>
>> s/specified/completed/g
>>
> 
> Ok.
> 
>>> +
>>> +This command requires a third argument of type @code{struct flock *}, as
>>> +for the @code{F_SETLKP} command.
>>> +
>>> +The @code{fcntl} return values and errors are the same as for the
>>> +@code{F_SETLKP} command, but these additional @code{errno} error conditions
>>> +are defined for this command:
>>> +
>>> +@table @code
>>> +@item EINTR
>>> +The function was interrupted by a signal while it was waiting.
>>> +@xref{Interrupted Primitives}.
>>> +
>>> +@end table
>>> +@end deftypevr
>>
>> OK.
>>
>>> +
>>> +File-private locks are useful in the same sorts of situations as classic
>>> +record locks.  They can also be used to synchronize file access between
>>> +threads within the same process by giving each thread its own open file
>>> +instance.
>>> +
>>> +Because they are only released automatically when the last reference to
>>> +an open file is destroyed, file-private locks allow more assurance that
>>> +the locks will not be released due to a library routine opening and
>>> +closing a file without the application being aware.
>>> +
>>> +As with classic record locks, file-private locks are also voluntary.
>>
>> Is it too much to ask for an example?
>>
>> Like the threaded example in https://lwn.net/Articles/586904/ ? :-)
>>
> 
> Sure, sounds like a good idea.

Thanks. Look at how other examples are distinct *.c files and included into
the texinfo.

>>> +
>>>  @node Interrupt Input
>>>  @section Interrupt-Driven Input
>>   
>> Please repost a new v2 when you get a chance.
>>
> 
> Will do. Thanks for the review!
> 

Very welcome.

Cheers,
Carlos.
  

On Sat, 12 Apr 2014 12:52:33 -0400
"Carlos O'Donell" <carlos@redhat.com> wrote:

> On 04/12/2014 07:03 AM, Jeff Layton wrote:
> > On Fri, 11 Apr 2014 20:29:32 -0400
> > "Carlos O'Donell" <carlos@redhat.com> wrote:
> > 
> >> On 04/11/2014 08:55 AM, Jeff Layton wrote:
> >>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> >>> ---
> >>>  ChangeLog        |   1 +
> >>>  manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  2 files changed, 182 insertions(+), 2 deletions(-)
> >>
> >> You are a great developer, submitting documentation along
> >> with new interfaces! You have all of my attention :-)
> >>
> >> Same comments about ChangeLog and BZ apply.
> >>
> >> The ChangeLog here should be:
> >>
> >> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> >>
> >> 	* manual/llio.texi: add section about file-private locks
> >>
> >>> diff --git a/ChangeLog b/ChangeLog
> >>> index 55a84e598e46..622315629a44 100644
> >>> --- a/ChangeLog
> >>> +++ b/ChangeLog
> >>> @@ -2,6 +2,7 @@
> >>>  
> >>>  	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> >>>  	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> >>> +	* manual/llio.texi: add section about file-private locks
> >>
> >> Other way around.
> >>
> >> If you are batching your ChangeLog's you'd write:
> >>
> >> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> >>
> >> 	* manual/llio.texi: add section about file-private locks
> >>
> >> 	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> >> 	(F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> >>
> >> With a space between them to indicate they were distinct
> >> commits but on the same date.
> >>   
> >>>  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
> >>>  
> >>> diff --git a/manual/llio.texi b/manual/llio.texi
> >>> index 6f8adfc607d7..bcaf0e419a90 100644
> >>> --- a/manual/llio.texi
> >>> +++ b/manual/llio.texi
> >>> @@ -57,6 +57,8 @@ directly.)
> >>>                                           flags associated with open files.
> >>>  * File Locks::                          Fcntl commands for implementing
> >>>                                           file locking.
> >>> +* File-private Locks::                  Fcntl commands for implementing
> >>> +                                         file-private locking.
> >>
> >> OK.
> >>
> >>>  * Interrupt Input::                     Getting an asynchronous signal when
> >>>                                           input arrives.
> >>>  * IOCTLs::                              Generic I/O Control operations.
> >>> @@ -2890,7 +2892,7 @@ Get flags associated with the open file.  @xref{File Status Flags}.
> >>>  Set flags associated with the open file.  @xref{File Status Flags}.
> >>>  
> >>>  @item F_GETLK
> >>> -Get a file lock.  @xref{File Locks}.
> >>> +Get (test) a file lock.  @xref{File Locks}.
> >>
> >> Why the parenthetical comment? Please write out what you'd like to say.
> >>   
> > 
> > Ok.
> > 
> >>>  @item F_SETLK
> >>>  Set or clear a file lock.  @xref{File Locks}.
> >>> @@ -2898,6 +2900,15 @@ Set or clear a file lock.  @xref{File Locks}.
> >>>  @item F_SETLKW
> >>>  Like @code{F_SETLK}, but wait for completion.  @xref{File Locks}.
> >>>  
> >>> +@item F_GETLKP
> >>> +Get (test) a file-private lock.  @xref{File Locks}.
> >>> +
> >>> +@item F_SETLKP
> >>> +Set or clear a file lock.  @xref{File Locks}.
> >>> +
> >>> +@item F_SETLKPW
> >>> +Like @code{F_SETLKP}, but wait for completion.  @xref{File Locks}.
> >>
> >> If these three are Linux specific please say so at the end of each
> >> e.g. `Specific to Linux.'
> >>
> >> We have kFreeBSD and Hurd using glibc and I would like to know which
> >> constants apply to which OS. Although kFreeBSD is not upstream we
> >> still have Hurd.
> >>
> > 
> > These are currently linux-specific, but eventually I'd like to see
> > other OS' adopt this as well. I'm attempting to get this incorporated
> > as part of the POSIX spec, but that will probably take a while.
> 
> Until they are in POSIX please annotate them as being specific to Linux.
> We can then revise the documentation once they are generic.
> 

Ok.

> >>> +
> >>>  @item F_GETOWN
> >>>  Get process or process group ID to receive @code{SIGIO} signals.
> >>>  @xref{Interrupt Input}.
> >>> @@ -3576,6 +3587,10 @@ set_nonblock_flag (int desc, int value)
> >>>  
> >>>  @cindex file locks
> >>>  @cindex record locking
> >>> +This section describes "classic" record locks.  There is also a newer type
> >>> +of record lock that is associated with the open file table entry instead
> >>
> >> You should make explicit mention of the distinction between file descriptor
> >> and file table entry here since it impacts the semantics of file-private
> >> locks on dup (as you note below).
> >>
> > 
> > I'll disagree here. This section is about process-associated locks, so
> > I don't think it's really appropriate to make that distinction here.
> > The descriptor/file-table entry distinction doesn't have any bearing on
> > how process-associated locks work. I think we *do* need to make that
> > distinction in the file-private lock section.
> 
> I agree, I'd lost track that we were in the process-associated locks section.
> 
> >>> +of the process.  @xref{File-private Locks}
> >>
> >> The distinction has nothing to do with classical versus new, it has to do
> >> with the association of the lock and how it behaves. Please reword this
> >> to avoid quote "classic" distinction and instead talk about where and how
> >> the locks are applied.
> >>
> >> Also note that documentation exists forever, and in 5 years the file table
> >> entry locks won't be "newer type", instead they will just be another type
> >> of lock. Thus avoid saying things like "newer type" and just say "open
> >> file table entry locks."
> >>
> >> e.g.
> >>
> >> This section describe record locks whose locks are associated with the
> >> process. There is also support for record locks associated with open file
> >> table entries instead of the process.  @xref{File-private Locks}.
> >>
> >> Note the period after the @xref is expected style.
> >>
> > 
> > Ok.
> > 
> >>> +
> >>>  The remaining @code{fcntl} commands are used to support @dfn{record
> >>>  locking}, which permits multiple cooperating programs to prevent each
> >>>  other from simultaneously accessing parts of a file in error-prone
> >>> @@ -3641,7 +3656,9 @@ the file.
> >>>  @item pid_t l_pid
> >>>  This field is the process ID (@pxref{Process Creation Concepts}) of the
> >>>  process holding the lock.  It is filled in by calling @code{fcntl} with
> >>> -the @code{F_GETLK} command, but is ignored when making a lock.
> >>> +the @code{F_GETLK} command, but is ignored when making a lock.  If the
> >>> +conflicting lock is a File-private lock (@pxref{File-private Locks}),
> >>> +then this field will be set to @math{-1}.
> >>>  @end table
> >>>  @end deftp
> >>>  
> >>> @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
> >>>  controlling access to a file.  There is still potential for access to
> >>>  the file by programs that don't use the lock protocol.
> >>>  
> >>> +@node File-private Locks
> >>> +@section File-private Locks
> >>> +
> >>> +In contrast to classic record locks (@pxref{File Locks}), file-private
> >>
> >> See comments above about "classic" or "new type."
> >>
> >> Please reword like "In contrast to process-associated record locks..."
> >>
> >>> +locks are associated with an open file table entry rather than a
> >>> +process.  File-private locks set on an open file descriptor will never
> >>> +conflict with existing file-private locks set on that file descriptor.
> >>> +
> >>> +File-private locks are also inherited by child processes across
> >>> +@code{fork} (@pxref{Creating a Process}), along with the file
> >>
> >> What about clone with/without CLONE_FILES?
> >>
> > 
> > Ok, I'll add that in as well.
> 
> Thanks.
> 
> >>> +descriptor.  For this reason, the @code{l_pid} field in @code{struct
> >>> +flock} is meaningless for file private locks.  For the @code{F_GETLK} and
> >>
> >> It can't be meaningless, it has to have some meaning. Setting it to -1 means
> >> "the locks is associated with an open file descriptor and not a process."
> >> Please reword e.g. For this reason, the @code{l_pid} field in @code {struct
> >> flock} is set to @math{-1} for file private locks to indicate that the lock
> >> is associated with an open file table entry instead of a process... 
> >>
> > 
> > Hmm ok...
> 
> On second thought we should not be that proscriptive. We should just say that
> if l_pid is -1 that the lock is not associated with a process, but leave
> it at that. We may eventually come up with other lock types that are not
> associated with a process and those locks can also set l_pid to -1.
> 

Ok, sounds good.

> >>> +@code{F_GETLKP} commands, the @code{l_pid} value is always set to
> >>> +@math{-1} if the blocking lock is a file-private one.
> >>
> >> OK.
> >>
> >>> +
> >>> +Note that using @code{dup} (@pxref{Duplicating Descriptors}) to clone a
> >>
> >> s/Note that using/Using/g
> >>
> >> s/to clone/to copy/g -- We are creating a "copy" and that's the language
> >> we use throughout for dup, not "clone".
> >>
> >>> +file descriptor does not give you a new instance of the open file, but
> >>> +instead just clones a reference to an existing open file.  Thus,
> >>
> >> Suggest:
> >> ... but instead copies the reference to the existing file table entry
> >> for the open file.
> >>
> > 
> > Ok.
> > 
> >>> +file-private locks set on a file descriptor cloned by @code{dup} will
> >>> +never conflict with file-private locks set on the original descriptor.
> >>
> >> OK. The dup'd fd acts as if it was the original fd since it never came
> >> from a distinct call to open.
> >>
> > 
> > Right.
> > 
> >>> +
> >>> +File-private locks always conflict with classic record locks, even if
> >>> +acquired by the same process or on the same open file descriptor.
> >>
> >> OK.
> >>
> >>> +
> >>> +File-private locks use the same @code{struct flock} as classic POSIX
> >>> +locks as an argument (@pxref{File Locks}) and the macros for the
> >>> +cmd values are also declared in the header file @file{fcntl.h}. To
> >>> +use them, the macro @code{_GNU_SOURCE} must be defined prior to
> >>> +including @file{fcntl.h}.
> >>
> >> OK.
> >>
> >>> +
> >>> +In contrast to the @code{F_SETLK} or @code{F_SETLKW} commands, any
> >>> +@code{struct flock} used as an argument to the @code{F_SETLKP} or
> >>> +@code{F_SETLKPW} commands must have the @code{l_pid} value set to
> >>> +@math{0}.
> >>
> >> Why not -1? This means that between F_GETLK and F_SETLK I need
> >> to explicitly zero l_pid otherwise the F_SETLK fails?
> >>
> > 
> > This change was suggested by Neil Brown in the comments on the LWN
> > article. My intent is to leave as little undefined behavior as
> > possible. If we ensure that this is set to something distinct then we
> > have the possibility to extend this interface in the future by overloading
> > the l_pid field.
> > 
> > As to why zero and not -1...I don't think it really matters. You'd need
> > to reset that value anyway since there's no guarantee that you'd get -1
> > back in that field on a F_GETLK or F_GETLKP call. In the event that
> > you're not reusing a struct flock, using 0 here makes it easier to use
> > a C99 initializer to set it up.
> 
> In that case I think we should explicitly call out that l_pid needs to
> be checked and cleared before subsequent calls to F_GETLK/F_GETLKP.
> 

Ummm, I guess. Code that calls F_GETLK and then F_SETLK is inherently
racy though (someone could acquire the lock just after you test it).
While we could warn about this, there are enough other gotchas that I'm
not sure warning about it is really helpful in practice...


> OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?
> 


Sorry, forgot to answer this:

"P" is for "Private". What can I say...I suck at naming things, and no
one else proposed anything better. It's not too late to change that if
you can come up with a better moniker, but we'd need to move fast
before v3.15 ships.


> >>> +
> >>> +@pindex fcntl.h.
> >>> +
> >>> +@deftypevr Macro int F_GETLKP
> >>> +This macro is used as the @var{command} argument to @code{fcntl}, to
> >>> +specify that it should get information about a lock.  This command
> >>> +requires a third argument of type @w{@code{struct flock *}} to be passed
> >>> +to @code{fcntl}, so that the form of the call is:
> >>
> >> Missing sentence?
> >>
> > 
> > Oops, good catch.
> > 
> >>> +
> >>> +If there is a lock already in place that would block the lock described
> >>> +by the @var{lockp} argument, information about that lock overwrites
> >>> +@code{*@var{lockp}}.  Existing locks are not reported if they are
> >>> +compatible with making a new lock as specified.  Thus, you should
> >>> +specify a lock type of @code{F_WRLCK} if you want to find out about both
> >>> +read and write locks, or @code{F_RDLCK} if you want to find out about
> >>> +write locks only.
> >>
> >> OK.
> >>
> >>> +
> >>> +There might be more than one lock affecting the region specified by the
> >>> +@var{lockp} argument, but @code{fcntl} only returns information about
> >>> +one of them. 
> >>
> >> Suggest adding more weasel words:
> >>
> >> Which lock information is returned when multiple locks are present is
> >> unspecified.
> >>
> >> Feel free to merge that with the sentence above into something more
> >> compact. I only want to say that you should be explicit that it is
> >> unspecified which lock information is returned if there are multiple
> >> locks overlapping the region of interest.
> >>
> > 
> > Ok. Note that I copied a lot of this verbiage from the original File
> > Locking manual entry. It may be appropriate to update it with similar
> > weasel-wordage.
> 
> Sure. I hope that when you say "manual entry" you mean the glibc manual,
> otherwise there are copyright issues if you copied this from somewhere
> else.
> 

Yes, I copied the glibc manual.

> >>> ... The @code{l_whence} member of the @var{lockp} structure is
> >>> +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> >>> +set to identify the locked region.
> >>> +
> >>> +If no lock applies, the only change to the @var{lockp} structure is to
> >>> +update the @code{l_type} to a value of @code{F_UNLCK}.
> >>> +
> >>> +The normal return value from @code{fcntl} with this command is an
> >>> +unspecified value other than @math{-1}, which is reserved to indicate an
> >>> +error.  The following @code{errno} error conditions are defined for
> >>> +this command:
> >>> +
> >>> +@table @code
> >>> +@item EBADF
> >>> +The @var{filedes} argument is invalid.
> >>> +
> >>> +@item EINVAL
> >>> +Either the @var{lockp} argument doesn't specify valid lock information,
> >>> +or the file associated with @var{filedes} doesn't support locks.
> >>> +@end table
> >>> +@end deftypevr
> >>
> >> OK.
> >>
> >>> +
> >>> +@comment fcntl.h
> >>> +@comment POSIX.1
> >>> +@deftypevr Macro int F_SETLKP
> >>> +This macro is used as the @var{command} argument to @code{fcntl}, to
> >>> +specify that it should set or clear a lock.  This command requires a
> >>> +third argument of type @w{@code{struct flock *}} to be passed to
> >>> +@code{fcntl}, so that the form of the call is:
> >>> +
> >>> +@smallexample
> >>> +fcntl (@var{filedes}, F_SETLKP, @var{lockp})
> >>> +@end smallexample
> >>> +
> >>> +If the opened file already has a lock on any part of the
> >>> +region, the old lock on that part is replaced with the new lock.  You
> >>> +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> >>> +
> >>> +If the lock cannot be set, @code{fcntl} returns immediately with a value
> >>> +of @math{-1}.  This function does not block waiting for other tasks
> >>> +to release locks.  If @code{fcntl} succeeds, it returns a value other
> >>> +than @math{-1}.
> >>> +
> >>> +The following @code{errno} error conditions are defined for this
> >>> +function:
> >>> +
> >>> +@table @code
> >>> +@item EAGAIN
> >>> +@itemx EACCES
> >>> +The lock cannot be set because it is blocked by an existing lock on the
> >>> +file.  Some systems use @code{EAGAIN} in this case, and other systems
> >>> +use @code{EACCES}; your program should treat them alike, after
> >>> +@code{F_SETLKP}.  (@gnulinuxhurdsystems{} always use @code{EAGAIN}.)
> >>> +
> >>> +@item EBADF
> >>> +Either: the @var{filedes} argument is invalid; you requested a read lock
> >>> +but the @var{filedes} is not open for read access; or, you requested a
> >>> +write lock but the @var{filedes} is not open for write access.
> >>> +
> >>> +@item EINVAL
> >>> +Either the @var{lockp} argument doesn't specify valid lock information,
> >>> +or the file associated with @var{filedes} doesn't support locks.
> >>> +
> >>> +@item ENOLCK
> >>> +The system has run out of file lock resources; there are already too
> >>> +many file locks in place.
> >>> +
> >>> +Well-designed file systems never report this error, because they have no
> >>> +limitation on the number of locks.  However, you must still take account
> >>> +of the possibility of this error, as it could result from network access
> >>> +to a file system on another machine.
> >>> +@end table
> >>> +@end deftypevr
> >>
> >> OK.
> >>
> >>> +
> >>> +@comment fcntl.h
> >>> +@comment POSIX.1
> >>> +@deftypevr Macro int F_SETLKPW
> >>> +This macro is used as the @var{command} argument to @code{fcntl}, to
> >>> +specify that it should set or clear a lock.  It is just like the
> >>> +@code{F_SETLKP} command, but causes the process to block (or wait)
> >>
> >> s/block (or wait)/wait/g
> >>
> >>> +until the request can be specified.
> >>
> >> s/specified/completed/g
> >>
> > 
> > Ok.
> > 
> >>> +
> >>> +This command requires a third argument of type @code{struct flock *}, as
> >>> +for the @code{F_SETLKP} command.
> >>> +
> >>> +The @code{fcntl} return values and errors are the same as for the
> >>> +@code{F_SETLKP} command, but these additional @code{errno} error conditions
> >>> +are defined for this command:
> >>> +
> >>> +@table @code
> >>> +@item EINTR
> >>> +The function was interrupted by a signal while it was waiting.
> >>> +@xref{Interrupted Primitives}.
> >>> +
> >>> +@end table
> >>> +@end deftypevr
> >>
> >> OK.
> >>
> >>> +
> >>> +File-private locks are useful in the same sorts of situations as classic
> >>> +record locks.  They can also be used to synchronize file access between
> >>> +threads within the same process by giving each thread its own open file
> >>> +instance.
> >>> +
> >>> +Because they are only released automatically when the last reference to
> >>> +an open file is destroyed, file-private locks allow more assurance that
> >>> +the locks will not be released due to a library routine opening and
> >>> +closing a file without the application being aware.
> >>> +
> >>> +As with classic record locks, file-private locks are also voluntary.
> >>
> >> Is it too much to ask for an example?
> >>
> >> Like the threaded example in https://lwn.net/Articles/586904/ ? :-)
> >>
> > 
> > Sure, sounds like a good idea.
> 
> Thanks. Look at how other examples are distinct *.c files and included into
> the texinfo.
> 

Will do.

> >>> +
> >>>  @node Interrupt Input
> >>>  @section Interrupt-Driven Input
> >>   
> >> Please repost a new v2 when you get a chance.
> >>
> > 
> > Will do. Thanks for the review!
> > 
> 
> Very welcome.
> 
> Cheers,
> Carlos.
  

On 04/11/2014 02:55 PM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

[...]

> @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for

s/voluntary/advisory/

>  controlling access to a file.  There is still potential for access to
>  the file by programs that don't use the lock protocol.
>  
> +@node File-private Locks
> +@section File-private Locks
> +
> +In contrast to classic record locks (@pxref{File Locks}), file-private
> +locks are associated with an open file table entry rather than a
> +process.  File-private locks set on an open file descriptor will never

s/File-private locks/New file-private locks/

> +conflict with existing file-private locks set on that file descriptor.

Perhaps add a sentence here that a lock conversion may be involved?

[...]

> +There might be more than one lock affecting the region specified by the
> +@var{lockp} argument, but @code{fcntl} only returns information about
> +one of them.  The @code{l_whence} member of the @var{lockp} structure is
> +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> +set to identify the locked region.
> +
> +If no lock applies, the only change to the @var{lockp} structure is to

s/If no lock applies/If no conflicting lock exists/

> +update the @code{l_type} to a value of @code{F_UNLCK}.

Slightly clumsy wording

s/update the @code{l_type} to a value of/
  update the @code{l_type} field to the value/

s/update the @code{l_type} to a value of/
  update @code{l_type} to the value/


> +The normal return value from @code{fcntl} with this command is an
> +unspecified value other than @math{-1}, which is reserved to indicate an
> +error.  

This doesn't sound correct. At the glibc level isn't the return value 
either 0 for success or -1 on error?

[...]

> +If the opened file already has a lock on any part of the
> +region, the old lock on that part is replaced with the new lock.  You
> +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> +
> +If the lock cannot be set, @code{fcntl} returns immediately with a value
> +of @math{-1}.  This function does not block waiting for other tasks
> +to release locks.  If @code{fcntl} succeeds, it returns a value other
> +than @math{-1}.

See comment above.

[...]

> +File-private locks are useful in the same sorts of situations as classic
> +record locks.  They can also be used to synchronize file access between
> +threads within the same process by giving each thread its own open file
> +instance.

I think this could be explained more clearly. How about:

    They can also be used to synchronize file access between threads
    within the same process by giving having each thread perform its
    own @code{open} of the file, to obtain its own open file instance.

> +Because they are only released automatically when the last reference to
> +an open file is destroyed, file-private locks allow more assurance that

Suggest s/destroyed/closed/ (that gives the reader a clue what the 
user-space operation is).

> +the locks will not be released due to a library routine opening and
> +closing a file without the application being aware.

I suggest rewording as:

    ..., file-private locks avoid the possibility that locks are
    released due to a library routine opening and closing a 
    file without the application being aware.

> +
> +As with classic record locks, file-private locks are also voluntary.

s/voluntary/advisory/

Cheers,

Michael
  

On Wed, 16 Apr 2014 15:13:45 +0200
"Michael Kerrisk (man-pages)" <mtk.lists@gmail.com> wrote:

> On 04/11/2014 02:55 PM, Jeff Layton wrote:
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> [...]
> 

You may want to look at the v3 patch I sent yesterday. It has some
changes, but I think your comments below still apply to it.


> > @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
> 
> s/voluntary/advisory/
> 

Ok...

> >  controlling access to a file.  There is still potential for access to
> >  the file by programs that don't use the lock protocol.
> >  
> > +@node File-private Locks
> > +@section File-private Locks
> > +
> > +In contrast to classic record locks (@pxref{File Locks}), file-private
> > +locks are associated with an open file table entry rather than a
> > +process.  File-private locks set on an open file descriptor will never
> 
> s/File-private locks/New file-private locks/
> 

Carlos suggested avoiding adjectives like "new" or "classic" because in
a year or two, these will no longer be "new". I've moved to using
"process-associated" instead of "classic" in the latest rev and
dropping the "new" moniker.

> > +conflict with existing file-private locks set on that file descriptor.
> 
> Perhaps add a sentence here that a lock conversion may be involved?
> 
> [...]
> 

Sure, sounds reasonable.

> > +There might be more than one lock affecting the region specified by the
> > +@var{lockp} argument, but @code{fcntl} only returns information about
> > +one of them.  The @code{l_whence} member of the @var{lockp} structure is
> > +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> > +set to identify the locked region.
> > +
> > +If no lock applies, the only change to the @var{lockp} structure is to
> 
> s/If no lock applies/If no conflicting lock exists/
> 
> > +update the @code{l_type} to a value of @code{F_UNLCK}.
> 
> Slightly clumsy wording
> 
> s/update the @code{l_type} to a value of/
>   update the @code{l_type} field to the value/
> 
> s/update the @code{l_type} to a value of/
>   update @code{l_type} to the value/
> 
> 
> > +The normal return value from @code{fcntl} with this command is an
> > +unspecified value other than @math{-1}, which is reserved to indicate an
> > +error.  
> 
> This doesn't sound correct. At the glibc level isn't the return value 
> either 0 for success or -1 on error?
> 
> [...]
> 

Good catch.  I copied that verbatim from the older File lock section.

That may be the case on non-linux kernels, but on linux, you'll get
back 0 on success. I think we'd want to mandate that for other OS' that
implement this as well. Will fix.

> > +If the opened file already has a lock on any part of the
> > +region, the old lock on that part is replaced with the new lock.  You
> > +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> > +
> > +If the lock cannot be set, @code{fcntl} returns immediately with a value
> > +of @math{-1}.  This function does not block waiting for other tasks
> > +to release locks.  If @code{fcntl} succeeds, it returns a value other
> > +than @math{-1}.
> 
> See comment above.
> 

Thanks, will fix.

> [...]
> 
> > +File-private locks are useful in the same sorts of situations as classic
> > +record locks.  They can also be used to synchronize file access between
> > +threads within the same process by giving each thread its own open file
> > +instance.
> 
> I think this could be explained more clearly. How about:
> 
>     They can also be used to synchronize file access between threads
>     within the same process by giving having each thread perform its
>     own @code{open} of the file, to obtain its own open file instance.
> 

Much better. I'll use that.

> > +Because they are only released automatically when the last reference to
> > +an open file is destroyed, file-private locks allow more assurance that
> 
> Suggest s/destroyed/closed/ (that gives the reader a clue what the 
> user-space operation is).
> 
> > +the locks will not be released due to a library routine opening and
> > +closing a file without the application being aware.
> 
> I suggest rewording as:
> 
>     ..., file-private locks avoid the possibility that locks are
>     released due to a library routine opening and closing a 
>     file without the application being aware.
> 

Much better. I'll use that too.

> > +
> > +As with classic record locks, file-private locks are also voluntary.
> 
> s/voluntary/advisory/
> 
> Cheers,
> 
> Michael

Thanks for the review. I'll plan to post v4 later today that
incorporates your suggestions once I give Carlos and others a chance to
comment. Once we get the manual sorted out here, I'll resubmit the fcntl
manpage patch.
  

Hi Jeff,

On Wed, Apr 16, 2014 at 3:48 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 16 Apr 2014 15:13:45 +0200
> "Michael Kerrisk (man-pages)" <mtk.lists@gmail.com> wrote:
>
>> On 04/11/2014 02:55 PM, Jeff Layton wrote:
>> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>
>> [...]
>>
>
> You may want to look at the v3 patch I sent yesterday. It has some
> changes, but I think your comments below still apply to it.

Ahhh -- I missed that. (Please CC mtk.manpages@mail.com on v4.) Still,
I tried to take case that I did not duplicate any of the points that
Carlos made, so I guess most of what I said was relevant˘seems so from
your feedback).

>> >  controlling access to a file.  There is still potential for access to
>> >  the file by programs that don't use the lock protocol.
>> >
>> > +@node File-private Locks
>> > +@section File-private Locks
>> > +
>> > +In contrast to classic record locks (@pxref{File Locks}), file-private
>> > +locks are associated with an open file table entry rather than a
>> > +process.  File-private locks set on an open file descriptor will never
>>
>> s/File-private locks/New file-private locks/
>>
>
> Carlos suggested avoiding adjectives like "new" or "classic" because in
> a year or two, these will no longer be "new". I've moved to using
> "process-associated" instead of "classic" in the latest rev and
> dropping the "new" moniker.

I think you've missed my point. Here, "new" is not about the "new type
of file lock" its about placing a "new" lock on a a file that has an
"existing" lock. Perhaps "new" is still not quite right though.
Perhaps something like:

[[
Using @code{fcntl} to apply a file-private lock on a region that
already has an existing file-private lock that was created via the
same open file descriptor will never cause a lock conflict...
]]

I have some comments on the naming that of this locking system that
I'll put elsewhere into this thread.

> Thanks for the review.

You're welcome. Thanks for doing the documentation. As Carlos pointed
out, it's a little exceptional that that happens.

Cheers,

Michael
  

Jeff, (and Carlos)

> On Sat, 12 Apr 2014 12:52:33 -0400
> "Carlos O'Donell" <carlos@redhat.com> wrote:

[...]


>> OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?

That does not strike me as OT at all. The existing byte-range locking
system has persisted (despite egregious faults) for well over two
decades. One supposes that Jeff's new improved version might be around
at least as long. With that in mind, and before setting in stone (and
pushing into POSIX) a model of thinking that thousands of programmers
will live with for a long time, it's worth thinking about names.

> Sorry, forgot to answer this:
>
> "P" is for "Private". What can I say...I suck at naming things, and no
> one else proposed anything better. It's not too late to change that if
> you can come up with a better moniker, but we'd need to move fast
> before v3.15 ships.

I agree on the "suck" bit ;-). Carlos nailed it when he referred to
traditional byte-range locks as being "process-associated". That is
the distinction between these two two locking APIs: one associates
locks with the process, the other associates them with a file handle.
The terminology should reflect that difference.

>From that perspective, the word "private" here makes no sense at all.
What does "private" mean in this context? (By their very definition,
file locks are not private--they're something that is visible between
processes.) Some thought should be given to fixing that name, now, so
that we have better terminology for the coming decades ;-).

I would at the least suggest renaming these locks to something like:

    file-associated locks

or

   file-handle locks

or (using POSIX terminology)

    file-description locks

but that last might be a bit confusing.

With some thought, you might also come up with an even better name.
But, I think *all* of the above alternatives are better than
"file-private locks".

Please consider moving to one of those pieces of terminology, or a
better one of your own creation. Note that also implies that the names
of the constants should probably change. (Already, I found the
spelling of F_SETLKPW a little strange, since I expected F_SETLKWP.)
For example, if you went with the "file handle" terminology, then
something like:

F_FH_SETLK
F_FH_SETLKW
F_FH_GETLK

That also has the advantage of making the new constants more visually
distinctive in code. (I could easily see someone accidentally omitting
the "P" in F_SETLKP, and it would not be immediately obvious at a
casual scan.)

Cheers,

Michael
  

On Wed, 16 Apr 2014 17:37:53 +0200
Michael Kerrisk <mtk.manpages@gmail.com> wrote:

> Jeff, (and Carlos)
> 
> > On Sat, 12 Apr 2014 12:52:33 -0400
> > "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
> [...]
> 
> 
> >> OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?
> 
> That does not strike me as OT at all. The existing byte-range locking
> system has persisted (despite egregious faults) for well over two
> decades. One supposes that Jeff's new improved version might be around
> at least as long. With that in mind, and before setting in stone (and
> pushing into POSIX) a model of thinking that thousands of programmers
> will live with for a long time, it's worth thinking about names.
> 
> > Sorry, forgot to answer this:
> >
> > "P" is for "Private". What can I say...I suck at naming things, and no
> > one else proposed anything better. It's not too late to change that if
> > you can come up with a better moniker, but we'd need to move fast
> > before v3.15 ships.
> 
> I agree on the "suck" bit ;-). Carlos nailed it when he referred to
> traditional byte-range locks as being "process-associated". That is
> the distinction between these two two locking APIs: one associates
> locks with the process, the other associates them with a file handle.
> The terminology should reflect that difference.
> 
> From that perspective, the word "private" here makes no sense at all.
> What does "private" mean in this context? (By their very definition,
> file locks are not private--they're something that is visible between
> processes.) Some thought should be given to fixing that name, now, so
> that we have better terminology for the coming decades ;-).
> 
> I would at the least suggest renaming these locks to something like:
> 
>     file-associated locks
> 

That might be ok.

> or
> 
>    file-handle locks
> 

I'd avoid this. "File handle" has certain connotations in context of
NFS, and other (completely different) connotations in other situations.

> or (using POSIX terminology)
> 
>     file-description locks
> 
> but that last might be a bit confusing.
> 

Again, somewhat confusing since they aren't tied to the file-descriptor
per-se. Consider interactions with dup(), for instance.

> With some thought, you might also come up with an even better name.
> But, I think *all* of the above alternatives are better than
> "file-private locks".
> 

Another possibility is file-owned locks. That's really
closer to how the model works inside the kernel anyway. We have an
opaque "owner" of the file (an unsigned long basically). Process
associated locks use a pointer to the file descriptor table there.
File-private locks (or whatever we want to call them) use a pointer to
the "struct file".

> Please consider moving to one of those pieces of terminology, or a
> better one of your own creation. Note that also implies that the names
> of the constants should probably change. (Already, I found the
> spelling of F_SETLKPW a little strange, since I expected F_SETLKWP.)
> For example, if you went with the "file handle" terminology, then
> something like:
> 
> F_FH_SETLK
> F_FH_SETLKW
> F_FH_GETLK
> 
> That also has the advantage of making the new constants more visually
> distinctive in code. (I could easily see someone accidentally omitting
> the "P" in F_SETLKP, and it would not be immediately obvious at a
> casual scan.)
> 

Where were these comments over the last several months? :)

It would have been better to do this before the v3.15 merge window. As
it stands now, I think we can reasonably get the public facing
interface changed as long as we do it in time for v3.15. I'd need to do
some cleanup of the internal naming for v3.16 to make it all
consistent, but that's doable.

Given my track record on naming things, I don't really trust myself to
do this well, so you tell me...what's the ideal name and set of
constants here?
  

On Wed, 16 Apr 2014 12:00:58 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 16 Apr 2014 17:37:53 +0200
> Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> 
> > Jeff, (and Carlos)
> > 
> > > On Sat, 12 Apr 2014 12:52:33 -0400
> > > "Carlos O'Donell" <carlos@redhat.com> wrote:
> > 
> > [...]
> > 
> > 
> > >> OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?
> > 
> > That does not strike me as OT at all. The existing byte-range locking
> > system has persisted (despite egregious faults) for well over two
> > decades. One supposes that Jeff's new improved version might be around
> > at least as long. With that in mind, and before setting in stone (and
> > pushing into POSIX) a model of thinking that thousands of programmers
> > will live with for a long time, it's worth thinking about names.
> > 
> > > Sorry, forgot to answer this:
> > >
> > > "P" is for "Private". What can I say...I suck at naming things, and no
> > > one else proposed anything better. It's not too late to change that if
> > > you can come up with a better moniker, but we'd need to move fast
> > > before v3.15 ships.
> > 
> > I agree on the "suck" bit ;-). Carlos nailed it when he referred to
> > traditional byte-range locks as being "process-associated". That is
> > the distinction between these two two locking APIs: one associates
> > locks with the process, the other associates them with a file handle.
> > The terminology should reflect that difference.
> > 
> > From that perspective, the word "private" here makes no sense at all.
> > What does "private" mean in this context? (By their very definition,
> > file locks are not private--they're something that is visible between
> > processes.) Some thought should be given to fixing that name, now, so
> > that we have better terminology for the coming decades ;-).
> > 
> > I would at the least suggest renaming these locks to something like:
> > 
> >     file-associated locks
> > 
> 
> That might be ok.
> 
> > or
> > 
> >    file-handle locks
> > 
> 
> I'd avoid this. "File handle" has certain connotations in context of
> NFS, and other (completely different) connotations in other situations.
> 
> > or (using POSIX terminology)
> > 
> >     file-description locks
> > 
> > but that last might be a bit confusing.
> > 
> 
> Again, somewhat confusing since they aren't tied to the file-descriptor
> per-se. Consider interactions with dup(), for instance.
> 
> > With some thought, you might also come up with an even better name.
> > But, I think *all* of the above alternatives are better than
> > "file-private locks".
> > 
> 
> Another possibility is file-owned locks. That's really
> closer to how the model works inside the kernel anyway. We have an
> opaque "owner" of the file (an unsigned long basically). Process
> associated locks use a pointer to the file descriptor table there.
> File-private locks (or whatever we want to call them) use a pointer to
> the "struct file".
> 
> > Please consider moving to one of those pieces of terminology, or a
> > better one of your own creation. Note that also implies that the names
> > of the constants should probably change. (Already, I found the
> > spelling of F_SETLKPW a little strange, since I expected F_SETLKWP.)
> > For example, if you went with the "file handle" terminology, then
> > something like:
> > 
> > F_FH_SETLK
> > F_FH_SETLKW
> > F_FH_GETLK
> > 
> > That also has the advantage of making the new constants more visually
> > distinctive in code. (I could easily see someone accidentally omitting
> > the "P" in F_SETLKP, and it would not be immediately obvious at a
> > casual scan.)
> > 
> 
> Where were these comments over the last several months? :)
> 
> It would have been better to do this before the v3.15 merge window. As
> it stands now, I think we can reasonably get the public facing
> interface changed as long as we do it in time for v3.15. I'd need to do
> some cleanup of the internal naming for v3.16 to make it all
> consistent, but that's doable.
> 
> Given my track record on naming things, I don't really trust myself to
> do this well, so you tell me...what's the ideal name and set of
> constants here?
> 


OTOH, as long as the numeric constants match up, it doesn't really
matter what the macros in the kernel are called. The only thing I think
we have to fix before v3.15 ships is the label in /proc/locks. We can't
reasonably change that after v3.15 ships.

FWIW, I'd probably prefer we rename these "file-associated locks" if
we're going to change the name. "file-owned" is probably closer to how
the internals work, but that'd probably confuse people with respect to
inode user/group owners.

That said, it's probably worth taking an extra day or two, and throwing
out a note to linux-fsdevel/linux-kernel to see if anyone else has
better names.

Thoughts?
  

Hi Jeff,

On Wed, Apr 16, 2014 at 6:00 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 16 Apr 2014 17:37:53 +0200
> Michael Kerrisk <mtk.manpages@gmail.com> wrote:
>
>> Jeff, (and Carlos)
>>
>> > On Sat, 12 Apr 2014 12:52:33 -0400
>> > "Carlos O'Donell" <carlos@redhat.com> wrote:
>>
>> [...]
>>
>>
>> >> OT: Why is it F_*LKP? What does the "P" part of "LKP" mean?
>>
>> That does not strike me as OT at all. The existing byte-range locking
>> system has persisted (despite egregious faults) for well over two
>> decades. One supposes that Jeff's new improved version might be around
>> at least as long. With that in mind, and before setting in stone (and
>> pushing into POSIX) a model of thinking that thousands of programmers
>> will live with for a long time, it's worth thinking about names.
>>
>> > Sorry, forgot to answer this:
>> >
>> > "P" is for "Private". What can I say...I suck at naming things, and no
>> > one else proposed anything better. It's not too late to change that if
>> > you can come up with a better moniker, but we'd need to move fast
>> > before v3.15 ships.
>>
>> I agree on the "suck" bit ;-). Carlos nailed it when he referred to
>> traditional byte-range locks as being "process-associated". That is
>> the distinction between these two two locking APIs: one associates
>> locks with the process, the other associates them with a file handle.
>> The terminology should reflect that difference.
>>
>> From that perspective, the word "private" here makes no sense at all.
>> What does "private" mean in this context? (By their very definition,
>> file locks are not private--they're something that is visible between
>> processes.) Some thought should be given to fixing that name, now, so
>> that we have better terminology for the coming decades ;-).
>>
>> I would at the least suggest renaming these locks to something like:
>>
>>     file-associated locks
>>
>
> That might be ok.
>
>> or
>>
>>    file-handle locks
>>
>
> I'd avoid this. "File handle" has certain connotations in context of
> NFS, and other (completely different) connotations in other situations.

Fair enough. It's the simplest of the alternatives, but I take your point.

>> or (using POSIX terminology)
>>
>>     file-description locks
>>
>> but that last might be a bit confusing.
>>
>
> Again, somewhat confusing since they aren't tied to the file-descriptor
> per-se. Consider interactions with dup(), for instance.

I think you've kind of shown my point about it being confusing. Note
that I wrote "file description", not "file descriptor". In POSIX a
"file description" is the thing that a file descriptor" refers to--in
other words, what is variously also called a "file handle" or an "open
file table entry". I like the term (and by extension, I like "file
description lock"), because POSIX has a clearly defined, unambiguous
meaning for it. But, the problem is that people too easily misread
"file description" as "file descriptor".

>> With some thought, you might also come up with an even better name.
>> But, I think *all* of the above alternatives are better than
>> "file-private locks".
>>
>
> Another possibility is file-owned locks. That's really
> closer to how the model works inside the kernel anyway. We have an
> opaque "owner" of the file (an unsigned long basically). Process
> associated locks use a pointer to the file descriptor table there.
> File-private locks (or whatever we want to call them) use a pointer to
> the "struct file".

Yes, but it seems to me here that you're getting into an implementer's
term, not a *users* term. When I see "file-owned file lock" it feels
pretty vacuous as a term. "File-associated file lock" feels a _little_
better (especially when contrasted with "process-associated file
locks"). "File-handle lock" feels even better (because snappier), but
I acknowledge your point about the ambiguities of "file handle".
"File-description locks" is probably a term that works only for people
who read the standards for fun ;-).

>> Please consider moving to one of those pieces of terminology, or a
>> better one of your own creation. Note that also implies that the names
>> of the constants should probably change. (Already, I found the
>> spelling of F_SETLKPW a little strange, since I expected F_SETLKWP.)
>> For example, if you went with the "file handle" terminology, then
>> something like:
>>
>> F_FH_SETLK
>> F_FH_SETLKW
>> F_FH_GETLK
>>
>> That also has the advantage of making the new constants more visually
>> distinctive in code. (I could easily see someone accidentally omitting
>> the "P" in F_SETLKP, and it would not be immediately obvious at a
>> casual scan.)
>>
>
> Where were these comments over the last several months? :)

I have very limited time, unfortunately, so only get to some things
late... (Sorry about that, because you did do a good job of sending
documentation patches early...)

> It would have been better to do this before the v3.15 merge window. As
> it stands now, I think we can reasonably get the public facing
> interface changed as long as we do it in time for v3.15. I'd need to do
> some cleanup of the internal naming for v3.16 to make it all
> consistent, but that's doable.
>
> Given my track record on naming things, I don't really trust myself to
> do this well, so you tell me...what's the ideal name and set of
> constants here?

I have no final answer for you, sorry. I do think we can do better
than "file-private locks", and I think the constants should have more
visually distinctive names. "File-associated file locks" seems to be
our best shot so far, but maybe someone else has something better.

Cheers,

Michael
  

> OTOH, as long as the numeric constants match up, it doesn't really
> matter what the macros in the kernel are called. The only thing I think
> we have to fix before v3.15 ships is the label in /proc/locks. We can't
> reasonably change that after v3.15 ships.
> 
> FWIW, I'd probably prefer we rename these "file-associated locks" if
> we're going to change the name. "file-owned" is probably closer to how
> the internals work, but that'd probably confuse people with respect to
> inode user/group owners.
> 
> That said, it's probably worth taking an extra day or two, and throwing
> out a note to linux-fsdevel/linux-kernel to see if anyone else has
> better names.

Agreed. Thanks, Jeff, for kicking off a thread for that.

Cheers,

Michael