[Ocfs2-tools-devel] Re: [PATCH] Fix qdiskd device scanning logic

[Fabio Massimo Di Nitto]

Joel Becker wrote:
> On Wed, Nov 28, 2007 at 10:07:21PM +0100, Fabio Massimo Di Nitto wrote:
>> the original idea of scanning /dev was not too bad but after porting it to
>> ocfs2-tools and brainstorming again with the guys, we found a few things that
>> could be done better.
>>
>> Let's change a direct scan of /dev into:
>>
>> - scan /sys/block first for well.. block devices.
>> - sysfs exports enough info for us to filter out removable devices
>>   like cdroms that we don't care to even check and to gather
>>   maj/min of each device.
> 
> 	We need to check /sys/block/<xxx>/device/media too.  A removable
> cdrom should be skipped.  A removable USB/FW drive?  Perhaps not.

Ok, we will need to assume that if we can't find device/media or device/type, it
is a good one. Same way as removable work as it is not present in all block
devices (see ram/loop for example).

> 
>> - scan /dev only to match maj/min and gather the device name.
> 
> 	You should do this up front to create a mapping table.
> Currently, it will re-recurse /dev for each device found in /sys.  While
> the data will be cached in the kernel, it's much better to have a lookup
> table in your code.  You also save all checking on non-block devices in
> additional paths.  If a device appears after your table was built, you
> just rescan.

Ok, i understand why you are asking this and it is perfectly doable. Generally I
think that it is a bit over engineered for what we need to achieve and it adds
some extra complexity to the code, such as caching of /dev (that is racy),
invalidate the cache and rescan.
I will poke around and see how complex this become using your approach and we
will see how it goes.

> 	Also, it needs better handling of "I want a list of all
> devices".  It currently only does "print out in a static way".
> ocfs2-tools wants a list of all devices.  Sometimes we want "all
> devices", sometimes "all ocfs2 devices", sometimes "all non-ocfs2
> devices".

Right, but this is something you can abstract from the scanning function itself
into a higher level. To know what is an ocfs2 device, you still need to scan all
of them to build a list in the first place.

So I think the scanning function (also given your points below) should just
really scan for valid block devices (all of them) and stop there. Whatever you
do with the result change from apps to apps and IMHO should be done one level up
in the stack where you can do caching of the results, invalidate the cache and
ask for a rescan (etc..).

> 	Wouldn't we be better off with a scan API that you call without
> the path prefix (it knows how to find /sys/block and /dev), but with a
> callback function?  That function can take the path in /dev,
> is_removable, the media type, and a void*.  It can then decide on
> whether the block device is valid or not, adding to the void* as it
> pleases.  The return code from the callback would specify "<0 == error,
> 0 = continue, 1 = quit".  Then a simple "lookup label" find would use
> the callback to check the label against the void*.  When it matches, it
> fills in the void* with the device name and returns 1.  That sort of
> thing.

We can just use the callback in scandir to achieve the same. I didn't want to
use it before because it makes the code a bit less readable IMHO but i was
discussing the exact same issue with Lon :)

Fabio

-- 
I'm going to make him an offer he can't refuse.