Avati, Please find the updated patch set for review at gerrit. http://review.gluster.org/#/c/5936/ Changes made to address the points (1) (2) and (3) below. By the usage of the suggested glfs_resolve_inode approach. I have not yet changes glfs_h_unlink to use the glfs_resolve_at. (more on this a little later). So currently, the review request is for all APIs other than, glfs_h_unlink, glfs_h_extract_gfid, glfs_h_create_from_gfid glfs_resolve_at: Using this function the terminal name will be a force look up anyway (as force_lookup will be passed as 1 based on !next_component). We need to avoid this _extra_ lookup in the unlink case, which is why all the inode_grep(s) etc. were added to the glfs_h_lookup in the first place. Having said the above, we should still leverage glfs_resolve_at anyway, as there seem to be other corner cases where the resolved inode and subvol maybe from different graphs. So I think I want to modify glfs_resolve_at to make a conditional force_lookup, based on iatt being NULL or not. IOW, change the call to glfs_resolve_component with the conditional as, (reval || (!next_component && iatt)). So that callers that do not want the iatt filled, can skip the syncop_lookup. Request comments on the glfs_resolve_at proposal. Shyam. ----- Original Message ----- > From: "Anand Avati" <avati@xxxxxxxxxxx> > To: "Shyamsundar Ranganathan" <srangana@xxxxxxxxxx> > Cc: "Gluster Devel" <gluster-devel@xxxxxxxxxx> > Sent: Wednesday, September 18, 2013 11:39:27 AM > Subject: Re: RFC/Review: libgfapi object handle based extensions > Minor comments are made in gerrit. Here is a larger (more important) comment > for which email is probably more convenient. > There is a problem in the general pattern of the fops, for example > glfs_h_setattrs() (and others too) > 1. glfs_validate_inode() has the assumption that object->inode deref is a > guarded operation, but here we are doing an unguarded deref in the paramter > glfs_resolve_base(). > 2. A more important issue, glfs_active_subvol() and glfs_validate_inode() are > not atomic. glfs_active_subvol() can return an xlator from one graph, but by > the time glfs_validate_inode() is called, a graph switch could have happened > and inode can get resolved to a different graph. And in syncop_XXXXXX() we > end up calling on graph1 with inode belonging to graph2. > 3. ESTALE_RETRY is a fundamentally wrong thing to do with handle based > operations. The ESTALE_RETRY macro exists for path based FOPs where the > resolved handle could have turned stale by the time we perform the FOP > (where resolution and FOP are non-atomic). Over here, the handle is > predetermined, and it does not make sense to retry on ESTALE (notice that FD > based fops in glfs-fops.c also do not have ESTALE_RETRY for this same > reason) > I think the pattern should be similar to FD based fops which specifically > address both the above problems. Here's an outline: > glfs_h_XXXX(struct glfs *fs, glfs_object *object, ...) > { > xlator_t *subvol = NULL; > inode_t *inode = NULL; > __glfs_entry_fs (fs); > subvol = glfs_active_subvol (fs); > if (!subvol) { errno = EIO; ... goto out; } > inode = glfs_resolve_inode (fs, object, subvol); > if (!inode) { errno = ESTALE; ... goto out; } > loc.inode = inode; > ret = syncop_XXXX(subvol, &loc, ...); > } > Notice the signature of glfs_resolve_inode(). What it does: given a > glfs_object, and a subvol, it returns an inode_t which is resolved on that > subvol. This way the syncop_XXX() is performed with matching subvol and > inode. Also it returns the inode pointer so that no unsafe object->inode > deref is done by the caller. Again, this is the same pattern followed by the > fd based fops already. > Also, as mentioned in one of the comments, please consider using > glfs_resolve_at() and avoiding manual construction of loc_t. > Thanks, > Avati
