On Mon, Apr 19 2010, Jörn Engel wrote:
quoted text
> On Mon, 19 April 2010 09:38:44 +0200, Jens Axboe wrote:
> > On Sat, Apr 17 2010, Jörn Engel wrote:
> > > Moin David,
> > > 
> > > if I read the code correctly, JFFS2 will happily perform a NOP on
> > > sys_sync() again.  And this time it appears to be Jens' fault.
> > > 
> > > JFFS2 does not appear to set s_bdi anywhere.  And as of 32a88aa1,
> > > __sync_filesystem() will return 0 if s_bdi is not set.  As a result,
> > > sync_fs() is never called for jffs2 and whatever remains in the wbuf
> > > will not make it to the device.
> > > 
> > > The patch also adds a BUG_ON to catch this problem in any remaining or
> > > future offenders.  I am not sure about network filesystems, but at
> > > least bdev- and mtd-based ones should be caught.
> > > 
> > > Opinions?
> > 
> > I think that BUG_ON() would be a lot better as a printk() and fail mount
> > instead. There's really little point in killing the kernel for something
> > you could easily warn about and recover nicely.
> 
> *shrug*
> The BUG_ON directly above is not qualitatively different.  In both cases
> the only solution is to find and fix the bug in some other file,
> recompile and try again.  But ultimately I don't care, as long as we
> catch the bug before people lose their data.
> 
> Feel free to respin this patch.  You caused the problem in the first
> place. ;)
> 
> For the record, while I consider the two-liner that causes this mess a
> real turd, the rest of your patch was brilliant.  A shame I didn't catch
> this earlier.

Thanks, we definitely should have put a debug statement to catch this in
from day 1, good debugging should be an important part of any new
infrastructure.

Care to send your jffs2 patch separately to David? Then I'll commit a
modified variant for complaining about missing ->s_bdi on mount.

-- 
Jens Axboe

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