* [PATCH 1/4] blockdev: Don't use i_devices inode field
2014-11-04 10:27 [PATCH 0/4 v3] fs: Remove i_devices from struct inode Jan Kara
@ 2014-11-04 10:27 ` Jan Kara
2014-11-04 10:27 ` [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it Jan Kara
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2014-11-04 10:27 UTC (permalink / raw)
To: Al Viro; +Cc: Christoph Hellwig, LKML, linux-fsdevel, Jan Kara
Block devices use i_devices inode field to track all inodes that
reference a particular block device (through i_bdev field) so that this
reference can be removed when block device inode is being evicted from
memory. However we get a reference to the block device (in fact an inode
holding the block device structure) when setting up i_bdev in
bd_acquire() and we drop the reference only in bd_forget() when clearing
i_bdev. Thus inode holding block device structure can be evicted only
after all inodes referencing it are evicted and the whole excercise with
i_devices is pointless. Remove the i_devices handling.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/block_dev.c | 17 +++--------------
include/linux/fs.h | 1 -
2 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index cc9d4114cda0..493cd69df9a6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -458,7 +458,6 @@ static void init_once(void *foo)
memset(bdev, 0, sizeof(*bdev));
mutex_init(&bdev->bd_mutex);
- INIT_LIST_HEAD(&bdev->bd_inodes);
INIT_LIST_HEAD(&bdev->bd_list);
#ifdef CONFIG_SYSFS
INIT_LIST_HEAD(&bdev->bd_holder_disks);
@@ -468,24 +467,14 @@ static void init_once(void *foo)
mutex_init(&bdev->bd_fsfreeze_mutex);
}
-static inline void __bd_forget(struct inode *inode)
-{
- list_del_init(&inode->i_devices);
- inode->i_bdev = NULL;
- inode->i_mapping = &inode->i_data;
-}
-
static void bdev_evict_inode(struct inode *inode)
{
struct block_device *bdev = &BDEV_I(inode)->bdev;
- struct list_head *p;
+
truncate_inode_pages_final(&inode->i_data);
invalidate_inode_buffers(inode); /* is it needed here? */
clear_inode(inode);
spin_lock(&bdev_lock);
- while ( (p = bdev->bd_inodes.next) != &bdev->bd_inodes ) {
- __bd_forget(list_entry(p, struct inode, i_devices));
- }
list_del_init(&bdev->bd_list);
spin_unlock(&bdev_lock);
}
@@ -645,7 +634,6 @@ static struct block_device *bd_acquire(struct inode *inode)
ihold(bdev->bd_inode);
inode->i_bdev = bdev;
inode->i_mapping = bdev->bd_inode->i_mapping;
- list_add(&inode->i_devices, &bdev->bd_inodes);
}
spin_unlock(&bdev_lock);
}
@@ -666,7 +654,8 @@ void bd_forget(struct inode *inode)
spin_lock(&bdev_lock);
if (!sb_is_blkdev_sb(inode->i_sb))
bdev = inode->i_bdev;
- __bd_forget(inode);
+ inode->i_bdev = NULL;
+ inode->i_mapping = &inode->i_data;
spin_unlock(&bdev_lock);
if (bdev)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a957d4366c24..d7fd7959a933 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -413,7 +413,6 @@ struct block_device {
struct inode * bd_inode; /* will die */
struct super_block * bd_super;
struct mutex bd_mutex; /* open/close mutex */
- struct list_head bd_inodes;
void * bd_claiming;
void * bd_holder;
int bd_holders;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it
2014-11-04 10:27 [PATCH 0/4 v3] fs: Remove i_devices from struct inode Jan Kara
2014-11-04 10:27 ` [PATCH 1/4] blockdev: Don't use i_devices inode field Jan Kara
@ 2014-11-04 10:27 ` Jan Kara
2014-11-04 10:27 ` [PATCH 3/4] chardev: Don't use i_devices inode field Jan Kara
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2014-11-04 10:27 UTC (permalink / raw)
To: Al Viro; +Cc: Christoph Hellwig, LKML, linux-fsdevel, Jan Kara
Currently i_cdev reference to a character device isn't accounted in the
reference count of the character device. This then requires us to track
all references through a list of all inodes referencing a character
device which is somewhat clumsy and requires list_head in each inode in
the system.
So make i_cdev a reference like any other.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/char_dev.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/fs/char_dev.c b/fs/char_dev.c
index f77f7702fabe..518b3016ab92 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -388,12 +388,13 @@ static int chrdev_open(struct inode *inode, struct file *filp)
we dropped the lock. */
p = inode->i_cdev;
if (!p) {
+ /* Use reference from kobj_lookup for i_cdev ref */
inode->i_cdev = p = new;
list_add(&inode->i_devices, &p->list);
new = NULL;
- } else if (!cdev_get(p))
- ret = -ENXIO;
- } else if (!cdev_get(p))
+ }
+ }
+ if (!cdev_get(p))
ret = -ENXIO;
spin_unlock(&cdev_lock);
cdev_put(new);
@@ -421,10 +422,15 @@ static int chrdev_open(struct inode *inode, struct file *filp)
void cd_forget(struct inode *inode)
{
+ struct cdev *to_put;
+
spin_lock(&cdev_lock);
list_del_init(&inode->i_devices);
+ to_put = inode->i_cdev;
inode->i_cdev = NULL;
spin_unlock(&cdev_lock);
+ if (to_put)
+ cdev_put(to_put);
}
static void cdev_purge(struct cdev *cdev)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] chardev: Don't use i_devices inode field
2014-11-04 10:27 [PATCH 0/4 v3] fs: Remove i_devices from struct inode Jan Kara
2014-11-04 10:27 ` [PATCH 1/4] blockdev: Don't use i_devices inode field Jan Kara
2014-11-04 10:27 ` [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it Jan Kara
@ 2014-11-04 10:27 ` Jan Kara
2014-11-04 10:27 ` [PATCH 4/4] fs: Remove i_devices list head Jan Kara
2014-11-04 15:39 ` [PATCH 0/4 v3] fs: Remove i_devices from struct inode Al Viro
4 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2014-11-04 10:27 UTC (permalink / raw)
To: Al Viro; +Cc: Christoph Hellwig, LKML, linux-fsdevel, Jan Kara
Now that character device can be freed only after all inodes referencing
it through i_cdev are gone, we can remove all the tracking of inodes
pointing to a character device.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/char_dev.c | 22 +---------------------
include/linux/cdev.h | 1 -
2 files changed, 1 insertion(+), 22 deletions(-)
diff --git a/fs/char_dev.c b/fs/char_dev.c
index 518b3016ab92..4c155efc1556 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -390,7 +390,6 @@ static int chrdev_open(struct inode *inode, struct file *filp)
if (!p) {
/* Use reference from kobj_lookup for i_cdev ref */
inode->i_cdev = p = new;
- list_add(&inode->i_devices, &p->list);
new = NULL;
}
}
@@ -425,7 +424,6 @@ void cd_forget(struct inode *inode)
struct cdev *to_put;
spin_lock(&cdev_lock);
- list_del_init(&inode->i_devices);
to_put = inode->i_cdev;
inode->i_cdev = NULL;
spin_unlock(&cdev_lock);
@@ -433,18 +431,6 @@ void cd_forget(struct inode *inode)
cdev_put(to_put);
}
-static void cdev_purge(struct cdev *cdev)
-{
- spin_lock(&cdev_lock);
- while (!list_empty(&cdev->list)) {
- struct inode *inode;
- inode = container_of(cdev->list.next, struct inode, i_devices);
- list_del_init(&inode->i_devices);
- inode->i_cdev = NULL;
- }
- spin_unlock(&cdev_lock);
-}
-
/*
* Dummy default file-operations: the only thing this does
* is contain the open that then fills in the correct operations
@@ -515,10 +501,8 @@ void cdev_del(struct cdev *p)
static void cdev_default_release(struct kobject *kobj)
{
- struct cdev *p = container_of(kobj, struct cdev, kobj);
struct kobject *parent = kobj->parent;
- cdev_purge(p);
kobject_put(parent);
}
@@ -527,7 +511,6 @@ static void cdev_dynamic_release(struct kobject *kobj)
struct cdev *p = container_of(kobj, struct cdev, kobj);
struct kobject *parent = kobj->parent;
- cdev_purge(p);
kfree(p);
kobject_put(parent);
}
@@ -548,10 +531,8 @@ static struct kobj_type ktype_cdev_dynamic = {
struct cdev *cdev_alloc(void)
{
struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
- if (p) {
- INIT_LIST_HEAD(&p->list);
+ if (p)
kobject_init(&p->kobj, &ktype_cdev_dynamic);
- }
return p;
}
@@ -566,7 +547,6 @@ struct cdev *cdev_alloc(void)
void cdev_init(struct cdev *cdev, const struct file_operations *fops)
{
memset(cdev, 0, sizeof *cdev);
- INIT_LIST_HEAD(&cdev->list);
kobject_init(&cdev->kobj, &ktype_cdev_default);
cdev->ops = fops;
}
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index fb4591977b03..fe00138b5106 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -13,7 +13,6 @@ struct cdev {
struct kobject kobj;
struct module *owner;
const struct file_operations *ops;
- struct list_head list;
dev_t dev;
unsigned int count;
};
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] fs: Remove i_devices list head
2014-11-04 10:27 [PATCH 0/4 v3] fs: Remove i_devices from struct inode Jan Kara
` (2 preceding siblings ...)
2014-11-04 10:27 ` [PATCH 3/4] chardev: Don't use i_devices inode field Jan Kara
@ 2014-11-04 10:27 ` Jan Kara
2014-11-04 15:39 ` [PATCH 0/4 v3] fs: Remove i_devices from struct inode Al Viro
4 siblings, 0 replies; 16+ messages in thread
From: Jan Kara @ 2014-11-04 10:27 UTC (permalink / raw)
To: Al Viro; +Cc: Christoph Hellwig, LKML, linux-fsdevel, Jan Kara
Noone uses i_devices anymore. Remove it (thus saving two pointers in
every inode).
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/inode.c | 1 -
include/linux/fs.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 26753ba7b6d6..cb7acd45dce5 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -366,7 +366,6 @@ void inode_init_once(struct inode *inode)
{
memset(inode, 0, sizeof(*inode));
INIT_HLIST_NODE(&inode->i_hash);
- INIT_LIST_HEAD(&inode->i_devices);
INIT_LIST_HEAD(&inode->i_wb_list);
INIT_LIST_HEAD(&inode->i_lru);
address_space_init_once(&inode->i_data);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d7fd7959a933..8c4f5678e75e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -595,7 +595,6 @@ struct inode {
#ifdef CONFIG_QUOTA
struct dquot *i_dquot[MAXQUOTAS];
#endif
- struct list_head i_devices;
union {
struct pipe_inode_info *i_pipe;
struct block_device *i_bdev;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 10:27 [PATCH 0/4 v3] fs: Remove i_devices from struct inode Jan Kara
` (3 preceding siblings ...)
2014-11-04 10:27 ` [PATCH 4/4] fs: Remove i_devices list head Jan Kara
@ 2014-11-04 15:39 ` Al Viro
2014-11-04 19:47 ` Andy Lutomirski
4 siblings, 1 reply; 16+ messages in thread
From: Al Viro @ 2014-11-04 15:39 UTC (permalink / raw)
To: Jan Kara; +Cc: Christoph Hellwig, LKML, linux-fsdevel
On Tue, Nov 04, 2014 at 11:27:27AM +0100, Jan Kara wrote:
> Hello,
>
> this patch set removes use of i_devices from block and character device
> code and thus we can remove the list head from struct inode thus saving two
> pointers in it. As Christoph has reviewed the series, can you please merge
> it Al? Thanks!
>
> Since v2 I've added reviewed-by tags from Christoph and changed one variable
> name in cdev_forget().
>
> Since v1 I have split the patches and properly handled character devices (I
> broke them last time as Christoph pointed out).
My problem with that is in buggered module refcounts (which was the reason
for doing those non-counting references back then). Suppose you open
/dev/some_char_device and close it; having the module pinned down until
the inode of that sucker gets evicted by dcache/icache memory pressure
would be wrong - it _isn't_ in use, and there's no way short of forcing
the full eviction of VFS caches to get it possible to unload...
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 15:39 ` [PATCH 0/4 v3] fs: Remove i_devices from struct inode Al Viro
@ 2014-11-04 19:47 ` Andy Lutomirski
2014-11-04 19:52 ` Andy Lutomirski
2014-11-04 19:55 ` Jan Kara
0 siblings, 2 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-11-04 19:47 UTC (permalink / raw)
To: Al Viro, Jan Kara; +Cc: Christoph Hellwig, LKML, linux-fsdevel
On 11/04/2014 07:39 AM, Al Viro wrote:
> On Tue, Nov 04, 2014 at 11:27:27AM +0100, Jan Kara wrote:
>> Hello,
>>
>> this patch set removes use of i_devices from block and character device
>> code and thus we can remove the list head from struct inode thus saving two
>> pointers in it. As Christoph has reviewed the series, can you please merge
>> it Al? Thanks!
>>
>> Since v2 I've added reviewed-by tags from Christoph and changed one variable
>> name in cdev_forget().
>>
>> Since v1 I have split the patches and properly handled character devices (I
>> broke them last time as Christoph pointed out).
>
> My problem with that is in buggered module refcounts (which was the reason
> for doing those non-counting references back then). Suppose you open
> /dev/some_char_device and close it; having the module pinned down until
> the inode of that sucker gets evicted by dcache/icache memory pressure
> would be wrong - it _isn't_ in use, and there's no way short of forcing
> the full eviction of VFS caches to get it possible to unload...
>
At the risk of asking what may be a rather dumb question...
Why do device node inodes need to be cached at all? In other words,
when you try open a device node, can't the kernel materialize the inode
from just information that's in the dentry without touching the
filesystem at all? If that's true, couldn't all device inodes be
dropped from icache as soon as they're unreferenced?
(Yes, there's mtime, but I never understood why tracking mtime on device
nodes made any sense in the first place.)
--Andy
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 19:47 ` Andy Lutomirski
@ 2014-11-04 19:52 ` Andy Lutomirski
2014-11-04 20:16 ` Al Viro
2014-11-04 19:55 ` Jan Kara
1 sibling, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-11-04 19:52 UTC (permalink / raw)
To: Al Viro, Jan Kara; +Cc: Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 4, 2014 at 11:47 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On 11/04/2014 07:39 AM, Al Viro wrote:
>> On Tue, Nov 04, 2014 at 11:27:27AM +0100, Jan Kara wrote:
>>> Hello,
>>>
>>> this patch set removes use of i_devices from block and character device
>>> code and thus we can remove the list head from struct inode thus saving two
>>> pointers in it. As Christoph has reviewed the series, can you please merge
>>> it Al? Thanks!
>>>
>>> Since v2 I've added reviewed-by tags from Christoph and changed one variable
>>> name in cdev_forget().
>>>
>>> Since v1 I have split the patches and properly handled character devices (I
>>> broke them last time as Christoph pointed out).
>>
>> My problem with that is in buggered module refcounts (which was the reason
>> for doing those non-counting references back then). Suppose you open
>> /dev/some_char_device and close it; having the module pinned down until
>> the inode of that sucker gets evicted by dcache/icache memory pressure
>> would be wrong - it _isn't_ in use, and there's no way short of forcing
>> the full eviction of VFS caches to get it possible to unload...
>>
>
> At the risk of asking what may be a rather dumb question...
>
> Why do device node inodes need to be cached at all? In other words,
> when you try open a device node, can't the kernel materialize the inode
> from just information that's in the dentry without touching the
> filesystem at all? If that's true, couldn't all device inodes be
> dropped from icache as soon as they're unreferenced?
>
> (Yes, there's mtime, but I never understood why tracking mtime on device
> nodes made any sense in the first place.)
On further reflection, this is indeed a dumb question. i_rdev is in
the inode, so of course the inode is useful.
Let me try again, though: what if a chardev inode replaced i_cdev with
NULL and dropped its kobj reference in iput_final? This would add a
bit over overhead to things that repeatedly open and close the same
device node, but I doubt this matters much.
--Andy
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 19:52 ` Andy Lutomirski
@ 2014-11-04 20:16 ` Al Viro
2014-11-04 20:24 ` Andy Lutomirski
0 siblings, 1 reply; 16+ messages in thread
From: Al Viro @ 2014-11-04 20:16 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Jan Kara, Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 04, 2014 at 11:52:22AM -0800, Andy Lutomirski wrote:
> Let me try again, though: what if a chardev inode replaced i_cdev with
> NULL and dropped its kobj reference in iput_final? This would add a
> bit over overhead to things that repeatedly open and close the same
> device node, but I doubt this matters much.
How is it different from what Jan proposed? The whole problem is that
iput_final() is too late. We could clean the reference on close, all
right, and have it looked up again on every open, but I'm not at all sure
that situation with opening/closing cdev is never a hot path, especially with
udev playing silly buggers, etc.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 20:16 ` Al Viro
@ 2014-11-04 20:24 ` Andy Lutomirski
0 siblings, 0 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-11-04 20:24 UTC (permalink / raw)
To: Al Viro; +Cc: Jan Kara, Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 4, 2014 at 12:16 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Nov 04, 2014 at 11:52:22AM -0800, Andy Lutomirski wrote:
>
>> Let me try again, though: what if a chardev inode replaced i_cdev with
>> NULL and dropped its kobj reference in iput_final? This would add a
>> bit over overhead to things that repeatedly open and close the same
>> device node, but I doubt this matters much.
>
> How is it different from what Jan proposed? The whole problem is that
> iput_final() is too late. We could clean the reference on close, all
> right, and have it looked up again on every open, but I'm not at all sure
> that situation with opening/closing cdev is never a hot path, especially with
> udev playing silly buggers, etc.
I thought the issue was inodes on the LRU list holding references open.
Anyway, the only expensive part of recreating the reference seems to
be kobj_lookup, and that doesn't look so bad. If there's a case where
the performance impact would matter at all, maybe that's an argument
for fixing it.
And is udev really opening the same device node more than once? I can
see once for blkid and such, but I don't see where a second open call
would come from.
--Andy
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 19:47 ` Andy Lutomirski
2014-11-04 19:52 ` Andy Lutomirski
@ 2014-11-04 19:55 ` Jan Kara
2014-11-04 20:14 ` Andy Lutomirski
1 sibling, 1 reply; 16+ messages in thread
From: Jan Kara @ 2014-11-04 19:55 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Al Viro, Jan Kara, Christoph Hellwig, LKML, linux-fsdevel
On Tue 04-11-14 11:47:21, Andy Lutomirski wrote:
> On 11/04/2014 07:39 AM, Al Viro wrote:
> > On Tue, Nov 04, 2014 at 11:27:27AM +0100, Jan Kara wrote:
> >> Hello,
> >>
> >> this patch set removes use of i_devices from block and character device
> >> code and thus we can remove the list head from struct inode thus saving two
> >> pointers in it. As Christoph has reviewed the series, can you please merge
> >> it Al? Thanks!
> >>
> >> Since v2 I've added reviewed-by tags from Christoph and changed one variable
> >> name in cdev_forget().
> >>
> >> Since v1 I have split the patches and properly handled character devices (I
> >> broke them last time as Christoph pointed out).
> >
> > My problem with that is in buggered module refcounts (which was the reason
> > for doing those non-counting references back then). Suppose you open
> > /dev/some_char_device and close it; having the module pinned down until
> > the inode of that sucker gets evicted by dcache/icache memory pressure
> > would be wrong - it _isn't_ in use, and there's no way short of forcing
> > the full eviction of VFS caches to get it possible to unload...
> >
>
> At the risk of asking what may be a rather dumb question...
>
> Why do device node inodes need to be cached at all? In other words,
> when you try open a device node, can't the kernel materialize the inode
> from just information that's in the dentry without touching the
> filesystem at all? If that's true, couldn't all device inodes be
> dropped from icache as soon as they're unreferenced?
>
> (Yes, there's mtime, but I never understood why tracking mtime on device
> nodes made any sense in the first place.)
I can see a few reasons:
1) positive dentry without inode - no-no for dcache.
2) how would you get the information which device the dentry references?
3) what would you gain to outweight the complications and special code
paths?
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 19:55 ` Jan Kara
@ 2014-11-04 20:14 ` Andy Lutomirski
2014-11-04 20:20 ` Al Viro
0 siblings, 1 reply; 16+ messages in thread
From: Andy Lutomirski @ 2014-11-04 20:14 UTC (permalink / raw)
To: Jan Kara; +Cc: Al Viro, Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 4, 2014 at 11:55 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 04-11-14 11:47:21, Andy Lutomirski wrote:
>> On 11/04/2014 07:39 AM, Al Viro wrote:
>> > On Tue, Nov 04, 2014 at 11:27:27AM +0100, Jan Kara wrote:
>> >> Hello,
>> >>
>> >> this patch set removes use of i_devices from block and character device
>> >> code and thus we can remove the list head from struct inode thus saving two
>> >> pointers in it. As Christoph has reviewed the series, can you please merge
>> >> it Al? Thanks!
>> >>
>> >> Since v2 I've added reviewed-by tags from Christoph and changed one variable
>> >> name in cdev_forget().
>> >>
>> >> Since v1 I have split the patches and properly handled character devices (I
>> >> broke them last time as Christoph pointed out).
>> >
>> > My problem with that is in buggered module refcounts (which was the reason
>> > for doing those non-counting references back then). Suppose you open
>> > /dev/some_char_device and close it; having the module pinned down until
>> > the inode of that sucker gets evicted by dcache/icache memory pressure
>> > would be wrong - it _isn't_ in use, and there's no way short of forcing
>> > the full eviction of VFS caches to get it possible to unload...
>> >
>>
>> At the risk of asking what may be a rather dumb question...
>>
>> Why do device node inodes need to be cached at all? In other words,
>> when you try open a device node, can't the kernel materialize the inode
>> from just information that's in the dentry without touching the
>> filesystem at all? If that's true, couldn't all device inodes be
>> dropped from icache as soon as they're unreferenced?
>>
>> (Yes, there's mtime, but I never understood why tracking mtime on device
>> nodes made any sense in the first place.)
> I can see a few reasons:
> 1) positive dentry without inode - no-no for dcache.
> 2) how would you get the information which device the dentry references?
> 3) what would you gain to outweight the complications and special code
> paths?
>
Yeah, this idea clearly doesn't work. But I wonder whether the
revised variant (which may be just moving cd_forget and bd_forget from
evict to iput_final) would work.
--Andy
> Honza
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 20:14 ` Andy Lutomirski
@ 2014-11-04 20:20 ` Al Viro
2014-11-04 20:25 ` Andy Lutomirski
0 siblings, 1 reply; 16+ messages in thread
From: Al Viro @ 2014-11-04 20:20 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Jan Kara, Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 04, 2014 at 12:14:26PM -0800, Andy Lutomirski wrote:
> Yeah, this idea clearly doesn't work. But I wonder whether the
> revised variant (which may be just moving cd_forget and bd_forget from
> evict to iput_final) would work.
How the hell is it better? Neither of those will happen as long as dentry
is retained. And with /dev on tmpfs (which is bloody common) _that_ won't
happen until device node is unlinked...
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4 v3] fs: Remove i_devices from struct inode
2014-11-04 20:20 ` Al Viro
@ 2014-11-04 20:25 ` Andy Lutomirski
0 siblings, 0 replies; 16+ messages in thread
From: Andy Lutomirski @ 2014-11-04 20:25 UTC (permalink / raw)
To: Al Viro; +Cc: Jan Kara, Christoph Hellwig, LKML, Linux FS Devel
On Tue, Nov 4, 2014 at 12:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Nov 04, 2014 at 12:14:26PM -0800, Andy Lutomirski wrote:
>
>> Yeah, this idea clearly doesn't work. But I wonder whether the
>> revised variant (which may be just moving cd_forget and bd_forget from
>> evict to iput_final) would work.
>
> How the hell is it better? Neither of those will happen as long as dentry
> is retained. And with /dev on tmpfs (which is bloody common) _that_ won't
> happen until device node is unlinked...
It's by my misunderstanding of how inode refcounting works. The idea
would be to drop the cdev reference when there are no non-VFS-internal
references to the inode left.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/4] chardev: Don't use i_devices inode field
2014-10-22 20:14 [PATCH 0/4 v2] " Jan Kara
@ 2014-10-22 20:14 ` Jan Kara
2014-11-04 9:54 ` Christoph Hellwig
0 siblings, 1 reply; 16+ messages in thread
From: Jan Kara @ 2014-10-22 20:14 UTC (permalink / raw)
To: Al Viro; +Cc: hch, linux-fsdevel, LKML, Jan Kara
Now that character device can be freed only after all inodes referencing
it through i_cdev are gone, we can remove all the tracking of inodes
pointing to a character device.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/char_dev.c | 22 +---------------------
include/linux/cdev.h | 1 -
2 files changed, 1 insertion(+), 22 deletions(-)
diff --git a/fs/char_dev.c b/fs/char_dev.c
index 21c210dbdba1..08a1404f590b 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -390,7 +390,6 @@ static int chrdev_open(struct inode *inode, struct file *filp)
if (!p) {
/* Use reference from kobj_lookup for i_cdev ref */
inode->i_cdev = p = new;
- list_add(&inode->i_devices, &p->list);
new = NULL;
}
}
@@ -425,7 +424,6 @@ void cd_forget(struct inode *inode)
struct cdev *cdev;
spin_lock(&cdev_lock);
- list_del_init(&inode->i_devices);
cdev = inode->i_cdev;
inode->i_cdev = NULL;
spin_unlock(&cdev_lock);
@@ -433,18 +431,6 @@ void cd_forget(struct inode *inode)
cdev_put(cdev);
}
-static void cdev_purge(struct cdev *cdev)
-{
- spin_lock(&cdev_lock);
- while (!list_empty(&cdev->list)) {
- struct inode *inode;
- inode = container_of(cdev->list.next, struct inode, i_devices);
- list_del_init(&inode->i_devices);
- inode->i_cdev = NULL;
- }
- spin_unlock(&cdev_lock);
-}
-
/*
* Dummy default file-operations: the only thing this does
* is contain the open that then fills in the correct operations
@@ -515,10 +501,8 @@ void cdev_del(struct cdev *p)
static void cdev_default_release(struct kobject *kobj)
{
- struct cdev *p = container_of(kobj, struct cdev, kobj);
struct kobject *parent = kobj->parent;
- cdev_purge(p);
kobject_put(parent);
}
@@ -527,7 +511,6 @@ static void cdev_dynamic_release(struct kobject *kobj)
struct cdev *p = container_of(kobj, struct cdev, kobj);
struct kobject *parent = kobj->parent;
- cdev_purge(p);
kfree(p);
kobject_put(parent);
}
@@ -548,10 +531,8 @@ static struct kobj_type ktype_cdev_dynamic = {
struct cdev *cdev_alloc(void)
{
struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
- if (p) {
- INIT_LIST_HEAD(&p->list);
+ if (p)
kobject_init(&p->kobj, &ktype_cdev_dynamic);
- }
return p;
}
@@ -566,7 +547,6 @@ struct cdev *cdev_alloc(void)
void cdev_init(struct cdev *cdev, const struct file_operations *fops)
{
memset(cdev, 0, sizeof *cdev);
- INIT_LIST_HEAD(&cdev->list);
kobject_init(&cdev->kobj, &ktype_cdev_default);
cdev->ops = fops;
}
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index fb4591977b03..fe00138b5106 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -13,7 +13,6 @@ struct cdev {
struct kobject kobj;
struct module *owner;
const struct file_operations *ops;
- struct list_head list;
dev_t dev;
unsigned int count;
};
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread