Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Tejun Heo
On Thu, Nov 20, 2014 at 04:14:29PM +0200, Boaz Harrosh wrote: > On 11/20/2014 03:11 PM, Tejun Heo wrote: > > Hello, Boaz. > > > <> > > W/ preloading, one way to do it is, > > > > if (preload()) > > handle -ENOMEM; > > lock; > > error = insert(); > > if (error) > >

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Boaz Harrosh
On 11/20/2014 03:11 PM, Tejun Heo wrote: > Hello, Boaz. > <> > W/ preloading, one way to do it is, > > if (preload()) > handle -ENOMEM; > lock; > error = insert(); > if (error) > handle error which can't be -ENOMEM; > unlock; >

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Tejun Heo
On Thu, Nov 20, 2014 at 08:11:18AM -0500, Tejun Heo wrote: ... > creating another one-off behavior. If we demote this to something > which is shared only between block and char devs, we can go simpler > and less versatile. Where should it go tho? So, one option is implementing something minimal

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Tejun Heo
Hello, Boaz. On Thu, Nov 20, 2014 at 02:36:53PM +0200, Boaz Harrosh wrote: > Actually with my proposed change to "the code you submitted here" > there are *less* unnecessary allocations. In both our imp we have a > waste when element already exist in the tree, and your imp already >

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Boaz Harrosh
On 11/20/2014 01:50 PM, Tejun Heo wrote: > Hello, Boaz. > > On Thu, Nov 20, 2014 at 12:42:53PM +0200, Boaz Harrosh wrote: >> if I understand correctly the motivation here is that the allocation >> of the internal element is done GFP_KERNEL at this call >> >> Then the add() below can be under the

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Tejun Heo
Hello, Boaz. On Thu, Nov 20, 2014 at 12:42:53PM +0200, Boaz Harrosh wrote: > if I understand correctly the motivation here is that the allocation > of the internal element is done GFP_KERNEL at this call > > Then the add() below can be under the spin_lock. > > So why don't you just return an

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-20 Thread Boaz Harrosh
On 11/14/2014 12:11 AM, Tejun Heo wrote: > inode->i_devices is a list_head used to link device inodes to the > corresponding block_device or cdev. This patch makes block_device and > cdev usfe ptrset to keep track of the inodes instead of linking > inode->i_devices allowing removal of the field

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Boaz Harrosh
On 11/14/2014 12:11 AM, Tejun Heo wrote: inode-i_devices is a list_head used to link device inodes to the corresponding block_device or cdev. This patch makes block_device and cdev usfe ptrset to keep track of the inodes instead of linking inode-i_devices allowing removal of the field and

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Tejun Heo
Hello, Boaz. On Thu, Nov 20, 2014 at 12:42:53PM +0200, Boaz Harrosh wrote: if I understand correctly the motivation here is that the allocation of the internal element is done GFP_KERNEL at this call Then the add() below can be under the spin_lock. So why don't you just return an element

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Boaz Harrosh
On 11/20/2014 01:50 PM, Tejun Heo wrote: Hello, Boaz. On Thu, Nov 20, 2014 at 12:42:53PM +0200, Boaz Harrosh wrote: if I understand correctly the motivation here is that the allocation of the internal element is done GFP_KERNEL at this call Then the add() below can be under the spin_lock.

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Tejun Heo
Hello, Boaz. On Thu, Nov 20, 2014 at 02:36:53PM +0200, Boaz Harrosh wrote: Actually with my proposed change to the code you submitted here there are *less* unnecessary allocations. In both our imp we have a waste when element already exist in the tree, and your imp already waists an

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Tejun Heo
On Thu, Nov 20, 2014 at 08:11:18AM -0500, Tejun Heo wrote: ... creating another one-off behavior. If we demote this to something which is shared only between block and char devs, we can go simpler and less versatile. Where should it go tho? So, one option is implementing something minimal in

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Boaz Harrosh
On 11/20/2014 03:11 PM, Tejun Heo wrote: Hello, Boaz. W/ preloading, one way to do it is, if (preload()) handle -ENOMEM; lock; error = insert(); if (error) handle error which can't be -ENOMEM; unlock; preload_end();

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-20 Thread Tejun Heo
On Thu, Nov 20, 2014 at 04:14:29PM +0200, Boaz Harrosh wrote: On 11/20/2014 03:11 PM, Tejun Heo wrote: Hello, Boaz. W/ preloading, one way to do it is, if (preload()) handle -ENOMEM; lock; error = insert(); if (error) handle error

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-18 Thread Tejun Heo
Hey, Boaz. On Tue, Nov 18, 2014 at 02:10:50PM +0200, Boaz Harrosh wrote: > Right? well if so than those special inodes do > not hold any data and never do IO of any kind > so all the inode members that are needed for IO > are candidates. It may not carry dirty pages but the inode itself still

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-18 Thread Boaz Harrosh
On 11/14/2014 12:11 AM, Tejun Heo wrote: <> > @@ -426,7 +427,7 @@ struct block_device { > struct inode * bd_inode; /* will die */ > struct super_block *bd_super; > struct mutexbd_mutex; /* open/close mutex */ > - struct list_head

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-18 Thread Boaz Harrosh
On 11/14/2014 12:11 AM, Tejun Heo wrote: @@ -426,7 +427,7 @@ struct block_device { struct inode * bd_inode; /* will die */ struct super_block *bd_super; struct mutexbd_mutex; /* open/close mutex */ - struct list_head

Re: [PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-18 Thread Tejun Heo
Hey, Boaz. On Tue, Nov 18, 2014 at 02:10:50PM +0200, Boaz Harrosh wrote: Right? well if so than those special inodes do not hold any data and never do IO of any kind so all the inode members that are needed for IO are candidates. It may not carry dirty pages but the inode itself still can

[PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices

2014-11-13 Thread Tejun Heo
inode->i_devices is a list_head used to link device inodes to the corresponding block_device or cdev. This patch makes block_device and cdev usfe ptrset to keep track of the inodes instead of linking inode->i_devices allowing removal of the field and thus reduction of struct inode by two

[PATCH vfs 2/2] {block|char}_dev: remove inode-i_devices

2014-11-13 Thread Tejun Heo
inode-i_devices is a list_head used to link device inodes to the corresponding block_device or cdev. This patch makes block_device and cdev usfe ptrset to keep track of the inodes instead of linking inode-i_devices allowing removal of the field and thus reduction of struct inode by two pointers.