Kevin P. Fleming writes:
> > What I don't want to see:
>
> I'll respond to these points in the context of the patches I've already
> submitted.
>
> > - ide-floppy hacked up to explicitely create devfs entries. As far as
> > I'm concerned, that driver is correct
>
> I did not make any changes of this type; I only added two
> devfs-related calls in ide-floppy, to unregister any existing
> partition entries if the media is ejected or reformatted. IMHO this
> is the proper way to go, rather than have the entries stay around
> until the media is somehow revalidated. I also added explicit
> removal of any existing partition entries in ide.c itself, whenever
> it is about to call the 'revalidate' operation for one of its
> subdrivers. Without this change, old partition entries will never be
> removed (think of inserting a disk with only part2 on it, where the
> previous disk had part3 and part4; without explicit removal, the
> directory will show part2, part3 and part4). This same change was
> made in other block device drivers, but not all of them.
IIRC, Paul made a patch that did this. I was just stating my position
after reading through the various threads.
However, as I recall, at least one version of your patch completely
unregistered a disc and then re-registered it. That's definately the
wrong approach, as I've said before, because it opens up a race. The
same disc could end up with a different number in /dev/discs.
Apologies if your patch doesn't do this now: I haven't looked
closely. That's partly because it's long, and I believe a short patch
should be fine.
> > - structural changes to devfs code in fs/partitions/check.c
>
> This seems to be a non-sensical restriction; why should the code in
> this file be above modification? If the proper way to solve the
> problem(s) requires finer granularity of registering/deregistering
> disc and partition entries, the only way to do this is modify the
> existing code or create entirely new functions (with duplicate
> functionality).
Because I believe that most (maybe even all) of the problems are
solvable with a few minor tweaks. So I'd prefer to start with a few
tweaks and see if that fixes it.
> > - a patch that's more than 20 lines long.
>
> I can't see where this is even a relevant point. Are you saying
> there is no valid solution that requires more than 20 lines of patch
> code?
I'm not sure, but it's what my gut feeling is.
> How can you know that?
Instinct :-)
> > The solution, as I see it, is to force grok_partitions() to call
> > devfs_register_disc() when size==0 and the GENHD_FL_REMOVABLE flag is
> > set. This will cause the "disc" entry in the
> > /dev/ide/host*/bus*/lun*/target* directory to be created, and it will
> > tag that directory for media revalidation.
> >
> > If you read that directory, or attempt a lookup of some leaf node, the
> > media will be revalidated. If there's new media in the drive, the
> > normal partition handling code should be invoked, and thus the
> > appropriate leaf nodes will be created. Take the media out, and the
> > next readdir or lookup will remove partition entries (assuming the
> > ide-floppy driver doesn't set the size back to 0). Magic. Just the way
> > god^H^H^HI intended.
>
> Nope, grok_partitions does not cause existing entries to be removed,
> unless the "size" is left at its original value, in which case your
> system log will be filled with "unable to read partition table"
> errors and errors from the ide layer trying to read non-existent
> media.
So, let me get this right (I haven't looked closely at ide-floppy, so
bear with me): when there is no media present, the driver sets the
size to zero at revalidate time, even if previously the size was
non-zero?
> > So, I've appended a patch which does this. Please try it out and let
> > me know what you think. And hey! The patch is a mere 11 lines long.
>
> As noted in my other email (which I accidentally sent only to
> Richard), this does not compile, and with minor changes it compiles
> but Oopses my machine. It also addresses only a small part of the
> problem.
It was like a Linus "proof of concept patch": throw it together and
let someone else test it. There were two stupid bugs. I've fixed
those, after spending 2 minutes reviewing the code instead of 1
minute :-/
So please try the appended patch again. It doesn't claim to solve the
unregistering problem, but that's OK. I want to take this a step at a
time. A bit more tweaking should (I hope) take care of the remaining
problems.
> I really can't believe that it is so hard to get such a simple set
> of patches incorporated that fix a multitude of problems. Once
> again, I will list the existing problems as I see them; anyone else,
> feel free to add items or tell me I'm hallucinating :-)
Your patch was several pages long. That didn't give me the feeling it
was "simple".
> - without media in the drive at driver load time (either kernel
> startup or module load), no entries are created, no access is
> possible to the drive
My patch should fix that.
> - when media is ejected, invalid partition entries remain in the
> directory
Future work.
> - when media is repartitioned, invalid partition entries remain in
> the directory
Future work, same solution as above.
> - when media is reformatted, invalid partition entries remain in the
> directory
Future work, same solution as above.
> - when ide-floppy (or ide-hd) modules are unloaded, the
> disc/partition entries (and the directory) are not removed
Future work.
Regards,
Richard....
Permanent: rgooch@xxxxxxxxxxxxx
Current: rgooch@xxxxxxxxxxxxxxx
--- check.c~ Thu Oct 11 18:25:10 2001
+++ check.c Wed Dec 5 23:14:42 2001
@@ -383,6 +383,8 @@
dev->part[first_minor].nr_sects = size;
/* No such device or no minors to use for partitions */
+ if ( !size && dev->flags && (dev->flags[devnum] & GENHD_FL_REMOVABLE) )
+ devfs_register_disc (dev, first_minor);
if (!size || minors == 1)
return;
|