[Gluster-devel] RFC/Review: libgfapi object handle based extensions

[Anand Avati]

On Mon, Sep 16, 2013 at 4:18 AM, Shyamsundar Ranganathan <
srangana at redhat.com> wrote:

> ----- Original Message -----
>
> > From: "Anand Avati" <avati at gluster.org>
> > Sent: Friday, September 13, 2013 11:09:37 PM
>
> > Shyam,
> > Thanks for sending this out. Can you post your patches to
> review.gluster.org
> > and link the URL in this thread? That would make things a lot more clear
> for
> > feedback and review.
>
> Done, please find the same here, http://review.gluster.org/#/c/5936/
>
> Shyam
>


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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20130917/81fbc5cb/attachment-0001.html>