* [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) [not found] <20091117174617.285298261@vyatta.com> @ 2009-11-17 17:46 ` Stephen Hemminger 2009-11-19 15:19 ` Jan Kara 2009-11-17 17:46 ` [PATCH 2/2] ext2: add wait flag support to sync_fs Stephen Hemminger 1 sibling, 1 reply; 9+ messages in thread From: Stephen Hemminger @ 2009-11-17 17:46 UTC (permalink / raw) To: linux-ext4 [-- Attachment #1: ext2-uptodate.patch --] [-- Type: text/plain, Size: 1444 bytes --] This fixes a WARN backtrace in mark_buffer_dirty() that occurs during unmount when a USB or floppy device is removed. The super block update from a previous operation has marked the buffer as in error, and the flag has to be cleared before doing the update. (Similar code already exists in ext4). Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> --- Patch against 2.6.32-rc7 Revised to handle both sync_fs and put_super updating superblock. --- a/fs/ext2/super.c 2009-11-17 09:13:21.346375999 -0800 +++ b/fs/ext2/super.c 2009-11-17 09:14:12.177002522 -0800 @@ -1099,11 +1099,24 @@ static void ext2_commit_super (struct su static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es) { + struct buffer_head *sbh = EXT2_SB(sb)->s_sbh; + + if (buffer_write_io_error(sbh)) { + /* + * This happens if USB or floppy device is yanked out. + * Maybe user put device back in so warn and update again. + */ + printk(KERN_ERR + "EXT2-fs: previous I/O error to superblock detected\n"); + clear_buffer_write_io_error(sbh); + set_buffer_uptodate(sbh); + } + es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb)); es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb)); es->s_wtime = cpu_to_le32(get_seconds()); - mark_buffer_dirty(EXT2_SB(sb)->s_sbh); - sync_dirty_buffer(EXT2_SB(sb)->s_sbh); + mark_buffer_dirty(sbh); + sync_dirty_buffer(sbh); sb->s_dirt = 0; } -- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) 2009-11-17 17:46 ` [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) Stephen Hemminger @ 2009-11-19 15:19 ` Jan Kara 2009-11-19 18:13 ` Stephen Hemminger 0 siblings, 1 reply; 9+ messages in thread From: Jan Kara @ 2009-11-19 15:19 UTC (permalink / raw) To: Stephen Hemminger; +Cc: linux-ext4 > This fixes a WARN backtrace in mark_buffer_dirty() that occurs during > unmount when a USB or floppy device is removed. > The super block update from a previous operation has marked the buffer > as in error, and the flag has to be cleared before doing the update. > (Similar code already exists in ext4). > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> Although I agree with Nick that ultimate solution would be to stop clearing the uptodate flag, I think the change is fine in principle as a workaround for warning that only scares people. > --- a/fs/ext2/super.c 2009-11-17 09:13:21.346375999 -0800 > +++ b/fs/ext2/super.c 2009-11-17 09:14:12.177002522 -0800 > @@ -1099,11 +1099,24 @@ static void ext2_commit_super (struct su > > static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es) > { > + struct buffer_head *sbh = EXT2_SB(sb)->s_sbh; > + > + if (buffer_write_io_error(sbh)) { > + /* > + * This happens if USB or floppy device is yanked out. > + * Maybe user put device back in so warn and update again. > + */ > + printk(KERN_ERR > + "EXT2-fs: previous I/O error to superblock detected\n"); > + clear_buffer_write_io_error(sbh); > + set_buffer_uptodate(sbh); It's not much about puting the device back. It's really just about avoiding the warning in mark_buffer_dirty(). So I'd just silently set_buffer_uptodate and be done with it. For superblock we are darn sure that in memory copy is the one that has the latest data :) > + } > + > es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb)); > es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb)); > es->s_wtime = cpu_to_le32(get_seconds()); > - mark_buffer_dirty(EXT2_SB(sb)->s_sbh); > - sync_dirty_buffer(EXT2_SB(sb)->s_sbh); > + mark_buffer_dirty(sbh); > + sync_dirty_buffer(sbh); > sb->s_dirt = 0; > } Honza -- Jan Kara <jack@suse.cz> SuSE CR Labs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) 2009-11-19 15:19 ` Jan Kara @ 2009-11-19 18:13 ` Stephen Hemminger 2009-11-20 9:23 ` Jan Kara 0 siblings, 1 reply; 9+ messages in thread From: Stephen Hemminger @ 2009-11-19 18:13 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4 On Thu, 19 Nov 2009 16:19:53 +0100 Jan Kara <jack@suse.cz> wrote: > > + if (buffer_write_io_error(sbh)) { > > + /* > > + * This happens if USB or floppy device is yanked out. > > + * Maybe user put device back in so warn and update again. > > + */ > > + printk(KERN_ERR > > + "EXT2-fs: previous I/O error to superblock detected\n"); > > + clear_buffer_write_io_error(sbh); > > + set_buffer_uptodate(sbh); > It's not much about puting the device back. It's really just about > avoiding the warning in mark_buffer_dirty(). So I'd just silently > set_buffer_uptodate and be done with it. For superblock we are darn sure > that in memory copy is the one that has the latest data :) This code mirrors ext4_commit_super, why should ext2 be any different? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) 2009-11-19 18:13 ` Stephen Hemminger @ 2009-11-20 9:23 ` Jan Kara 0 siblings, 0 replies; 9+ messages in thread From: Jan Kara @ 2009-11-20 9:23 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Jan Kara, linux-ext4 On Thu 19-11-09 10:13:17, Stephen Hemminger wrote: > On Thu, 19 Nov 2009 16:19:53 +0100 > Jan Kara <jack@suse.cz> wrote: > > > > + if (buffer_write_io_error(sbh)) { > > > + /* > > > + * This happens if USB or floppy device is yanked out. > > > + * Maybe user put device back in so warn and update again. > > > + */ > > > + printk(KERN_ERR > > > + "EXT2-fs: previous I/O error to superblock detected\n"); > > > + clear_buffer_write_io_error(sbh); > > > + set_buffer_uptodate(sbh); > > It's not much about puting the device back. It's really just about > > avoiding the warning in mark_buffer_dirty(). So I'd just silently > > set_buffer_uptodate and be done with it. For superblock we are darn sure > > that in memory copy is the one that has the latest data :) > > This code mirrors ext4_commit_super, why should ext2 be any different? OK, my remark was mostly about the comment which is different in ext4 ;). I'll just fixup the comment and merge the patch. Thanks. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ext2: add wait flag support to sync_fs [not found] <20091117174617.285298261@vyatta.com> 2009-11-17 17:46 ` [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) Stephen Hemminger @ 2009-11-17 17:46 ` Stephen Hemminger 2009-11-19 15:34 ` Jan Kara 1 sibling, 1 reply; 9+ messages in thread From: Stephen Hemminger @ 2009-11-17 17:46 UTC (permalink / raw) To: linux-ext4 [-- Attachment #1: ext2-syncfs-wait.patch --] [-- Type: text/plain, Size: 612 bytes --] Make ext2 safer against accidental data loss during removal by adding support for waiting for super block update on sync. Don't know why this wasn't done originally, all the other file systems have it. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> --- a/fs/ext2/super.c 2009-11-17 09:14:12.177002522 -0800 +++ b/fs/ext2/super.c 2009-11-17 09:14:32.698005421 -0800 @@ -1147,6 +1147,8 @@ static int ext2_sync_fs(struct super_blo ext2_sync_super(sb, es); } else { ext2_commit_super(sb, es); + if (wait) + sync_dirty_buffer(EXT2_SB(sb)->s_sbh); } sb->s_dirt = 0; unlock_kernel(); -- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext2: add wait flag support to sync_fs 2009-11-17 17:46 ` [PATCH 2/2] ext2: add wait flag support to sync_fs Stephen Hemminger @ 2009-11-19 15:34 ` Jan Kara 2009-11-20 10:36 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Jan Kara @ 2009-11-19 15:34 UTC (permalink / raw) To: Stephen Hemminger; +Cc: linux-ext4, hch > Make ext2 safer against accidental data loss during removal > by adding support for waiting for super block update on sync. > Don't know why this wasn't done originally, all the other file > systems have it. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > --- a/fs/ext2/super.c 2009-11-17 09:14:12.177002522 -0800 > +++ b/fs/ext2/super.c 2009-11-17 09:14:32.698005421 -0800 > @@ -1147,6 +1147,8 @@ static int ext2_sync_fs(struct super_blo > ext2_sync_super(sb, es); > } else { > ext2_commit_super(sb, es); > + if (wait) > + sync_dirty_buffer(EXT2_SB(sb)->s_sbh); > } > sb->s_dirt = 0; > unlock_kernel(); Looking at the code it just looks strange. Part of it is because of Christoph's conversion of ext2_write_super to ext2_sync_fs (40f31dd47e7c3d15af1f9845eda0fa0c4c33f32f) but the VALID_FS handling oddness seems to be even older. IMHO we should just clear the VALID_FS flag on mount and in write_super() and sync_fs() just update block and inode counters. wait == 1 case should then really wait for superblock buffer, wait == 0 should not wait. BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1 from ext2_write_super()? I'd think (and looking into callsites seem to confirm that) that ->write_super() was meant to be asynchronous... I've added this to my todo... Honza -- Jan Kara <jack@suse.cz> SuSE CR Labs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext2: add wait flag support to sync_fs 2009-11-19 15:34 ` Jan Kara @ 2009-11-20 10:36 ` Christoph Hellwig 2009-11-20 17:08 ` Stephen Hemminger 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2009-11-20 10:36 UTC (permalink / raw) To: Jan Kara; +Cc: Stephen Hemminger, linux-ext4, hch On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote: > BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1 > from ext2_write_super()? I'd think (and looking into callsites seem to > confirm that) that ->write_super() was meant to be asynchronous... No particular reason - the argument wasn't and still isn't used in ext2. And yes, now that ->sync_fs is mandatory ->write_super should be asynchronous. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext2: add wait flag support to sync_fs 2009-11-20 10:36 ` Christoph Hellwig @ 2009-11-20 17:08 ` Stephen Hemminger 2009-11-20 21:06 ` Jan Kara 0 siblings, 1 reply; 9+ messages in thread From: Stephen Hemminger @ 2009-11-20 17:08 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jan Kara, linux-ext4, hch On Fri, 20 Nov 2009 05:36:22 -0500 Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote: > > BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1 > > from ext2_write_super()? I'd think (and looking into callsites seem to > > confirm that) that ->write_super() was meant to be asynchronous... > > No particular reason - the argument wasn't and still isn't used in ext2. > And yes, now that ->sync_fs is mandatory ->write_super should be > asynchronous. > Shouldn't super block (and all other) updates be synchronous if ext2 is mounted with SYNC and DIRSYNC? -- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] ext2: add wait flag support to sync_fs 2009-11-20 17:08 ` Stephen Hemminger @ 2009-11-20 21:06 ` Jan Kara 0 siblings, 0 replies; 9+ messages in thread From: Jan Kara @ 2009-11-20 21:06 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Christoph Hellwig, Jan Kara, linux-ext4 On Fri 20-11-09 09:08:58, Stephen Hemminger wrote: > On Fri, 20 Nov 2009 05:36:22 -0500 > Christoph Hellwig <hch@infradead.org> wrote: > > > On Thu, Nov 19, 2009 at 04:34:19PM +0100, Jan Kara wrote: > > > BTW: Christoph, why did you choose to call ext2_sync_fs with wait == 1 > > > from ext2_write_super()? I'd think (and looking into callsites seem to > > > confirm that) that ->write_super() was meant to be asynchronous... > > > > No particular reason - the argument wasn't and still isn't used in ext2. > > And yes, now that ->sync_fs is mandatory ->write_super should be > > asynchronous. > > Shouldn't super block (and all other) updates be synchronous if ext2 > is mounted with SYNC and DIRSYNC? Well, looking at the code, we don't seem to do that ;) Maybe we should but would it really bring anything? The only thing which will go wrong are counters of free blocks and inodes and those will be recomputed by fsck anyway. And note that SYNC does not guarantee you that you don't need fsck if you just pull the device out without umount. It just limits the damage... Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-11-20 21:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20091117174617.285298261@vyatta.com>
2009-11-17 17:46 ` [PATCH 1/2] ext2: clear uptodate flag on super block I/O error (v2) Stephen Hemminger
2009-11-19 15:19 ` Jan Kara
2009-11-19 18:13 ` Stephen Hemminger
2009-11-20 9:23 ` Jan Kara
2009-11-17 17:46 ` [PATCH 2/2] ext2: add wait flag support to sync_fs Stephen Hemminger
2009-11-19 15:34 ` Jan Kara
2009-11-20 10:36 ` Christoph Hellwig
2009-11-20 17:08 ` Stephen Hemminger
2009-11-20 21:06 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox