Re: [PATCH -mm] swsusp: Freeze filesystems during suspend

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi,

On Wednesday, 1 November 2006 12:47, Pavel Machek wrote:
> Hi!
> 
> > Freeze all filesystems during the suspend by calling freeze_bdev() for each of
> > them and thaw them during the resume using thaw_bdev().
> > 
> > This is needed by swsusp, because some filesystems (eg. XFS) use work queues
> > and worker_threads run with PF_NOFREEZE set, so they can cause some writes
> > to be performed after the suspend image has been created which may corrupt
> > the filesystem.  The additional benefit of it is that if the resume fails, the
> > filesystems will be in a consistent state and there won't be any journal replays
> > needed.
> > 
> > The freezing of filesystems is carried out when processes are being frozen, so
> > on the majority of architectures it also will happen during a
> > suspend to RAM.
> 
> 
> > @@ -119,7 +120,7 @@ int freeze_processes(void)
> >  		read_unlock(&tasklist_lock);
> >  		todo += nr_user;
> >  		if (!user_frozen && !nr_user) {
> > -			sys_sync();
> > +			freeze_filesystems();
> >  			start_time = jiffies;
> >  		}
> >  		user_frozen = !nr_user;
> 
> 
> Do all filesystems implement freeze?

I think so.

> If not, we may want to keep that sync...

But the sync() won't hurt anyway I think.

> 
> 
> > @@ -156,28 +157,43 @@ int freeze_processes(void)
> >  void thaw_some_processes(int all)
> >  {
> >  	struct task_struct *g, *p;
> > -	int pass = 0; /* Pass 0 = Kernel space, 1 = Userspace */
> >  
> >  	printk("Restarting tasks... ");
> >  	read_lock(&tasklist_lock);
> > -	do {
> > -		do_each_thread(g, p) {
> > -			/*
> > -			 * is_user = 0 if kernel thread or borrowed mm,
> > -			 * 1 otherwise.
> > -			 */
> > -			int is_user = !!(p->mm && !(p->flags & PF_BORROWED_MM));
> > -			if (!freezeable(p) || (is_user != pass))
> > -				continue;
> > -			if (!thaw_process(p))
> > -				printk(KERN_INFO
> > -					"Strange, %s not stopped\n", p->comm);
> > -		} while_each_thread(g, p);
> >  
> > -		pass++;
> > -	} while (pass < 2 && all);
> > +	do_each_thread(g, p) {
> > +		if (!freezeable(p))
> > +			continue;
> > +
> > +		/* Don't thaw userland processes, for now */
> > +		if (p->mm && !(p->flags & PF_BORROWED_MM))
> > +			continue;
> > +
> > +		if (!thaw_process(p))
> > +			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> > +	} while_each_thread(g, p);
> > +
> > +	read_unlock(&tasklist_lock);
> > +	if (!all)
> > +		goto Exit;
> > +
> > +	thaw_filesystems();
> > +	read_lock(&tasklist_lock);
> > +
> > +	do_each_thread(g, p) {
> > +		if (!freezeable(p))
> > +			continue;
> > +
> > +		/* Kernel threads should have been thawed already */
> > +		if (!p->mm || (p->flags & PF_BORROWED_MM))
> > +			continue;
> > +
> > +		if (!thaw_process(p))
> > +			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> > +	} while_each_thread(g, p);
> >  
> >  	read_unlock(&tasklist_lock);
> > +Exit:
> >  	schedule();
> >  	printk("done.\n");
> 
> 
> Could we do without the code duplication?

Okay, I'll move the loop(s) into a separate function.

> > +/**
> > + * freeze_filesystems - lock all filesystems and force them into a consistent
> > + * state
> > + */
> > +void freeze_filesystems(void)
> > +{
> > +	struct super_block *sb;
> > +
> > +	lockdep_off();
> 
> You should not just turn off lockdep because you don't like its
> output.
> 
> Perhaps tasklist_lock does not nest with whatever freeze_bdev needs?

The locks taken in one call to freeze_bdev() nest with analogous locks
taken in the other calls to freeze_bdev().  Actually we take several locks of
the same (I think) class in a row and keep them all until thaw_filesystems()
is called, which is quite unusual.  I don't think there's any way in which we
can convince lockdep that it's all okay other than switching it off.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller