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

[Anand Avati]

On Mon, Sep 16, 2013 at 4:18 AM, Shyamsundar Ranganathan <address@hidden> wrote:

----- Original Message -----

> From: "Anand Avati" <address@hidden>
> 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