[Gluster-devel] libgfapi changes to add lk_owner and lease ID

[Niels de Vos]

On Fri, Dec 04, 2015 at 07:51:41AM -0500, Ira Cooper wrote:
> Niels de Vos <ndevos at redhat.com> writes:
> 
> > On Thu, Dec 03, 2015 at 06:34:56PM -0500, Ira Cooper wrote:
> >> Kaleb KEITHLEY <kkeithle at redhat.com> writes:
> >> 
> >> > On 12/03/2015 03:10 PM, Ira Cooper wrote:
> >> >> Poornima Gurusiddaiah <pgurusid at redhat.com> writes:
> >> >> 
> >> >>> Hi, 
> >> >>>
> >> >>> Brief Background: 
> >> >>> ============ 
> >> >>> For the below two features, we need ligfapi to take 2 other parameters from the applications for most number of fops. 
> >> >>> http://review.gluster.org/#/c/12014/ 
> >> >>> http://review.gluster.org/#/c/11980/ 
> >> >>>
> >> >>> For leases to work as explained in the above doc, every file data read/write fop needs to be associated with a lease ID. This is specially required for Samba and NFS-Ganesha as they inturn serve other clients who request for leases. For Gluster to identify the end client (which is not Samba/NFS Ganesha) we need lease ID to be filled by Samba/NFS Ganesha. 
> >> >>>
> >> >>> For mandatory locks feature to work as explained, every file data read/write fop needs to be associated with a lk_owner. In linux Kernel VFS takes care of filling the lk_ownere for the file system. In libgfapi case, the applications calling into libgfapi should be providing lk_owner with every fop. This is again required mainly for Samba and NFS Ganesha, as they serve multiple clients. 
> >> >>>
> >> >>> Possible solutions: 
> >> >>> ============= 
> >> >>> 1. Modify all the required APIs to take 2 other parameter, lease ID and lk_owner. But that would mean backward compatibility issues and is a programming overhead for applications not interested in Leases and mandatory lock feature. 
> >> >>> 2. Add an API called glfs_set_fop_attrs (lease ID, lk_owner) which works similar to glfs_set_uid(uid). The API sets a thread local storage (pthread_key) with the values provided, the further fops on that thread will pick the lease ID and lk_owner from the thread local storage (pthread_key). There are few minor details that needs to be worked out: 
> >> >>> - In case of async API will end up using lease ID and lk_owner from wrong thread. 
> >> >>> - unset lease ID and lk_owner after every fop to ensure there is no stale lease ID or lk_owner set? 
> >> >>> - For fd based fops we can store the lease ID and lk_owner in glfd, so that the application writed need not set it for every fop. But for handle based fops lease ID and lk_owner needs to be set explicitly every-time. 
> >> >>>
> >> >>> Solution 2 is more preferable except for that it adds overhead of calling another API, for the libgfapi users who intends to use these features. 
> >> >>> A prototype of solution 2 can be found at http://review.gluster.org/#/c/12876/ 
> >> >>>
> >> >
> >> > why not use storage in the client_t instead of thread local?
> >> 
> >> It comes down to the use case.  For Samba, the right spot is almost
> >> certainly the fd, because lease keys are a per-handle (which we map to
> >> fd) property.
> >> 
> >> client_t is a horror show, due to race conditions between threads, IMHO.
> >
> > If there are known races, should we not address that? Got a bug that
> > explains it in more detail?
> 
> Niels,
> 
> For samba, if we do multi-threaded open, Kaleb's proposal is a
> race-condition.  I haven't gone through every use of client_t and seen
> if it is racy.
> 
> The race here is pretty simple:
> 
> Thread 1: Sets lease_id
> Thread 2: Sets lease_id
> Thread 1: Opens file. (wrong lease_id)
> 
> If these two threads represent requests from different clients, client_t
> won't work, unless there's a client_t per-thread.
> 
> For global things on the connection, client_t is fine, and appropriate.
> For this?  No.
> 
> This is a property per-open, and belongs in the glfs_fd and glfs_object,
> IMHO.

Okay, so you meant to say that client_t "is a horror show" for this
particular use-case (lease_id). It indeed does not sound suitable to use
client_t here.

I'm not much of a fan for using Thread Local Storage and would prefer to
see a more close to atomic way like my suggestion for compound
procedures in an other email in this thread. Got an opinion about that?

Thanks,
Niels
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20151204/2f13911a/attachment.sig>