<div dir="ltr">On Mon, Sep 16, 2013 at 4:18 AM, Shyamsundar Ranganathan <span dir="ltr"><<a href="mailto:srangana@redhat.com" target="_blank">srangana@redhat.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">----- Original Message -----<br>
<br>
> From: "Anand Avati" <<a href="mailto:avati@gluster.org">avati@gluster.org</a>><br>
> Sent: Friday, September 13, 2013 11:09:37 PM<br>
<div class="im"><br>
> Shyam,<br>
> Thanks for sending this out. Can you post your patches to <a href="http://review.gluster.org" target="_blank">review.gluster.org</a><br>
> and link the URL in this thread? That would make things a lot more clear for<br>
> feedback and review.<br>
<br>
</div>Done, please find the same here, <a href="http://review.gluster.org/#/c/5936/" target="_blank">http://review.gluster.org/#/c/5936/</a><br>
<br>
Shyam<br>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Minor comments are made in gerrit. Here is a larger (more important) comment for which email is probably more convenient.</div><div class="gmail_extra">
<br></div><div class="gmail_extra"><div class="gmail_extra">There is a problem in the general pattern of the fops, for example glfs_h_setattrs() (and others too)</div><div class="gmail_extra"><br></div><div class="gmail_extra">
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().</div><div class="gmail_extra"><br></div><div class="gmail_extra">
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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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)</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">I think the pattern should be similar to FD based fops which specifically address both the above problems. Here's an outline:</div><div class="gmail_extra"><br>
</div><div class="gmail_extra"> glfs_h_XXXX(struct glfs *fs, glfs_object *object, ...)</div><div class="gmail_extra"> {</div><div class="gmail_extra"> xlator_t *subvol = NULL;</div><div class="gmail_extra"> inode_t *inode = NULL;</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"> __glfs_entry_fs (fs);</div><div class="gmail_extra"><br></div><div class="gmail_extra"> subvol = glfs_active_subvol (fs);</div><div class="gmail_extra"> if (!subvol) { errno = EIO; ... goto out; }</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"> inode = glfs_resolve_inode (fs, object, subvol);</div><div class="gmail_extra"> if (!inode) { errno = ESTALE; ... goto out; }</div><div class="gmail_extra">
<br></div><div class="gmail_extra"> loc.inode = inode;</div><div class="gmail_extra"> ret = syncop_XXXX(subvol, &loc, ...);</div><div class="gmail_extra"><br></div><div class="gmail_extra"> }</div><div class="gmail_extra">
<br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Also, as mentioned in one of the comments, please consider using glfs_resolve_at() and avoiding manual construction of loc_t.</div><div class="gmail_extra"><br></div>
<div class="gmail_extra">Thanks,</div><div class="gmail_extra">Avati</div><div class="gmail_extra"><br></div></div></div>