On 04/17/2015 04:51 PM, Raghavendra Talur wrote: > On Friday 17 April 2015 03:53 PM, Poornima Gurusiddaiah wrote: >> Hi, >> >> There are two concerns in the usage of libgfapi which have been >> present from day one, but now >> with new users of libgfapi its a necessity to fix these: >> >> 1. When libgfapi is used by the GlusterFS internal xlators, 'THIS' >> gets overwritten: >> Eg: when snapview-server creates a new fs instance for every snap that >> is created. >> Currently any libgfapi calls inside the xlator overwrites the THIS >> value to contain glfs-master(gfapi xlator). >> Hence after the api exits, any further code in the parent xlator >> referring to THIS will refer >> to the glfs-master(gfapi xlator). >> >> Proposed solutions: >> - Store and restore THIS in every API exposed by libgfapi, patch for >> the same can be found at http://review.gluster.org/#/c/9797/ >> - Other solution suggested by Niels was to not have internal xlators >> calling libgfapi and >> move the core functionality to libglusterfs. But even with this the >> nested mount/ctx issue can still exist. > > > Restating the problem to make sure I understand it correctly: > When a "glusterfs" process wants to access a volume using libgfapi we > have a problem at hand where THIS->ctx of original glusterfs process and > libgfapi cause conflicts. > Examples could be > 1. Snapd which is a glusterfs brick process wants to become a glusterfs > client process for a different volume using libgfapi. > 2. Someday glusterd may choose to become client to a management volume > using libgfapi. > 3. Someone writes an application in libgfapi which gives unified view of > multiple glusterfs volumes using libgfapi. > 4. This is can *possibly* happen without libgfapi if One glusterfs > process does more than one glusterfs_ctx_new in a nested fashion. (I am > not sure it is technically possible with current infra or if it is > already done somewhere). > > So the question boils down to "Can we have two ctx in a nested fashion > in a single process and switch as and when required?, If so how?" > > With Poornima's patch here, we have taken care of all cases where it is > libgfapi encapsulated in another (glusterfs|libgfapi) process. > > What we have not solved is the case where a glusterfsd process creates > another nested ctx as mentioned in case 4 above. > > If we think we will never encounter case 4 above then it may just be > enough to go ahead with this patch and solve this for libgfapi. > > I am not sure if I understand Niels' suggestion completely. How can a > gluster xlator access a different volume than what it is part of, if not > through libgfapi? > > >> >> 2. When libgfapi APIs are called by the application on a fs object, >> that is already closed(glfs_fini()'ed): >> Ideally it is the applications responsibility to take care, to not do >> such things. But its also good >> to not crash libgfapi when such ops are performed by the application. >> We have already seen these issues in snapview server. >> >> Proposed Solutions/workarounds: >> - Do not free the fs object(leaks few bytes) have a state bit to say >> valid or invalid. In every API check >> for the fs validity before proceeding. Patch for same >> @http://review.gluster.org/#/c/9797/ >> - As suggested by Niels, have a fs global pool which tracks >> allocated/freed fs objects. >> - Have the applications fix it, so that they do not call fops on >> closed fs object(unmounted fs). >> This mandates multithreaded/asynchronous applications to have some >> synchronization mechanism. >> > > IMHO, allocation and free of memory of fs objects should > never be libgfapi's responsibility. They must be allocated by the > application as per struct defined in glfs.h. > > Once a fini() is called on a fs, it is like a close() on a fd. > As Poornima suggests, just set the invalid flag which is part of the > struct. It should be application's responsibilty to free the fs memory > after fini returns. > > The above patch checks for validity of fs in entry of > every fop and just returns EINVAL whenever the invalid flag is set. > > The only downside to this is that it breaks api backward compatibility. > glfs_new currently takes only volname as its input. > new glfs_new would take a preallocated fs and a volname as its input. We already had a problem in gluster volume and same approach is been taken in for http://review.gluster.org/#/c/10238 ~Atin > > >> Please let me know your comments on the same. >> >> Regards, >> Poornima >> >> >> >> _______________________________________________ >> Gluster-devel mailing list >> Gluster-devel@xxxxxxxxxxx >> http://www.gluster.org/mailman/listinfo/gluster-devel >> > > _______________________________________________ > Gluster-devel mailing list > Gluster-devel@xxxxxxxxxxx > http://www.gluster.org/mailman/listinfo/gluster-devel -- ~Atin _______________________________________________ Gluster-devel mailing list Gluster-devel@xxxxxxxxxxx http://www.gluster.org/mailman/listinfo/gluster-devel
