FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Cluster Development

 
 
LinkBack Thread Tools
 
Old 02-17-2010, 07:12 PM
David Teigland
 
Default dlm: Remove obsolete lockspace lookup

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
 
Old 02-18-2010, 08:16 AM
Steven Whitehouse
 
Default dlm: Remove obsolete lockspace lookup

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
 
Old 02-18-2010, 08:04 PM
David Teigland
 
Default dlm: Remove obsolete lockspace lookup

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
>
 
Old 02-19-2010, 10:52 AM
Steven Whitehouse
 
Default dlm: Remove obsolete lockspace lookup

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.
 

Thread Tools




All times are GMT. The time now is 10:03 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org