[Cluster-devel] [dlm] Two small sysfs patches

[Cluster-devel] [dlm] Two small sysfs patches

[Steven Whitehouse]

Hi,

Please queue the following two patches for the next merge window
for dlm. The first one adds a new sysfs variable so that the
lockspace can be obtained without resorting to parsing the
initial line of the sysfs message.

The second one removes some obsolete code relating to one of the
sysfs files,

Steve.

[Cluster-devel] [PATCH 1/2] dlm: Send lockspace name with uevents

[Steven Whitehouse]

Although it is possible to get this information from the path,
its much easier to provide the lockspace as a seperate env
variable.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
---
 fs/dlm/lockspace.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
index c010ecf..26a8bd4 100644
--- a/fs/dlm/lockspace.c
+++ b/fs/dlm/lockspace.c
@@ -191,6 +191,18 @@ static int do_uevent(struct dlm_ls *ls, int in)
 	return error;
 }
 
+static int dlm_uevent(struct kset *kset, struct kobject *kobj,
+		      struct kobj_uevent_env *env)
+{
+	struct dlm_ls *ls = container_of(kobj, struct dlm_ls, ls_kobj);
+
+	add_uevent_var(env, "LOCKSPACE=%s", ls->ls_name);
+	return 0;
+}
+
+static struct kset_uevent_ops dlm_uevent_ops = {
+	.uevent = dlm_uevent,
+};
 
 int __init dlm_lockspace_init(void)
 {
@@ -199,7 +211,7 @@ int __init dlm_lockspace_init(void)
 	INIT_LIST_HEAD(&lslist);
 	spin_lock_init(&lslist_lock);
 
-	dlm_kset = kset_create_and_add("dlm", NULL, kernel_kobj);
+	dlm_kset = kset_create_and_add("dlm", &dlm_uevent_ops, kernel_kobj);
 	if (!dlm_kset) {
 		printk(KERN_WARNING "%s: can not create kset\n", __func__);
 		return -ENOMEM;
-- 
1.6.2.5

[Cluster-devel] [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[Steven Whitehouse]

We don't need to look up the lockspace in this particular
case since we already have a pointer to it (which was being
dereferenced in order to do the lookup in the first place).

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
---
 fs/dlm/lockspace.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
index 26a8bd4..ce0fdf5 100644
--- a/fs/dlm/lockspace.c
+++ b/fs/dlm/lockspace.c
@@ -37,10 +37,6 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
 	ssize_t ret = len;
 	int n = simple_strtol(buf, NULL, 0);
 
-	ls = dlm_find_lockspace_local(ls->ls_local_handle);
-	if (!ls)
-		return -EINVAL;
-
 	switch (n) {
 	case 0:
 		dlm_ls_stop(ls);
@@ -51,7 +47,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
 	default:
 		ret = -EINVAL;
 	}
-	dlm_put_lockspace(ls);
+
 	return ret;
 }
 
-- 
1.6.2.5

[Cluster-devel] [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[David Teigland]

On Wed, Feb 17, 2010 at 09:41:35AM +0000, Steven Whitehouse wrote:
> We don't need to look up the lockspace in this particular
> case since we already have a pointer to it (which was being
> dereferenced in order to do the lookup in the first place).

It'll take more to convince me that that reference from find isn't needed.
My assumption is that I added it because it was.

Dave

> Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
> ---
>  fs/dlm/lockspace.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> index 26a8bd4..ce0fdf5 100644
> --- a/fs/dlm/lockspace.c
> +++ b/fs/dlm/lockspace.c
> @@ -37,10 +37,6 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
>  	ssize_t ret = len;
>  	int n = simple_strtol(buf, NULL, 0);
>  
> -	ls = dlm_find_lockspace_local(ls->ls_local_handle);
> -	if (!ls)
> -		return -EINVAL;
> -
>  	switch (n) {
>  	case 0:
>  		dlm_ls_stop(ls);
> @@ -51,7 +47,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
>  	default:
>  		ret = -EINVAL;
>  	}
> -	dlm_put_lockspace(ls);
> +
>  	return ret;
>  }
>  
> -- 
> 1.6.2.5

[Cluster-devel] [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[Steven Whitehouse]

Hi,

On Wed, 2010-02-17 at 15:12 -0500, David Teigland wrote:
> On Wed, Feb 17, 2010 at 09:41:35AM +0000, Steven Whitehouse wrote:
> > We don't need to look up the lockspace in this particular
> > case since we already have a pointer to it (which was being
> > dereferenced in order to do the lookup in the first place).
> 
> It'll take more to convince me that that reference from find isn't needed.
> My assumption is that I added it because it was.
> 
> Dave
> 
I'm not sure what more I can say here.... this is a sysfs file store
function and one of the reasons for using it is that sysfs looks after
the ref counting for you.

Even aside from that, if you don't have a reference to the lockspace,
then the dereference that is done to discover the lockspace name would
be invalid, since the structure might have already been freed before the
reference is obtained.

You could also compare with with the other store and show functions in
that same file and notice that none of them try to grab a reference to
the lockspace in that way. So if this is required, then it must be
required for those functions too.

Either way there is something not quite right here and having studied
the code in some detail, I'm pretty sure this is the correct fix,

Steve.

> > Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
> > ---
> >  fs/dlm/lockspace.c |    6 +-----
> >  1 files changed, 1 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> > index 26a8bd4..ce0fdf5 100644
> > --- a/fs/dlm/lockspace.c
> > +++ b/fs/dlm/lockspace.c
> > @@ -37,10 +37,6 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
> >  	ssize_t ret = len;
> >  	int n = simple_strtol(buf, NULL, 0);
> >  
> > -	ls = dlm_find_lockspace_local(ls->ls_local_handle);
> > -	if (!ls)
> > -		return -EINVAL;
> > -
> >  	switch (n) {
> >  	case 0:
> >  		dlm_ls_stop(ls);
> > @@ -51,7 +47,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > -	dlm_put_lockspace(ls);
> > +
> >  	return ret;
> >  }
> >  
> > -- 
> > 1.6.2.5

[Cluster-devel] [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[David Teigland]

On Thu, Feb 18, 2010 at 09:16:03AM +0000, Steven Whitehouse wrote:
> I'm not sure what more I can say here.... this is a sysfs file store
> function and one of the reasons for using it is that sysfs looks after
> the ref counting for you.
> 
> Even aside from that, if you don't have a reference to the lockspace,
> then the dereference that is done to discover the lockspace name would
> be invalid, since the structure might have already been freed before the
> reference is obtained.
> 
> You could also compare with with the other store and show functions in
> that same file and notice that none of them try to grab a reference to
> the lockspace in that way. So if this is required, then it must be
> required for those functions too.
> 
> Either way there is something not quite right here and having studied
> the code in some detail, I'm pretty sure this is the correct fix,

I guess you didn't see this oops in your tests.  Can you show that the
situation in this commit is no longer possible?

commit e2de7f565521a76fbbb927f701c5a1d381c71a93
Author: Patrick Caulfield <pcaulfie@redhat.com>
Date:   Mon Nov 6 08:53:28 2006 +0000

    [DLM] fix oops in kref_put when removing a lockspace
    
    Now that the lockspace struct is freed when the last sysfs object is released
    this patch prevents use of that lockspace by sysfs. We attempt to re-get the
    lockspace from the lockspace list and fail the request if it has been removed.
    
    Signed-Off-By: Patrick Caulfield <pcaulfie@redhat.com>
    Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>

diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
index 499ee11..f8842ca 100644
--- a/fs/dlm/lockspace.c
+++ b/fs/dlm/lockspace.c
@@ -43,6 +43,10 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
 	ssize_t ret = len;
 	int n = simple_strtol(buf, NULL, 0);
 
+	ls = dlm_find_lockspace_local(ls->ls_local_handle);
+	if (!ls)
+		return -EINVAL;
+
 	switch (n) {
 	case 0:
 		dlm_ls_stop(ls);
@@ -53,6 +57,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
 	default:
 		ret = -EINVAL;
 	}
+	dlm_put_lockspace(ls);
 	return ret;
 }
 

> 
> Steve.
> 
> > > Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
> > > ---
> > >  fs/dlm/lockspace.c |    6 +-----
> > >  1 files changed, 1 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> > > index 26a8bd4..ce0fdf5 100644
> > > --- a/fs/dlm/lockspace.c
> > > +++ b/fs/dlm/lockspace.c
> > > @@ -37,10 +37,6 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
> > >  	ssize_t ret = len;
> > >  	int n = simple_strtol(buf, NULL, 0);
> > >  
> > > -	ls = dlm_find_lockspace_local(ls->ls_local_handle);
> > > -	if (!ls)
> > > -		return -EINVAL;
> > > -
> > >  	switch (n) {
> > >  	case 0:
> > >  		dlm_ls_stop(ls);
> > > @@ -51,7 +47,7 @@ static ssize_t dlm_control_store(struct dlm_ls *ls, const char *buf, size_t len)
> > >  	default:
> > >  		ret = -EINVAL;
> > >  	}
> > > -	dlm_put_lockspace(ls);
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > -- 
> > > 1.6.2.5
> 

[Cluster-devel] [PATCH 2/2] dlm: Remove obsolete lockspace lookup

[Steven Whitehouse]

Hi,

On Thu, 2010-02-18 at 16:04 -0500, David Teigland wrote:
> On Thu, Feb 18, 2010 at 09:16:03AM +0000, Steven Whitehouse wrote:
> > I'm not sure what more I can say here.... this is a sysfs file store
> > function and one of the reasons for using it is that sysfs looks after
> > the ref counting for you.
> > 
> > Even aside from that, if you don't have a reference to the lockspace,
> > then the dereference that is done to discover the lockspace name would
> > be invalid, since the structure might have already been freed before the
> > reference is obtained.
> > 
> > You could also compare with with the other store and show functions in
> > that same file and notice that none of them try to grab a reference to
> > the lockspace in that way. So if this is required, then it must be
> > required for those functions too.
> > 
> > Either way there is something not quite right here and having studied
> > the code in some detail, I'm pretty sure this is the correct fix,
> 
> I guess you didn't see this oops in your tests.  Can you show that the
> situation in this commit is no longer possible?
> 
No, I didn't hit it. I'm not sure how to reproduce whatever situation
led to this in the first place.

There was a clue though in the patch prior to the one you pointed out in
the git tree, the comment in this patch doesn't make a lot of sense
until without the context from that patch. I noticed that where the
sysfs function does this:

> +	ls = dlm_find_lockspace_local(ls->ls_local_handle);
> +	if (!ls)
> +		return -EINVAL;
> +

it isn't primarily a ref count operation. Yes, it does get a ref count
on the object if it is successful, but the main purpose is testing to
see if the shutdown process has started (i.e. is the lockspace still on
the ls_list). If the list removal used a list_del_init rather than a
list del, the dlm_find_lockspace_local() call could be replaced with:

	spin_lock(&lslist_lock);
	ret = list_empty(&ls->ls_list);
	if (!ret)
		ls->ls_count++;
	spin_unlock(&lslist_lock);
	if (ret)
		return -EINVAL;

which might be a bit less confusing, and also saves traversing the list
of lockspaces. This is basically a "hold" operation, rather than a
find/get type operation.

My confusion has arisen from the fact that there are three ref counters
for the lockspace object. One is ls_count, one is ls_create_count and
the other the is kobject ref count.

ls_create_count seems to deal with user references, ls_count seems to be
used for internal references and the kobject ref count only seems to be
incremented/decremented on initial object creation/removal.

Probably the correct long term solution is to at least merge the
ls_count into kobject ref count system, and maybe the ls_create_count
too. I'll have to do some more investigation before I can see whether
there are any reasons why that isn't possible.

Either way, we are getting away from what was originally a small and
simple patch, so I'll suggest to ignore this one for now, and just apply
the first one of the two which I sent. I'll have another look at this in
the mean time,

Steve.