LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Boaz Harrosh <bharrosh@panasas.com>
Cc: "Sven Köhler" <skoehler@upb.de>,
	"Christoph Hellwig" <hch@infradead.org>,
	"Jeff Garzik" <jeff@garzik.org>,
	linux-scsi <linux-scsi@vger.kernel.org>,
	linux-kernel@vger.kernel.org,
	"Joerg Dorchain" <joerg@dorchain.net>,
	"Jon Chelton" <jchelton@ffpglobal.com>,
	"Stefan Priebe - allied internet ag"
	<s.priebe@allied-internet.ag>
Subject: Re: [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash
Date: Wed, 13 Feb 2008 11:36:38 -0600	[thread overview]
Message-ID: <1202924198.3109.52.camel@localhost.localdomain> (raw)
In-Reply-To: <47B324E7.9040502@panasas.com>

On Wed, 2008-02-13 at 19:12 +0200, Boaz Harrosh wrote:
> On Wed, Feb 13 2008 at 19:03 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@panasas.com> wrote:
> >>>>> On Wed, Feb 13 2008 at 17:44 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> 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
> > 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.
> > 
> > For the quick fix, just keep the current infrastructure and put back the
> > gdth_flush() command where it can be effective.
> > 
> > James
> > 
> > 
> Totally untested.
> 
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Subject: [PATCH] gdth: bugfix for the at-exit problems
> 
> gdth_exit would first remove all cards then stop the timer
> and would not sync with the timer function. This caused a crash
> in gdth_timer() when module was unloaded.
> So del_timer_sync the timer before we delete the cards.
> 
> also the reboot notifier function would crash. So unify
> the exit and halt functions with a gdth_shutdown() that's
> called by both.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/gdth.c |   99 ++++++++++++++++++++------------------------------
>  1 files changed, 40 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 8eb78be..7bb9b45 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -183,7 +183,6 @@ static int gdth_ioctl(struct inode *inode, struct file *filep,
>                        unsigned int cmd, unsigned long arg);
>  
>  static void gdth_flush(gdth_ha_str *ha);
> -static int gdth_halt(struct notifier_block *nb, ulong event, void *buf);
>  static int gdth_queuecommand(Scsi_Cmnd *scp,void (*done)(Scsi_Cmnd *));
>  static int __gdth_queuecommand(gdth_ha_str *ha, struct scsi_cmnd *scp,
>  				struct gdth_cmndinfo *cmndinfo);
> @@ -418,12 +417,6 @@ static inline void gdth_set_sglist(struct scsi_cmnd *cmd,
>  #include "gdth_proc.h"
>  #include "gdth_proc.c"
>  
> -/* notifier block to get a notify on system shutdown/halt/reboot */
> -static struct notifier_block gdth_notifier = {
> -    gdth_halt, NULL, 0
> -};
> -static int notifier_disabled = 0;
> -
>  static gdth_ha_str *gdth_find_ha(int hanum)
>  {
>  	gdth_ha_str *ha;
> @@ -3793,6 +3786,8 @@ static void gdth_timeout(ulong data)
>      gdth_ha_str *ha;
>      ulong flags;
>  
> +    BUG_ON(list_empty(&gdth_instances));
> +
>      ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
>      spin_lock_irqsave(&ha->smp_lock, flags);
>  
> @@ -4668,45 +4663,6 @@ static void gdth_flush(gdth_ha_str *ha)
>      }
>  }
>  
> -/* shutdown routine */
> -static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
> -{
> -    gdth_ha_str *ha;
> -#ifndef __alpha__
> -    gdth_cmd_str    gdtcmd;
> -    char            cmnd[MAX_COMMAND_SIZE];   
> -#endif
> -
> -    if (notifier_disabled)
> -        return NOTIFY_OK;
> -
> -    TRACE2(("gdth_halt() event %d\n",(int)event));
> -    if (event != SYS_RESTART && event != SYS_HALT && event != SYS_POWER_OFF)
> -        return NOTIFY_DONE;
> -
> -    notifier_disabled = 1;
> -    printk("GDT-HA: Flushing all host drives .. ");
> -    list_for_each_entry(ha, &gdth_instances, list) {
> -        gdth_flush(ha);
> -
> -#ifndef __alpha__
> -        /* controller reset */
> -        memset(cmnd, 0xff, MAX_COMMAND_SIZE);
> -        gdtcmd.BoardNode = LOCALBOARD;
> -        gdtcmd.Service = CACHESERVICE;
> -        gdtcmd.OpCode = GDT_RESET;
> -        TRACE2(("gdth_halt(): reset controller %d\n", ha->hanum));
> -        gdth_execute(ha->shost, &gdtcmd, cmnd, 10, NULL);
> -#endif
> -    }
> -    printk("Done.\n");
> -
> -#ifdef GDTH_STATISTICS
> -    del_timer(&gdth_timer);
> -#endif
> -    return NOTIFY_OK;
> -}
> -
>  /* configure lun */
>  static int gdth_slave_configure(struct scsi_device *sdev)
>  {
> @@ -5141,13 +5097,13 @@ static void gdth_remove_one(gdth_ha_str *ha)
>  
>  	scsi_remove_host(shp);
>  
> +	gdth_flush(ha);
> +
>  	if (ha->sdev) {
>  		scsi_free_host_dev(ha->sdev);
>  		ha->sdev = NULL;
>  	}
>  
> -	gdth_flush(ha);
> -
>  	if (shp->irq)
>  		free_irq(shp->irq,ha);
>  
> @@ -5173,6 +5129,40 @@ static void gdth_remove_one(gdth_ha_str *ha)
>  	scsi_host_put(shp);
>  }
>  
> +static void gdth_shutdown(void);
> +static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
> +{
> +	TRACE2(("gdth_halt() event %d\n",(int)event));
> +	if (event != SYS_RESTART && event != SYS_HALT && event != SYS_POWER_OFF)
> +		return NOTIFY_DONE;
> +
> +	gdth_shutdown();
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block gdth_notifier = {
> +    gdth_halt, NULL, 0
> +};
> +
> +bool gdth_shutdown_done;

Static police alert!  Just make it static and move it into
gdth_shutdown()

> +static void gdth_shutdown()
> +{
> +	gdth_ha_str *ha;
> +	if (gdth_shutdown_done)
> +		return;
> +
> +	gdth_shutdown_done = true;
> +	unregister_chrdev(major,"gdth");
> +	unregister_reboot_notifier(&gdth_notifier);

I'm not sure you can do this, aren't reboot notifiers called with the
rwsem held?  In which case the unregister which also takes the rwsem
will hang the system.

James



  reply	other threads:[~2008-02-13 17:37 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-30 19:47 [BUG?] GDTH driver not working after upgrade to 2.6.24 Sven Köhler
2008-01-31 10:08 ` Boaz Harrosh
2008-01-31 11:07   ` Sven Köhler
2008-01-31 12:35     ` Boaz Harrosh
2008-01-31 16:39       ` Jan Engelhardt
2008-01-31 16:52         ` Boaz Harrosh
2008-02-12 17:30   ` [BUGFIXES 0/2] gdth: fix 2.6.24 driver breakage Boaz Harrosh
2008-02-12 17:35     ` [BUGFIX 1/2] gdth: scan for scsi devices Boaz Harrosh
2008-02-12 18:05       ` Christoph Hellwig
2008-02-12 17:40     ` [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash Boaz Harrosh
2008-02-13  7:06       ` Stefan Priebe - allied internet ag
2008-02-13  9:03         ` Boaz Harrosh
2008-02-13 19:38           ` Jan Engelhardt
2008-02-14 15:58             ` Boaz Harrosh
2008-02-13 10:48       ` Boaz Harrosh
2008-02-13 15:44       ` James Bottomley
2008-02-13 15:54         ` Boaz Harrosh
2008-02-13 16:33           ` Boaz Harrosh
2008-02-13 16:35             ` [PATCH ver2] gdth: bugfix for the at-exit problems Boaz Harrosh
2008-02-13 16:45             ` [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash James Bottomley
2008-02-13 16:50               ` Boaz Harrosh
2008-02-13 17:03                 ` James Bottomley
2008-02-13 17:12                   ` Boaz Harrosh
2008-02-13 17:36                     ` James Bottomley [this message]
2008-02-14 10:49                       ` Boaz Harrosh
2008-02-14 11:58                         ` [PATCH] gdth: bugfix for the at-exit problems Boaz Harrosh
2008-02-14 16:10                           ` James Bottomley
2008-02-14 16:18                             ` Boaz Harrosh
2008-02-13 17:18                   ` [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash Boaz Harrosh
2008-02-13 17:33                     ` James Bottomley
2008-02-14  6:51                   ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1202924198.3109.52.camel@localhost.localdomain \
    --to=james.bottomley@hansenpartnership.com \
    --cc=bharrosh@panasas.com \
    --cc=hch@infradead.org \
    --cc=jchelton@ffpglobal.com \
    --cc=jeff@garzik.org \
    --cc=joerg@dorchain.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=s.priebe@allied-internet.ag \
    --cc=skoehler@upb.de \
    --subject='Re: [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).