[Cluster-devel] GFS2: Always call gfs2_holder_uninit after gfs_holder_init?

[Cluster-devel] GFS2: Always call gfs2_holder_uninit after gfs_holder_init?

[Daniel DeFreez]

Hi,

I am trying to understand some of the internals of glocks. It looks like the
basic pattern is:

1. Initialize a holder with gfs2_holder_init
2. Enqueue this holder onto the glock
3. Dequeue the holder
4. Uninitialize the holder with gfs2_holder_uninit

My question is this: are there situations where the holder structure does not
need to be uninitialized?  I ask because I have run across a couple of
cases where it is not, such as in gfs2_get_flags:

gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
error = gfs2_glock_nq(&gh);
if (error)
    return error;
...
gfs2_glock_dq(&gh);
gfs2_holder_uninit(&gh);

Is this correct? Here gh is initialized but never uninitialized on the error
path. It seems like this would cause an inaccurate lockref count. In
many other cases the holder structure is uninitialized, even if lock
acquisition fails.

Thanks,

Daniel

[Cluster-devel] GFS2: Always call gfs2_holder_uninit after gfs_holder_init?

[Bob Peterson]

----- Original Message -----
> Hi,
> 
> I am trying to understand some of the internals of glocks. It looks like the
> basic pattern is:
> 
> 1. Initialize a holder with gfs2_holder_init
> 2. Enqueue this holder onto the glock
> 3. Dequeue the holder
> 4. Uninitialize the holder with gfs2_holder_uninit
> 
> My question is this: are there situations where the holder structure does not
> need to be uninitialized?  I ask because I have run across a couple of
> cases where it is not, such as in gfs2_get_flags:
> 
> gfs2_holder_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
> error = gfs2_glock_nq(&gh);
> if (error)
>     return error;
> ...
> gfs2_glock_dq(&gh);
> gfs2_holder_uninit(&gh);
> 
> Is this correct? Here gh is initialized but never uninitialized on the error
> path. It seems like this would cause an inaccurate lockref count. In
> many other cases the holder structure is uninitialized, even if lock
> acquisition fails.
> 
> Thanks,
> 
> Daniel

Hi Daniel,

The glock init/uninit stuff serves to get a reference on the glock, and
we need to take great care to do proper accounting on the glock reference
count, or the glock won't go away, which can cause (1) problems syncing
the metadata to disk, and (2) problems unmounting the file system.
The code you pointed out, gfs2_get_flags, appears to be a bug.
I agree with you that it should probably do holder_uninit in the error
case. I can patch it up if you want, unless you want to submit a patch.
However, under normal circumstances, the gfs2_glock_nq should not fail
especially for a SHARED lock like that. It can fail if the file system
is being unmounted, or other very unlikely circumstances.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] GFS2: Always call gfs2_holder_uninit after gfs_holder_init?

[Daniel DeFreez]

Bob,

Thank you for the explanation. I will send a patch in a few minutes that
includes the other location that follows a similar path. I'm sending the patch
for the sake of practice, so disregard if it isn't helpful.

Daniel