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

Rafael J. Wysocki
2006-11-01T12:07:55Z

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