gluster-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anand Avati
Subject: Re: [Gluster-devel] RFC/Review: libgfapi object handle based extensions
Date: Sun, 29 Sep 2013 22:08:59 -0700

Also note that the same glfs_object must be re-used in readdirplus (once we have a _h_ equivalent of the API)

Avati

On Sun, Sep 29, 2013 at 10:05 PM, Anand Avati <address@hidden> wrote:
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?

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.

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.

Avati

On Thu, Sep 19, 2013 at 5:13 AM, Shyamsundar Ranganathan <address@hidden> wrote:
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" <address@hidden>
> To: "Shyamsundar Ranganathan" <address@hidden>
> Cc: "Gluster Devel" <address@hidden>
> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]