[GEDI] gfapi: possible approaches to get lk_owner support

[Frank Filz]

On 10/03/2017 01:32 AM, Niels de Vos wrote:
> [please include integration at gluster.org to all emails related to gfapi]
>
> On Tue, Oct 03, 2017 at 12:52:29PM +0530, Soumya Koduri wrote:
>> Hi,
>>
>> Current API available to take posix locks - "glfs_posix_lock()" doesn't
>> allow applications to set lk_owner. But NFS-ganesha now has a requirement on
>> the backend filesystem to be able to support lk_owners as well. So to
>> address that, we have below options. Before taking up with upstream release
>> mailing lists, wanted to discuss within the team wrt to possible best
>> approach. Request your comments/thoughts wrt to correctness or timelines.
>>
>> *Approach (A)*
>> There is already a patch from Anoop cc'ed (pending review) [1] which defines
>> a new lock API of following syntax -
>>
>> int glfs_file_lock (glfs_fd_t *fd, int cmd, struct flock *flock,
>>                     lock_mode_t lk_mode, unsigned long int lk_owner) __THROW
>>
>>
>> Pros: It is added to provide mandatory lk support in gfapi and has a
>> provision of setting lk_owner.
>>
>> Cons: But since this is a new feature, it needs to go through thorough
>> review wrt to semantics and behavior and also dependent on couple of other
>> patches for feature completeness [2].
>>
>> Also as per mandatory lk support design, multiple glfd can be associated
>> with only single lk_owner but not otherwise i.e, one glfd cannot be
>> associated with multiple lk_owners
>>
>>
>> *Approach (B)*
>> Define new opaque structure "struct glfs_lk" similar to
>> "glfs_t"/"glfs_upcall" routines (structure contents hidden from application)
>> and provide public APIs ("glfs_set_lock_*) to set each of the field.
>>
>> In this new struct, include lkowner field
>>
>> struct glfs_lk (
>> struct flock f_lk;
>> uint64_t lk_owner;
>> ..
>> }
>>
>> APIs:
>> glfs_set_flock (struct glfs_lk, ..)
>> glfs_set_lkowner (struct glfs_lk, ..)
>> glfs_set_file_lock (struct glfd, struct glfs_lk)
>>
>> Pros: This structure can be extended in future (to accommodate mandatory lk
>> mode etc) without impacting applications.
>>
>> Cons: One feedback I received is that it may be cumbersome to use that many
>> routines and not stick to standard API. And we need to work on all these new
>> structures and APIs right from beginning followed by reviews and thorough
>> testing.
> A glfs_lk struct would give us a lot of flexibility. This could be used
> to address mandatory locks as well.
>
> It is relatively cumbersome to have accessor functions for each hidden,
> but public member of a struct. It is not very flexible, and ties
> applications very much to certain versions of Gluster. This is not
> always desirable, a way to detect if certain members are available seems
> to be much appreciated. An alternative interface to get/set members of
> structs (without making the struct public, preventing us to modify it),
> is to use an API similar to getsockopt()/setsockopt():
>
>   int glfs_lk_set (struct glfs_lk, enum glfs_lk_member, void* value, int size)
>
> This makes it easier to extend, we can add new glfs_lk_member values,
> new code that uses newly added members can check for an error return to
> see if setting the value failed (might be unsupported).
>
>> *Approach (c)* -- suggested by Niels already
>> As per the mandatory lock feature semantics, each glfd can be associated
>> with only one single lk_owner. So since we may need to stick to that (if not
>> now at-least whenever mandatory lk support comes in ), can we add just one
>> below new API -
>>
>> glfs_set_lkowner (struct glfd, uint64_t lkowner);
>>
>> Use this API before calling existing "glfs_posix_lock()". Modify this API
>> implementation to read lk_owner from glfd and continue with rest of the
>> processing.
>>
>> Pros: seems fairly easy to implement and get reviewed.
>>
>> Cons: Jiffin's patch [3] on using same glfd for different lk_owners cannot
>> be applied. But this shall be the case with any of the approaches discussed
>> above.
Jiffin's patch does a glfs_dup, if that creates a separate glfd that we
can then set lock owner differently on, then it would work.

>> This works as well, and I like the simplicity.
>>
>> I personally now favor approach(3) but request your inputs on any possible
>> limitations I may be over-looking.
> My preference goes to approach (2), because it can be extended easily.
> However, if (3) can cover all of the locking requirements, then that
> would be okay too.
>
> Note that lk_owner is an 'opaque' in the XDR definitions, we will need
> to pass a size when setting it.
FSAL is only passed a void * for lock owner, which is plenty sufficient.

Frank