On Fri, Dec 04, 2015 at 07:51:41AM -0500, Ira Cooper wrote: > Niels de Vos <[email protected]> writes: > > > On Thu, Dec 03, 2015 at 06:34:56PM -0500, Ira Cooper wrote: > >> Kaleb KEITHLEY <[email protected]> writes: > >> > >> > On 12/03/2015 03:10 PM, Ira Cooper wrote: > >> >> Poornima Gurusiddaiah <[email protected]> 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
signature.asc
Description: PGP signature
_______________________________________________ Gluster-devel mailing list [email protected] http://www.gluster.org/mailman/listinfo/gluster-devel
