[GEDI] gfapi: possible approaches to get lk_owner support

[Niels de Vos]

[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.

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.

Thanks,
Niels


> 
> Thanks,
> Soumya
> 
> [1] https://review.gluster.org/#/c/11177
> [2] https://review.gluster.org/#/c/12876
> [3] https://review.gerrithub.io/380696