[PATCH 0/4 v2] fs: Remove i_devices from struct inode

[PATCH 0/4 v2] fs: Remove i_devices from struct inode

[Jan Kara]

  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.

Since v1 I have split the patches and properly handled character devices (I
broke them last time as Christoph pointed out).

								Honza

[PATCH 1/4] blockdev: Don't use i_devices inode field

[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.

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

Re: [PATCH 1/4] blockdev: Don't use i_devices inode field

[Christoph Hellwig]

On Wed, Oct 22, 2014 at 10:14:10PM +0200, Jan Kara wrote:
> 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.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[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.

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..21c210dbdba1 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 *cdev;
+
 	spin_lock(&cdev_lock);
 	list_del_init(&inode->i_devices);
+	cdev = inode->i_cdev;
 	inode->i_cdev = NULL;
 	spin_unlock(&cdev_lock);
+	if (cdev)
+		cdev_put(cdev);
 }
 
 static void cdev_purge(struct cdev *cdev)
-- 
1.8.1.4

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Christoph Hellwig]

On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> 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.

Looks good.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Maybe you can rename the new variable to something like to_put also?

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Jan Kara]

On Tue 04-11-14 10:52:58, Christoph Hellwig wrote:
> On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> > 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.
> 
> Looks good.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
  Thanks for review.

> Maybe you can rename the new variable to something like to_put also?
  Yes, that's better. Done.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Al Viro]

On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> 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.

So no rmmod for you until an inode of character device node you had
closed a while ago finally gets evicted from icache?  Or am I misreading
you?

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Jan Kara]

On Tue 04-11-14 14:46:11, Al Viro wrote:
> On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> > 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.
> 
> So no rmmod for you until an inode of character device node you had
> closed a while ago finally gets evicted from icache?  Or am I misreading
> you?
  Yes, this is a consequence of the change. I should have noted in the
changelog I guess.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Al Viro]

On Tue, Nov 04, 2014 at 04:37:52PM +0100, Jan Kara wrote:
> On Tue 04-11-14 14:46:11, Al Viro wrote:
> > On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> > > 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.
> > 
> > So no rmmod for you until an inode of character device node you had
> > closed a while ago finally gets evicted from icache?  Or am I misreading
> > you?
>   Yes, this is a consequence of the change. I should have noted in the
> changelog I guess.

That consequence looks broken, IMO.

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Jan Kara]

On Tue 04-11-14 15:40:30, Al Viro wrote:
> On Tue, Nov 04, 2014 at 04:37:52PM +0100, Jan Kara wrote:
> > On Tue 04-11-14 14:46:11, Al Viro wrote:
> > > On Wed, Oct 22, 2014 at 10:14:11PM +0200, Jan Kara wrote:
> > > > 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.
> > > 
> > > So no rmmod for you until an inode of character device node you had
> > > closed a while ago finally gets evicted from icache?  Or am I misreading
> > > you?
> >   Yes, this is a consequence of the change. I should have noted in the
> > changelog I guess.
> 
> That consequence looks broken, IMO.
  Hum, it already behaves for block devices that way (and noone complained
- but admittedly block devices tied to strange modules are less common than
character devices). Also rmmod isn't that common IMO, but I see your point
that it's unintuitive behavior.

Alternatively we could clear i_cdev when the inode isn't open anymore (and
thus doesn't hold any references to cdev). The downside is we have to
re-lookup the character device on the first inode open. Would such a
solution look acceptable to you?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Al Viro]

On Tue, Nov 04, 2014 at 04:55:43PM +0100, Jan Kara wrote:
> > That consequence looks broken, IMO.
>   Hum, it already behaves for block devices that way (and noone complained
> - but admittedly block devices tied to strange modules are less common than
> character devices). Also rmmod isn't that common IMO, but I see your point
> that it's unintuitive behavior.

Umm...  Since when?  The last time I looked, bdev module refcounts were
tied to struct gendisk, not to struct block_device.

Re: [PATCH 2/4] chardev: Increment cdev reference count when i_cdev references it

[Jan Kara]

On Tue 04-11-14 16:03:03, Al Viro wrote:
> On Tue, Nov 04, 2014 at 04:55:43PM +0100, Jan Kara wrote:
> > > That consequence looks broken, IMO.
> >   Hum, it already behaves for block devices that way (and noone complained
> > - but admittedly block devices tied to strange modules are less common than
> > character devices). Also rmmod isn't that common IMO, but I see your point
> > that it's unintuitive behavior.
> 
> Umm...  Since when?  The last time I looked, bdev module refcounts were
> tied to struct gendisk, not to struct block_device.
  Ah, right you are. So how about clearing i_cdev and dropping reference to
it when inode is not open anymore? Doing first open after a cleanup would
be more expensive but it doesn't seem too much. For block devices first
open of it is also more expensive - although there bd_openers is per block
device while for character devices we would have the openers per device
inode.  I don't think that's a real difference in practice unless you run
some container service. If you think it would help, we could add some
intermediate structure for character devices with similar rules like struct
block_device for block devices (and character device references would be
handled in the same way as we handle gendisk references for block devices).
However I don't think it's really worth the bother...


								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 3/4] chardev: Don't use i_devices inode field

[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

Re: [PATCH 3/4] chardev: Don't use i_devices inode field

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] fs: Remove i_devices list head

[Jan Kara]

Noone uses i_devices anymore. Remove it (thus saving two pointers in
every inode).

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

Re: [PATCH 4/4] fs: Remove i_devices list head

[Christoph Hellwig]

On Wed, Oct 22, 2014 at 10:14:13PM +0200, Jan Kara wrote:
> Noone uses i_devices anymore. Remove it (thus saving two pointers in
> every inode).

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/4 v2] fs: Remove i_devices from struct inode

[Jan Kara]

On Wed 22-10-14 22:14:09, 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.
> 
> Since v1 I have split the patches and properly handled character devices (I
> broke them last time as Christoph pointed out).
  Any opinion on this guys?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 0/4 v3] fs: Remove i_devices from struct inode

[Jan Kara]

  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).

								Honza

[PATCH 1/4] blockdev: Don't use i_devices inode field

[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