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

[Boaz Harrosh <boaz@plexistor.com>]

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.
>>
>> So why don't you just return an element here to caller and give it to
>> add below. No Preemption-disable, no percpu variable, simple. Like:
> 
> Hmmm... mostly because preloading is more convenient and but also
> because it provides better separation from internal implementation
> details.  e.g. This may be implemented using a different data
> structure (e.g. bonsai tree) 

Two things:
1. This can be easily hidden by returning a none defined type
   which internals are only known to the implementation so
   even if you change the implementation users need not change.
   Like just a (void *) but it is better to be type-full
   like:
   struct pset_new;
   struct pset_new *pset_preload()

   And the internals of struct pset_new is only known to implementation
2. Obfuscation: Currently this is the proposed implementation if jugging
   by the previous imp it is good for 15 years.
   Else since when are we afraid to change two users?

> which may require differing number of new
> elements even on success.  With the scheme you're describing, the
> operation would be constantly allocating and freeing memory areas
> (which may be multiple) unnecessarily.

  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 allocation in every pset_preload()

  And again you are talking about a future undefined "what if", let
  us look at the very sound imp you proposed here with rbtree and
  do the best we can with that one.

> 
> One thing which is debatable is how to handle preloading errors.  We
> can have the preload fail and then assume that the later insertion
> won't fail with -ENOMEM (often through BUG/WARN_ON()); however, it
> often, but not always, is that those insertion operations may fail
> with different error codes too and requires error handling anyway, 

  Again Theoretical. With your current code the only failing I see
  from add() is allocation, so with my imp it will never fail. One
  thing good with embedded list_heads is the void returning add.
  And so with my proposition: void returning add.

  When some new imp will be needed we can cross the bridge then.

  For now you have convinced me that an rbtree is good, and I want to
  solve the preemption-disable, none interrupt ugliness, per-cpu vars,
  as well as the double alloc in the normal lots-of-free-memory case.

> so
> overall it seems better to defer the allocation error to the actual
> insertion point.  

That one I did not understand.

> It also makes conceptual sense.  The preloading
> simply upgrades the allocation mask the insertion operation uses.
> 

How is "upgrades", better then "always have the best mask"

> Thanks.
> 

Thanks
Boaz