<div dir="ltr"><div>Also note that the same glfs_object must be re-used in readdirplus (once we have a _h_ equivalent of the API)</div><div><br></div><div>Avati</div><div><br></div>On Sun, Sep 29, 2013 at 10:05 PM, Anand Avati <span dir="ltr"><<a href="mailto:avati@gluster.org" target="_blank">avati@gluster.org</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I see a pretty core issue - lifecycle management of 'struct glfs_object'. What is the structure representing? When is it created? When is it destroyed? How does it relate to inode_t?<div>
<br></div><div>
Looks like for every lookup() we are creating a new glfs_object, even if the looked up inode was already looked up before (in the cache) and had a glfs_object created for it in the recent past.</div><div><br></div><div>We need a stronger relationship between the two with a clearer relationship. It is probably necessary for a glfs_object to represent mulitple inode_t's at different points in time depending on graph switches, but for a given inode_t we need only one glfs_object. We definitely must NOT have a new glfs_object per lookup call.</div>
<span class="HOEnZb"><font color="#888888">
<div><br></div><div>Avati</div></font></span><div><div class="h5"><div><br><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 19, 2013 at 5:13 AM, Shyamsundar Ranganathan <span dir="ltr"><<a href="mailto:srangana@redhat.com" target="_blank">srangana@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Avati,<br>
<br>
Please find the updated patch set for review at gerrit.<br>
<a href="http://review.gluster.org/#/c/5936/" target="_blank">http://review.gluster.org/#/c/5936/</a><br>
<br>
Changes made to address the points (1) (2) and (3) below. By the usage of the suggested glfs_resolve_inode approach.<br>
<br>
I have not yet changes glfs_h_unlink to use the glfs_resolve_at. (more on this a little later).<br>
<br>
So currently, the review request is for all APIs other than,<br>
glfs_h_unlink, glfs_h_extract_gfid, glfs_h_create_from_gfid<br>
<br>
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.<br>
<br>
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.<br>
<br>
Request comments on the glfs_resolve_at proposal.<br>
<br>
Shyam.<br>
<div><br>
----- Original Message -----<br>
<br>
> From: "Anand Avati" <<a href="mailto:avati@gluster.org" target="_blank">avati@gluster.org</a>><br>
</div>> To: "Shyamsundar Ranganathan" <<a href="mailto:srangana@redhat.com" target="_blank">srangana@redhat.com</a>><br>
> Cc: "Gluster Devel" <<a href="mailto:gluster-devel@nongnu.org" target="_blank">gluster-devel@nongnu.org</a>><br>
> Sent: Wednesday, September 18, 2013 11:39:27 AM<br>
> Subject: Re: RFC/Review: libgfapi object handle based extensions<br>
<div><div><br>
> Minor comments are made in gerrit. Here is a larger (more important) comment<br>
> for which email is probably more convenient.<br>
<br>
> There is a problem in the general pattern of the fops, for example<br>
> glfs_h_setattrs() (and others too)<br>
<br>
> 1. glfs_validate_inode() has the assumption that object->inode deref is a<br>
> guarded operation, but here we are doing an unguarded deref in the paramter<br>
> glfs_resolve_base().<br>
<br>
> 2. A more important issue, glfs_active_subvol() and glfs_validate_inode() are<br>
> not atomic. glfs_active_subvol() can return an xlator from one graph, but by<br>
> the time glfs_validate_inode() is called, a graph switch could have happened<br>
> and inode can get resolved to a different graph. And in syncop_XXXXXX() we<br>
> end up calling on graph1 with inode belonging to graph2.<br>
<br>
> 3. ESTALE_RETRY is a fundamentally wrong thing to do with handle based<br>
> operations. The ESTALE_RETRY macro exists for path based FOPs where the<br>
> resolved handle could have turned stale by the time we perform the FOP<br>
> (where resolution and FOP are non-atomic). Over here, the handle is<br>
> predetermined, and it does not make sense to retry on ESTALE (notice that FD<br>
> based fops in glfs-fops.c also do not have ESTALE_RETRY for this same<br>
> reason)<br>
<br>
> I think the pattern should be similar to FD based fops which specifically<br>
> address both the above problems. Here's an outline:<br>
<br>
> glfs_h_XXXX(struct glfs *fs, glfs_object *object, ...)<br>
> {<br>
> xlator_t *subvol = NULL;<br>
> inode_t *inode = NULL;<br>
<br>
> __glfs_entry_fs (fs);<br>
<br>
> subvol = glfs_active_subvol (fs);<br>
> if (!subvol) { errno = EIO; ... goto out; }<br>
<br>
> inode = glfs_resolve_inode (fs, object, subvol);<br>
> if (!inode) { errno = ESTALE; ... goto out; }<br>
<br>
> loc.inode = inode;<br>
> ret = syncop_XXXX(subvol, &loc, ...);<br>
<br>
> }<br>
<br>
> Notice the signature of glfs_resolve_inode(). What it does: given a<br>
> glfs_object, and a subvol, it returns an inode_t which is resolved on that<br>
> subvol. This way the syncop_XXX() is performed with matching subvol and<br>
> inode. Also it returns the inode pointer so that no unsafe object->inode<br>
> deref is done by the caller. Again, this is the same pattern followed by the<br>
> fd based fops already.<br>
<br>
> Also, as mentioned in one of the comments, please consider using<br>
> glfs_resolve_at() and avoiding manual construction of loc_t.<br>
<br>
> Thanks,<br>
> Avati<br>
</div></div></blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div></div>