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

["Carlos O'Donell" <carlos at redhat dot com>]

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.