2008-11-18 01:41:10

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

(lkml was missing, adding it back and quoting whole body for reference)

Tejun Heo wrote:
> Greg KH wrote:
>> On Mon, Nov 17, 2008 at 06:18:45PM +0900, Tejun Heo wrote:
>>> (this is a private message)
>>>
>>> PING. :-)
>> Heh.
>>
>> I've lost this out of my tree, as I didn't know you still wanted this
>> applied. If you need it for your char-fuse code, then I don't have an
>> objection to it, and you can take it through your tree.
>>
>> Or do you want me to take it through mine?
>>
>> Your choice :)
>
> I'll push it through Miklos' tree w/ your ACK then.
>
> Thanks. :-)
>


--
tejun


2008-11-18 13:44:38

by Boaz Harrosh

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

Tejun Heo wrote:
> (lkml was missing, adding it back and quoting whole body for reference)
>
> Tejun Heo wrote:
>> Greg KH wrote:
>>> On Mon, Nov 17, 2008 at 06:18:45PM +0900, Tejun Heo wrote:
>>>> (this is a private message)
>>>>
>>>> PING. :-)
>>> Heh.
>>>
>>> I've lost this out of my tree, as I didn't know you still wanted this
>>> applied. If you need it for your char-fuse code, then I don't have an
>>> objection to it, and you can take it through your tree.
>>>
>>> Or do you want me to take it through mine?
>>>
>>> Your choice :)
>> I'll push it through Miklos' tree w/ your ACK then.
>>
>> Thanks. :-)
>>
>
>

Tejun Hi.

I just saw this thread for the first time and it left me confused.
What was the final verdict. Is this patch going in at the end?
Which incarnation of it? is there a public git tree I can try?

The reason I ask is because I have just the same principal work in one of
my test trees. What I have is a Filesystem, osdfs, that is mounted
on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
an OSD device does not use __open, like user mode it needs some kernel
reference counting to keep the char-device up. On the other hand
the actual teardown and unmap of the char-device is done from the scsi-ml
remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
the device but keep the memory allocated and available until the last reference.
All this is usually done using the Release() of the block-device. But for me
I only have a char-device. Currently what I had to do is keep another kref
to govern the device's lifecycle and sync every thing together. A Release() at
the char-dev would let me reuse what's there and let me clean all that code up.

While Investigating the problem and compering what was done on the block-device
side, I've seen more then a few places that private reference counting could be
dropped completely, and the char-dev could be used. Off my head some of these places
are:
- UBI used by UBIFS
- sg.c which does not have a Kernel user but needs it's char device until
scsi-Remove and/or __close()

and other places as well.

Thanks for doing this
Boaz

2008-11-18 15:58:56

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

Hello,

Boaz Harrosh wrote:
> I just saw this thread for the first time and it left me confused.
> What was the final verdict. Is this patch going in at the end?
> Which incarnation of it? is there a public git tree I can try?

Yes it is and as posted and no there isn't no public git tree yet, I was
planning on pushing it through Miklos' tree.

> The reason I ask is because I have just the same principal work in one of
> my test trees. What I have is a Filesystem, osdfs, that is mounted
> on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
> an OSD device does not use __open, like user mode it needs some kernel
> reference counting to keep the char-device up. On the other hand
> the actual teardown and unmap of the char-device is done from the scsi-ml
> remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
> the device but keep the memory allocated and available until the last reference.
> All this is usually done using the Release() of the block-device. But for me
> I only have a char-device. Currently what I had to do is keep another kref
> to govern the device's lifecycle and sync every thing together. A Release() at
> the char-dev would let me reuse what's there and let me clean all that code up.
>
> While Investigating the problem and compering what was done on the block-device
> side, I've seen more then a few places that private reference counting could be
> dropped completely, and the char-dev could be used. Off my head some of these places
> are:
> - UBI used by UBIFS
> - sg.c which does not have a Kernel user but needs it's char device until
> scsi-Remove and/or __close()
>
> and other places as well.

Hmmm... if you can use this change, I think you can push it through
whatever tree you push patches through, things can be taken care of when
merging or we can put it in Greg's tree and ask Miklos to pull from the
tree first but Greg doesn't keep a stable tree. Hmm... if your patches
are gonna go through some public tree, I think we can ask Miklos pull
from the tree too. Guys, what do you think?

Thanks.

--
tejun

2008-11-19 17:59:49

by Miklos Szeredi

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

On Wed, 19 Nov 2008, Tejun Heo wrote:
> Hmmm... if you can use this change, I think you can push it through
> whatever tree you push patches through, things can be taken care of when
> merging or we can put it in Greg's tree and ask Miklos to pull from the
> tree first but Greg doesn't keep a stable tree. Hmm... if your patches
> are gonna go through some public tree, I think we can ask Miklos pull
> from the tree too. Guys, what do you think?

Sure, since this is not fuse specific, it better not go through my
tree anyway.

Thanks,
Miklos

2008-11-20 06:13:34

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

On Wed, Nov 19, 2008 at 12:58:26AM +0900, Tejun Heo wrote:
> Hello,
>
> Boaz Harrosh wrote:
> > I just saw this thread for the first time and it left me confused.
> > What was the final verdict. Is this patch going in at the end?
> > Which incarnation of it? is there a public git tree I can try?
>
> Yes it is and as posted and no there isn't no public git tree yet, I was
> planning on pushing it through Miklos' tree.
>
> > The reason I ask is because I have just the same principal work in one of
> > my test trees. What I have is a Filesystem, osdfs, that is mounted
> > on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
> > an OSD device does not use __open, like user mode it needs some kernel
> > reference counting to keep the char-device up. On the other hand
> > the actual teardown and unmap of the char-device is done from the scsi-ml
> > remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
> > the device but keep the memory allocated and available until the last reference.
> > All this is usually done using the Release() of the block-device. But for me
> > I only have a char-device. Currently what I had to do is keep another kref
> > to govern the device's lifecycle and sync every thing together. A Release() at
> > the char-dev would let me reuse what's there and let me clean all that code up.
> >
> > While Investigating the problem and compering what was done on the block-device
> > side, I've seen more then a few places that private reference counting could be
> > dropped completely, and the char-dev could be used. Off my head some of these places
> > are:
> > - UBI used by UBIFS
> > - sg.c which does not have a Kernel user but needs it's char device until
> > scsi-Remove and/or __close()
> >
> > and other places as well.
>
> Hmmm... if you can use this change, I think you can push it through
> whatever tree you push patches through, things can be taken care of when
> merging or we can put it in Greg's tree and ask Miklos to pull from the
> tree first but Greg doesn't keep a stable tree.

I don't keep a stable tree? What do you mean? It's a quilt tree, and
it does get included in -next, so I think it does work :)

Send it to me again and I'll push it through my tree if no one else
objects.

thanks,

greg k-h

2008-11-20 06:28:49

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

Greg KH wrote:
>> Hmmm... if you can use this change, I think you can push it through
>> whatever tree you push patches through, things can be taken care of when
>> merging or we can put it in Greg's tree and ask Miklos to pull from the
>> tree first but Greg doesn't keep a stable tree.
>
> I don't keep a stable tree? What do you mean? It's a quilt tree, and
> it does get included in -next, so I think it does work :)

Heh... I meant stable (in the sense that it doesn't get rebased) git
tree which Miklos can pull from.

> Send it to me again and I'll push it through my tree if no one else
> objects.

Yeap, will resend it and let you and Miklos sort it out.

Thanks.

--
tejun

2008-11-20 11:46:12

by Tejun Heo

[permalink] [raw]
Subject: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it

Add cdev->release() so that cdev can be considered in more involved
object lifetime management. cdev_alloc() used a separate ktype for
auto-free release(). This patch converts it to use cdev->release() so
that there's no need for separate ktype and cdev_init() can be used
for auto-free variant too.

Signed-off-by: Tejun Heo <[email protected]>
---
fs/char_dev.c | 30 +++++++++++++-----------------
include/linux/cdev.h | 1 +
2 files changed, 14 insertions(+), 17 deletions(-)

Index: work/fs/char_dev.c
===================================================================
--- work.orig/fs/char_dev.c
+++ work/fs/char_dev.c
@@ -482,26 +482,22 @@ void cdev_del(struct cdev *p)
}


-static void cdev_default_release(struct kobject *kobj)
+static void cdev_release(struct kobject *kobj)
{
struct cdev *p = container_of(kobj, struct cdev, kobj);
cdev_purge(p);
+ if (p->release)
+ p->release(p);
}

-static void cdev_dynamic_release(struct kobject *kobj)
-{
- struct cdev *p = container_of(kobj, struct cdev, kobj);
- cdev_purge(p);
- kfree(p);
-}
-
-static struct kobj_type ktype_cdev_default = {
- .release = cdev_default_release,
+static struct kobj_type cdev_ktype = {
+ .release = cdev_release,
};

-static struct kobj_type ktype_cdev_dynamic = {
- .release = cdev_dynamic_release,
-};
+static void cdev_alloc_release(struct cdev *cdev)
+{
+ kfree(cdev);
+}

/**
* cdev_alloc() - allocate a cdev structure
@@ -510,10 +506,10 @@ static struct kobj_type ktype_cdev_dynam
*/
struct cdev *cdev_alloc(void)
{
- struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
+ struct cdev *p = kmalloc(sizeof(struct cdev), GFP_KERNEL);
if (p) {
- INIT_LIST_HEAD(&p->list);
- kobject_init(&p->kobj, &ktype_cdev_dynamic);
+ cdev_init(p, NULL);
+ p->release = cdev_alloc_release;
}
return p;
}
@@ -530,7 +526,7 @@ void cdev_init(struct cdev *cdev, const
{
memset(cdev, 0, sizeof *cdev);
INIT_LIST_HEAD(&cdev->list);
- kobject_init(&cdev->kobj, &ktype_cdev_default);
+ kobject_init(&cdev->kobj, &cdev_ktype);
cdev->ops = fops;
}

Index: work/include/linux/cdev.h
===================================================================
--- work.orig/include/linux/cdev.h
+++ work/include/linux/cdev.h
@@ -16,6 +16,7 @@ struct cdev {
struct list_head list;
dev_t dev;
unsigned int count;
+ void (*release)(struct cdev *);
};

void cdev_init(struct cdev *, const struct file_operations *);