Inode data integrity patches
Subsystems (18)
cover Inode data integrity patches
These are not for merge quite yet, need a bit more changelog and comments polishing, and splitting out filesystems patches, but if I don't get negative comments about them, I'll be asking to merge them in next window.
Thanks,
Nick
PATCH 1/8 fs: mark_inode_dirty barrier fix
Filesystems appear to be using ->dirty_inode, expecting that the dirtying operating is done and visible to all CPUs (eg. setting private inode dirty bits, without any barriers themselves). So release the dirty "critical section" with a barrier before calling ->dirty_inode.
Cost is not significantly changed, because we're just moving the barrier. Those filesystems that do use ->dirty_inode should have to care slightly less about barriers, which is a good thing.
fs/fs-writeback.c +9 -6
@@ -934,6 +934,15 @@ void __mark_inode_dirty(struct inode *inbool wakeup_bdi = false;/* +* Make sure that changes are seen by all cpus before we test i_state +* or mark anything as being dirty. Ie. all dirty state should be +* written to the inode and visible. Like an "unlock" operation, the +* mark_inode_dirty call must "release" our ordering window that is +* opened when we started modifying the inode. +*/ +smp_mb (); + +/* * Don't do this for I_DIRTY_PAGES - that doesn't actually * dirty the inode itself */
@@ -942,12 +951,6 @@ void __mark_inode_dirty(struct inode *in sb->s_op->dirty_inode (inode); } -/* -* make sure that changes are seen by all cpus before we test i_state -* -- mikulas -*/ -smp_mb (); -/* avoid the locking if we can */ if ((inode->i_state & flags) == flags)return ;
PATCH 2/8 fs: simple fsync race fix
In reply to Nick PigginIt is incorrect to test inode dirty bits without participating in the inode writeback protocol. Inode writeback sets I_SYNC and clears I_DIRTY_?, then writes out the particular bits, then clears I_SYNC when it is done. BTW. it may not completely write all pages out, so I_DIRTY_PAGES would get set again.
This is a standard pattern used throughout the kernel's writeback caches (I_SYNC ~= I_WRITEBACK, if that makes it clearer).
And so it is not possible to determine an inode's dirty status just by checking I_DIRTY bits. Especially not for the purpose of data integrity syncs.
Missing the check for these bits means that fsync can complete while writeback to the inode is underway. Inode writeback functions get this right, so call into them rather than try to shortcut things by testing dirty state improperly.
fs/libfs.c +0 -5
@@ -895,11 +895,6 @@ int generic_file_fsync(struct file *fileint ret; ret =sync_mapping_buffers (inode->i_mapping); -if (!(inode->i_state &I_DIRTY )) -return ret; -if (datasync && !(inode->i_state &I_DIRTY_DATASYNC )) -return ret; - err =sync_inode_metadata (inode,1 );if (ret ==0 ) ret = err;
fs/exofs/file.c +0 -5
@@ -48,11 +48,6 @@ static int exofs_file_fsync(struct filestruct inode *inode = filp->f_mapping->host;struct super_block *sb; -if (!(inode->i_state &I_DIRTY )) -return 0 ; -if (datasync && !(inode->i_state &I_DIRTY_DATASYNC )) -return 0 ; - ret =sync_inode_metadata (inode,1 );/* This is a good place to write the sb */
PATCH 3/8 fs: introduce inode writeback helpers
In reply to Nick PigginInode dirty state cannot be securely tested without participating properly in the inode writeback protocol. Some filesystems need to check this state, so break out the code into helpers and make them available.
This could also be used to reduce strange interactions between background writeback and fsync. Currently if we fsync a single page in a file, the entire file gets requeued to the back of the background IO list, even if it is due for writeout and has a large number of pages. That's left for a later time.
fs/fs-writeback.c +110 -58
@@ -299,26 +299,25 @@ static void inode_wait_for_writeback(str } } -/* -* Write out an inode's dirty pages. Called under inode_lock. Either the -* caller has ref on the inode (either via __iget or via syscall against an fd) -* or the inode has I_WILL_FREE set (via generic_forget_inode) +/** +* inode_writeback_begin -- prepare to writeback an inode +* @indoe: inode to write back +* @wait: synch writeout or not +* @Returns: 0 if wait == 0 and this call would block (due to other writeback). +* otherwise returns 1. * -* If `wait' is set, wait on the writeout. +* Context: inode_lock must be held, may be dropped. Returns with it held. * -* The whole writeout design is quite complex and fragile. We want to avoid -* starvation of particular inodes when others are being redirtied, prevent -* livelocks, etc. +* inode_writeback_begin sets up an inode to be written back (data and/or +* metadata). This must be called before examining I_DIRTY state of the +* inode, and should be called at least before any data integrity writeout. * -* Called under inode_lock. +* If inode_writeback_begin returns 1, it must be followed by a call to +* inode_writeback_end. */ -static int -writeback_single_inode (struct inode *inode,struct writeback_control *wbc) +int inode_writeback_begin (struct inode *inode,int wait) { -struct address_space *mapping = inode->i_mapping; -unsigned dirty; -int ret; - +assert_spin_locked (&inode_lock);if (!atomic_read (&inode->i_count))WARN_ON (!(inode->i_state & (I_WILL_FREE |I_FREEING )));else
@@ -327,16 +326,10 @@ writeback_single_inode(struct inode *inoif (inode->i_state &I_SYNC ) {/* * If this inode is locked for writeback and we are not doing -* writeback-for-data-integrity, move it to b_more_io so that -* writeback can proceed with the other inodes on s_io. -* -* We'll have another go at writing back this inode when we -* completed a full scan of b_io. +* writeback-for-data-integrity, skip it. */ -if (wbc->sync_mode !=WB_SYNC_ALL ) { -requeue_io (inode); +if (!wait)return 0 ; - }/* * It's a data-integrity sync. We must wait.
@@ -346,9 +339,91 @@ writeback_single_inode(struct inode *inoBUG_ON (inode->i_state &I_SYNC ); -/* Set I_SYNC, reset I_DIRTY_PAGES */ inode->i_state |=I_SYNC ; inode->i_state &= ~I_DIRTY_PAGES ; + +return 1 ; +} +EXPORT_SYMBOL (inode_writeback_begin); + +/** +* inode_writeback_end - end a writeback section opened by inode_writeback_begin +* @inode: inode in question +* @Returns: 0 if the inode still has dirty pagecache, otherwise 1. +* +* Context: inode_lock must be held, not dropped. +* +* inode_writeback_end must follow a successful call to inode_writeback_begin +* after we have finished submitting writeback to the inode. +*/ +int inode_writeback_end (struct inode *inode) +{ +int ret =1 ; + +assert_spin_locked (&inode_lock); +BUG_ON (!(inode->i_state &I_SYNC )); + +if (!(inode->i_state &I_FREEING )) { +if (mapping_tagged (inode->i_mapping,PAGECACHE_TAG_DIRTY )) { +/* +* We didn't write back all the pages. nfs_writepages() +* sometimes bales out without doing anything. +*/ + inode->i_state |=I_DIRTY_PAGES ; + ret =0 ; + }else if (inode->i_state &I_DIRTY ) { +/* +* Filesystems can dirty the inode during writeback +* operations, such as delayed allocation during +* submission or metadata updates after data IO +* completion. +*/ +redirty_tail (inode); + }else { +/* +* The inode is clean. At this point we either have +* a reference to the inode or it's on it's way out. +* No need to add it back to the LRU. +*/ +list_del_init (&inode->i_wb_list); + } + } + inode->i_state &= ~I_SYNC ; +inode_sync_complete (inode); + +return ret; +} +EXPORT_SYMBOL (inode_writeback_end); + +/* +* Write out an inode's dirty pages. Called under inode_lock. Either the +* caller has ref on the inode (either via __iget or via syscall against an fd) +* or the inode has I_WILL_FREE set (via generic_forget_inode) +* +* If `wait' is set, wait on the writeout. +* +* The whole writeout design is quite complex and fragile. We want to avoid +* starvation of particular inodes when others are being redirtied, prevent +* livelocks, etc. +* +* Called under inode_lock. +*/ +static int +writeback_single_inode (struct inode *inode,struct writeback_control *wbc) +{ +struct address_space *mapping = inode->i_mapping; +unsigned dirty; +int ret; + +if (!inode_writeback_begin (inode, wbc->sync_mode ==WB_SYNC_ALL )) { +/* +* We'll have another go at writing back this inode when we +* completed a full scan of b_io. +*/ +requeue_io (inode); +return 0 ; + } +spin_unlock (&inode_lock); ret =do_writepages (mapping, wbc);
@@ -381,47 +456,24 @@ writeback_single_inode(struct inode *ino }spin_lock (&inode_lock); - inode->i_state &= ~I_SYNC ; -if (!(inode->i_state &I_FREEING )) { -if (mapping_tagged (mapping,PAGECACHE_TAG_DIRTY )) { -/* -* We didn't write back all the pages. nfs_writepages() -* sometimes bales out without doing anything. -*/ - inode->i_state |=I_DIRTY_PAGES ; -if (wbc->nr_to_write <=0 ) { -/* -* slice used up: queue for next turn -*/ -requeue_io (inode); - }else { -/* -* Writeback blocked by something other than -* congestion. Delay the inode for some time to -* avoid spinning on the CPU (100% iowait) -* retrying writeback of the dirty page/inode -* that cannot be performed immediately. -*/ -redirty_tail (inode); - } - }else if (inode->i_state &I_DIRTY ) { +if (!inode_writeback_end (inode)) { +if (wbc->nr_to_write <=0 ) {/* -* Filesystems can dirty the inode during writeback -* operations, such as delayed allocation during -* submission or metadata updates after data IO -* completion. +* slice used up: queue for next turn */ -redirty_tail (inode); +requeue_io (inode); }else {/* -* The inode is clean. At this point we either have -* a reference to the inode or it's on it's way out. -* No need to add it back to the LRU. +* Writeback blocked by something other than +* congestion. Delay the inode for some time to +* avoid spinning on the CPU (100% iowait) +* retrying writeback of the dirty page/inode +* that cannot be performed immediately. */ -list_del_init (&inode->i_wb_list); +redirty_tail (inode); } } -inode_sync_complete (inode); +return ret; }
include/linux/fs.h +2 -0
@@ -1766,6 +1766,8 @@ static inline void file_accessed(structint sync_inode (struct inode *inode,struct writeback_control *wbc);int sync_inode_metadata (struct inode *inode,int wait); +int inode_writeback_begin (struct inode *inode,int wait); +int inode_writeback_end (struct inode *inode);struct file_system_type {const char *name;
PATCH 4/8 fs: preserve inode dirty bits on failed metadata writeback
In reply to Nick PigginOtherwise we think the inode is clean even if syncing failed.
fs/fs-writeback.c +13 -3
@@ -447,15 +447,25 @@ writeback_single_inode(struct inode *inospin_lock (&inode_lock); dirty = inode->i_state &I_DIRTY ; inode->i_state &= ~(I_DIRTY_SYNC |I_DIRTY_DATASYNC ); -spin_unlock (&inode_lock);/* Don't write the inode if only I_DIRTY_PAGES was set */ if (dirty & (I_DIRTY_SYNC |I_DIRTY_DATASYNC )) { -int err =write_inode (inode, wbc); +int err; + +spin_unlock (&inode_lock); + err =write_inode (inode, wbc);if (ret ==0 ) ret = err; +spin_lock (&inode_lock); +if (err) { +/* +* Inode writeout failed, restore inode metadata +* dirty bits. +*/ + inode->i_state |= dirty & + (I_DIRTY_SYNC |I_DIRTY_DATASYNC ); + } } -spin_lock (&inode_lock);if (!inode_writeback_end (inode)) {if (wbc->nr_to_write <=0 ) {/*
PATCH 5/8 fs: ext2 inode sync fix
In reply to Nick PigginThere is a big fuckup with inode metadata writeback (I suspect in a lot of filesystems, but I've only dared to look at a couple as yet).
ext2 relies on ->write_inode being called from sync_inode_metadata in fsync in order to sync the inode. However I_DIRTY_SYNC gets cleared after a call to this guy, and it doesn't actually write back and wait on the inode block unless it is called for sync. This means that write_inode from background writeback can kill the inode dirty bits without the data getting to disk. Fsync will subsequently miss it.
The fix is for ->write_inode to dirty the buffers/cache, and then ->fsync to write back the dirty data. In the full filesystem sync case, buffercache writeback in the generic code will write back the dirty data. Other filesystems could use ->sync_fs for this.
fs/ext2/inode.c +2 -10
@@ -1211,7 +1211,7 @@ static int ext2_setsize(struct inode *inreturn 0 ; } -static struct ext2_inode *ext2_get_inode (struct super_block *sb,ino_t ino, +struct ext2_inode *ext2_get_inode (struct super_block *sb,ino_t ino,struct buffer_head **p) {struct buffer_head * bh;
@@ -1505,16 +1505,8 @@ static int __ext2_write_inode(struct ino }else for (n =0 ; n <EXT2_N_BLOCKS ; n++) raw_inode->i_block[n] = ei->i_data[n];mark_buffer_dirty (bh); -if (do_sync) { -sync_dirty_buffer (bh); -if (buffer_req (bh) && !buffer_uptodate (bh)) { -printk ("IO error syncing ext2 inode [%s:%08lx]\n" , - sb->s_id, (unsigned long ) ino); - err = -EIO ; - } - } - ei->i_state &= ~EXT2_STATE_NEW ;brelse (bh); + ei->i_state &= ~EXT2_STATE_NEW ;return err; }
fs/ext2/file.c +21 -3
@@ -21,6 +21,7 @@#include <linux/time.h> #include <linux/pagemap.h> #include <linux/quotaops.h> +#include <linux/buffer_head.h> #include "ext2.h" #include "xattr.h" #include "acl.h"
@@ -43,16 +44,33 @@ static int ext2_release_file (struct inoint ext2_fsync (struct file *file,int datasync) {int ret; -struct super_block *sb = file->f_mapping->host->i_sb; -struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping; +struct inode *inode = file->f_mapping->host; +ino_t ino = inode->i_ino; +struct super_block *sb = inode->i_sb; +struct address_space *sb_mapping = sb->s_bdev->bd_inode->i_mapping; +struct buffer_head *bh; +struct ext2_inode *raw_inode; ret =generic_file_fsync (file, datasync); -if (ret == -EIO ||test_and_clear_bit (AS_EIO , &mapping->flags)) { +if (ret == -EIO ||test_and_clear_bit (AS_EIO , &sb_mapping->flags)) {/* We don't really know where the IO error happened... */ ext2_error (sb, __func__,"detected IO error when writing metadata buffers" ); +return -EIO ; + } + + raw_inode =ext2_get_inode (sb, ino, &bh); +if (IS_ERR (raw_inode)) +return -EIO ; + +sync_dirty_buffer (bh); +if (buffer_req (bh) && !buffer_uptodate (bh)) { +printk ("IO error syncing ext2 inode [%s:%08lx]\n" , + sb->s_id, (unsigned long ) ino); ret = -EIO ; } +brelse (bh); +return ret; }
fs/ext2/ext2.h +2 -0
@@ -124,6 +124,8 @@ extern int ext2_get_block(struct inode *extern int ext2_setattr (struct dentry *,struct iattr *);extern void ext2_set_inode_flags (struct inode *inode);extern void ext2_get_inode_flags (struct ext2_inode_info *); +extern struct ext2_inode *ext2_get_inode (struct super_block *sb,ino_t ino, +struct buffer_head **p);extern int ext2_fiemap (struct inode *inode,struct fiemap_extent_info *fieinfo,u64 start,u64 len);
fs/adfs/inode.c +1 -1
@@ -383,7 +383,7 @@ int adfs_write_inode(struct inode *inode obj.attr =ADFS_I (inode)->attr; obj.size = inode->i_size; - ret =adfs_dir_update (sb, &obj, wbc->sync_mode ==WB_SYNC_ALL ); + ret =adfs_dir_update (sb, &obj,1 /* XXX: fix fsync and use 'wbc->sync_mode == WB_SYNC_ALL' */ );unlock_kernel ();return ret; }
fs/affs/file.c +1 -0
@@ -931,6 +931,7 @@ int affs_file_fsync(struct file *filp, iint ret, err; ret =write_inode_now (inode,0 ); +/* XXX: could just sync the buffer been dirtied by write_inode */ err =sync_blockdev (inode->i_sb->s_bdev);if (!ret) ret = err;
fs/bfs/inode.c +1 -1
@@ -151,7 +151,7 @@ static int bfs_write_inode(struct inode di->i_eoffset =cpu_to_le32 (i_sblock *BFS_BSIZE + inode->i_size -1 );mark_buffer_dirty (bh); -if (wbc->sync_mode ==WB_SYNC_ALL ) { +if (1 /* XXX: fix fsync and use wbc->sync_mode == WB_SYNC_ALL */ ) {sync_dirty_buffer (bh);if (buffer_req (bh) && !buffer_uptodate (bh)) err = -EIO ;
fs/exofs/inode.c +1 -1
@@ -1273,7 +1273,7 @@ static int exofs_update_inode(struct inoint exofs_write_inode (struct inode *inode,struct writeback_control *wbc) { -return exofs_update_inode (inode, wbc->sync_mode ==WB_SYNC_ALL ); +return exofs_update_inode (inode,1 /* XXX: fix fsync and use wbc->sync_mode == WB_SYNC_ALL */ ); }/*
fs/ext4/inode.c +1 -1
@@ -5243,7 +5243,7 @@ int ext4_write_inode(struct inode *inode err =__ext4_get_inode_loc (inode, &iloc,0 );if (err)return err; -if (wbc->sync_mode ==WB_SYNC_ALL ) +if (1 /* XXX: fix fxync and use wbc->sync_mode == WB_SYNC_ALL */ )sync_dirty_buffer (iloc.bh);if (buffer_req (iloc.bh) && !buffer_uptodate (iloc.bh)) { EXT4_ERROR_INODE_BLOCK(inode, iloc.bh->b_blocknr,
fs/fat/inode.c +1 -1
@@ -645,7 +645,7 @@ static int __fat_write_inode(struct inodspin_unlock (&sbi->inode_hash_lock);mark_buffer_dirty (bh); err =0 ; -if (wait) +if (1 /* XXX: fix fsync and use wait */ ) err =sync_dirty_buffer (bh);brelse (bh);return err;
fs/jfs/inode.c +1 -1
@@ -123,7 +123,7 @@ int jfs_commit_inode(struct inode *inodeint jfs_write_inode (struct inode *inode,struct writeback_control *wbc) { -int wait = wbc->sync_mode ==WB_SYNC_ALL ; +int wait =1 ;/* XXX fix fsync and use wbc->sync_mode == WB_SYNC_ALL; */ if (test_cflag (COMMIT_Nolink, inode))return 0 ;
fs/minix/inode.c +1 -1
@@ -576,7 +576,7 @@ static int minix_write_inode(struct inod bh =V2_minix_update_inode (inode);if (!bh)return -EIO ; -if (wbc->sync_mode ==WB_SYNC_ALL &&buffer_dirty (bh)) { +if (1 /* XXX: fix fsync and use wbc->sync_mode == WB_SYNC_ALL */ &&buffer_dirty (bh)) {sync_dirty_buffer (bh);if (buffer_req (bh) && !buffer_uptodate (bh)) { printk("IO error syncing minix inode [%s:%08lx]\n",
fs/omfs/inode.c +1 -1
@@ -169,7 +169,7 @@ static int __omfs_write_inode(struct inostatic int omfs_write_inode (struct inode *inode,struct writeback_control *wbc) { -return __omfs_write_inode (inode, wbc->sync_mode ==WB_SYNC_ALL ); +return __omfs_write_inode (inode,1 /* XXX: fix fsync and use wbc->sync_mode == WB_SYNC_ALL */ ); }int omfs_sync_inode (struct inode *inode)
fs/reiserfs/inode.c +2 -0
@@ -1635,6 +1635,8 @@ int reiserfs_write_inode(struct inode *i** these cases are just when the system needs ram, not when the ** inode needs to reach disk for safety, and they can safely be ** ignored because the altered inode has already been logged. +** XXX: is this really OK? The caller clears the inode dirty bit, so +** a subsequent sync for integrity might never reach here. */ if (wbc->sync_mode ==WB_SYNC_ALL && !(current->flags &PF_MEMALLOC )) {reiserfs_write_lock (inode->i_sb);
fs/sysv/inode.c +1 -1
@@ -286,7 +286,7 @@ static int __sysv_write_inode(struct inowrite3byte (sbi, (u8 *)&si->i_data[block], &raw_inode->i_data[3 *block]);mark_buffer_dirty (bh); -if (wait) { +if (1 /* XXX: fix fsync and use wait */ ) {sync_dirty_buffer (bh);if (buffer_req (bh) && !buffer_uptodate (bh)) { printk ("IO error syncing sysv inode [%s:%08x]\n",
fs/udf/inode.c +1 -1
@@ -1598,7 +1598,7 @@ static int udf_update_inode(struct inode/* write the data blocks */ mark_buffer_dirty (bh); -if (do_sync) { +if (1 /* XXX fix fsync and use do_sync */ ) {sync_dirty_buffer (bh);if (buffer_write_io_error (bh)) { printk(KERN_WARNING "IO error syncing udf inode "
fs/ufs/inode.c +1 -1
@@ -889,7 +889,7 @@ static int ufs_update_inode(struct inode }mark_buffer_dirty (bh); -if (do_sync) +if (1 /* XXX: fix fsync and use do_sync */ )sync_dirty_buffer (bh);brelse (bh);
Ted Ts'o
maintainerRe: [patch 5/8] fs: ext2 inode sync fix
Quoted 10 lines · Nick Piggin
There is a big fuckup with inode metadata writeback (I suspect in a lot of filesystems, but I've only dared to look at a couple as yet).
ext2 relies on ->write_inode being called from
sync_inode_metadatain fsync in order to sync the inode. HoweverI_DIRTY_SYNCgets cleared after a call to this guy, and it doesn't actually write back and wait on the inode block unless it is called for sync. This means thatwrite_inodefrom background writeback can kill the inode dirty bits without the data getting to disk. Fsync will subsequently miss it.
Quoted 6 lines
The fix is for ->write_inode to dirty the buffers/cache, and then ->fsync to write back the dirty data. In the full filesystem sync case, buffercache writeback in the generic code will write back the dirty data. Other filesystems could use ->sync_fs for this.
Signed-off-by Nick Piggin <[email protected]>
What are your plans for this patch? Is this something you were planning on pushing during the merge window, or should file system maintainers try to fix up their code, since I gather this particular change is independent of the previous patches in this patchset. Is that correct?
- Ted
PATCH 6/8 fs: fsync optimisations
In reply to Nick PigginOptimise fsync by adding a datasync parameter to sync_inode_metadata to DTRT with writing back the inode (->write_inode in theory should have a datasync parameter too perhaps, but that's for another time).
Also, implement the metadata sync optimally rather than reusing the normal data writeback path. This means less useless moving the inode around the writeback lists, and less dropping and retaking of inode_lock, and avoiding the data writeback call with nr_pages == 0.
drivers/staging/pohmelfs/inode.c +1 -1
@@ -883,7 +883,7 @@ static int pohmelfs_fsync(struct file *f {struct inode *inode = file->f_mapping->host; -return sync_inode_metadata (inode,1 ); +return sync_inode_metadata (inode, datasync,1 ); } ssize_t pohmelfs_write(struct file *file, const char __user *buf,
fs/exofs/file.c +1 -1
@@ -48,7 +48,7 @@ static int exofs_file_fsync(struct filestruct inode *inode = filp->f_mapping->host;struct super_block *sb; - ret =sync_inode_metadata (inode,1 ); + ret =sync_inode_metadata (inode, datasync,1 );/* This is a good place to write the sb */ /* TODO: Sechedule an sb-sync on create */
fs/ext2/dir.c +1 -1
@@ -98,7 +98,7 @@ static int ext2_commit_chunk(struct pageif (IS_DIRSYNC (dir)) { err =write_one_page (page,1 );if (!err) - err =sync_inode_metadata (dir,1 ); + err =sync_inode_metadata (dir,0 ,1 ); }else {unlock_page (page); }
fs/ext2/inode.c +1 -1
@@ -1203,7 +1203,7 @@ static int ext2_setsize(struct inode *in inode->i_mtime = inode->i_ctime =CURRENT_TIME_SEC ;if (inode_needs_sync (inode)) {sync_mapping_buffers (inode->i_mapping); -sync_inode_metadata (inode,1 ); +sync_inode_metadata (inode,0 ,1 ); }else {mark_inode_dirty (inode); }
fs/ext2/xattr.c +1 -1
@@ -699,7 +699,7 @@ ext2_xattr_set2(struct inode *inode, strEXT2_I (inode)->i_file_acl = new_bh ? new_bh->b_blocknr :0 ; inode->i_ctime =CURRENT_TIME_SEC ;if (IS_SYNC (inode)) { - error =sync_inode_metadata (inode,1 ); + error =sync_inode_metadata (inode,0 ,1 );/* In case sync failed due to ENOSPC the inode was actually * written (only some dirty data were not) so we just proceed * as if nothing happened and cleanup the unused block */
fs/fs-writeback.c +40 -3
60 lines not on this page · raw message
fs/libfs.c +1 -1
@@ -895,7 +895,7 @@ int generic_file_fsync(struct file *fileint ret; ret =sync_mapping_buffers (inode->i_mapping); - err =sync_inode_metadata (inode,1 ); + err =sync_inode_metadata (inode, datasync,1 );if (ret ==0 ) ret = err;return ret;
fs/nfsd/vfs.c +1 -1
@@ -287,7 +287,7 @@ commit_metadata(struct svc_fh *fhp)if (export_ops->commit_metadata)return export_ops->commit_metadata (inode); -return sync_inode_metadata (inode,1 ); +return sync_inode_metadata (inode,0 ,1 ); }/*
include/linux/fs.h +1 -1
@@ -1765,7 +1765,7 @@ static inline void file_accessed(struct }int sync_inode (struct inode *inode,struct writeback_control *wbc); -int sync_inode_metadata (struct inode *inode,int wait); +int sync_inode_metadata (struct inode *inode,int datasync,int wait);int inode_writeback_begin (struct inode *inode,int wait);int inode_writeback_end (struct inode *inode);
PATCH 7/8 fs: fix or note I_DIRTY handling bugs in filesystems
In reply to Nick PigginChecking I_DIRTY* bits is racy unless we're under I_SYNC, because there might be a concurrent writeback thread that has cleared I_DIRTY, but has not yet completed the ->write_inode call.
This means our (typically integrity/fsync) operation can finish before previously dirty data has been written safely to disk.
Solve it by exporting inode_writeback_begin/end to filesystems, and have them use that before checking dirty flags, where it matters.
I'm not thrilled at exporting inode_lock, however I didn't see a good way to make that code fully generic (eg. some cases want to avoid taking and dropping locks if there is nothing to do, future users might want to be smarter about dirty bits and keep their own inode dirty bits in synch under the same lock, etc).
Not signed off yet
drivers/staging/pohmelfs/inode.c +1 -0
@@ -373,6 +373,7 @@ static int pohmelfs_write_inode_create_cdprintk ("%s: parent: %llu, ino: %llu, inode: %p.\n" , __func__, parent->ino, n->ino, inode); +/* XXX: is this race WRT writeback? */ if (inode && (inode->i_state &I_DIRTY )) {struct pohmelfs_inode *pi =POHMELFS_I (inode);pohmelfs_write_create_inode (pi);
fs/gfs2/file.c +28 -6
@@ -557,7 +557,7 @@ static int gfs2_close(struct inode *inodstatic int gfs2_fsync (struct file *file,int datasync) {struct inode *inode = file->f_mapping->host; -int sync_state = inode->i_state & (I_DIRTY_SYNC |I_DIRTY_DATASYNC ); +unsigned dirty, mask;int ret =0 ;if (gfs2_is_jdata (GFS2_I (inode))) {
@@ -565,13 +565,35 @@ static int gfs2_fsync(struct file *file,return 0 ; } -if (sync_state !=0 ) { -if (!datasync) - ret =write_inode_now (inode,0 ); +spin_lock (&inode_lock); +inode_writeback_begin (inode,1 ); -if (gfs2_is_stuffed (GFS2_I (inode))) -gfs2_log_flush (GFS2_SB (inode),GFS2_I (inode)->i_gl); +if (datasync) + mask =I_DIRTY_DATASYNC ; +else + mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; + dirty = inode->i_state & mask; + inode->i_state &= ~mask; +if (dirty) { +spin_unlock (&inode_lock); + +if (!datasync) { +struct writeback_control wbc = { + .sync_mode =WB_SYNC_ALL , + }; + ret = inode->i_sb->s_op->write_inode (inode, &wbc); + }else { +if (gfs2_is_stuffed (GFS2_I (inode))) +gfs2_log_flush (GFS2_SB (inode), +GFS2_I (inode)->i_gl); + } + +spin_lock (&inode_lock); } +if (ret) + inode->i_state |= dirty; +inode_writeback_end (inode); +spin_unlock (&inode_lock);return ret; }
fs/jffs2/fs.c +1 -0
@@ -361,6 +361,7 @@ void jffs2_dirty_inode(struct inode *ino {struct iattr iattr; +/* XXX: huh? How does this make sense? */ if (!(inode->i_state &I_DIRTY_DATASYNC )) {D2 (printk (KERN_DEBUG "jffs2_dirty_inode() not calling setattr() for ino #%lu\n" , inode->i_ino));return ;
fs/jfs/file.c +23 -6
@@ -19,6 +19,7 @@#include <linux/mm.h> #include <linux/fs.h> +#include <linux/writeback.h> #include <linux/quotaops.h> #include "jfs_incore.h" #include "jfs_inode.h"
@@ -31,18 +32,34 @@int jfs_fsync (struct file *file,int datasync) {struct inode *inode = file->f_mapping->host; -int rc =0 ; +unsigned dirty, mask; +int err =0 ; -if (!(inode->i_state &I_DIRTY ) || - (datasync && !(inode->i_state &I_DIRTY_DATASYNC ))) { +spin_lock (&inode_lock); +inode_writeback_begin (inode,1 ); + +if (datasync) + mask =I_DIRTY_DATASYNC ; +else + mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; + dirty = inode->i_state & mask; + inode->i_state &= ~mask; +spin_unlock (&inode_lock); + +if (!dirty) {/* Make sure committed changes hit the disk */ jfs_flush_journal (JFS_SBI (inode->i_sb)->log,1 ); -return rc; + }else { + err =jfs_commit_inode (inode,1 ); } - rc |=jfs_commit_inode (inode,1 ); +spin_lock (&inode_lock); +if (err) + inode->i_state |= dirty; +inode_writeback_end (inode); +spin_unlock (&inode_lock); -return rc ? -EIO :0 ; +return err ? -EIO :0 ; }static int jfs_open (struct inode *inode,struct file *file)
fs/nfsd/vfs.c +3 -4
@@ -969,10 +969,9 @@ static int wait_for_concurrent_writes(stdprintk ("nfsd: write resume %d\n" ,task_pid_nr (current)); } -if (inode->i_state &I_DIRTY ) { -dprintk ("nfsd: write sync %d\n" ,task_pid_nr (current)); - err =vfs_fsync (file,0 ); - } +dprintk ("nfsd: write sync %d\n" ,task_pid_nr (current)); + err =vfs_fsync (file,0 ); + last_ino = inode->i_ino; last_dev = inode->i_sb->s_dev;return err;
fs/ocfs2/file.c +19 -1
@@ -176,12 +176,24 @@ static int ocfs2_sync_file(struct file *journal_t *journal;struct inode *inode = file->f_mapping->host;struct ocfs2_super *osb =OCFS2_SB (inode->i_sb); +unsigned dirty, mask;mlog_entry ("(0x%p, %d, 0x%p, '%.*s')\n" , file, datasync, file->f_path.dentry, file->f_path.dentry->d_name.len, file->f_path.dentry->d_name.name); -if (datasync && !(inode->i_state &I_DIRTY_DATASYNC )) { +spin_lock (&inode_lock); +inode_writeback_begin (inode,1 ); +if (datasync) + mask =I_DIRTY_DATASYNC ; +else + mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; + dirty = inode->i_state & mask; + inode->i_state &= ~mask; +spin_unlock (&inode_lock); + +if (datasync && dirty) { +/* * We still have to flush drive's caches to get data to the * platter
@@ -195,6 +207,12 @@ static int ocfs2_sync_file(struct file * err =jbd2_journal_force_commit (journal); bail: +spin_lock (&inode_lock); +if (err) + inode->i_state |= dirty; +inode_writeback_end (inode); +spin_unlock (&inode_lock); +mlog_exit (err);return (err <0 ) ? -EIO :0 ;
fs/ubifs/file.c +3 -5
@@ -1313,11 +1313,9 @@ int ubifs_fsync(struct file *file, int d* VFS has already synchronized dirty pages for this inode. Synchronize * the inode unless this is a 'datasync()' call. */ -if (!datasync || (inode->i_state &I_DIRTY_DATASYNC )) { - err = inode->i_sb->s_op->write_inode (inode,NULL ); -if (err) -return err; - } + err =sync_inode_metadata (inode, datasync,1 ); +if (err) +return err;/* * Nodes related to this inode may still sit in a write-buffer. Flush
fs/ufs/truncate.c +1 -1
@@ -479,7 +479,7 @@ int ufs_truncate(struct inode *inode, lo retry |=ufs_trunc_tindirect (inode);if (!retry)break ; -if (IS_SYNC (inode) && (inode->i_state &I_DIRTY )) +if (IS_SYNC (inode))ufs_sync_inode (inode);blk_run_address_space (inode->i_mapping);yield ();
fs/xfs/linux-2.6/xfs_file.c +29 -13
72 lines not on this page · raw message
fs/inode.c +1 -0
@@ -82,6 +82,7 @@ static struct hlist_head *inode_hashtabl* the i_state of an inode while it is in use.. */ DEFINE_SPINLOCK (inode_lock); +EXPORT_SYMBOL (inode_lock);/* * iprune_sem provides exclusion between the kswapd or try_to_free_pages
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
As mentioned last round I think the exporting of inode_lock and pusing of the I_DIRTY* complexities into the filesystems can be avoided. See the patch below, which compiles and passes xfstests for xfs, but otherwise isn't quite done yet. The only code change vs the opencoded variant in the patch is that we do a useless inode_lock roundtrip for a non-dirty inode on gfs2, which is I think is acceptable, especially once we have the lock split anyway.
The other thing I don't like yet is passing the datasync flag - the callback shouldn't care about what we were called with, but rather about which bits it needs to sync out - which the dirty flag already tells us about.
IMHO the behaviour in ocfs2 and gfs2 that relies on it is plain wrong:
- ocfs2 really should always force the journal if any bit we care about in the inode is dirty, and only do the pure cache flush is nothing we care about is dirty (similar to the more complex code in XFS)
- gfs2 seems really weird. Doesn't it need to do any log force if an inode has a pending transaction? Currently it only does for stuffed inodes, and if datasync was set, which seems weird. Also I can't see why we'd never want to call into ->write_inode to write out the inode for the datasync case - except for not catching timestamp updates fdatasync really isn't any different from fsync.
fs/fs-writeback.c +44 -2
@@ -315,7 +315,7 @@ static void inode_wait_for_writeback(str* If inode_writeback_begin returns 1, it must be followed by a call to * inode_writeback_end. */ -int inode_writeback_begin (struct inode *inode,int wait) +static int inode_writeback_begin (struct inode *inode,int wait) {assert_spin_locked (&inode_lock);if (!atomic_read (&inode->i_count))
@@ -356,7 +356,7 @@ EXPORT_SYMBOL(inode_writeback_begin);* inode_writeback_end must follow a successful call to inode_writeback_begin * after we have finished submitting writeback to the inode. */ -int inode_writeback_end (struct inode *inode) +static int inode_writeback_end (struct inode *inode) {int ret =1 ;
@@ -1362,3 +1362,45 @@ out:return ret; }EXPORT_SYMBOL (sync_inode_metadata); + +/** +* fsync_helper - helper to implement writeback-coherent fsync +* @inode: inode to sync +* @datasync: only synchronize essential metadata if true +* @sync_inode: actual fsync implementation +* +* This function should be used by all filesystems implementing that +* use VFS writeback and implement a complex fsync method that does not +* use sync_inode_metadata(). It ensures correct participation in the +* writeback locking protocol, and leaves all real work to the @sync_inode +* callback. Note that the callback is also called if the inode wasn't +* found to be dirty - it's dirty argument contains the exact dirty bits +* found in the inode before releasing the inode lock. +*/ +int fsync_helper (struct inode *inode,int datasync, +int (*sync_inode)(struct inode *inode,int datasync,int dirty)) +{ +unsigned dirty, mask; +int ret =0 ; + +spin_lock (&inode_lock); +inode_writeback_begin (inode,1 ); + +if (datasync) + mask =I_DIRTY_DATASYNC ; +else + mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; + dirty = inode->i_state & mask; + inode->i_state &= ~mask; +spin_unlock (&inode_lock); + + ret =sync_inode (inode, datasync, dirty); + +spin_lock (&inode_lock); +if (ret) + inode->i_state |= dirty; +inode_writeback_end (inode); +spin_unlock (&inode_lock); +return ret; +} +EXPORT_SYMBOL (fsync_helper);
fs/gfs2/file.c +16 -39
78 lines not on this page · raw message
fs/jfs/file.c +7 -24
@@ -29,37 +29,20 @@#include "jfs_acl.h" #include "jfs_debug.h" -int jfs_fsync (struct file *file,int datasync) +static int jfs_fsync_int (struct inode *inode,int datasync,int dirty) { -struct inode *inode = file->f_mapping->host; -unsigned dirty, mask; -int err =0 ; - -spin_lock (&inode_lock); -inode_writeback_begin (inode,1 ); - -if (datasync) - mask =I_DIRTY_DATASYNC ; -else - mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; - dirty = inode->i_state & mask; - inode->i_state &= ~mask; -spin_unlock (&inode_lock); -if (!dirty) {/* Make sure committed changes hit the disk */ jfs_flush_journal (JFS_SBI (inode->i_sb)->log,1 ); - }else { - err =jfs_commit_inode (inode,1 ); +return 0 ; } -spin_lock (&inode_lock); -if (err) - inode->i_state |= dirty; -inode_writeback_end (inode); -spin_unlock (&inode_lock); +return jfs_commit_inode (inode,1 ) ? -EIO :0 ; +} -return err ? -EIO :0 ; +int jfs_fsync (struct file *file,int datasync) +{ +return fsync_helper (file->f_mapping->host, datasync, jfs_fsync_int); }static int jfs_open (struct inode *inode,struct file *file)
include/linux/fs.h +2 -2
@@ -1766,8 +1766,8 @@ static inline void file_accessed(structint sync_inode (struct inode *inode,struct writeback_control *wbc);int sync_inode_metadata (struct inode *inode,int datasync,int wait); -int inode_writeback_begin (struct inode *inode,int wait); -int inode_writeback_end (struct inode *inode); +int fsync_helper (struct inode *inode,int datasync, +int (*sync_inode)(struct inode *inode,int datasync,int dirty));struct file_system_type {const char *name;
fs/ocfs2/file.c +7 -33
@@ -170,52 +170,26 @@ static int ocfs2_dir_release(struct inodreturn 0 ; } -static int ocfs2_sync_file (struct file *file,int datasync) +static int ocfs2_fsync_int (struct inode *inode,int datasync,int dirty) { -int err =0 ; -journal_t *journal; -struct inode *inode = file->f_mapping->host;struct ocfs2_super *osb =OCFS2_SB (inode->i_sb); -unsigned dirty, mask; - -mlog_entry ("(0x%p, %d, 0x%p, '%.*s')\n" , file, datasync, - file->f_path.dentry, file->f_path.dentry->d_name.len, - file->f_path.dentry->d_name.name); - -spin_lock (&inode_lock); -inode_writeback_begin (inode,1 ); -if (datasync) - mask =I_DIRTY_DATASYNC ; -else - mask =I_DIRTY_SYNC |I_DIRTY_DATASYNC ; - dirty = inode->i_state & mask; - inode->i_state &= ~mask; -spin_unlock (&inode_lock);if (datasync && dirty) { -/* * We still have to flush drive's caches to get data to the * platter */ if (osb->s_mount_opt &OCFS2_MOUNT_BARRIER )blkdev_issue_flush (inode->i_sb->s_bdev,GFP_KERNEL ,NULL ); - goto bail; +return 0 ; } - journal = osb->journal->j_journal; - err =jbd2_journal_force_commit (journal); - -bail: -spin_lock (&inode_lock); -if (err) - inode->i_state |= dirty; -inode_writeback_end (inode); -spin_unlock (&inode_lock); - -mlog_exit (err); +return jbd2_journal_force_commit (osb->journal->j_journal) ? -EIO :0 ; +} -return (err <0 ) ? -EIO :0 ; +static int ocfs2_sync_file (struct file *file,int datasync) +{ +return fsync_helper (file->f_mapping->host, datasync, ocfs2_fsync_int); } int ocfs2_should_update_atime(struct inode *inode,
fs/xfs/linux-2.6/xfs_file.c +21 -30
83 lines not on this page · raw message
fs/inode.c +0 -1
@@ -82,7 +82,6 @@ static struct hlist_head *inode_hashtabl* the i_state of an inode while it is in use.. */ DEFINE_SPINLOCK (inode_lock); -EXPORT_SYMBOL (inode_lock);/* * iprune_sem provides exclusion between the kswapd or try_to_free_pages
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Hi,
Quoted 15 lines · Christoph Hellwig
As mentioned last round I think the exporting of
inode_lockand pusing of the I_DIRTY* complexities into the filesystems can be avoided. See the patch below, which compiles and passes xfstests for xfs, but otherwise isn't quite done yet. The only code change vs the opencoded variant in the patch is that we do a uselessinode_lockroundtrip for a non-dirty inode on gfs2, which is I think is acceptable, especially once we have the lock split anyway.The other thing I don't like yet is passing the datasync flag - the callback shouldn't care about what we were called with, but rather about which bits it needs to sync out - which the dirty flag already tells us about.
IMHO the behaviour in ocfs2 and gfs2 that relies on it is plain wrong:
Quoted 10 lines
- ocfs2 really should always force the journal if any bit we care about in the inode is dirty, and only do the pure cache flush is nothing we care about is dirty (similar to the more complex code in XFS)
- gfs2 seems really weird. Doesn't it need to do any log force if an inode has a pending transaction? Currently it only does for stuffed inodes, and if datasync was set, which seems weird. Also I can't see why we'd never want to call into ->write_inode to write out the inode for the datasync case - except for not catching timestamp updates fdatasync really isn't any different from fsync.
The algorithm was intended to be:
- With "journaled data" files
- Do a log flush conditional upon the inode's glock
- The core code then writes back any dirty pages
- With regular files/directories
- If datasync is not set, we need to write back the metadata including
timestamp updates, so that is done via ->write_inode. Note that an extra complication here is that we need to get the glock on the inode if we don't already have it in order to check and conditionally update the atime.The call to ->write_inode includes an implicit (conditional) log flush.
- If datasync is set, we assume that only the data pages need to be
written out. My understanding of datasync was that it was only supposed to write out data and never any of the metadata. The reason for the call to flush the log for "stuffed" files is that the data shares a disk block with the inode metadata, so we cannot avoid the log flush in this case, since we must unpin the block to write it back.
There is something strange going on here though since there should be a metadata sync included as well I think - I'm just working back through the changes to see where that was lost at the moment,
Steve.
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 4 lines · Steven Whitehouse
- With "journaled data" files
- Do a log flush conditional upon the inode's glock
- The core code then writes back any dirty pages
Any data writeback is done before calling ->fsync.
Quoted 5 lines
- With regular files/directories
- If datasync is not set, we need to write back the metadata including
timestamp updates, so that is done via ->write_inode. Note that an extra complication here is that we need to get the glock on the inode if we don't already have it in order to check and conditionally update the
Quoted 8 lines
atime.The call to ->write_inode includes an implicit (conditional) log flush.
- If datasync is set, we assume that only the data pages need to be
written out. My understanding of datasync was that it was only supposed to write out data and never any of the metadata. The reason for the call to flush the log for "stuffed" files is that the data shares a disk block with the inode metadata, so we cannot avoid the log flush in this case, since we must unpin the block to write it back.
What happens to indirect blocks, inode size updates, etc? In general the only correct form to use the datasync argument is along the lines of:
if ((inode->i_state & I_DIRTY_DATASYNC) ||
((inode->i_state & I_DIRTY_SYNC) && !datasync)) {
/* write out the inode */
} else {
/*
* VFS inode not dirty, no need to write it out.
*
* If the filesystem support asynchronous inode writes,
* we may have to wait for them here.
*/
}or rather mostly correct, as pointed out by Nick in this series, that's why the above gets replaced with an equivalent check that also participates in the writeback locking protocol in this series.
For gfs2 on current mainline an fsync respecting that would look like:
static int gfs2_fsync(struct file *file, int datasync)
{
struct inode *inode = file->f_mapping->host;
struct gfs2_inode *ip = GFS2_I(inode);
int ret = 0;
if (gfs2_is_jdata(ip) {
gfs2_log_flush(GFS2_SB(inode), ip);
return 0;
}
if ((inode->i_state & I_DIRTY_DATASYNC) ||
((inode->i_state & I_DIRTY_SYNC) && !datasync))
sync_inode_metadata(inode, 1);
else if (gfs2_is_stuffed(ip))
gfs2_log_flush(GFS2_SB(inode), ip->i_gl);
}Note that the asynchronous write_inode_now is replaced with a sync_inode_metadata, which doesn't incorrectly write data again, and makes sure we do a synchronous write.
I'm still not quite sure how the gfs2_log_flush are supposed to work. What's the reason we don't need the ->write_inode call for journaled data mode? Also is it guaranteed that we might not have an asynchronous transaction that update the inode in the log, e.g. why doesn't gfs2 need some sort of log flush even if the VFS inode is not dirty, unlike most other journaled filesystems.
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 38 lines · Christoph Hellwig
Quoted 4 lines · Steven Whitehouse
- With "journaled data" files
- Do a log flush conditional upon the inode's glock
- The core code then writes back any dirty pagesAny data writeback is done before calling ->fsync.
Quoted 5 lines
- With regular files/directories
- If datasync is not set, we need to write back the metadata including
timestamp updates, so that is done via ->write_inode. Note that an extra complication here is that we need to get the glock on the inode if we don't already have it in order to check and conditionally update theQuoted 8 lines
atime.The call to ->write_inode includes an implicit (conditional) log flush.
- If datasync is set, we assume that only the data pages need to be
written out. My understanding of datasync was that it was only supposed to write out data and never any of the metadata. The reason for the call to flush the log for "stuffed" files is that the data shares a disk block with the inode metadata, so we cannot avoid the log flush in this case, since we must unpin the block to write it back.What happens to indirect blocks, inode size updates, etc? In general the only correct form to use the datasync argument is along the lines of:
if ((inode->i_state & I_DIRTY_DATASYNC) || ((inode->i_state & I_DIRTY_SYNC) && !datasync)) { /* write out the inode */ } else { /* * VFS inode not dirty, no need to write it out. * * If the filesystem support asynchronous inode writes, * we may have to wait for them here. */ }
Quoted 3 lines
or rather mostly correct, as pointed out by Nick in this series, that's why the above gets replaced with an equivalent check that also participates in the writeback locking protocol in this series.
Just to recap, basically we have 2 main problems in vfs/filesystems:
- i_state dirtyness is checked outside the correct synchronization protcol, so it may be seen as clean before a concurrent writer has finished.
- .write_inode is only guaranteed to be called once regardless of sync or async mode, for a dirty inode at a sync point. Many filesystems were incorrectly assuming they would be called once *in synchronous mode*.
The optimal approach for .write_inode seems to be clean the struct inode so that it may be eventually reclaimed. Then have your .fsync and .sync_fs implementations enforce the actual data integrity.
Note that "clean struct inode" often means to copy the metadata somewhere else to be scheduled for asynch writeout. You have to be careful to note that if you allow the inode to be evicted at this point without data integrity point also in .evict_inode, then you need to keep in mind that .sync_fs (and subsequent .fsync, if the inode is re opened) need to still enforce integrity for these potentially evicted inodes.
Everyone happy with this? Please review your filesystems and look at my patches :)
Thanks,
Nick
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
In reply to Christoph HellwigHi,
Quoted 38 lines · Christoph Hellwig
Quoted 4 lines · Steven Whitehouse
- With "journaled data" files
- Do a log flush conditional upon the inode's glock
- The core code then writes back any dirty pagesAny data writeback is done before calling ->fsync.
Quoted 5 lines
- With regular files/directories
- If datasync is not set, we need to write back the metadata including
timestamp updates, so that is done via ->write_inode. Note that an extra complication here is that we need to get the glock on the inode if we don't already have it in order to check and conditionally update theQuoted 8 lines
atime.The call to ->write_inode includes an implicit (conditional) log flush.
- If datasync is set, we assume that only the data pages need to be
written out. My understanding of datasync was that it was only supposed to write out data and never any of the metadata. The reason for the call to flush the log for "stuffed" files is that the data shares a disk block with the inode metadata, so we cannot avoid the log flush in this case, since we must unpin the block to write it back.What happens to indirect blocks, inode size updates, etc? In general the only correct form to use the datasync argument is along the lines of:
if ((inode->i_state & I_DIRTY_DATASYNC) || ((inode->i_state & I_DIRTY_SYNC) && !datasync)) { /* write out the inode */ } else { /* * VFS inode not dirty, no need to write it out. * * If the filesystem support asynchronous inode writes, * we may have to wait for them here. */ }
Quoted 4 lines
or rather mostly correct, as pointed out by Nick in this series, that's why the above gets replaced with an equivalent check that also participates in the writeback locking protocol in this series.
Yes, that looks much better than what we have at the moment.
Quoted 24 lines
For gfs2 on current mainline an fsync respecting that would look like:
static int
gfs2_fsync(struct file *file, int datasync){ struct inode *inode = file->f_mapping->host; struct gfs2_inode *ip = GFS2_I(inode); int ret = 0; if (gfs2_is_jdata(ip) { gfs2_log_flush(GFS2_SB(inode), ip); return 0; } if ((inode->i_state & I_DIRTY_DATASYNC) || ((inode->i_state & I_DIRTY_SYNC) && !datasync)) sync_inode_metadata(inode, 1); else if (gfs2_is_stuffed(ip)) gfs2_log_flush(GFS2_SB(inode), ip->i_gl); }Note that the asynchronous
write_inode_nowis replaced with async_inode_metadata, which doesn't incorrectly write data again, and makes sure we do a synchronous write.
Quoted 7 lines
I'm still not quite sure how the
gfs2_log_flushare supposed to work. What's the reason we don't need the ->write_inode call for journaled data mode? Also is it guaranteed that we might not have an asynchronous transaction that update the inode in the log, e.g. why doesn't gfs2 need some sort of log flush even if the VFS inode is not dirty, unlike most other journaled filesystems.
I think that has just been missed due to the way in which the code has developed. It appears to be needed to me, but originally all the timestamp updates were handled internally by GFS2 and in a synchronous manner, so that there was no need for ->write_inode() in that case. I think that needs to be added now the vfs looks after atime updates though, in order to be correct.
After the log flush there should also be a write on the metadata mapping as per the inode_go_sync() function which is very similar (but not quite similar enough to use the same code, I think) function,
Steve.
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
In reply to Christoph HellwigQuoted 2 lines · Christoph Hellwig
As mentioned last round I think the exporting of
inode_lockand pusing of the I_DIRTY* complexities into the filesystems can be avoided. See
Yes I did see that, I hoped to continue discussion of that detail.
Let me start out by saying OK I will agree to hold off that change until inode_lock is removed at least, and concentrate on just the fixes.
However I strongly believe that filesystems should be able to access and manipulate the inode dirty state directly. If you agree with that, then I think they should be able to access the lock required for that. Filesystems will want to keep their internal state in synch with vfs visible state most likely (eg. like your hfsplus patches), and _every_ time we do "loose" coupling between state bits like this (eg. page and buffer state; page and pte state; etc), it turns out to be a huge mess of races and subtle code and ordering.
Quoted 3 lines
the patch below, which compiles and passes xfstests for xfs, but otherwise isn't quite done yet. The only code change vs the opencoded variant in the patch is that we do a useless
inode_lockroundtrip
I dislike this style, except where it has some real advantages like get_block case. I prefer just to make the existing inode_writeback_begin into a "__special" variant, and make inode_writeback_begin just do the locking and masking for filesystems.
Quoted 2 lines
for a non-dirty inode on gfs2, which is I think is acceptable, especially once we have the lock split anyway.
The bigger issue IMO is if filesystems want to be smarter with dirty bit handling and keep more internal state in sync with it. I don't see any problem at all with allowing them to lock the dirty state. (but will hold off the patch for now, as said).
Thanks,
Nick
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 8 lines · Nick Piggin
However I strongly believe that filesystems should be able to access and manipulate the inode dirty state directly. If you agree with that, then I think they should be able to access the lock required for that. Filesystems will want to keep their internal state in synch with vfs visible state most likely (eg. like your hfsplus patches), and _every_ time we do "loose" coupling between state bits like this (eg. page and buffer state; page and pte state; etc), it turns out to be a huge mess of races and subtle code and ordering.
I've probably done the two most complicated fsync implementations in xfs and hfsplys myself, and I'd really prefer the interface to be as simple as possible. The way the I_DIRTY_* flags and the datasync parameter to ->fsync interact are almost a receipe for getting it wrong, which in fact most implementations that tried to be smart did. See gfs2 and ocfs2 comments in this threads for classic examples.
If we actually get filesystems that need to do smarts in checking/clearing the I_DIRTY_* flag we can discuss proper interfaces for it - duplicating guts of i_state manipulations sounds like a relatively bad idea for that. Note that my fsync_helper still gives filesystems a lot of information, just not in a redundant and confusing way. The dirty argument still tells if datasync-relevant or non-datasyns relevant (aka timestampts) metadata was dirty, so if a filesystem needs to write them back in different ways it still can.
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Nick Piggin
However I strongly believe that filesystems should be able to access and manipulate the inode dirty state directly. If you agree with that, then I think they should be able to access the lock required for that. Filesystems will want to keep their internal state in synch with vfs visible state most likely (eg. like your hfsplus patches), and _every_ time we do "loose" coupling between state bits like this (eg. page and buffer state; page and pte state; etc), it turns out to be a huge mess of races and subtle code and ordering.
Quoted 3 lines
I've probably done the two most complicated fsync implementations in xfs and hfsplys myself, and I'd really prefer the interface to be as simple as possible.
Agree and as I said I'll change inode_writeback_begin/end to do the locking and just return the dirty bit mask etc.
Quoted 4 lines
The way the I_DIRTY_* flags and the datasync parameter to
->fsync interact are almost a receipe for getting it wrong, which in fact most implementations that tried to be smart did. See gfs2 and ocfs2 comments in this threads for classic examples.
Right if you want a helper to get the correct mask of bits required that's fine and I agree, but locking is a different issue too: if filesystems are trying to keep private state in sync with vfs state, then they _need_ to do it properly with the proper locking. I think your hfsplus implementation had a bug or two in this area didn't it? (although I ended up getting side tracked with all these bugs half way through looking at that).
Quoted 4 lines
If we actually get filesystems that need to do smarts in checking/clearing the I_DIRTY_* flag we can discuss proper interfaces for it - duplicating guts of
i_statemanipulations sounds like a relatively bad idea for that.
I disagree, but we'll explore it further later.
It is a couple of lines to check and clear dirty bits. Not rocket science and I think it is far better to make it explicit what is happening to the filesystem. The disconnect between what the vfs is doing and what the filesystems throught should be happening is what caused all these bugs to start with.
Anyway, long story short, I'll drop the inode_lock export and move the locking and manipulation into inode_writeback_begin/end for now.
Thanks,
Nick
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 7 lines · Nick Piggin
Right if you want a helper to get the correct mask of bits required that's fine and I agree, but locking is a different issue too: if filesystems are trying to keep private state in sync with vfs state, then they _need_ to do it properly with the proper locking. I think your hfsplus implementation had a bug or two in this area didn't it? (although I ended up getting side tracked with all these bugs half way through looking at that).
It's not locking, but ordering that was the issue. It's an issue caused by the VFS interfaces, but not really related to the area you work on currently. If ->dirty_inode told us what was dirtied it would be a lot simpler. Alternatively we should just stop requiring filesystems to participate in the I_DIRTY_SYNC/DATASYNC protocol. In general it's much easier for the filesystem to keep that state by itself in proper granularity. But I lost the fight to have the timestamp updates go through proper methods instead of just writing into the VFS inode and marking the inode dirty long ago, so we'll have to live with it.
Re: [patch 7/8] fs: fix or note I_DIRTY handling bugs in filesystems
Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Nick Piggin
Right if you want a helper to get the correct mask of bits required that's fine and I agree, but locking is a different issue too: if filesystems are trying to keep private state in sync with vfs state, then they _need_ to do it properly with the proper locking. I think your hfsplus implementation had a bug or two in this area didn't it? (although I ended up getting side tracked with all these bugs half way through looking at that).
It's not locking, but ordering that was the issue. It's an issue caused
Right, but ordering should probably not be an issue if filesystem could manage the dirty state and locking by itself.
Quoted 2 lines
by the VFS interfaces, but not really related to the area you work on currently. If ->dirty_inode told us what was dirtied it would be
Well it's one side of the issue, the other side is indeed the dirtying side.
Quoted 6 lines
a lot simpler. Alternatively we should just stop requiring filesystems to participate in the I_DIRTY_SYNC/DATASYNC protocol. In general it's much easier for the filesystem to keep that state by itself in proper granularity. But I lost the fight to have the timestamp updates go through proper methods instead of just writing into the VFS inode and marking the inode dirty long ago, so we'll have to live with it.
I wouldn't mind revisiting that, once these correctness fixes are in (and yes that's another good reason to hold off with allowing filesystems to do the i_state locking just yet).
Allowing the filesystem to entirely manage the setting and clearing of dirty bits would be a good idea. Then just have a single bit that specifies they want background writeout to run a callback together when it does data writeout for that inode. Everything else would be done in the fs.
Page dirtying has similarly silly conventions like having the caller clear dirty before calling into ->writepage, which means the filesystem has to have hacks like redirty_page_for_io and has a hard time keeping page dirty state in sync with page private metadata. (completely different issue but similar general problem of having things half managed by one side and half by the other).
PATCH 8/8 fs: add i_op->sync_inode
In reply to Nick PigginAdd a sync_inode i_op, for filesystems that can fsync with the inode and not requiring the file (which is most of them), and use it in filesystems and nfsd to correctly sync things.
Get rid of sync_inode, unexport sync_inodes_sb and make it internal, remove the 'wait' parameter from sync_inode_metadata, and document these things a little better.
The core of the problem is that ->write_inode(inode, sync==1) is not guaranteed to have the inode metadata on disk. This is because filesystems that do asynchronous ->write_inode stuff don't "obey" sync==1. Thus, generic code cannot assume that ->write_inode is all that is required for a full sync. (filesystem specific code can, but that's a bit ugly I'd like to phase it out or clarify with different function names).
Not signed off yet
--- Documentation/filesystems/porting | 6 ++++ drivers/staging/pohmelfs/inode.c | 2 - fs/ext2/dir.c | 4 ++ fs/ext2/ext2.h | 4 +- fs/ext2/file.c | 32 +++------------------ fs/ext2/inode.c | 38 ++++++++++++++++++++++++- fs/ext2/namei.c | 1 fs/ext2/xattr.c | 4 ++ fs/fat/file.c | 33 ++++++++++------------ fs/fat/inode.c | 21 +++++--------- fs/fs-writeback.c | 23 --------------- fs/internal.h | 5 +++ fs/libfs.c | 57 ++++++++++++++++++++++++++++++++++++++ fs/nfs/write.c | 16 ++++------ fs/nfsd/vfs.c | 3 ++ fs/sync.c | 8 +++++ include/linux/fs.h | 12 +++++++- include/linux/writeback.h | 1 18 files changed, 172 insertions(+), 98 deletions(-)
fs/libfs.c +59 -1
@@ -880,6 +880,64 @@ struct dentry *generic_fh_to_parent(struEXPORT_SYMBOL_GPL (generic_fh_to_parent);/** +* generic_sync_inode - generic sync implementation for simple filesystems +* @inode: inode to synchronize +* @flags: INODE_SYNC_ flag +* @start: start range (if INODE_SYNC_DATA is set) +* @end: end range (if INODE_SYNC_DATA is set) +* @Returns: 0 on success, otherwise -errno. +* +* This is a generic implementation of the sync_inode method for simple +* filesystems which track all non-inode metadata in the buffers list +* hanging off the address_space structure. Their ->write_inode call must +* _always_ synchronously write back inode metadata (regardless if it was +* called for sync or async, because there is no other way to get it on +* disk). +* +* More advanced filesystems will want to always asynchronously dirty inode +* metadata in ->write_inode (regardless if it is sync or async), and then +* synchronously verify that it is on disk in their ->fsync, ->sync_inode, +* and ->sync_fs routines. generic_sync_inode could still be used in their +* sync_inode call, so long as they subsequently verify the metadata is on +* disk. +*/ +int generic_sync_inode (struct inode *inode,unsigned int flags, +loff_t start,loff_t end) +{ +struct address_space *mapping = inode->i_mapping; +int err, ret =0 ; + +if (flags &INODE_SYNC_DATA ) { + err =filemap_write_and_wait_range (mapping, start, end); +if (!ret) + ret = err; + } + +if (flags &INODE_SYNC_DATA_METADATA ) { +/* +* We need to protect against concurrent writers, which could +* cause livelocks in fsync_buffers_list(). +*/ +mutex_lock (&inode->i_mutex); + err =sync_mapping_buffers (mapping); +if (!ret) + ret = err; +mutex_unlock (&inode->i_mutex); + } + +if (flags & (INODE_SYNC_DATA_METADATA |INODE_SYNC_METADATA )) { +int datasync = !(flags &INODE_SYNC_METADATA ); + + err =sync_inode_metadata (inode, datasync); +if (!ret) + ret = err; + } + +return ret; +} +EXPORT_SYMBOL (generic_sync_inode); + +/** * generic_file_fsync - generic fsync implementation for simple filesystems * @file: file to synchronize * @datasync: only synchronize essential metadata if true
@@ -895,7 +953,7 @@ int generic_file_fsync(struct file *fileint ret; ret =sync_mapping_buffers (inode->i_mapping); - err =sync_inode_metadata (inode, datasync,1 ); + err =sync_inode_metadata (inode, datasync);if (ret ==0 ) ret = err;return ret;
include/linux/fs.h +14 -2
@@ -1545,6 +1545,16 @@ struct file_operations {int (*setlease)(struct file *,long ,struct file_lock **); }; +#define INODE_SYNC_DATA 0x01 +#define INODE_SYNC_DATA_METADATA 0x02 +#define INODE_SYNC_METADATA 0x04 + +#define INODE_SYNC_FSYNC (INODE_SYNC_DATA | \ + INODE_SYNC_DATA_METADATA | \ + INODE_SYNC_METADATA ) +#define INODE_SYNC_FDATASYNC (INODE_SYNC_DATA | \ + INODE_SYNC_DATA_METADATA) +struct inode_operations {int (*create) (struct inode *,struct dentry *,int ,struct nameidata *);struct dentry * (*lookup) (struct inode *,struct dentry *,struct nameidata *);
@@ -1573,6 +1583,7 @@ struct inode_operations { loff_t len);int (*fiemap)(struct inode *,struct fiemap_extent_info *,u64 start,u64 len); +int (*sync_inode)(struct inode *,unsigned int ,loff_t ,loff_t ); };struct seq_file ;
@@ -1764,10 +1775,11 @@ static inline void file_accessed(structtouch_atime (file->f_path.mnt, file->f_path.dentry); } -int sync_inode (struct inode *inode,struct writeback_control *wbc); -int sync_inode_metadata (struct inode *inode,int datasync,int wait); +int sync_inode_metadata (struct inode *inode,int datasync);int inode_writeback_begin (struct inode *inode,int wait);int inode_writeback_end (struct inode *inode); +int generic_sync_inode (struct inode *inode,unsigned int flags, +loff_t start,loff_t end);struct file_system_type {const char *name;
fs/ext2/ext2.h +2 -2
@@ -124,10 +124,10 @@ extern int ext2_get_block(struct inode *extern int ext2_setattr (struct dentry *,struct iattr *);extern void ext2_set_inode_flags (struct inode *inode);extern void ext2_get_inode_flags (struct ext2_inode_info *); -extern struct ext2_inode *ext2_get_inode (struct super_block *sb,ino_t ino, -struct buffer_head **p);extern int ext2_fiemap (struct inode *inode,struct fiemap_extent_info *fieinfo,u64 start,u64 len); +extern int ext2_sync_inode (struct inode *inode,unsigned int flags, +loff_t start,loff_t end);/* ioctl.c */ extern long ext2_ioctl (struct file *,unsigned int ,unsigned long );
fs/ext2/file.c +5 -27
@@ -43,35 +43,12 @@ static int ext2_release_file (struct inoint ext2_fsync (struct file *file,int datasync) { -int ret;struct inode *inode = file->f_mapping->host; -ino_t ino = inode->i_ino; -struct super_block *sb = inode->i_sb; -struct address_space *sb_mapping = sb->s_bdev->bd_inode->i_mapping; -struct buffer_head *bh; -struct ext2_inode *raw_inode; +unsigned int flags =INODE_SYNC_DATA ; +if (!datasync) + flags |=INODE_SYNC_METADATA ; - ret =generic_file_fsync (file, datasync); -if (ret == -EIO ||test_and_clear_bit (AS_EIO , &sb_mapping->flags)) { -/* We don't really know where the IO error happened... */ -ext2_error (sb, __func__, -"detected IO error when writing metadata buffers" ); -return -EIO ; - } - - raw_inode =ext2_get_inode (sb, ino, &bh); -if (IS_ERR (raw_inode)) -return -EIO ; - -sync_dirty_buffer (bh); -if (buffer_req (bh) && !buffer_uptodate (bh)) { -printk ("IO error syncing ext2 inode [%s:%08lx]\n" , - sb->s_id, (unsigned long ) ino); - ret = -EIO ; - } -brelse (bh); - -return ret; +return ext2_sync_inode (inode, flags,0 ,LLONG_MAX ); }/*
@@ -122,4 +99,5 @@ const struct inode_operations ext2_file_ .setattr = ext2_setattr, .check_acl = ext2_check_acl, .fiemap = ext2_fiemap, + .sync_inode = ext2_sync_inode, };
fs/ext2/inode.c +37 -3
@@ -1202,13 +1202,14 @@ static int ext2_setsize(struct inode *in inode->i_mtime = inode->i_ctime =CURRENT_TIME_SEC ;if (inode_needs_sync (inode)) { -sync_mapping_buffers (inode->i_mapping); -sync_inode_metadata (inode,0 ,1 ); + error = inode->i_op->sync_inode (inode, +INODE_SYNC_DATA_METADATA |INODE_SYNC_METADATA , +0 ,LLONG_MAX ); }else {mark_inode_dirty (inode); } -return 0 ; +return error; } struct ext2_inode *ext2_get_inode(struct super_block *sb, ino_t ino,
@@ -1515,6 +1516,39 @@ int ext2_write_inode(struct inode *inodereturn __ext2_write_inode (inode, wbc->sync_mode ==WB_SYNC_ALL ); } +int ext2_sync_inode (struct inode *inode,unsigned int flags, +loff_t start,loff_t end) +{ +int ret; +ino_t ino = inode->i_ino; +struct super_block *sb = inode->i_sb; +struct address_space *sb_mapping = sb->s_bdev->bd_inode->i_mapping; +struct buffer_head *bh; +struct ext2_inode *raw_inode; + + ret =generic_sync_inode (inode, flags, start, end); +if (ret == -EIO ||test_and_clear_bit (AS_EIO , &sb_mapping->flags)) { +/* We don't really know where the IO error happened... */ +ext2_error (sb, __func__, +"detected IO error when syncing inode" ); +return -EIO ; + } + + raw_inode =ext2_get_inode (sb, ino, &bh); +if (IS_ERR (raw_inode)) +return -EIO ; + +sync_dirty_buffer (bh); +if (buffer_req (bh) && !buffer_uptodate (bh)) { +printk ("IO error syncing ext2 inode [%s:%08lx]\n" , + sb->s_id, (unsigned long ) ino); + ret = -EIO ; + } +brelse (bh); + +return ret; +} +int ext2_setattr (struct dentry *dentry,struct iattr *iattr) {struct inode *inode = dentry->d_inode;
fs/ext2/namei.c +1 -0
@@ -418,6 +418,7 @@ const struct inode_operations ext2_dir_i#endif .setattr = ext2_setattr, .check_acl = ext2_check_acl, + .sync_inode = ext2_sync_inode, };const struct inode_operations ext2_special_inode_operations = {
fs/ext2/dir.c +3 -1
@@ -98,7 +98,9 @@ static int ext2_commit_chunk(struct pageif (IS_DIRSYNC (dir)) { err =write_one_page (page,1 );if (!err) - err =sync_inode_metadata (dir,0 ,1 ); + err = dir->i_op->sync_inode (dir, +INODE_SYNC_DATA_METADATA |INODE_SYNC_METADATA , +0 ,LLONG_MAX ); }else {unlock_page (page); }
fs/ext2/xattr.c +3 -1
@@ -699,7 +699,9 @@ ext2_xattr_set2(struct inode *inode, strEXT2_I (inode)->i_file_acl = new_bh ? new_bh->b_blocknr :0 ; inode->i_ctime =CURRENT_TIME_SEC ;if (IS_SYNC (inode)) { - error =sync_inode_metadata (inode,0 ,1 ); + error = inode->i_op->sync_inode (inode, +INODE_SYNC_METADATA , +0 ,LLONG_MAX );/* In case sync failed due to ENOSPC the inode was actually * written (only some dirty data were not) so we just proceed * as if nothing happened and cleanup the unused block */
fs/nfsd/vfs.c +4 -1
@@ -287,7 +287,10 @@ commit_metadata(struct svc_fh *fhp)if (export_ops->commit_metadata)return export_ops->commit_metadata (inode); -return sync_inode_metadata (inode,0 ,1 ); +if (inode->i_op->sync_inode) +return inode->i_op->sync_inode (inode,INODE_SYNC_METADATA , +0 ,0 ); +return sync_inode_metadata (inode,0 ); }/*
fs/sync.c +8 -0
@@ -142,8 +142,16 @@ void emergency_sync(void)int vfs_fsync_range (struct file *file,loff_t start,loff_t end,int datasync) {struct address_space *mapping = file->f_mapping; +struct inode *inode = mapping->host;int err, ret; +if (inode->i_op->sync_inode) { +unsigned int flags =INODE_SYNC_DATA |INODE_SYNC_DATA_METADATA ; +if (!datasync) + flags |=INODE_SYNC_METADATA ; +return inode->i_op->sync_inode (inode, flags, start, end); + } +if (!file->f_op || !file->f_op->fsync) { ret = -EINVAL ; goto out;
drivers/staging/pohmelfs/inode.c +2 -2
@@ -884,7 +884,7 @@ static int pohmelfs_fsync(struct file *f {struct inode *inode = file->f_mapping->host; -return sync_inode_metadata (inode, datasync,1 ); +return sync_inode_metadata (inode, datasync); } ssize_t pohmelfs_write(struct file *file, const char __user *buf,
@@ -1951,7 +1951,7 @@ static struct dentry *pohmelfs_mount(str*/ static void pohmelfs_kill_super (struct super_block *sb) { -sync_inodes_sb (sb); +sync_filesystem (sb);kill_anon_super (sb); }
fs/internal.h +5 -0
@@ -108,3 +108,8 @@ extern void release_open_intent(struct nextern int get_nr_dirty_inodes (void );extern void evict_inodes (struct super_block *);extern int invalidate_inodes (struct super_block *); + +/* +* fs-writeback.c +*/ +extern void sync_inodes_sb (struct super_block *);
include/linux/writeback.h +0 -1
@@ -61,7 +61,6 @@ void writeback_inodes_sb(struct super_blvoid writeback_inodes_sb_nr (struct super_block *,unsigned long nr);int writeback_inodes_sb_if_idle (struct super_block *);int writeback_inodes_sb_nr_if_idle (struct super_block *,unsigned long nr); -void sync_inodes_sb (struct super_block *);void writeback_inodes_wb (struct bdi_writeback *wb,struct writeback_control *wbc);long wb_do_writeback (struct bdi_writeback *wb,int force_wait);
fs/fs-writeback.c +17 -36
87 lines not on this page · raw message
fs/fat/file.c +16 -17
@@ -160,7 +160,6 @@ int fat_file_fsync(struct file *filp, inreturn res ? res : err; } -const struct file_operations fat_file_operations = { .llseek = generic_file_llseek, .read = do_sync_read,
@@ -192,22 +191,9 @@ static int fat_cont_expand(struct inodeif (IS_SYNC (inode)) {int err2; -/* -* Opencode syncing since we don't have a file open to use -* standard fsync path. -*/ - err =filemap_fdatawrite_range (mapping, start, - start + count -1 ); - err2 =sync_mapping_buffers (mapping); -if (!err) - err = err2; - err2 =write_inode_now (inode,1 ); -if (!err) - err = err2; -if (!err) { - err =filemap_fdatawait_range (mapping, start, - start + count -1 ); - } + err = inode->i_op->sync_inode (inode, +INODE_SYNC_DATA |INODE_SYNC_DATA_METADATA | +INODE_SYNC_METADATA , start, start + count -1 ); } out:return err;
@@ -440,7 +426,20 @@ int fat_setattr(struct dentry *dentry, s }EXPORT_SYMBOL_GPL (fat_setattr); +static int fat_inode_sync_inode (struct inode *inode,unsigned int flags, +loff_t start,loff_t end) +{ +int err; + + err =generic_sync_inode (inode, flags, start, end); +if (err) +return err; + err =sync_mapping_buffers (MSDOS_SB (inode->i_sb)->fat_inode->i_mapping); +return err; +} +const struct inode_operations fat_file_inode_operations = { .setattr = fat_setattr, .getattr = fat_getattr, + .sync_inode = fat_inode_sync_inode, };
Documentation/filesystems/porting +6 -0
@@ -318,3 +318,9 @@ if it's zero is not *and* *never* *had* may happen while the inode is in the middle of ->write_inode(); e.g. if you blindly free the on-disk inode, you may end up doing that while ->write_inode() is writing to it. + +-- +[mandatory] + Inode writeback and syncing has undergone an overhaul. +write_inode_now doesn't sync (unless ->write_inode *always* syncs) +
fs/fat/inode.c +8 -13
@@ -1534,20 +1534,15 @@ EXPORT_SYMBOL_GPL(fat_fill_super);*/ static int writeback_inode (struct inode *inode) { - -int ret;struct address_space *mapping = inode->i_mapping; -struct writeback_control wbc = { - .sync_mode =WB_SYNC_NONE , - .nr_to_write =0 , - }; -/* if we used WB_SYNC_ALL, sync_inode waits for the io for the -* inode to finish. So WB_SYNC_NONE is sent down to sync_inode -* and filemap_fdatawrite is used for the data blocks -*/ - ret =sync_inode (inode, &wbc); -if (!ret) - ret =filemap_fdatawrite (mapping); +int ret; + + ret =sync_inode_metadata (inode,0 ); +if (ret) +return ret; + + ret =filemap_fdatawrite (mapping); +return ret; }
fs/nfs/write.c +7 -9
@@ -1416,8 +1416,7 @@ int nfs_commit_inode(struct inode *inodereturn res;/* Note: If we exit without ensuring that the commit is complete, * we must mark the inode as dirty. Otherwise, future calls to -* sync_inode() with the WB_SYNC_ALL flag set will fail to ensure -* that the data is on the disk. +* ->write_inode() will fail to ensure that the data is on the disk. */ out_mark_dirty:__mark_inode_dirty (inode,I_DIRTY_DATASYNC );
@@ -1472,14 +1471,13 @@ int nfs_write_inode(struct inode *inode,*/ int nfs_wb_all (struct inode *inode) { -struct writeback_control wbc = { - .sync_mode =WB_SYNC_ALL , - .nr_to_write =LONG_MAX , - .range_start =0 , - .range_end =LLONG_MAX , - }; +int ret; -return sync_inode (inode, &wbc); + ret =filemap_fdatawrite (inode->i_mapping); +if (ret) +return ret; + ret =sync_inode_metadata (inode,0 ); +return ret; }int nfs_wb_page_cancel (struct inode *inode,struct page *page)
fs/exofs/file.c +1 -1
@@ -48,7 +48,7 @@ static int exofs_file_fsync(struct filestruct inode *inode = filp->f_mapping->host;struct super_block *sb; - ret =sync_inode_metadata (inode, datasync,1 ); + ret =sync_inode_metadata (inode, datasync);/* This is a good place to write the sb */ /* TODO: Sechedule an sb-sync on create */
fs/ubifs/file.c +1 -1
@@ -1313,7 +1313,7 @@ int ubifs_fsync(struct file *file, int d* VFS has already synchronized dirty pages for this inode. Synchronize * the inode unless this is a 'datasync()' call. */ - err =sync_inode_metadata (inode, datasync,1 ); + err =sync_inode_metadata (inode, datasync);if (err)return err;
Re: [patch 8/8] fs: add i_op->sync_inode
- the sync_inodes_sb export removal looks fine to, but should be a separate patch with a good changelog
- why is the sync_inode_metadata wait parameter removed? Especially as we would need it for some callers that need to be converted to it. E.g. in this patch converts writeback_inode from non-blocking to blocking behaviour.
- except for that the sync_inode() removal is fine, I had planned for that already. But again, please a separate and well-documented patch.
As for the actualy sync_inode operation: I don't think it's helpful. The *_sync_inode helpers in ext2 and fat are fine, but there's little point in going through an iops vector for it. Also adding the file syncing really complicates the API for now reason, epecially with the range interface.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 2 lines · Christoph Hellwig
- the
sync_inodes_sbexport removal looks fine to, but should be a separate patch with a good changelog
OK
Quoted 4 lines
- why is the
sync_inode_metadatawait parameter removed? Especially as we would need it for some callers that need to be converted to it. E.g. in this patch convertswriteback_inodefrom non-blocking to blocking behaviour.
Basically the blocking or non blocking behaviour of ->write_inode is irrelevant and will go away. Specific behaviour cannot be relied upon from generic code, only filesystems. But even filesystems should not differentiate between blocking and non-blocking (where they do, they have been buggy). That is the *other* big bug in the code (the first one being syncing code not waiting for writeback).
So after my series, we guarantee ->write_inode is called after the inode has been dirtied. We make no guarantees about what mode it is called in (blocking or non blocking). So the filesystem should either have _all_ write_inode calls do sync writeout, or have them all do part of the op (eg. clean struct inode by dirtying pagecache) and then syncing in fsync and sync_fs. In the latter scheme, it doesn't make sense to do anything more in the case of a "sync" call.
Basically you can spot the bugs by looking for where the .write_inode functions differentiate the writeback mode.
So, that's all very confusing with the current set of APIs that are called sync_inode, sync_inode_metadata, etc. So I'm trying to reduce and remove these gradually.
Quoted 3 lines
- except for that the
sync_inode()removal is fine, I had planned for that already. But again, please a separate and well-documented patch.
Yes ok.
Quoted 5 lines
As for the actualy
sync_inodeoperation: I don't think it's helpful. The*_sync_inodehelpers in ext2 and fat are fine, but there's little point in going through an iops vector for it. Also adding the file syncing really complicates the API for now reason, epecially with the range interface.
It does have a reason, which is the nfsd syncing callback -- using sync_inode_metadata there is actually wrong and should really be replaced with a warning that the fs cannot support such syncing.
See the problem I explained above -- it really needs to do a full fsync call. But it doesn't have a file descriptor, and most filesystems don't need one, so I think a sync_inode operation is nice.
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and sync_file_range and such properly too.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 7 lines · Nick Piggin
Basically the blocking or non blocking behaviour of ->write_inode is irrelevant and will go away. Specific behaviour cannot be relied upon from generic code, only filesystems. But even filesystems should not differentiate between blocking and non-blocking (where they do, they have been buggy). That is the *other* big bug in the code (the first one being syncing code not waiting for writeback).
Quoted 7 lines
So after my series, we guarantee ->write_inode is called after the inode has been dirtied. We make no guarantees about what mode it is called in (blocking or non blocking). So the filesystem should either have _all_
write_inodecalls do sync writeout, or have them all do part of the op (eg. clean struct inode by dirtying pagecache) and then syncing in fsync andsync_fs. In the latter scheme, it doesn't make sense to do anything more in the case of a "sync" call.
There is absolutely no problem with writing out inodes asynchronously, it's just that the writeback code can't really cope with it right now. The fix is to only update i_state on I/O completion.
The blocking vs non-blocking difference is actually quite important for performance still. In fact for most modern filesystems we simply don't every want to do a normal writeout for the !sync case. Either it would be a no-op, or do something like we do in xfs currently, where we make sure to read the inode to start the read/modify/write cycle early, and put it at the end of the delayed write queue. The sync one on the other hand just logs the inode, so that the log force in ->sync_fs/etc can write them all out in one go.
Quoted 6 lines
Quoted 5 lines
As for the actualy
sync_inodeoperation: I don't think it's helpful. The*_sync_inodehelpers in ext2 and fat are fine, but there's little point in going through an iops vector for it. Also adding the file syncing really complicates the API for now reason, epecially with the range interface.
Quoted 7 lines
It does have a reason, which is the nfsd syncing callback -- using
sync_inode_metadatathere is actually wrong and should really be replaced with a warning that the fs cannot support such syncing.See the problem I explained above -- it really needs to do a full fsync call. But it doesn't have a file descriptor, and most filesystems don't need one, so I think a
sync_inodeoperation is nice.
It doesn't need to do an fsync, it's actually a quite different operations. That's why we added the ->commit_metadata operations. What NFSD really wants is to guarantee synchronous metadata operations. Traditionally unix required the filesystems to be mounted using -o wsync for that, but we try to emulate that from NFSD without affecting other access to the filesystem. The ->commit_metadata is there to ensure any previously started async operations completes. It does not have to push all dirty state to disk like fsync. Just compare the complexities of xfs_file_fsync and xfs_fs_nfs_commit_metadata - the latter simply checks if the inode has been logged, and if yes forces the log to disk up to the last operation on this inode. fsync does the same force if the inode is not dirty, but otherwise has to start a new transactions. It also has to wait for pending I/O completions before dealing with metadata, and issue barriers to data devices, which is completly superflous for nfs operations.
Quoted 6 lines
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and
sync_file_rangeand such properly too.
We currently do all that just fine before calling into ->fsync. That doesn't mean we can't move it into ->fsync if a good reason for it comes up, but
a) it needs a good rational
b) it should stay in ->fsync instead of beeing mixed up with the method used for other metadata writeback
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 16 lines · Christoph Hellwig
Quoted 7 lines · Nick Piggin
Basically the blocking or non blocking behaviour of ->write_inode is irrelevant and will go away. Specific behaviour cannot be relied upon from generic code, only filesystems. But even filesystems should not differentiate between blocking and non-blocking (where they do, they have been buggy). That is the *other* big bug in the code (the first one being syncing code not waiting for writeback).
Quoted 7 lines
So after my series, we guarantee ->write_inode is called after the inode has been dirtied. We make no guarantees about what mode it is called in (blocking or non blocking). So the filesystem should either have _all_
write_inodecalls do sync writeout, or have them all do part of the op (eg. clean struct inode by dirtying pagecache) and then syncing in fsync andsync_fs. In the latter scheme, it doesn't make sense to do anything more in the case of a "sync" call.
Quoted 3 lines
There is absolutely no problem with writing out inodes asynchronously, it's just that the writeback code can't really cope with it right now. The fix is to only update
i_stateon I/O completion.
I don't know what you mean by this. As a filesystem generic API, .write_inode says almost nothing. Some filesystems will do one thing, some may do something in future after patches to the vfs to allow more fancy work, others do other things, generic code can't assume any of that.
Quoted 2 lines
The blocking vs non-blocking difference is actually quite important for performance still.
I don't see how it could be. All integrity points still have to wait for all potentially non blocking write_inode, so if that's the case just make *all* the write_inode non blocking to begin with. Simpler code and you'd get more performance at least in the sync(3) case where writeback and waiting of multiple inodes can be batched.
Quoted 7 lines
In fact for most modern filesystems we simply don't
every want to do a normal writeout for the !sync case. Either it would be a no-op, or do something like we do in xfs currently, where we make sure to read the inode to start the read/modify/write cycle early, and put it at the end of the delayed write queue. The sync one on the other hand just logs the inode, so that the log force in ->sync_fs/etc can write them all out in one go.
Great, so why would you ever do a sync .write_inode?
Quoted 14 lines
Quoted 6 lines
Quoted 5 lines
As for the actualy
sync_inodeoperation: I don't think it's helpful. The*_sync_inodehelpers in ext2 and fat are fine, but there's little point in going through an iops vector for it. Also adding the file syncing really complicates the API for now reason, epecially with the range interface.Quoted 7 lines
It does have a reason, which is the nfsd syncing callback -- using
sync_inode_metadatathere is actually wrong and should really be replaced with a warning that the fs cannot support such syncing.See the problem I explained above -- it really needs to do a full fsync call. But it doesn't have a file descriptor, and most filesystems don't need one, so I think a
sync_inodeoperation is nice.
Quoted 15 lines
It doesn't need to do an fsync, it's actually a quite different operations. That's why we added the ->commit_metadata operations. What NFSD really wants is to guarantee synchronous metadata operations. Traditionally unix required the filesystems to be mounted using -o wsync for that, but we try to emulate that from NFSD without affecting other access to the filesystem. The ->commit_metadata is there to ensure any previously started async operations completes. It does not have to push all dirty state to disk like fsync. Just compare the complexities of
xfs_file_fsyncandxfs_fs_nfs_commit_metadata- the latter simply checks if the inode has been logged, and if yes forces the log to disk up to the last operation on this inode. fsync does the same force if the inode is not dirty, but otherwise has to start a new transactions. It also has to wait for pending I/O completions before dealing with metadata, and issue barriers to data devices, which is completly superflous for nfs operations.
Thanks for the interesting comments, but I didn't say it needs an fsync, I said sync_inode_metadata is not sufficient by definition because it guarantees nothing.
Quoted 8 lines
Quoted 6 lines
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and
sync_file_rangeand such properly too.We currently do all that just fine before calling into ->fsync.
What do you mean we do all that just fine? A filesystem can't schedule data submission and waiting optimally versus metadata, it can't do metadata operations or remote commits corresponding to data ranges, and it doesn't support nfs sync operations.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 8 lines · Nick Piggin
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and
sync_file_rangeand such properly too.
Quoted 6 lines
Quoted 1 line
We currently do all that just fine before calling into ->fsync.
What do you mean we do all that just fine? A filesystem can't schedule data submission and waiting optimally versus metadata, it can't do metadata operations or remote commits corresponding to data ranges, and it doesn't support nfs sync operations.
And actually I think it is much better to have sync_inode, which means we'll be able to get rid of commit_metadata (which should be an inode operation anyway, not an export operation which really should deal with exporting filesystems to a non-vfs namespace, not nfsd hacks).
commit_metadata would just be sync_inode with a null range or no data sync flag set.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 7 lines · Nick Piggin
And actually I think it is much better to have
sync_inode, which means we'll be able to get rid ofcommit_metadata(which should be an inode operation anyway, not an export operation which really should deal with exporting filesystems to a non-vfs namespace, not nfsd hacks).
commit_metadatawould just besync_inodewith a null range or no data sync flag set.
As explained in the previous mail it's not just not writing data, it's conceptually quite different from fsync.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Nick Piggin
And actually I think it is much better to have
sync_inode, which means we'll be able to get rid ofcommit_metadata(which should be an inode operation anyway, not an export operation which really should deal with exporting filesystems to a non-vfs namespace, not nfsd hacks).
commit_metadatawould just besync_inodewith a null range or no data sync flag set.As explained in the previous mail it's not just not writing data, it's conceptually quite different from fsync.
OK I missed that part about not requiring dirty metadata to be written, just currently ongoing async operations. But then I don't understand how it would be used by nfsd, how does nfsd start some async operation on the inode metadata such that ->commit_metadata would do anything useful for it?
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 5 lines · Nick Piggin
OK I missed that part about not requiring dirty metadata to be written, just currently ongoing async operations. But then I don't understand how it would be used by nfsd, how does nfsd start some async operation on the inode metadata such that ->commit_metadata would do anything useful for it?
NFSD calls various inode operations (create/mkdir/mknod/link/symlink/ rename/unlink/rmdir/setattr) and then requires those operations to be on disk before completing the request to the client, but it does not require other dirty state to be written (data, unlogged size or timestamp updates). Take a look at the XFS implementation: it just checks if the inode is still pinned (that is in the in-memory log, but not commited to disk) and if so forces the log up to the log buffer that contains the last changes to the inode.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Nick Piggin
OK I missed that part about not requiring dirty metadata to be written, just currently ongoing async operations. But then I don't understand how it would be used by nfsd, how does nfsd start some async operation on the inode metadata such that ->commit_metadata would do anything useful for it?
Quoted 8 lines
NFSD calls various inode operations (create/mkdir/mknod/link/symlink/ rename/unlink/rmdir/setattr) and then requires those operations to be on disk before completing the request to the client, but it does not require other dirty state to be written (data, unlogged size or timestamp updates). Take a look at the XFS implementation: it just checks if the inode is still pinned (that is in the in-memory log, but not commited to disk) and if so forces the log up to the log buffer that contains the last changes to the inode.
OK, I don't exactly see why a sync_inode with appropriate flag could not solve that problem. I'll take a bit more look through nfs and xfs. Thanks...
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 3 lines · Nick Piggin
OK, I don't exactly see why a
sync_inodewith appropriate flag could not solve that problem. I'll take a bit more look through nfs and xfs. Thanks...
We could also overload all inode operations into a single ioctl like method, but that doesn't make it a good design.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Nick Piggin
OK, I don't exactly see why a
sync_inodewith appropriate flag could not solve that problem. I'll take a bit more look through nfs and xfs. Thanks...We could also overload all inode operations into a single ioctl like method, but that doesn't make it a good design.
I've been a bit busy lately and haven't had time to continue this discussion. Thanks for your input so far, and I'll get back to it soon.
I will try to get out a set of changes for current merge window that fix the bulk of the problems without going into areas you disagree with, and then we can have a smaller set of changes we to agree on for next merge window. I will come back and reply to your points when I get a bit more time.
Re: [patch 8/8] fs: add i_op->sync_inode
In reply to Nick PigginQuoted 9 lines · Nick Piggin
Quoted 3 lines
There is absolutely no problem with writing out inodes asynchronously, it's just that the writeback code can't really cope with it right now. The fix is to only update
i_stateon I/O completion.I don't know what you mean by this. As a filesystem generic API, .write_inode says almost nothing. Some filesystems will do one thing, some may do something in future after patches to the vfs to allow more fancy work, others do other things, generic code can't assume any of that.
There is no problem with having a method (let's call it write_inode for simplicity, it could be sync_inode or got knows what else) to write clear the dirty state of the VFS inode asynchronously, as long as we make sure the bits are only updated once it's been successfull. That is for an async call we can't let the caller of ->write_inode update it, but need to do it in an I/O completion callback.
Quoted 8 lines
Quoted 2 lines
The blocking vs non-blocking difference is actually quite important for performance still.
I don't see how it could be. All integrity points still have to wait for all potentially non blocking
write_inode, so if that's the case just make *all* thewrite_inodenon blocking to begin with. Simpler code and you'd get more performance at least in the sync(3) case where writeback and waiting of multiple inodes can be batched.
The problem is that currently we almost never do a pure blocking ->write_inode. The way the sync code is written we always do a non-blocking one first, then a blocking one. If you always do the synchronous one we'll get a lot more overhead - the first previous asynchronous one will write the inode (be it just into the log, or for real), then we write back data, and then we'll have to write it again becaus it has usually been redirtied again due to the data writeback in the meantime. If you want to get rid of the asynchronous ones you need to get rid of the callers, and not just change the behaviour in the implementations.
Quoted 9 lines
Quoted 7 lines
In fact for most modern filesystems we simply don't
every want to do a normal writeout for the !sync case. Either it would be a no-op, or do something like we do in xfs currently, where we make sure to read the inode to start the read/modify/write cycle early, and put it at the end of the delayed write queue. The sync one on the other hand just logs the inode, so that the log force in ->sync_fs/etc can write them all out in one go.Great, so why would you ever do a sync .write_inode?
We need to propagate the VFS dirty state into the fs-internal state, e.g. for XFS start a transaction. The reason for that is that the VFS simply writes timestamps into the inode and marks it dirty instead of telling the filesystem about timestamp updates. For XFS in 2.6.38+ timestamp updates and i_size updates are the only unlogged metadata changes, and thus now the only thing going through ->write_inode.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and
sync_file_rangeand such properly too.
Quoted 6 lines
Quoted 1 line
We currently do all that just fine before calling into ->fsync.
What do you mean we do all that just fine? A filesystem can't schedule data submission and waiting optimally versus metadata, it can't do metadata operations or remote commits corresponding to data ranges, and it doesn't support nfs sync operations.
A normal filesystems needs to wait for data I/O to finish before updating the metadata for data integrity reasons - otherwise we can expose stale data in case of a crash - that's one of the things we fixed a couple of kernel releases ago. As I said I have no problems moving the data operations into ->fsync or adding range information if we actually need it. Show me a filesystem that really needs this to get good performance and we can adapt the ->fsync prototype. If we need it at all we'll need it for network/distributed filesystems, though - which have a tendency to actually require the file argument and won't be able to use an inode operations. Nevermind the fact that file_operations are what we use for all data I/O operations, and messing this up for the sake of it doesn't seem like an overly useful idea.
Care to explain what you mean with nfs sync operations?
Quoted 1 line
---end quoted text---
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · Nick Piggin
Quoted 3 lines
There is absolutely no problem with writing out inodes asynchronously, it's just that the writeback code can't really cope with it right now. The fix is to only update
i_stateon I/O completion.I don't know what you mean by this. As a filesystem generic API, .write_inode says almost nothing. Some filesystems will do one thing, some may do something in future after patches to the vfs to allow more fancy work, others do other things, generic code can't assume any of that.
Quoted 6 lines
There is no problem with having a method (let's call it
write_inodefor simplicity, it could besync_inodeor got knows what else) to write clear the dirty state of the VFS inode asynchronously, as long as we make sure the bits are only updated once it's been successfull. That is for an async call we can't let the caller of ->write_inode update it, but need to do it in an I/O completion callback.
Right, but that would be a bit of a change to generic code and a really big change to make all filesystems support the API.
What I am saying right now is that .write_inode doesn't mean anything to generic code, except that it does something "nice" for writeback/reclaim purposes, and it needs to be called within the sync / fsync protocol.
But .write_inode on its own doesn't mean anything. Not even if 99% of the filesystems implement it in a particular way. Hence, sync_inode_metadata and such can't be used in generic code like nfsd.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
The blocking vs non-blocking difference is actually quite important for performance still.
I don't see how it could be. All integrity points still have to wait for all potentially non blocking
write_inode, so if that's the case just make *all* thewrite_inodenon blocking to begin with. Simpler code and you'd get more performance at least in the sync(3) case where writeback and waiting of multiple inodes can be batched.
Quoted 8 lines
The problem is that currently we almost never do a pure blocking ->write_inode. The way the sync code is written we always do a non-blocking one first, then a blocking one. If you always do the synchronous one we'll get a lot more overhead - the first previous asynchronous one will write the inode (be it just into the log, or for real), then we write back data, and then we'll have to write it again becaus it has usually been redirtied again due to the data writeback in the meantime.
It doesn't matter, the integrity still has to be enforced in .sync_fs, because sync .write_inode may *never* get called, because of the fact that async .write_inode also clears the inode metadata dirty bits.
So if .sync_fs has to enforce integrity anyway, then you don't ever need to do any actual waiting in your sync .write_inode. See?
Quoted 3 lines
If you want to get rid of the asynchronous ones you need
to get rid of the callers, and not just change the behaviour in the implementations.
The patches are working towards that. It's a lot less broken that it was, and (barring my oversights, or existing fs bugs) it should actually "work" now.
In cases where particular calls remain, it is for example where the filesystem calls their own handler and so it knows what it can expect.
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
In fact for most modern filesystems we simply don't
every want to do a normal writeout for the !sync case. Either it would be a no-op, or do something like we do in xfs currently, where we make sure to read the inode to start the read/modify/write cycle early, and put it at the end of the delayed write queue. The sync one on the other hand just logs the inode, so that the log force in ->sync_fs/etc can write them all out in one go.Great, so why would you ever do a sync .write_inode?
Quoted 7 lines
We need to propagate the VFS dirty state into the fs-internal state, e.g. for XFS start a transaction. The reason for that is that the VFS simply writes timestamps into the inode and marks it dirty instead of telling the filesystem about timestamp updates. For XFS in 2.6.38+ timestamp updates and
i_sizeupdates are the only unlogged metadata changes, and thus now the only thing going through ->write_inode.
Well then you have a bug, because a sync .write_inode *may never get called*. You may only get an async one, even in the case of fsync, because async writeback clears the vfs dirty bits.
Quoted 14 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Also giving filesystems the flexibility to do the data writeout can be more optimal by doing all writeout at once or ordering to suit their writeback patterns. Having range data could give some performance advantages writing back mappings or commit operations over network. I don't see it as a big complexity. It gives a chance to do range fsyncs and
sync_file_rangeand such properly too.Quoted 6 lines
Quoted 1 line
We currently do all that just fine before calling into ->fsync.
What do you mean we do all that just fine? A filesystem can't schedule data submission and waiting optimally versus metadata, it can't do metadata operations or remote commits corresponding to data ranges, and it doesn't support nfs sync operations.
Quoted 4 lines
A normal filesystems needs to wait for data I/O to finish before updating the metadata for data integrity reasons - otherwise we can expose stale data in case of a crash - that's one of the things we fixed a couple of kernel releases ago. As I said I have no problems moving the
Yes, and that's a filesystem-implementation detail.
Quoted 8 lines
data operations into ->fsync or adding range information if we actually need it. Show me a filesystem that really needs this to get good performance and we can adapt the ->fsync prototype. If we need it at all we'll need it for network/distributed filesystems, though - which have a tendency to actually require the file argument and won't be able to use an inode operations. Nevermind the fact that
file_operationsare what we use for all data I/O operations, and messing this up for the sake of it doesn't seem like an overly useful idea.
Just if I'm adding this API it makes sense to add the obvious parameters. A sync range does not "hugely complicate the API" at all.
Quoted 1 line
Care to explain what you mean with nfs sync operations?
The one that is attempted to be implemented with sync_inode_metadata.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 9 lines
Quoted 8 lines
The problem is that currently we almost never do a pure blocking ->write_inode. The way the sync code is written we always do a non-blocking one first, then a blocking one. If you always do the synchronous one we'll get a lot more overhead - the first previous asynchronous one will write the inode (be it just into the log, or for real), then we write back data, and then we'll have to write it again becaus it has usually been redirtied again due to the data writeback in the meantime.
Quoted 6 lines
It doesn't matter, the integrity still has to be enforced in .sync_fs, because sync .write_inode may *never* get called, because of the fact that async .write_inode also clears the inode metadata dirty bits.
So if .sync_fs has to enforce integrity anyway, then you don't ever need to do any actual waiting in your sync .write_inode. See?
I'm not talking about the actual waiting. Right now we have two different use cases for ->write_inode:
1) sync_mode == WB_SYNC_NONE
This tells the filesystem to start an opportunistic writeout.
2) sync_mode == WB_SYNC_ALL
This tells the filesystem it needs to to a mandatory writeout.
Note that writeout is losely defined. If a filesystems isn't exportable or implements the commit_metadata operation it's indeed enough to synchronize the state into internal fs data just enough for ->sync_fs.
Or that's how it should be. As you pointed out the way the writeback code treats the WB_SYNC_NONE writeouts makes this not work as expected.
There's various ways to fix this:
1) the one you advocate, that is treating all ->write_inode calls as if they were WB_SYNC_ALL. This does fix the issue of incorrectly updating the dirty state, but causes a lot of additional I/O - the way the sync process is designed we basically always call ->write_inode with WB_SYNC_NONE first, and then with WB_SYNC_ALL
2) keep the WB_SYNC_NONE calls, but never update dirty state for them. This also fixes the i_dirty state updates, but allows filesystems that keep internal dirty state to be smarted about avoiding I/O
3) remove the calls to ->write_inode with WB_SYNC_NONE. This might work well for calls from the sync() callchain, but we rely on inode background writeback from the flusher threads in lots of places. Note that we really do not want to block the flusher threads with blocking writes, which is another argument against (1).
4) require ->write_inode to update the dirty state itself after the inode is on disk or in a data structure caught by ->sync_fs. This keeps optimal behaviour, but requires a lot of code changes.
If we want a quick fix only (2) seems feasibly to me, with the option of implementing (4) and parts of (3) later on.
Quoted 11 lines
Quoted 7 lines
We need to propagate the VFS dirty state into the fs-internal state, e.g. for XFS start a transaction. The reason for that is that the VFS simply writes timestamps into the inode and marks it dirty instead of telling the filesystem about timestamp updates. For XFS in 2.6.38+ timestamp updates and
i_sizeupdates are the only unlogged metadata changes, and thus now the only thing going through ->write_inode.Well then you have a bug, because a sync .write_inode *may never get called*. You may only get an async one, even in the case of fsync, because async writeback clears the vfs dirty bits.
Yes, the bug about updating the dirty state for WB_SYNC_NONE affects
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 55 lines · Christoph Hellwig
Quoted 9 lines
Quoted 8 lines
The problem is that currently we almost never do a pure blocking ->write_inode. The way the sync code is written we always do a non-blocking one first, then a blocking one. If you always do the synchronous one we'll get a lot more overhead - the first previous asynchronous one will write the inode (be it just into the log, or for real), then we write back data, and then we'll have to write it again becaus it has usually been redirtied again due to the data writeback in the meantime.
Quoted 6 lines
It doesn't matter, the integrity still has to be enforced in .sync_fs, because sync .write_inode may *never* get called, because of the fact that async .write_inode also clears the inode metadata dirty bits.
So if .sync_fs has to enforce integrity anyway, then you don't ever need to do any actual waiting in your sync .write_inode. See?
I'm not talking about the actual waiting. Right now we have two different use cases for ->write_inode:
1)
sync_mode==WB_SYNC_NONEThis tells the filesystem to start an opportunistic writeout.
2)
sync_mode==WB_SYNC_ALLThis tells the filesystem it needs to to a mandatory writeout.
Note that writeout is losely defined. If a filesystems isn't exportable or implements the
commit_metadataoperation it's indeed enough to synchronize the state into internal fs data just enough for ->sync_fs.Or that's how it should be. As you pointed out the way the writeback code treats the
WB_SYNC_NONEwriteouts makes this not work as expected.There's various ways to fix this:
1) the one you advocate, that is treating all ->write_inode calls as if they were
WB_SYNC_ALL. This does fix the issue of incorrectly updating the dirty state, but causes a lot of additional I/O - the way the sync process is designed we basically always call ->write_inode withWB_SYNC_NONEfirst, and then withWB_SYNC_ALL
2) keep theWB_SYNC_NONEcalls, but never update dirty state for them. This also fixes thei_dirtystate updates, but allows filesystems that keep internal dirty state to be smarted about avoiding I/O
3) remove the calls to ->write_inode withWB_SYNC_NONE. This might work well for calls from thesync()callchain, but we rely on inode background writeback from the flusher threads in lots of places. Note that we really do not want to block the flusher threads with blocking writes, which is another argument against (1).
4) require ->write_inode to update the dirty state itself after the inode is on disk or in a data structure caught by ->sync_fs. This keeps optimal behaviour, but requires a lot of code changes.
Quoted 2 lines
If we want a quick fix only (2) seems feasibly to me, with the option of implementing (4) and parts of (3) later on.
No, you misunderstand 1. I am saying they should be treated as WB_SYNC_NONE.
In fact 2 would cause much more IO, because dirty writeout would never clean them so it will just keep writing them out. I don't know how 2 could be feasible.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
We need to propagate the VFS dirty state into the fs-internal state, e.g. for XFS start a transaction. The reason for that is that the VFS simply writes timestamps into the inode and marks it dirty instead of telling the filesystem about timestamp updates. For XFS in 2.6.38+ timestamp updates and
i_sizeupdates are the only unlogged metadata changes, and thus now the only thing going through ->write_inode.
Quoted 5 lines
Quoted 3 lines
Well then you have a bug, because a sync .write_inode *may never get called*. You may only get an async one, even in the case of fsync, because async writeback clears the vfs dirty bits.
Yes, the bug about updating the dirty state for
WB_SYNC_NONEaffects
So, back to my original question: what is the performance problem with treating write_inode as WB_SYNC_NONE, and then having .fsync and .sync_fs do the integrity?
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 6 lines · Nick Piggin
No, you misunderstand 1. I am saying they should be treated as
WB_SYNC_NONE.In fact 2 would cause much more IO, because dirty writeout would never clean them so it will just keep writing them out. I don't know how 2 could be feasible.
WB_SYNC_NONE means ->write_inode behaves non-blocking. That is we do not block on memory allocations, and we do not take locks blocking. Most journaling filesystems currently take the easy way out an make it a no-op due to that, but take a look at XFS how complicated it is to avoid the blocking if you want a non-noop implementation.
Quoted 3 lines
So, back to my original question: what is the performance problem with treating
write_inodeasWB_SYNC_NONE, and then having .fsync and .sync_fs do the integrity?
See above - we'll block in the flusher thread and cause it to stall, which is really nasty as it does all data I/O writeback. The salling may also block sync() although I don't think it's as important there.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Nick Piggin
No, you misunderstand 1. I am saying they should be treated as
WB_SYNC_NONE.In fact 2 would cause much more IO, because dirty writeout would never clean them so it will just keep writing them out. I don't know how 2 could be feasible.
Quoted 6 lines
WB_SYNC_NONEmeans ->write_inode behaves non-blocking. That is we do not block on memory allocations, and we do not take locks blocking. Most journaling filesystems currently take the easy way out an make it a no-op due to that, but take a look at XFS how complicated it is to avoid the blocking if you want a non-noop implementation.
Btw, there's an easy way how we could get this right, in fact the write_inode in XFS is already trying to do it, it's just the caller not copying with it:
- if we can't get locks for a non-blocking ->write_inode we return EAGAIN, and the callers sets the dirty bits again.
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 6 lines · Christoph Hellwig
Btw, there's an easy way how we could get this right, in fact the
write_inodein XFS is already trying to do it, it's just the caller not copying with it:- if we can't get locks for a non-blocking ->write_inode we return EAGAIN, and the callers sets the dirty bits again.
I just tried to implement this and noticed we're actually doing this inside XFS - if we get our EAGAIN error from the lower level code in ->write_inode we do a manual mark_inode_dirty_sync(). So as far as XFS is concerned ->write_inode always pushes data into a state where ->sync_fs writes it out, or if it was called with WB_SYNC_NONE and couldn't get the locks redirties the inode, and thus is not affected by the issue you mentioned. I think this is also a good model for other filesystems to follow.
Ted Ts'o
maintainerRe: [patch 8/8] fs: add i_op->sync_inode
In reply to Christoph HellwigQuoted 6 lines · Christoph Hellwig
Btw, there's an easy way how we could get this right, in fact the
write_inodein XFS is already trying to do it, it's just the caller not copying with it:- if we can't get locks for a non-blocking ->write_inode we return EAGAIN, and the callers sets the dirty bits again.
I like that solution; it might be one of the easier ways to maintain backwards compatibility. Especially since (correct me if I am wrong) the simpler file systems which always write out the inode in the case of a non-blocking write_inode, say, like say the fat file system, are immune from this specific problem, right?
- Ted
Re: [patch 8/8] fs: add i_op->sync_inode
Quoted 11 lines · Ted Ts'o
Quoted 5 lines
the
write_inodein XFS is already trying to do it, it's just the caller not copying with it:- if we can't get locks for a non-blocking ->write_inode we return EAGAIN, and the callers sets the dirty bits again.
I like that solution; it might be one of the easier ways to maintain backwards compatibility. Especially since (correct me if I am wrong) the simpler file systems which always write out the inode in the case of a non-blocking
write_inode, say, like say the fat file system, are immune from this specific problem, right?
Yes. Also one of the patches in Nick's series actually implements this already. Looks like we lost that fact when arguing about other things..
Ted Ts'o
maintainerRe: [patch 8/8] fs: add i_op->sync_inode
In reply to Nick PigginDocumentation/filesystems/porting if it's zero is not *and* *never* *had* · line 326
Quoted 7 lines · Nick Piggin
Documentation/filesystems/porting +5
-0+-- +[mandatory] + Inode writeback and syncing has undergone an overhaul. +write_inode_now doesn't sync (unless ->write_inode *always* syncs) +
Could you perhaps give a bit more detail in the documentation about what sync_inode() is supposed to do? I gather that it is supposed to (based on the flags), do the specified combination of (a) do writeback for the pages corresponding to the specified range (if INODE_SYNC_DATA is given), (b) write out the inode if necessary depending on INODE_SYNC_DATA_METADATA (for fdatasync) and INODE_SYNC_DATA (for fsync).
When sync_inode() returns, what is guaranteed is that the appropriate buffer_heads are dirtied (for both the metadata and the data if the file system is still using the buffer cache for data I/O) and/or the data has been pushed to the block I/O system, but the sync_inode() method does not wait force the dirtied buffers out to disk, and it does not wait for any initiated I/O to complete (either at the block I/O layer or guaranteeing that the data has hit the platters by issuing a barrier request).
Do I have that right? If so, could we please add that to the documentation, just so it's clear? Thanks!!
- Ted