Re: [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash

On Wed, Feb 13 2008 at 19:03 +0200, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
On Wed, 2008-02-13 at 18:50 +0200, Boaz Harrosh wrote:
On Wed, Feb 13 2008 at 18:45 +0200, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
On Wed, 2008-02-13 at 18:33 +0200, Boaz Harrosh wrote:
On Wed, Feb 13 2008 at 17:54 +0200, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
On Wed, Feb 13 2008 at 17:44 +0200, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
On Tue, 2008-02-12 at 19:40 +0200, Boaz Harrosh wrote:
- gdth_flush(ha);
-
This piece doesn't look right. gdth_flush() forces the internal cache
to disk backing. If you remove it, you're taking the chance that the
machine will be powered off without a writeback which can cause data
corruption.

James

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts
this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier);
to a gdth_halt() function, which would then redo half of what gdth_exit
does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called
on an halt or reboot? If so then there is no need for duplications and
the gdth_halt() should go.
No. The __exit section is actually discardable if you promise never to
remove the module.

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine?
module or built in? unload or shutdown?

It needs to register a reboot notifier, which gdth does.

However, the notifier is only called on reboot, so it also needs to
clean up correctly on module exit as well.

The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE

Why would we think that the controller does not support this command
is it not in the mandatory section of the standard?

command. That's done by a shutdown notifier from sd, so the correct
thing would always get done; however it does mean the driver has to be
in a condition to process the last sync cache command.

Why would it not be ready? what do other drivers do?
The drivers is ready until the very last module's .exit. Is that good
enough?


For the quick fix, just keep the current infrastructure and put back the
gdth_flush() command where it can be effective.


Just did. But if needed I would prefer to emulate the SCSI SYNCHRONIZE CACHE
command and not that boot notifier thing. Please advise.

James



Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash
    ... clean up correctly on module exit as well. ... The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE ... Why would we think that the controller does not support this command ... which means the SCSI comands are processed in the driver. ...
    (Linux-Kernel)
  • [2.6 patch] the scheduled eepro100 removal
    ... This patch contains the scheduled removal of the eepro100 driver. ... First set is undocumented and spelled per Intel recommendations. ... -static int use_io; ... Each Tx command block ...
    (Linux-Kernel)
  • [2.6 patch] the overdue eepro100 removal
    ... This patch contains the overdue removal of the eepro100 driver. ... First set is undocumented and spelled per Intel recommendations. ... -static int use_io; ... Each Tx command block ...
    (Linux-Kernel)
  • [2.6.24 patch] the planned eepro100 removal
    ... This patch contains the planned removal of the eepro100 driver. ... First set is undocumented and spelled per Intel recommendations. ... -static int use_io; ... Each Tx command block ...
    (Linux-Kernel)
  • [2.6.24 patch] the planned eepro100 removal
    ... This patch contains the planned removal of the eepro100 driver. ... First set is undocumented and spelled per Intel recommendations. ... -static int use_io; ... Each Tx command block ...
    (Linux-Kernel)