[Cluster-devel] [PATCH] Fix qdiskd device scanning logic

[Cluster-devel] [PATCH] Fix qdiskd device scanning logic

[Fabio Massimo Di Nitto]

Hi Lon,

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.
- scan /dev only to match maj/min and gather the device name.
- never dig or attempt to check hidden files.

Please review or apply.

Joel: once applied on top, this is basically the same code that would land
in ocfs2-tools.

Thanks
Fabio

-- 
I'm going to make him an offer he can't refuse.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_proc_part_scan.diff
Type: application/pgp-encrypted
Size: 5980 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20071128/a166c95f/attachment.pgp>

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

[Joel Becker]

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.

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

> - never dig or attempt to check hidden files.

Joel

-- 

"When ideas fail, words come in very handy." 
         - Goethe

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

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