[PATCH] Fix hanging close for /dev/mtd

[PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

Figured I should send this on its own.

Stracing a process using /dev/mtd shows that
it sometimes hangs for a long time when pdflush is erasing
sectors. /dev/mtd* close method calls the mtd->sync method
which can hang for a long time(minutes).

Futher, the sync method "syncs" all flash chips. It should
only sync the chips that a /dev/mtd* spans.
Also, the mtdchar/mtdblock drivers does not need to
call sync for readonly mappings.

>From c39777d86aa1bef8e053c3f78a5df0981e3a45a9 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Tue, 12 Jun 2007 19:25:56 +0200
Subject: [PATCH] Fix hanging close for /dev/mtd
When pdflush is erasing lots of sectors, drivers calling
mtd->sync will hang until all blocks are erased.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 fs/jffs2/erase.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index 24a48e9..33e5f13 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -150,7 +150,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 		}
 
 		/* Be nice */
-		cond_resched();
+		yield();
 		down(&c->erase_free_sem);
 		spin_lock(&c->erase_completion_lock);
 	}
-- 
1.5.2.1

Re: [PATCH] Fix hanging close for /dev/mtd

[Josh Boyer]

On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> Figured I should send this on its own.
> 
> Stracing a process using /dev/mtd shows that
> it sometimes hangs for a long time when pdflush is erasing
> sectors. /dev/mtd* close method calls the mtd->sync method
> which can hang for a long time(minutes).
> 
> Futher, the sync method "syncs" all flash chips. It should
> only sync the chips that a /dev/mtd* spans.
> Also, the mtdchar/mtdblock drivers does not need to
> call sync for readonly mappings.

So were you going to submit a separate patch for those issues?

josh

Re: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > Figured I should send this on its own.
> > 
> > Stracing a process using /dev/mtd shows that
> > it sometimes hangs for a long time when pdflush is erasing
> > sectors. /dev/mtd* close method calls the mtd->sync method
> > which can hang for a long time(minutes).
> > 
> > Futher, the sync method "syncs" all flash chips. It should
> > only sync the chips that a /dev/mtd* spans.
> > Also, the mtdchar/mtdblock drivers does not need to
> > call sync for readonly mappings.
> 
> So were you going to submit a separate patch for those issues?
> 
> josh

Wow, someone is reading my mails :)

Just did for mtdchar, the others are a bit more complex
so I won't bother with them.

 Jocke

Re: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

On Wed, 2007-06-13 at 13:48 +0200, Joakim Tjernlund wrote:
> On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> > On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > > Figured I should send this on its own.
> > > 
> > > Stracing a process using /dev/mtd shows that
> > > it sometimes hangs for a long time when pdflush is erasing
> > > sectors. /dev/mtd* close method calls the mtd->sync method
> > > which can hang for a long time(minutes).
> > > 
> > > Futher, the sync method "syncs" all flash chips. It should
> > > only sync the chips that a /dev/mtd* spans.
> > > Also, the mtdchar/mtdblock drivers does not need to
> > > call sync for readonly mappings.
> > 
> > So were you going to submit a separate patch for those issues?
> > 
> > josh
> 
> Wow, someone is reading my mails :)
> 
> Just did for mtdchar, the others are a bit more complex
> so I won't bother with them.
> 
>  Jocke

BTW, I don't think syncing is needed on NOR flashes at all.

 Jocke

Re: [PATCH] Fix hanging close for /dev/mtd

[Jörn Engel]

On Wed, 13 June 2007 13:48:21 +0200, Joakim Tjernlund wrote:
> On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> > On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > > Figured I should send this on its own.
> > > 
> > > Stracing a process using /dev/mtd shows that
> > > it sometimes hangs for a long time when pdflush is erasing
> > > sectors. /dev/mtd* close method calls the mtd->sync method
> > > which can hang for a long time(minutes).
> > > 
> > > Futher, the sync method "syncs" all flash chips. It should
> > > only sync the chips that a /dev/mtd* spans.
> > > Also, the mtdchar/mtdblock drivers does not need to
> > > call sync for readonly mappings.
> > 
> > So were you going to submit a separate patch for those issues?
> 
> Wow, someone is reading my mails :)

I read most of them.

In this case you made me wonder.  What is the mtd->sync method supposed
to accomplish anyway?  mtd->write is synchronous, mtd->erase de-facto
also is.  So it cannot be a sync in the sense of "flush all dirty data
to flash and don't return before you're finished".  Or can it?

Jörn

-- 
The strong give up and move away, while the weak give up and stay.
-- unknown

Re: [PATCH] Fix hanging close for /dev/mtd

[Josh Boyer]

On Wed, 2007-06-13 at 13:48 +0200, Joakim Tjernlund wrote:
> On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> > On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > > Figured I should send this on its own.
> > > 
> > > Stracing a process using /dev/mtd shows that
> > > it sometimes hangs for a long time when pdflush is erasing
> > > sectors. /dev/mtd* close method calls the mtd->sync method
> > > which can hang for a long time(minutes).
> > > 
> > > Futher, the sync method "syncs" all flash chips. It should
> > > only sync the chips that a /dev/mtd* spans.
> > > Also, the mtdchar/mtdblock drivers does not need to
> > > call sync for readonly mappings.
> > 
> > So were you going to submit a separate patch for those issues?
> > 
> > josh
> 
> Wow, someone is reading my mails :)

Heh, indeed :)

> Just did for mtdchar, the others are a bit more complex
> so I won't bother with them.

Great.  Oh, and I think your JFFS2 patch seems valid.  If I remember
correctly, David just did another similar s/cond_resched/yield/ change
that showed significant speed ups in OLPC boot times.

josh

Re: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

On Wed, 2007-06-13 at 07:31 -0500, Josh Boyer wrote:
> On Wed, 2007-06-13 at 13:48 +0200, Joakim Tjernlund wrote:
> > On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> > > On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > > > Figured I should send this on its own.
> > > > 
> > > > Stracing a process using /dev/mtd shows that
> > > > it sometimes hangs for a long time when pdflush is erasing
> > > > sectors. /dev/mtd* close method calls the mtd->sync method
> > > > which can hang for a long time(minutes).
> > > > 
> > > > Futher, the sync method "syncs" all flash chips. It should
> > > > only sync the chips that a /dev/mtd* spans.
> > > > Also, the mtdchar/mtdblock drivers does not need to
> > > > call sync for readonly mappings.
> > > 
> > > So were you going to submit a separate patch for those issues?
> > > 
> > > josh
> > 
> > Wow, someone is reading my mails :)
> 
> Heh, indeed :)
> 
> > Just did for mtdchar, the others are a bit more complex
> > so I won't bother with them.
> 
> Great.  Oh, and I think your JFFS2 patch seems valid.  If I remember
> correctly, David just did another similar s/cond_resched/yield/ change
> that showed significant speed ups in OLPC boot times.

Yes, he did. I think cond_resched idea isn't working as intended
with 2.6. I belive that the scheduler can promote a process/kernel
thread so that the cond_resched()/schedule() becomes a NOP.

  Jocke

Re: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

On Wed, 2007-06-13 at 13:54 +0200, Jörn Engel wrote:
> On Wed, 13 June 2007 13:48:21 +0200, Joakim Tjernlund wrote:
> > On Wed, 2007-06-13 at 06:44 -0500, Josh Boyer wrote:
> > > On Wed, 2007-06-13 at 11:53 +0200, Joakim Tjernlund wrote:
> > > > Figured I should send this on its own.
> > > > 
> > > > Stracing a process using /dev/mtd shows that
> > > > it sometimes hangs for a long time when pdflush is erasing
> > > > sectors. /dev/mtd* close method calls the mtd->sync method
> > > > which can hang for a long time(minutes).
> > > > 
> > > > Futher, the sync method "syncs" all flash chips. It should
> > > > only sync the chips that a /dev/mtd* spans.
> > > > Also, the mtdchar/mtdblock drivers does not need to
> > > > call sync for readonly mappings.
> > > 
> > > So were you going to submit a separate patch for those issues?
> > 
> > Wow, someone is reading my mails :)
> 
> I read most of them.
> 
> In this case you made me wonder.  What is the mtd->sync method supposed
> to accomplish anyway?  mtd->write is synchronous, mtd->erase de-facto
> also is.  So it cannot be a sync in the sense of "flush all dirty data
> to flash and don't return before you're finished".  Or can it?
> 
> Jörn

Yes, this is also my view(for NOR flash, NAND is diffrent).
mtd->erase can be suspended though, but it doesn't matter
in this case. However, I don't know enough to be 100% sure

 Jocke

Re: [PATCH] Fix hanging close for /dev/mtd

[Jörn Engel]

[ Putting David on Cc:.  This is a bug report for JFFS2 ]

On Wed, 13 June 2007 15:29:38 +0200, Joakim Tjernlund wrote:
> On Wed, 2007-06-13 at 13:54 +0200, Jörn Engel wrote:
> > 
> > In this case you made me wonder.  What is the mtd->sync method supposed
> > to accomplish anyway?  mtd->write is synchronous, mtd->erase de-facto
> > also is.  So it cannot be a sync in the sense of "flush all dirty data
> > to flash and don't return before you're finished".  Or can it?
> 
> Yes, this is also my view(for NOR flash, NAND is diffrent).
> mtd->erase can be suspended though, but it doesn't matter
> in this case. However, I don't know enough to be 100% sure

At the very least that method needs better documentation than:
	/* Sync */
	void (*sync) (struct mtd_info *mtd);

I can see two sane alternatives.

/*
 * An MTD driver may return from mtd->write() calls before the data has
 * safely been written to the medium.  Any amount of dirty data may be
 * cached by either the driver or the device.  If a filesystem depends
 * on the data being safely on the medium, it has to explicitly call
 * mtd->sync().
 */

/*
 * Mtd->write() and mtd->erase() methods may only return if the data has
 * been safely written to the device.  Special care needs to be taken
 * for devices that support caching.
 */

My tendency is to favor the first variant.  Block2mtd already implements
it.  And if we went for variant two, mtd->sync() would be pointless and
should get removed.

Now to the bug report:
JFFS2 must call mtd->sync() for its various sync routines.  If mounted
with -o sync or for files opened with O_SYNC, it has to call that after
every write.  If nothing else, it is currently broken on block2mtd.

Jörn

-- 
Linux [...] existed just for discussion between people who wanted
to show off how geeky they were.
-- Rob Enderle

Re: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

On Wed, 2007-06-13 at 16:29 +0200, Jörn Engel wrote:
> [ Putting David on Cc:.  This is a bug report for JFFS2 ]
> 
> On Wed, 13 June 2007 15:29:38 +0200, Joakim Tjernlund wrote:
> > On Wed, 2007-06-13 at 13:54 +0200, Jörn Engel wrote:
> > > 
> > > In this case you made me wonder.  What is the mtd->sync method supposed
> > > to accomplish anyway?  mtd->write is synchronous, mtd->erase de-facto
> > > also is.  So it cannot be a sync in the sense of "flush all dirty data
> > > to flash and don't return before you're finished".  Or can it?
> > 
> > Yes, this is also my view(for NOR flash, NAND is diffrent).
> > mtd->erase can be suspended though, but it doesn't matter
> > in this case. However, I don't know enough to be 100% sure
> 
> At the very least that method needs better documentation than:
> 	/* Sync */
> 	void (*sync) (struct mtd_info *mtd);
> 
> I can see two sane alternatives.
> 
> /*
>  * An MTD driver may return from mtd->write() calls before the data has
>  * safely been written to the medium.  Any amount of dirty data may be
>  * cached by either the driver or the device.  If a filesystem depends
>  * on the data being safely on the medium, it has to explicitly call
>  * mtd->sync().
>  */
> 
> /*
>  * Mtd->write() and mtd->erase() methods may only return if the data has
>  * been safely written to the device.  Special care needs to be taken
>  * for devices that support caching.
>  */
> 
> My tendency is to favor the first variant. 

   Me too.

>  Block2mtd already implements
> it.  And if we went for variant two, mtd->sync() would be pointless and
> should get removed.
> 
> Now to the bug report:
> JFFS2 must call mtd->sync() for its various sync routines.  If mounted
> with -o sync or for files opened with O_SYNC, it has to call that after
> every write.  If nothing else, it is currently broken on block2mtd.

Sure, but for NOR flash this sync can be a NOP, right?

 Jocke

Re: [PATCH] Fix hanging close for /dev/mtd

[Jörn Engel]

On Wed, 13 June 2007 19:16:31 +0200, Joakim Tjernlund wrote:
> > 
> > I can see two sane alternatives.
> > 
> > /*
> >  * An MTD driver may return from mtd->write() calls before the data has
> >  * safely been written to the medium.  Any amount of dirty data may be
> >  * cached by either the driver or the device.  If a filesystem depends
> >  * on the data being safely on the medium, it has to explicitly call
> >  * mtd->sync().
> >  */
> > 
> > /*
> >  * Mtd->write() and mtd->erase() methods may only return if the data has
> >  * been safely written to the device.  Special care needs to be taken
> >  * for devices that support caching.
> >  */
> > 
> > My tendency is to favor the first variant. 
> 
>    Me too.
> 
> >  Block2mtd already implements
> > it.  And if we went for variant two, mtd->sync() would be pointless and
> > should get removed.
> > 
> > Now to the bug report:
> > JFFS2 must call mtd->sync() for its various sync routines.  If mounted
> > with -o sync or for files opened with O_SYNC, it has to call that after
> > every write.  If nothing else, it is currently broken on block2mtd.
> 
> Sure, but for NOR flash this sync can be a NOP, right?

Actually, no.  After dwmw2 explained things in irc, I finally understand
the original meaning of that method.  It is supposed to wait until any
erase suspend (or program suspend) has finished.  You can view erase
suspend as a form of caching and erase as a form of writing.

If we wanted to avoid the cost without paying in reliability, we could
add an argument to sync().  Sync(now) would have the current semantics.
Sync(five minutes ago) would require that any writes/erases started more
than five minutes ago must be safe.  It add quite a bit of complexity,
though.  Maybe not worth it.

Jörn

-- 
The wise man seeks everything in himself; the ignorant man tries to get
everything from somebody else.
-- unknown

RE: [PATCH] Fix hanging close for /dev/mtd

[Joakim Tjernlund]

 

> -----Original Message-----
> From: Jörn Engel [mailto:joern@logfs.org] 
> Sent: den 13 juni 2007 19:59
> To: Joakim Tjernlund
> Cc: Jörn Engel; Linux MTD mailing list; David Woodhouse
> Subject: Re: [PATCH] Fix hanging close for /dev/mtd
> 
> On Wed, 13 June 2007 19:16:31 +0200, Joakim Tjernlund wrote:
> > > 
> > > I can see two sane alternatives.
> > > 
> > > /*
> > >  * An MTD driver may return from mtd->write() calls 
> before the data has
> > >  * safely been written to the medium.  Any amount of 
> dirty data may be
> > >  * cached by either the driver or the device.  If a 
> filesystem depends
> > >  * on the data being safely on the medium, it has to 
> explicitly call
> > >  * mtd->sync().
> > >  */
> > > 
> > > /*
> > >  * Mtd->write() and mtd->erase() methods may only return 
> if the data has
> > >  * been safely written to the device.  Special care needs 
> to be taken
> > >  * for devices that support caching.
> > >  */
> > > 
> > > My tendency is to favor the first variant. 
> > 
> >    Me too.
> > 
> > >  Block2mtd already implements
> > > it.  And if we went for variant two, mtd->sync() would be 
> pointless and
> > > should get removed.
> > > 
> > > Now to the bug report:
> > > JFFS2 must call mtd->sync() for its various sync 
> routines.  If mounted
> > > with -o sync or for files opened with O_SYNC, it has to 
> call that after
> > > every write.  If nothing else, it is currently broken on 
> block2mtd.
> > 
> > Sure, but for NOR flash this sync can be a NOP, right?
> 
> Actually, no.  After dwmw2 explained things in irc, I finally 
> understand
> the original meaning of that method.  It is supposed to wait until any
> erase suspend (or program suspend) has finished.  You can view erase
> suspend as a form of caching and erase as a form of writing.

Yes, I see that. Then it will enable writes/erases again.

> 
> If we wanted to avoid the cost without paying in reliability, we could
> add an argument to sync().  Sync(now) would have the current 
> semantics.
> Sync(five minutes ago) would require that any writes/erases 
> started more
> than five minutes ago must be safe.  It add quite a bit of complexity,
> though.  Maybe not worth it.

Still don't get what sync will do for you in mtdchar context. All
mtd writes are synchronous and will be in flash without sync. So
what extra function does it get you on NOR flashes?

David, you are most welcome to explain this on the list also and
comment on my latest patches.

 Jocke