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

Re: [Cluster-devel] [GFS2 Patch][TRY 2] GFS2: Extend the life of the reservations structure



Hi,

On Mon, 2012-05-14 at 08:58 -0400, Bob Peterson wrote:
> ----- Original Message -----
> | > @@ -722,7 +731,7 @@ static int gfs2_create_inode(struct inode *dir,
> | > struct dentry *dentry,
> | >  	gfs2_trans_end(sdp);
> | >  	/* Check if we reserved space in the rgrp. Function link_dinode
> | >  	may
> | >  	   not, depending on whether alloc is required. */
> | > -	if (dip->i_res)
> | > +	if (gfs2_mb_reserved(dip))
> | >  		gfs2_inplace_release(dip);
> | >  	gfs2_quota_unlock(dip);
> | >  	gfs2_qadata_put(dip);
> | > @@ -740,6 +749,7 @@ fail_gunlock:
> | >  		iput(inode);
> | >  	}
> | >  fail:
> | > +	gfs2_rs_delete(dip);
> | 
> | Should we really be dropping the ip->i_res here I wonder? I'm not
> | sure
> | that we want to forget this information just because (for example)
> | someone has tried to create a new inode and it has been failed for
> | some
> | reason or other.
> | 
> | Otherwise I think this looks good,
> | 
> | Steve.
> 
> Hi,
> 
> In this implementation, function gfs2_inplace_release just unlocks the
> rgrp glock and sets rs_requested to zero. In fact, the reservation still
> hangs around, attached to the inode until function gfs2_rs_delete is called
> from gfs2_release or gfs2_evict_inode. So instead of having two functions
> for allocation and deallocation, we now have four:
> 
That is all ok I think, but I'm referring to the call to
gfs2_rs_delete() in this case. We should probably call the combined
structure something like struct gfs2_alloc, since it deals with
allocation and not just reservations. However, the issue here was
dropping the reservation from the directory in the case that the
allocation has failed for some reason (maybe permisions, or something
like that)

Steve.

> gfs2_rs_alloc:        kmem_cache_zalloc
> gfs2_inplace_reserve: hold rgrp glock, rs_requested = reservation needed
> gfs2_inplace_release: dq rgrp glock, rs_requested = 0
> gfs2_rs_delete:       kmem_cache_free
> 
> I can change the names (suggestions welcome), because "gfs2_inplace_reserve"
> and its counterpart have changed their roles over the years. I'm just
> conditioned to look for those function names from years of conditioning. :)
> 
> Something more like "get_local_rgrp" is a more accurate reflection of
> what's happening, but that name is already used.  I've never liked how
> get_local_rgrp and gfs2_inplace_reserve both have retry loops. 
> Since they're now pretty small functions, and one calls the other, perhaps
> I should combine them into one function by the name "get_local_rgrp"?
> And perhaps that should be done in a separate patch?
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems



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