remove racy sync_page?

Hi,

I'm not completely sure whether this is the bug or not, nor what would be the performance consequences of my attached fix (wrt the block layer). So you're probably cc'ed because I've found similar threads with your names on them.

lock_page (and other waiters on page flags bits) use sync_page when sleeping on a bit. sync_page, however, requires that the page's mapping be pinned (which is what we're sometimes trying to lock the page for).

Blatant offender is set_page_dirty_lock, which falls to the race it purports to fix. Perhaps all callers could be fixed, however it seems that the pinned mapping lock_page precondition is counter-intuitive and I'd bet other callers to lock_page or wait_on_page_bit have got it wrong too.

Also: splice can change a page's mapping, so it would have been possible to use the wrong mapping to "sync" a page.

Can we get rid of the whole thing, confusing memory barriers and all? Nobody uses anything but the default sync_page, and if block rq plugging is terribly bad for performance, perhaps it should be reworked anyway? It shouldn't be a correctness thing, right?

Alternatives include RCU freeing of mappings and other things that increase in complexity from there (eg. audit and fix all callers). It is so much cooler to get rid of code though ;)

Comments?

SUSE Labs, Novell Inc.

Index: linux-2.6/mm/filemap.c
===================================================================
@@ -134,42 +134,6 @@ void remove_from_page_cache(struct page 
 	write_unlock_irq(&mapping->tree_lock);
 }
 
-static int sync_page(void *word)
-{
-	struct address_space *mapping;
-	struct page *page;
-
-	page = container_of((unsigned long *)word, struct page, flags);
-
-	/*
-	 * page_mapping() is being called without PG_locked held.
-	 * Some knowledge of the state and use of the page is used to
-	 * reduce the requirements down to a memory barrier.
-	 * The danger here is of a stale page_mapping() return value
-	 * indicating a struct address_space different from the one it's
-	 * associated with when it is associated with one.
-	 * After smp_mb(), it's either the correct page_mapping() for
-	 * the page, or an old page_mapping() and the page's own
-	 * page_mapping() has gone NULL.
-	 * The ->sync_page() address_space operation must tolerate
-	 * page_mapping() going NULL. By an amazing coincidence,
-	 * this comes about because none of the users of the page
-	 * in the ->sync_page() methods make essential use of the
-	 * page_mapping(), merely passing the page down to the backing
-	 * device's unplug functions when it's non-NULL, which in turn
-	 * ignore it for all cases but swap, where only page_private(page) is
-	 * of interest. When page_mapping() does go NULL, the entire
-	 * call stack gracefully ignores the page and returns.
-	 * -- wli
-	 */
-	smp_mb();
-	mapping = page_mapping(page);
-	if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
-		mapping->a_ops->sync_page(page);
-	io_schedule();
-	return 0;
-}
-
 /**
  * filemap_fdatawrite_range - start writeback against all of a mapping's
  * dirty pages that lie within the byte offsets <start, end>

@@ -456,6 +420,12 @@ struct page *page_cache_alloc_cold(struc
 EXPORT_SYMBOL(page_cache_alloc_cold);
 #endif
 
+static int __sleep_on_page_bit(void *word)
+{
+	io_schedule();
+	return 0;
+}
+
 /*
  * In order to wait for pages to become available there must be
  * waitqueues associated with pages. By using a hash table of

@@ -483,7 +453,7 @@ void fastcall wait_on_page_bit(struct pa
 	DEFINE_WAIT_BIT(wait, &page->flags, bit_nr);
 
 	if (test_bit(bit_nr, &page->flags))
-		__wait_on_bit(page_waitqueue(page), &wait, sync_page,
+		__wait_on_bit(page_waitqueue(page), &wait, __sleep_on_page_bit,
 							TASK_UNINTERRUPTIBLE);
 }
 EXPORT_SYMBOL(wait_on_page_bit);

@@ -529,17 +499,12 @@ EXPORT_SYMBOL(end_page_writeback);
 
 /*
  * Get a lock on the page, assuming we need to sleep to get it.
- *
- * Ugly: running sync_page() in state TASK_UNINTERRUPTIBLE is scary.  If some
- * random driver's requestfn sets TASK_RUNNING, we could busywait.  However
- * chances are that on the second loop, the block layer's plug list is empty,
- * so sync_page() will then return in state TASK_UNINTERRUPTIBLE.
  */
 void fastcall __lock_page(struct page *page)
 {
 	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
 
-	__wait_on_bit_lock(page_waitqueue(page), &wait, sync_page,
+	__wait_on_bit_lock(page_waitqueue(page), &wait, __sleep_on_page_bit,
 							TASK_UNINTERRUPTIBLE);
 }
 EXPORT_SYMBOL(__lock_page);

Index: linux-2.6/fs/block_dev.c
===================================================================
@@ -1080,7 +1080,6 @@ static long block_ioctl(struct file *fil
 struct address_space_operations def_blk_aops = {
 	.readpage	= blkdev_readpage,
 	.writepage	= blkdev_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= blkdev_prepare_write,
 	.commit_write	= blkdev_commit_write,
 	.writepages	= generic_writepages,

Index: linux-2.6/fs/buffer.c
===================================================================
@@ -3011,16 +3011,6 @@ out:
 }
 EXPORT_SYMBOL(try_to_free_buffers);
 
-void block_sync_page(struct page *page)
-{
-	struct address_space *mapping;
-
-	smp_mb();
-	mapping = page_mapping(page);
-	if (mapping)
-		blk_run_backing_dev(mapping->backing_dev_info, page);
-}
-
 /*
  * There are no bdflush tunables left.  But distributions are
  * still running obsolete flush daemons, so we terminate them here.

@@ -3164,7 +3154,6 @@ EXPORT_SYMBOL(__wait_on_buffer);
 EXPORT_SYMBOL(block_commit_write);
 EXPORT_SYMBOL(block_prepare_write);
 EXPORT_SYMBOL(block_read_full_page);
-EXPORT_SYMBOL(block_sync_page);
 EXPORT_SYMBOL(block_truncate_page);
 EXPORT_SYMBOL(block_write_full_page);
 EXPORT_SYMBOL(cont_prepare_write);

Index: linux-2.6/include/linux/buffer_head.h
===================================================================
@@ -203,7 +203,6 @@ int cont_prepare_write(struct page*, uns
 int generic_cont_expand(struct inode *inode, loff_t size);
 int generic_cont_expand_simple(struct inode *inode, loff_t size);
 int block_commit_write(struct page *page, unsigned from, unsigned to);
-void block_sync_page(struct page *);
 sector_t generic_block_bmap(struct address_space *, sector_t, get_block_t *);
 int generic_commit_write(struct file *, struct page *, unsigned, unsigned);
 int block_truncate_page(struct address_space *, loff_t, get_block_t *);

Index: linux-2.6/include/linux/fs.h
===================================================================
@@ -351,7 +351,6 @@ struct writeback_control;
 struct address_space_operations {
 	int (*writepage)(struct page *page, struct writeback_control *wbc);
 	int (*readpage)(struct file *, struct page *);
-	void (*sync_page)(struct page *);
 
 	/* Write back some dirty pages from this mapping. */
 	int (*writepages)(struct address_space *, struct writeback_control *);

Index: linux-2.6/mm/swap_state.c
===================================================================
@@ -21,12 +21,11 @@
 
 /*
  * swapper_space is a fiction, retained to simplify the path through
- * vmscan's shrink_list, to make sync_page look nicer, and to allow
- * future use of radix_tree tags in the swap cache.
+ * vmscan's shrink_list and to allow future use of radix_tree tags in
+ * the swap cache.
  */
 static struct address_space_operations swap_aops = {
 	.writepage	= swap_writepage,
-	.sync_page	= block_sync_page,
 	.set_page_dirty	= __set_page_dirty_nobuffers,
 	.migratepage	= migrate_page,
 };

Index: linux-2.6/fs/adfs/inode.c
===================================================================
@@ -75,7 +75,6 @@ static sector_t _adfs_bmap(struct addres
 static struct address_space_operations adfs_aops = {
 	.readpage	= adfs_readpage,
 	.writepage	= adfs_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= adfs_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= _adfs_bmap

Index: linux-2.6/fs/affs/file.c
===================================================================
@@ -409,7 +409,6 @@ static sector_t _affs_bmap(struct addres
 struct address_space_operations affs_aops = {
 	.readpage = affs_readpage,
 	.writepage = affs_writepage,
-	.sync_page = block_sync_page,
 	.prepare_write = affs_prepare_write,
 	.commit_write = generic_commit_write,
 	.bmap = _affs_bmap

@@ -762,7 +761,6 @@ out:
 struct address_space_operations affs_aops_ofs = {
 	.readpage = affs_readpage_ofs,
 	//.writepage = affs_writepage_ofs,
-	//.sync_page = affs_sync_page_ofs,
 	.prepare_write = affs_prepare_write_ofs,
 	.commit_write = affs_commit_write_ofs
 };

Index: linux-2.6/fs/afs/file.c
===================================================================
@@ -37,7 +37,6 @@ struct inode_operations afs_file_inode_o
 
 struct address_space_operations afs_fs_aops = {
 	.readpage	= afs_file_readpage,
-	.sync_page	= block_sync_page,
 	.set_page_dirty	= __set_page_dirty_nobuffers,
 	.releasepage	= afs_file_releasepage,
 	.invalidatepage	= afs_file_invalidatepage,

Index: linux-2.6/fs/befs/linuxvfs.c
===================================================================
@@ -75,7 +75,6 @@ static struct inode_operations befs_dir_
 
 static struct address_space_operations befs_aops = {
 	.readpage	= befs_readpage,
-	.sync_page	= block_sync_page,
 	.bmap		= befs_bmap,
 };
 

Index: linux-2.6/fs/bfs/file.c
===================================================================
@@ -156,7 +156,6 @@ static sector_t bfs_bmap(struct address_
 struct address_space_operations bfs_aops = {
 	.readpage	= bfs_readpage,
 	.writepage	= bfs_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= bfs_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= bfs_bmap,

Index: linux-2.6/fs/cifs/file.c
===================================================================
@@ -1383,34 +1383,6 @@ int cifs_fsync(struct file *file, struct
 	return rc;
 }
 
-/* static void cifs_sync_page(struct page *page)
-{
-	struct address_space *mapping;
-	struct inode *inode;
-	unsigned long index = page->index;
-	unsigned int rpages = 0;
-	int rc = 0;
-
-	cFYI(1, ("sync page %p",page));
-	mapping = page->mapping;
-	if (!mapping)
-		return 0;
-	inode = mapping->host;
-	if (!inode)
-		return; */
-
-/*	fill in rpages then 
-	result = cifs_pagein_inode(inode, index, rpages); */ /* BB finish */
-
-/*	cFYI(1, ("rpages is %d for sync page of Index %ld ", rpages, index));
-
-#if 0
-	if (rc < 0)
-		return rc;
-	return 0;
-#endif
-} */
-
 /*
  * As file closes, flush all cached write data for this inode checking
  * for write behind errors.

@@ -1952,6 +1924,5 @@ struct address_space_operations cifs_add
 	.prepare_write = cifs_prepare_write,
 	.commit_write = cifs_commit_write,
 	.set_page_dirty = __set_page_dirty_nobuffers,
-	/* .sync_page = cifs_sync_page, */
 	/* .direct_IO = */
 };

Index: linux-2.6/fs/efs/inode.c
===================================================================
@@ -23,7 +23,6 @@ static sector_t _efs_bmap(struct address
 }
 static struct address_space_operations efs_aops = {
 	.readpage = efs_readpage,
-	.sync_page = block_sync_page,
 	.bmap = _efs_bmap
 };
 

Index: linux-2.6/fs/ext2/inode.c
===================================================================
@@ -688,7 +688,6 @@ struct address_space_operations ext2_aop
 	.readpage		= ext2_readpage,
 	.readpages		= ext2_readpages,
 	.writepage		= ext2_writepage,
-	.sync_page		= block_sync_page,
 	.prepare_write		= ext2_prepare_write,
 	.commit_write		= generic_commit_write,
 	.bmap			= ext2_bmap,

@@ -706,7 +705,6 @@ struct address_space_operations ext2_nob
 	.readpage		= ext2_readpage,
 	.readpages		= ext2_readpages,
 	.writepage		= ext2_nobh_writepage,
-	.sync_page		= block_sync_page,
 	.prepare_write		= ext2_nobh_prepare_write,
 	.commit_write		= nobh_commit_write,
 	.bmap			= ext2_bmap,

Index: linux-2.6/fs/ext3/inode.c
===================================================================
@@ -1703,7 +1703,6 @@ static struct address_space_operations e
 	.readpage	= ext3_readpage,
 	.readpages	= ext3_readpages,
 	.writepage	= ext3_ordered_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= ext3_prepare_write,
 	.commit_write	= ext3_ordered_commit_write,
 	.bmap		= ext3_bmap,

@@ -1717,7 +1716,6 @@ static struct address_space_operations e
 	.readpage	= ext3_readpage,
 	.readpages	= ext3_readpages,
 	.writepage	= ext3_writeback_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= ext3_prepare_write,
 	.commit_write	= ext3_writeback_commit_write,
 	.bmap		= ext3_bmap,

@@ -1731,7 +1729,6 @@ static struct address_space_operations e
 	.readpage	= ext3_readpage,
 	.readpages	= ext3_readpages,
 	.writepage	= ext3_journalled_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= ext3_prepare_write,
 	.commit_write	= ext3_journalled_commit_write,
 	.set_page_dirty	= ext3_journalled_set_page_dirty,

Index: linux-2.6/fs/fat/inode.c
===================================================================
@@ -201,7 +201,6 @@ static struct address_space_operations f
 	.readpages	= fat_readpages,
 	.writepage	= fat_writepage,
 	.writepages	= fat_writepages,
-	.sync_page	= block_sync_page,
 	.prepare_write	= fat_prepare_write,
 	.commit_write	= fat_commit_write,
 	.direct_IO	= fat_direct_IO,

Index: linux-2.6/fs/freevxfs/vxfs_subr.c
===================================================================
@@ -45,7 +45,6 @@ static sector_t		vxfs_bmap(struct addres
 struct address_space_operations vxfs_aops = {
 	.readpage =		vxfs_readpage,
 	.bmap =			vxfs_bmap,
-	.sync_page =		block_sync_page,
 };
 
 inline void

Index: linux-2.6/fs/hfs/inode.c
===================================================================
@@ -117,7 +117,6 @@ static int hfs_writepages(struct address
 struct address_space_operations hfs_btree_aops = {
 	.readpage	= hfs_readpage,
 	.writepage	= hfs_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= hfs_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= hfs_bmap,

@@ -127,7 +126,6 @@ struct address_space_operations hfs_btre
 struct address_space_operations hfs_aops = {
 	.readpage	= hfs_readpage,
 	.writepage	= hfs_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= hfs_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= hfs_bmap,

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
@@ -112,7 +112,6 @@ static int hfsplus_writepages(struct add
 struct address_space_operations hfsplus_btree_aops = {
 	.readpage	= hfsplus_readpage,
 	.writepage	= hfsplus_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= hfsplus_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= hfsplus_bmap,

@@ -122,7 +121,6 @@ struct address_space_operations hfsplus_
 struct address_space_operations hfsplus_aops = {
 	.readpage	= hfsplus_readpage,
 	.writepage	= hfsplus_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= hfsplus_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= hfsplus_bmap,

Index: linux-2.6/fs/hpfs/file.c
===================================================================
@@ -102,7 +102,6 @@ static sector_t _hpfs_bmap(struct addres
 struct address_space_operations hpfs_aops = {
 	.readpage = hpfs_readpage,
 	.writepage = hpfs_writepage,
-	.sync_page = block_sync_page,
 	.prepare_write = hpfs_prepare_write,
 	.commit_write = generic_commit_write,
 	.bmap = _hpfs_bmap

Index: linux-2.6/fs/isofs/compress.c
===================================================================
@@ -314,7 +314,6 @@ eio:
 
 struct address_space_operations zisofs_aops = {
 	.readpage = zisofs_readpage,
-	/* No sync_page operation supported? */
 	/* No bmap operation supported */
 };
 

Index: linux-2.6/fs/isofs/inode.c
===================================================================
@@ -1054,7 +1054,6 @@ static sector_t _isofs_bmap(struct addre
 
 static struct address_space_operations isofs_aops = {
 	.readpage = isofs_readpage,
-	.sync_page = block_sync_page,
 	.bmap = _isofs_bmap
 };
 

Index: linux-2.6/fs/jfs/inode.c
===================================================================
@@ -310,7 +310,6 @@ struct address_space_operations jfs_aops
 	.readpages	= jfs_readpages,
 	.writepage	= jfs_writepage,
 	.writepages	= jfs_writepages,
-	.sync_page	= block_sync_page,
 	.prepare_write	= jfs_prepare_write,
 	.commit_write	= nobh_commit_write,
 	.bmap		= jfs_bmap,

Index: linux-2.6/fs/jfs/jfs_metapage.c
===================================================================
@@ -580,7 +580,6 @@ static void metapage_invalidatepage(stru
 struct address_space_operations jfs_metapage_aops = {
 	.readpage	= metapage_readpage,
 	.writepage	= metapage_writepage,
-	.sync_page	= block_sync_page,
 	.releasepage	= metapage_releasepage,
 	.invalidatepage	= metapage_invalidatepage,
 	.set_page_dirty	= __set_page_dirty_nobuffers,

Index: linux-2.6/fs/minix/inode.c
===================================================================
@@ -338,7 +338,6 @@ static sector_t minix_bmap(struct addres
 static struct address_space_operations minix_aops = {
 	.readpage = minix_readpage,
 	.writepage = minix_writepage,
-	.sync_page = block_sync_page,
 	.prepare_write = minix_prepare_write,
 	.commit_write = generic_commit_write,
 	.bmap = minix_bmap

Index: linux-2.6/fs/ntfs/aops.c
===================================================================
@@ -1546,8 +1546,6 @@ err_out:
  */
 struct address_space_operations ntfs_aops = {
 	.readpage	= ntfs_readpage,	/* Fill page with data. */
-	.sync_page	= block_sync_page,	/* Currently, just unplugs the
-						   disk request queue. */
 #ifdef NTFS_RW
 	.writepage	= ntfs_writepage,	/* Write dirty page to disk. */
 #endif /* NTFS_RW */

@@ -1562,8 +1560,6 @@ struct address_space_operations ntfs_aop
  */
 struct address_space_operations ntfs_mst_aops = {
 	.readpage	= ntfs_readpage,	/* Fill page with data. */
-	.sync_page	= block_sync_page,	/* Currently, just unplugs the
-						   disk request queue. */
 #ifdef NTFS_RW
 	.writepage	= ntfs_writepage,	/* Write dirty page to disk. */
 	.set_page_dirty	= __set_page_dirty_nobuffers,	/* Set the page dirty

Index: linux-2.6/fs/ntfs/inode.c
===================================================================
@@ -1830,7 +1830,7 @@ int ntfs_read_inode_mount(struct inode *
 	/* Need this to sanity check attribute list references to $MFT. */
 	vi->i_generation = ni->seq_no = le16_to_cpu(m->sequence_number);
 
-	/* Provides readpage() and sync_page() for map_mft_record(). */
+	/* Provides readpage() for map_mft_record(). */
 	vi->i_mapping->a_ops = &ntfs_mst_aops;
 
 	ctx = ntfs_attr_get_search_ctx(ni, m);

Index: linux-2.6/fs/ocfs2/aops.c
===================================================================
@@ -672,6 +672,5 @@ struct address_space_operations ocfs2_ao
 	.prepare_write	= ocfs2_prepare_write,
 	.commit_write	= ocfs2_commit_write,
 	.bmap		= ocfs2_bmap,
-	.sync_page	= block_sync_page,
 	.direct_IO	= ocfs2_direct_IO
 };

Index: linux-2.6/fs/qnx4/inode.c
===================================================================
@@ -451,7 +451,6 @@ static sector_t qnx4_bmap(struct address
 static struct address_space_operations qnx4_aops = {
 	.readpage	= qnx4_readpage,
 	.writepage	= qnx4_writepage,
-	.sync_page	= block_sync_page,
 	.prepare_write	= qnx4_prepare_write,
 	.commit_write	= generic_commit_write,
 	.bmap		= qnx4_bmap

Index: linux-2.6/fs/reiserfs/inode.c
===================================================================
@@ -3002,7 +3002,6 @@ struct address_space_operations reiserfs
 	.readpages = reiserfs_readpages,
 	.releasepage = reiserfs_releasepage,
 	.invalidatepage = reiserfs_invalidatepage,
-	.sync_page = block_sync_page,
 	.prepare_write = reiserfs_prepare_write,
 	.commit_write = reiserfs_commit_write,
 	.bmap = reiserfs_aop_bmap,

Index: linux-2.6/fs/sysv/itree.c
===================================================================
@@ -468,7 +468,6 @@ static sector_t sysv_bmap(struct address
 struct address_space_operations sysv_aops = {
 	.readpage = sysv_readpage,
 	.writepage = sysv_writepage,
-	.sync_page = block_sync_page,
 	.prepare_write = sysv_prepare_write,
 	.commit_write = generic_commit_write,
 	.bmap = sysv_bmap

Index: linux-2.6/fs/udf/file.c
===================================================================
@@ -98,7 +98,6 @@ static int udf_adinicb_commit_write(stru
 struct address_space_operations udf_adinicb_aops = {
 	.readpage		= udf_adinicb_readpage,
 	.writepage		= udf_adinicb_writepage,
-	.sync_page		= block_sync_page,
 	.prepare_write		= udf_adinicb_prepare_write,
 	.commit_write		= udf_adinicb_commit_write,
 };

Index: linux-2.6/fs/udf/inode.c
===================================================================
@@ -135,7 +135,6 @@ static sector_t udf_bmap(struct address_
 struct address_space_operations udf_aops = {
 	.readpage		= udf_readpage,
 	.writepage		= udf_writepage,
-	.sync_page		= block_sync_page,
 	.prepare_write		= udf_prepare_write,
 	.commit_write		= generic_commit_write,
 	.bmap			= udf_bmap,

Index: linux-2.6/fs/ufs/inode.c
===================================================================
@@ -534,7 +534,6 @@ static sector_t ufs_bmap(struct address_
 struct address_space_operations ufs_aops = {
 	.readpage = ufs_readpage,
 	.writepage = ufs_writepage,
-	.sync_page = block_sync_page,
 	.prepare_write = ufs_prepare_write,
 	.commit_write = generic_commit_write,
 	.bmap = ufs_bmap

Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
@@ -1452,7 +1452,6 @@ struct address_space_operations xfs_addr
 	.readpage		= xfs_vm_readpage,
 	.readpages		= xfs_vm_readpages,
 	.writepage		= xfs_vm_writepage,
-	.sync_page		= block_sync_page,
 	.releasepage		= xfs_vm_releasepage,
 	.invalidatepage		= xfs_vm_invalidatepage,
 	.prepare_write		= xfs_vm_prepare_write,

Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
@@ -1521,7 +1521,6 @@ xfs_mapping_buftarg(
 	struct inode		*inode;
 	struct address_space	*mapping;
 	static struct address_space_operations mapping_aops = {
-		.sync_page = block_sync_page,
 		.migratepage = fail_migrate_page,
 	};
 

"the bug". Are we suposed to know what yo're referring to here?

The performance risk is that someone will do lock_page() against a page whose IO is queued-but-not-yet-kicked-off. We'll go to sleep with no IO submitted until kblockd or someone else kicks off the IO for us.

Try disabling kblockd completely, see what effect that has on performance.

What this means is that it is not legal to run lock_page() against a pagecache page if you don't have a ref on the inode.

iirc the main (only?) offender here is direct-io reads into MAP_SHARED pagecache. (And similar things, like infiniband and nfs-direct).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

You were supposed to know, if I hadn't made a typo :) "a bug".

Yes.

Which is what I want to know. I don't exactly have an interesting disk setup.

Yes. So set_page_dirty_lock is broken, right?
And the wait_on_page_stuff needs an inode ref.
Also splice seems to have broken sync_page.

Well yes, writing to a page would be the main reason to set it dirty. Is splice broken as well? I'm not sure that it always has a ref on the inode when stealing a page.

It sounds like you think fixing the set_page_dirty_lock callers wouldn't be too difficult? I wouldn't know (although the ptrace one should be able to be turned into a set_page_dirty, because we're holding mmap_sem).

You're sure about all other lock_page()rs? I'm not, given that set_page_dirty_lock got it so wrong. But you'd have a better idea than me.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

You don't need one - just a single disk should show up such problems. I forget which workloads though. Perhaps just a linear read (readahead queues the I/O but doesn't unplug, subsequent lock_page() sulks).

yup.

Please describe the splice() problem which you've observed.

Whereabouts?

No, I think it's damn impossible ;)

get_user_pages() has gotten us a random pagecache page. How do we non-racily get at the address_space prior to locking that page?

I don't think we can.

No, I'm not sure.

However it is rare for the kernel to play with pagecache pages against which the caller doesn't have an inode ref. Think: how did the caller look up that page in the first place if not from the address_space in the first place?

- get_user_pages(): the current problem

- page LRU: OK, uses trylock first.

- pagetable walk??

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I guess so. Is plugging still needed now that the IO layer should get larger requests? Disabling it might result in a small initial request (although even that may be good for pipelining)...

Otherwise, we could make set_page_dirty_lock use a weird non-unplugging variant (although maybe that will upset Ken), but I'd rather look at simplification first ;)

sync_page wants to get either the current mapping, or a NULL one. The sync_page methods must then be able to handle running into a NULL mapping.

With splice, the mapping can change, so you can have the wrong sync_page callback run against the page.

The ->pin() calls in pipe_to_file and pipe_to_sendpage?

But the vma isn't going to disappear because mmap_sem is held; and the vma should hold a ref on the inode I think?

Am I wrong about mmap_sem?

Anyway, it is possible that most of the problems could be solved by locking the page at the time of lookup, and unlocking it on completion/dirtying... it's just that that would be a bit of a task. Can we somehow add BUG_ONs to lock_page to ensure we've got an inode ref?

--

Send instant messages to your online friends http://au.messenger.yahoo.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mysterious question, that. A few years ago I think Jens tried pulling unplugging out, but some devices still want it (magneto-optical storage iirc). And I think we did try removing it, and it caused hurt.

Yes, that would work. In fact the number of times when direct-io actually calls set_page_dirty_lock() is infinitesimal - I had to jump through hoops to even test that code. The speculative set-the-page-dirty-before-we-do-the-IO thing is very effective. So the performace impact of making such a change would be nil.

That's for the direct-io case. Other cases might be hurt more.

Also, perhaps we could poke kblockd to do it for us.

Oh.

One for Jens...

That's true during the get_user_pages() call. Be we run set_page_dirty_lock() much later, after IO completion.

But lock_page() requires access to the address_space. To kick the IO so we don't wait for ever.

WARN_ONs.

Would be nice to get rid of it. I guess that's out of the question for several releases, even if there was no performance problems.

OK, that'll probably be the way to go for upcoming releases. I'll post a patch soon.

Hard, I think, to get back to ->mapping. Well not hard if we just use a non-syncing lock_page, but in that case we don't need kblockd.

Don't know what we can do about that off the top of my head. Within block_sync_page there shouldn't be any problems (at worst, we'd unplug the wrong dev). Maybe the whole sync_page callback scheme can be removed so nobody tries to do anything funny with it? Call blk_run_backing_dev directly.

Oh, I thought you specifically meant the ptrace case. Yes, in general it is much harder.

It shouldn't wait for ever, because the unplug timer will go off and kblockd will do it eventually. And I was imagining that it would have a pin on the address space at the point it is looked up... But reworking all callers is just a pipe dream anyway, so nevermind ;)

Where we have synchronous IO, we do want the queue to be unplugged, however in most set_page_dirty_lock case this is normally not the case and a locked page should be rare. So hmm yes that would make sense to have it use a special lock_page...

But is it practical? Or I suspect the warning would only ever trigger in the really rare racy cases anyway, when dentry, inode, etc have been reclaimed.

Anyway, I'll come up with a less intrusive patch shortly...

--

Send instant messages to your online friends http://au.messenger.yahoo.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I did, back when we had problems due to the blk_plug_lock being a global one. I first wanted to investigate if plugging still made a difference, otherwise we could've just ripped it out back than and the problem would be solved. But it did get us about a 10% boost on normal SCSI drives (don't think I tested MO drives at all), so it was fixed up.

Maybe I'm being dense, but I don't see a problem there. You _should_ call the new mapping sync page if it has been migrated.

splice never incs/decs any inode related reference counts, so if it needs to then yes it's broken. Any references to kernel code that deals with that?

Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hi Jens,
Sorry, I don't think I gave you any reply to this...

Interesting. I'd like to know where from. I wonder if my idea of a process context plug/unplug would solve it...

But can some other thread calling lock_page first find the old mapping, and then run its ->sync_page which finds the new mapping? While it may not matter for anyone in-tree, it does break the API so it would be better to either fix it or rip it out than be silently buggy.

Most code in the VM that has an inode/mapping gets called from the VFS, which already does its thing somehow (I guess something like the file pins the dentry which pins the inode). An iget might solve it. Or you could use the lock_page_nosync() if/when the patch goes in (although I don't want that to spread too far just yet).

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Splicing a page from one mapping to another is rather worrying/exciting, but it does look safely done to me. remove_mapping checks page_count while page lock and old mapping->tree_lock are held, and gives up if anyone else has an interest in the page. And we already know it's unsafe to lock_page without holding a reference to the page, don't we?

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oh, that's true. I had thought that splice allows stealing pages with an elevated refcount, which Jens was thinking about at one stage. But I see that code isn't in mainline. AFAIKS it would allow other ->pin()ers to attempt to lock the page while it was being stolen.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

It got me in trouble, and we were already way too late into 2.6.17 to take on more risky stuff (splice already had a few!). So right now the page is definitely pruned and single while being stolen.

Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No worries :)

As far as I recall, just doing a simple diff between source directories on the same drive was a pretty good example of where the plugging gained you some. A little on the benchmarks as well, iirc. I would have _loved_ to rip plugging out at that point, so while I may not remember the details yet, don't think I did that work if I could have avoided it :-)

It looks ok to me, you can inspect fs/splice.c:pipe_to_file() and see if you can spot anything.

I'll be sure to look over that, thanks!

Jens Axboe

Why do you think the IO layer should get larger requests?

I really don't understand why people dislike plugging. It's obviously superior to non-plugged variants, exactly because it starts the IO only when _needed_, not at some random "IO request feeding point" boundary.

In other words, plugging works _correctly_ regardless of any random up-stream patterns. That's the kind of algorithms we want, we do NOT want to have the IO layer depend on upstream always doing the "Right Thing(tm)".

So exactly why would you want to remove it?

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

For workloads where plugging helps (ie. lots of smaller, contiguous requests going into the IO layer), should be pretty good these days due to multiple readahead and writeback.

If you submit IO, it will be needed at some point in time. The benefit of plugging is to hold it there in anticipation of more IO to merge with this request.

Not sure what you mean by IO request feeding point...?

It is a heuristic. I don't see anything obviously correct or incorrect about it. If you were to submit some asynch requests and _knew_ that no other requests would be merged with them, plugging would be the wrong thing to do because the disks would be needlessly idle.

Because of this bug ;)

I'm not saying it would never be of any benefit; I was hoping that it might be unneeded and getting rid of it would fix this bug and simplify a lot too.

Anyway, clearly a plug friendly bugfix needs to be implemented regardless.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Let me try again.

For workloads where plugging helps (ie. lots of smaller, contiguous requests going into the IO layer), the request pattern should be pretty good without plugging these days, due to multiple page readahead and writeback.

Taken to its logical conclusion, you are saying readahead / dirty page writeout throttling is obviously inferior, aren't you?

Non-rhetorically: Obviously there can be regressions in plugging, because you are holding the disk idle when you know there is work to be done.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Can I please put in a vote for not thinking that every device is disk drive?

I find plugging fairly important for raid5, particularly for write.

The more whole-stripe writes I can get, the better throughput I get. So I tend to keep a raid5 array plugged while any requests are arriving, and interpret 'plugged' to mean that incomplete stripes don't get processed while full stripes (needing no pre-reading) do get processed.

The only way "large requests" are going to replace plugging is they are perfectly aligned, which I don't expect to ever see.

As for your original problem.... I wonder if PG_locked is protecting too much? It protects against IO and it also protects against ->mapping changes. So if you want to ensure that ->mapping won't change, you need to wait for any pending read request to finish, which seems a bit dumb.
Maybe we need a new bit: PG_maplocked. You are only allowed to change ->mapping or ->index of you hold PG_locked and PG_maplocked, you are not allowed to wait for PG_locked while holding PG_maplocked, and you can read ->mapping or ->index while PG_locked or PG_maplocked are held.
Think of PG_locked like a mutex and PG_maplocked like a spinlock (and probably use bit_spinlock to get it).

Then set_page_dirty_lock would use PG_maplocked to get access to ->mapping, and then hold a reference on the address_space while calling into balance_dirty_pages ... I wonder how you hold a reference on an address space...

There are presumably few pieces of code that change ->mapping. Once they all take PG_maplocked as well as PG_locked, you can start freeing up other code to take PG_maplocked instead of PG_locked....

Does that make sense at all? Do we have any spare page bits?

NeilBrown

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Fair enough, thanks for the input. I was more imagining that IO tends to come down in decent chunks, but obviously that's still not sufficient for some. OK.

I don't think that is the problem. set_page_dirty_lock is really unlikely to get held up on read IO: that'd mean there were two things writing into that page at the same time.

Well the original problem is fixed by not doing the sync_page thing in set_page_dirty_lock. Is there any advantage to having another bit? Considering a) it will be very unlikely that a page is locked at the same time one would like to dirty it; and b) that would seem to imply adding extra atomic ops and barriers to reclaim and truncate (maybe others).

inode. Presumably PG_maplocked would pin it? I don't understand why you've brought balance_dirty_pages into it, though.

I'm sure it could be made to work, but I don't really see the point. If someone really wanted to do it, I guess the right way to go is have a PG_readin counterpart to PG_writeback (or even extend PG_writeback to PG_io)...

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

That's exactly my point - though I expand a bit more further down. i.e. set_page_dirty_lock is unlikely to get held up on a read IO, however it has to call into lock_page, and lock_page has no idea that it cannot be waiting for IO, and so it has to call sync_page just in case.

While I agree that avoiding the sync_page in this particular case is likely to solve the problem, it seems rather fragile. Sometimes you need to call ->sync_page when waiting for a lock, sometimes you don't. Why the difference? Because there really are two different locks here masquerading as one.

Yes there would be extra atomic ops at reclaim and truncate. Is that a problem? I have no idea, but you sometimes need to break eggs to fix an omelette. I suspect the extra bit would be very lightly contended.

hmmmm.... no idea, sorry.
I was trying to trace through set_page_dirty to see if it was safe to call it under a spinlock (or a bit_spinlock in this case). I must have taken a wrong turn somewhere...

Yes, PG_readin or PG_io would be better names, but might be hard to have a graceful transition to that sort of naming.

And the point is that once you separated that function from PG_locked, lock_page would not need to wait for IO, and so would not need to call sync_page, and so your problem would evaporate.

NeilBrown

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pageout by VM scanner doesn't benefit from those, and it is still quite important in some workloads (e.g., mmap intensive).

Nikita.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No.

That's fundamentally wrong.

The fact is, plugging is not about read-ahead and writeback. It's very fundamentally about the _boundaries_ between multiple requests, and in particular the time when the queue starts out empty so that we can build up things for devices that wand big requests, but even more so for devices where _seeking_ is very expensive.

Those boundaries haven't gone anywhere. The fact that we do read-ahead and write-back in chunks doesn't change anything: yes, we often have the "big requests" thing handled, but (a) not always and (b) upper layers fundamentally don't fix the seek issues.

I want to know that the block layer could - if we wanted to - do things like read-ahead for many distinct files, and for metadata. We don't currently do much of that yet, but the point is, plugging _allows_ us to. Exactly because it doesn't depend on upper layers feeding everything in one go.

Look at "sys_readahead()", and realize that it can be used to start IO for read ahead _across_many_small_files_. Last I tried it, it was hugely faster at populating the page cache than reading individual files (I used to do it with BK to bring everything into cache so that the regular ops would be fster - now git doesn't much need it).

And maybe it was just my imagination, but the disk seemed quieter too. It should be able to do better seek patterns at the beginning due to plugging (ie we won't start IO after the first file, but after the request queue fills up or something else needs to wait and we do an unplug event).

THAT is what plugging is good for. Our read-ahead does well for large requests, and that's important for some disk controllers in particular. But plugging is about avoiding startign the IO too early.

Think about the TCP plugging (which is actually newer, but perhaps easier to explain): it's useful not for the big file case (just use large reads and writes), but for the "different sources" case - for handling the gap between a header and the actual file contents. Exactly because it plugs in _between_ events.

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The requests can only get merged if contiguous requests from the upper layers come down, right?

So in a random IO workload, plugging is unlikely to help at all. In a contiguous IO workload, mpage should take *some* of the burden off plugging. But OK, it turns out not always, I accept that.

Why would plugging help if the requests can't get merged, though?

TCP plugging is a bit different because there is no page cache between the application and the device; and it is stream based so everything can be merged (within a single socket).

The same high level concept I agree, but I never said the concept was wrong; just hoped that as a heuristic, the block plugging was no longer useful. I've been set straight about that though ;)

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

It has nothing to do with merging. It has to do with IO patterns.

Seeking.

Seeking is damn expensive - much more so than command issue. People forget that sometimes.

If you can sort the requests so that you don't have to seek back and forth, that's often a HUGE win.

Yes, the requests will still be small, and yes, the IO might happen in 4kB chunks, but it happens a lot faster if you do it in a good elevator ordering and if you hit the track cache than if you seek back and forth.

And part of that is that you have to submit multiple requests when you start, and allow the elevator to work on it.

Now, of course, if you have tons of reqeusts already in flight, you don't care (you already have lots of work for the elevator), but at least in desktop loads the "starting from idle" case is pretty common. Getting just a few requests to start up with is good.

(Yes, tagged queueing makes it less of an issue, of course. I know, I know. But I _think_ a lot of disks will start seeking for an incoming command the moment they see it, just to get the best latency, rather than wait a millisecond or two to see if they get another request. So even with tagged queuing, the elevator can help, _especially_ for the initial request).

Why do you think we _have_ an elevator in the first place?

And just how well do you think it works if you submit one entry at a time (regardless of how _big_ it is) and start IO on it immediately? Vs trying to get several IO's out there, so that we can say "do this one first".

Sometimes I think harddisks have gotten too quiet - people no longer hear it when access patters are horrible. But the big issue with plugging was only partially about request coalescing, and was always about trying to get the _order_ right when you start to actually submit the requests to the hardware.

And yes, I realize that modern disks do remapping, and that we will never do a "perfect" job. But it's still true that the block number has _some_ (fairly big, in fact) relationship to the actual disk layout, and that avoiding seeking is a big deal.

Rotational latency is often an even bigger issue, of course, but we can't do much about that. We really can't estimate where the head is, like people used to try to do three decades ago. _That_ time is long past, but we can try to avoid long seeks, and it's still true that you can get blocks that are _close_ faster (if only because they may end up being on the same cylinder and not need a seek).

Even better than "same cylinder" is sometimes "same cache block" - disks often do track caching, and they aren't necessarily all that smart about it, so even if you don't read one huge contiguous block, it's much better to read an area _close_ to another than seek back and forth, because you're more likely to hit the disks own track cache.

And I know, disks aren't as sensitive to long seeks as they used to be (a short seek is almost as expensive as a long one, and a lot of it is the head settling time), but as another example - I think for CD-ROMs you can still have things like the motor spinning faster or slower depending on where the read head is, for example, meaning that short seeks are cheaper than long ones.

(Maybe constant angular velocity is what people use, though. I dunno).

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

My observations with (S)ATA tagged/native queuing, is that it doesn't make nearly the difference under Linux that it does under other OSs. Probably because our block layer is so good at ordering requests, either from plugging or simply from clever disk scheduling.

Yup. Agreed!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hmm well, I have seen 30% performance increase for a random read work load with NCQ, I'd say that is pretty nice. And of course there's the whole write cache issue, with NCQ you _could_ get away with playing more safe and disabling write back caching.

NCQ helps us with something we can never fix in software - the rotational latency. Ordering is only a small part of the picture.

Plus I think that more recent drives have a better NCQ implementation, the first models I tried were pure and utter crap. Lets just say it didn't instill a lot of confidence in firmware engineers at various unnamed drive companies.

Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yup. And it also helps reduce the command-to-command latencies.

I'm all for it, and have implemented tagged queuing for a variety of device drivers over the past five years (TCQ & NCQ). In every case people say.. wow, I expected more of a difference than that, while still noting the end result was faster under Linux than MS$.

Of course with artificial benchmarks, and the right firmware in the right drives, it's easier to create and see a difference. But I'm talking more life-like loads than just a multi-threaded random seek generator.

Cheers

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

That too, however that's a much much smaller part.

Definitely, the random pseudo read work load is about as good as it gets for NCQ, and it was tailored to keep the queue full at all times. So I agree, real life will see less of a benefit. But it's still there.

Jens Axboe

Ok, I decided to rerun a simple random read work load (with fio), using depths 1 and 32. The test is simple - it does random reads all over the drive size with 4kb block sizes. The reads are O_DIRECT. The test pattern was set to repeatable, so it's going through the same workload. The test spans the first 32G of the drive and runtime is capped at 20 seconds.

Now of course this is truly an artificial work load, however it's still interesting from the POV of what NCQ can potentially do for you. If you want something specific run, let me know and I can quickly do so (eg write through cache random writes).

sda:    Maxtor 7B300S0
sdb:    Maxtor 7L320S0
sdc:    SAMSUNG HD160JJ
sdd:    HDS725050KLA360 (Hitachi 500GB drive)

drive depth KiB/sec diff
----------------------------------------------------

sda              1              397
sda             32              685             +72%
sdb              1              397
sdb             32              525             +32%
sdc              1              372
sdc             32              511             +37%
sdd              1              489
sdd             32              942             +92%

fio file used, modify depth and device name, of course:

[/dev/sdd]
size=32g
ioengine=libaio
iodepth=32
rw=randread
bs=4k
timeout=20
direct=1

Jens Axboe

Did you modify the iodepth given to the test program, or to the drive? If the former, then some of the performance increase came from the Linux elevator.

Ideally exactly the same test would be run with the just the drive parameters changed.

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Just from the program. Since the software depth matched the software depth, I'd be surprised if it made much of a difference here. I can rerun the same test tomorrow with the drive depth modified the and software depth fixed at 32. Then the io scheduler can at least help the drive without NCQ out somewhat.

Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Same test, but with iodepth=48 for both ncq depth 1 and ncq depth 31. This gives the io scheduler something to work with for both cases.

sda:    Maxtor 7B300S0
sdb:    Maxtor 7L320S0
sdc:    SAMSUNG HD160JJ
sdd:    HDS725050KLA360 (Hitachi 500GB drive)

drive depth KiB/sec diff diff 1/1
----------------------------------------------------------------

sda              1/1            397
sda              1              513             +29%
sda             31              673             +31+    +69%

sdb              1/1            397
sdb              1              535             +35%
sdb             31              741             +38%    +87%

sdc              1/1            372
sdc              1              449             +21%
sdc             31              507             +13%    +36%

sdd              1/1            489
sdd              1              650             +33%
sdd             31              941             +45%    +92%

Conclusions: the io scheduler helps, NCQ help - both combined helps a lot. The Samsung firmware looks bad. Additional requests in io scheduler when using NCQ doesn't help, except for the new firmware Maxtor. Suspect. NCQ still helps a lot, > 30% for all drives except the Samsung.

Jens Axboe

NCQ can reorder to prefer small seeks to rotational delays, which the io scheduler can't due to lack of knowledge of the 2D geometry. Your measurements show that the larger drives benefit the most, as the fixed seek range means these drives have to seek less. Full range results would probably be a lot worse.

It would probably be possible to measure the drive geometry by experiment and teach the results to the io scheduler, and get the same benefits as NCQ, but that experiment could run for a long time.

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Eh yes, that's the entire point of NCQ basically, to get around rotational delays.

Check citeseer, there's been various efforts on that the last X years. It's always been ugly and memory consuming, and with NCQ in even commodity desktop machines today, it would be stupid to pursue such an effort.

Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

This is correct, but doesn't really explain why plugging might be good.

If requests go to disk immediately and the disk is able to keep up with the seeks, then plugging doesn't help. This is a low-bandwith case of course, but servicing each request immediately will keep the latency lower. The fact that the disk gets busier doesn't matter unless you worry about power consumption for the access arm.

If lots of requests come in and we don't do plugging, then lots of the requests will be nicely sorted while waiting for the first seek. And when this sorted lot goes to disk, further requests will be sorted. I.e. congestion itself can work as dynamic plugging, giving longer and longer sorted queues until the disk keeps up with us.

This should get us just enough sorting for the disk to keep up with the bandwidth we demand, and therefore minimal latency for that bandwith.

This is theory, and perhaps it doesn't quite reflect reaility. I cannot see why it shouldn't, though - it'd be interesting to know if I missed something obvious here.

Helge Hafting

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

only for writes. afaik for reads and other synchronous beasts we already unplug immediately anyway.

In addition unplugging happens after like 3 or 4 requests and after a few miliseconds, whatever comes first ... so you never wait really long (on a "what a seek costs" scale) anyway

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

OK, I didn't forget that it is expensive, but I didn't make the connection that this is what you were arguing for.

I would be surprised if plugging made a big difference to seeking:
1. the queue will be plugged only if there are no other requests (so, nothing else to seek to).
2. if the queue was plugged and we submitted one small request, another request to somewhere else on the drive that comes in can itself unplug the queue and cause seeking.

as-iosched is good at cutting down seeks because it doesn't "unplug" in situation #2.

Now having a mechanism for a task to batch up requests might be a good idea. Eg.

plug();
submit reads
unplug();
wait for page

I'd think this would give us the benefits of corse grained (per-queue) plugging and more (e.g. it works when the request queue isn't empty). And it would be simpler because the unplug point is explicit and doesn't need to be kicked by lock_page or wait_on_page

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

How's this different from what we have now? The plugging will happen implicitly, if we need to. If the queue is already running, chances are that there are requests there so you won't get to your first read first anyways.

The unplug(); wait_for_page(); is already required unless you want to wait for the plugging to time out (unlikely, since you are now waiting for io completion on one of them).

I kind of like having the implicit unplug, for several reasons. One is that people forget to unplug. We had all sorts of hangs there in 2.4 and earlier because of that. Making the plugging implicit should help that though. The other is that I don't see what the explicit unplug gains you. Once you start waiting for one of the pages submitted, that is exactly the point where you want to unplug in the first place.

Jens Axboe

OK I wasn't aware it was explicit in 2.4 and earlier.

Two upsides I see to explicit: firstly, it works on non-empty queues. Less of a problem perhaps, but only because it is statistically less likely to be the next submitted, so there will still be some improvement.

Second, for async work (aio, readahead, writeback, writeout for page reclaim), the point where you wait is probably not the best place to unplug.

Also, it would allow lock_page to be untangled.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

What do you think we're _talking_ about?

What do you think my example of sys_readahead() was all about?

WE DO HAVE EXACTLY THAT MECHANISM. IT'S CALLED PLUGGING!

What do you think plugging IS?

It's _exactly_ what you're talking about. And yes, we used to have explicit unplugging (a long long long time ago), and IT SUCKED. People would forget, but even more importantly, people would do it even when not needed because they didn't have a good place to do it because the waiter was in a totally different path.

The reason it's kicked by wait_on_page() is that is when it's needed.

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Plugging. Emphasis on task.

It isn't exactly that. I'm thinking per-task rather than per-queue might be a better idea because a) you don't know what else is putting requests on the queue, and b) the point where you wait is not always the best place to unplug.

Yes, and I'm talking about per-task vs per-queue plugging (because I want to get rid of the implicit unplug, because I want to make lock_page & co nicer).

I don't see what the problem is. Locks also suck if you forget to unlock them.

Example?

Yes, you already said that, and I showed cases where that isn't optimal.

I don't know why you think this way of doing plugging is fundamentally right and anything else must be wrong... it is always heuristic, isn't it?

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Locks are simple, and in fact are _made_ simple on purpose. We try very hard to unlock in the same function that we lock, for example. Because if we don't, bugs happen.

That's simply not _practical_ for IO. Think about it. Quite often, the waiting is done somewhere else than the actual submission.

Pretty much all of them.

Where do you wait for IO?

Would you perhaps say "wait_on_page()"?

In other words, we really _do_ exactly what you think we should do.

A _particular_ way of doing plugging is not "fundamentally right". I'm perfectly happy with chaning the place we unplug, if people want that. We've done it several times.

But plugging as a _concept_ is definitely fundamentally right, exactly because it allows us to have the notion of "plug + n*<random submit by different paths> + unplug".

And you were not suggesting moving unplugging around. You were suggesting removing the feature. Which is when I said "no f*cking way!".

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[trimming CC list]

I still think the submitter should plug before they start a set of requests (the submitter currently does not plug, the queue does when it empties), and unplug when it has finished submission (not when a process next waits, because that is suboptimal for asynch IO).

When you do this, the plug/unplug should become simple like locks too.

You're really keen on unplugging at the point of waiting. I don't get it.

OK.

Yeah the concept isn't bad. I think the queue based implementation, and unplug at wait time isn't great.

It may be a good concept but if it doesn't help, then removing it is a good idea too ;) Now I've been told it does help, so instead of removing it I suggest changing it. Just exploring ideas.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Actually, no. I'm _not_ really keen on unplugging at the point of waiting.

I'm keen on unplugging at a point that makes sense, and is safe.

The problem is, you're not even exploring alternatives. Where the hell _would_ you unplug it?

You can't unplug it in the place where we submit IO. That's _insane_, because it basically means never plugging at all.

And most of the callers don't even _know_ whether we submit IO or not. For example, let's pick a real and relevant example (they don't get much more relevant than this): "do_generic_mapping_read()".

Tell me WHERE you can unplug in that sequence. I will tell you where you can NOT unplug:

- you can NOT unplug _anywhere_ inside of the read-ahead logic, because we obviously don't want it there (it would break the whole concept of read-ahead, not to mention real code like sys_readahead()).

- you can NOT unplug in "->readpage()", for similar reasons (readahead again, and again, the whole point of unplugging is that we want to do several readpages and then unplug later)

- you can NOT just unplug in the path _after_ "readpage()", because the IO may have been started by SOMEBODY ELSE that just did read-ahead, and didn't unplug (on _purpose_ - the whole point of doing read-ahead is to allow concurrent IO execution, so a read-aheader that unplugs is broken by definition)

Those three are not just my "personal ideas". They are fundamental to how unplugging works, and more importantly, to the whole _point_ of plugging.

Agreed?

Now, look at where we _currently_ unplug. I claim that there are exactly _two_ places where we have to unplug ((1) we find a page that is not up-to-date and (2) we've started a read on a page ourselves), and I claim that those two places are _exactly_ the two places where we currently do "lock_page()".

Again, this is not a "what if" schenario, or something where my "opinions" are at stake. This is hard, cold, fact. We could do the unplugging outside of the lock-page, but we'd do it in exactly that situation.

So what is your alternative? Put the explicit unplug at every possible occurrence of lock_page() (and keep in mind that some of them don't want
it: we only want it when the lock-page will block, which is not always true. Some people lock the page not because it's under IO, and they're waiting for it to be unlocked, but because it's dirty and they're going to start IO on it - the lock_page() generally won't block on that, and if it doesn't, we don't want to kick previous IO).

In other words, we actually want to unplug at lock_page(), BUT ONLY IF IT NEEDS OT WAIT, which is by no means all of them. So it's more than just "add an unplug at lock_page()" it's literally "add an unplug at any lock-page that blocks".

Still wondering why it ends up being _inside_ lock_page()?

Linus

Btw, don't get me wrong. In the read case, every time we do a lock_page(), we might as well unplug unconditionally, because we effectively know we're going to wait (we just checked that it's not up-to-date). Sure, there's a race window, but we don't care - the window is very small, and in the end, unplugging isn't a correctness issue as long as you do it at least as often as required (ie unplugging too much is ok and at worst just makes for bad performance - so a very unlikely race that causes _extra_ unplugging is fine as long as it's unlikely. forgetting to unplug is bad).

In the write case, lock_page() may or may not need an unplug. In those cases, it needs unplugging if it was locked before, but obviously not if it wasn't.

In the "random case" where we use the page lock not because we want to start IO on it, but because we need to make sure that page->mapping doesn't change, we don't really care about the IO, but we do need to unplug just to make sure that the IO will complete.

And I suspect your objection to unplugging is not really about unplugging itself. It's literally about the fact that we use the same page lock for IO and for the ->mapping thing, isn't it?

IOW, you don't actually dislike plugging itself, you dislike it due to the effects of a totally unrelated locking issue, namely that we use the same lock for two totally independent things. If the ->mapping thing were to use a PG_map_lock that didn't affect plugging one way or the other, you wouldn't have any issues with unplugging, would you?

And I think _that_ is what really gets us to the problem.

Linus

Nearly, but not quite that far: it's that we sync_page in lock_page.

I don't think using the single lock for both is too bad (in many ways they are related eg. you don't want the page to be truncated while IO is in progress).

No I don't dislike plugging at all ;) Just this tangle as you say.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

...

Umm, this happens with the current lock_page() after readpage. And with per-task plugs, you do not unplug anybody else.

Sure it isn't perfect (you can still concoct a sequence of concurrent tasks doing funny things that result in a slightly suboptimal request pattern), but to me it sounds as good or better in some cases than what we have now.

I keep telling you. Put the unplug after submission of IO. Not before waiting for IO.

I'll give you one example of how it could be better (feel free to ask for others too). Your sys_readahead(). Someone asks to readahead 1MB of data, currently if the queue is empty, that's going to sit around for 4ms before starting the read. 4ms later, the app comes back hoping for the request to have finished. Unfortunately it takes another 4ms. Throughput is halved.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

If this wasn't clear: I don't mean per-task plugs as in "the task explicitly plugs and unplugs the block device"[*]; I mean really per-task plugs.

[*] That would be crazy anyway because that would imply some random
task can plug an filled request queue that is going full tilt.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

That would be insane. It would mean that you'd have to unplug whether you wanted to or not. Ie you've now made "sys_readahead()" impossible to do well, and doing read-ahead across multiple files.

You're ignoring all the _reasons_ for plugging in the first place.

Linus

I don't think it is.

Now it is you who are ignoring what I've been saying. I've been saying that I don't think your sys_readahead examples have had much to do with plugging:

1. If there are no other requests to seek to, plugging doesn't matter;
2. If there are other requests to seek to, the queue won't be plugged or will soon become unplugged anyway. So the current system isn't somehow going to do the right thing every time and be immune to seeking.

I'm not, but the current system of plugging isn't ideal. Anyway, there isn't any point going on about it, if I want to change anything I at least need to come back with numbers.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Lastly, an app can still issue multiple sys_readaheads and still get seek protection -- it isn't like disks have suddenly got faster than CPUs overnight.

as-iosched.c can handle the case where a request goes out to disk, and later a new one comes in nearby (deadline IIRC isn't good at this unfortunately, nor does it seek backwards)

OTOH, if you're using as-iosched.c, you explicitly get a good amount of seek protection anyway.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

And that's exactly where we have the lock_page().

And you ignored the list of _requirements_ I had, so you just missed the other place you _have_ to have the unplug, namely in the "found a page that was not yet up-to-date" case (look for the other lock_page()). Because the person who started the IO might be off doing something else, and may not be unplugging now (or ever, in the case of readahead).

In other words, when you start arguing, at least read my emails. Your suggestion would have introduced a bug by not waiting on that other place.

Linus

In some cases.

I know you have to unplug there. I didn't ignore anything. Where is the problem?

No. I'm saying the "person" who started IO _could_ be the one unplugging. Always.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

I think this is all a question of heuristics. There is a trade off between throughput and latency, and making the right decisions require prescience, and I don't think either Intel or AMD have implemented yet.

One could argue that the best you can get would involve making decisions at the lowest level - where device characteristics are known - and using hints from upper levels like "I'm going to submit lots of requests now" and "Ok, I'm done" and "I want this page NOW". These should be hints and the device can treat them as it likes.

Maybe a disk device could estimate the time it would take to process the current plugged queue, and if the first request were older than half that time, unplug the queue, unless there was a genuine expectation of more requests very soon...

Currently the decisions are made at a reasonably low level, but the only hint is "I want some page somewhere on this device now, or maybe I just want to stop anyone changing ->mapping", which is a fairly broad and inaccurate hint....

But the real point is that as this is an heuristic, it is very hard to argue in the abstract about what is best. We really need measurements to show that changes to have a measurable effect.

I have an interesting issue with plugging and raid5 at the moment. As I mentioned earlier, plugging only affects incomplete stripes being written. So if a very long, totally sequential write is being issued (dd of=/dev/md0) then we never really need to unplug (except may at the end). But unplugging does happen (3msec timeout at least) and so we sometimes end up pre-reading for partial stripes even though there is really no need.

What I would really like is for ->unplug_fn to know something about why it is being called.
- if it is for read, I'll unplug component devices, but not incomplete writes.
- if it is for write, I'll unplug writes, but I'd really rather know which address was required
- if it is a timeout, I'll unplug writes that have been pending for more than 2msec.

I suspect there is at least one workload that this would help, but I would need to measure to be sure.

... maybe I'll try adding a 'how' option to ->unplug_fn and see what I can come up with...

NeilBrown

Btw, that's not how it has always been done.

For the longest time, it was actually triggered by scheduler activity, in particular, plugging used to be a workqueue event that was triggered by the scheduler (or any explicit points when you wanted it to be triggered earlier).

So whenever you scheduled, _all_ plugs would be unplugged.

It was specialized to wait_for_page() in order to avoid unnecessary overhead in scheduling (making it more directed), and to allow you to leave the request around for further merging/sorting even if a process had to wait for something unrelated.

But in particular, the old non-directed unplug didn't work well in SMP environments (because _one_ CPU re-scheduling obviously doesn't mean anything for the _other_ CPU that is actually working on setting up the request queue).

The point being that we could certainly do it somewhere else. Doing it in wait_for_page() (and at least historically, in waiting for bh's) is really nothing more than trying to have as few points as possible where it's done, and at the same time not missing any.

And yes, I'd _love_ to have better interfaces to let people take advantage of this than sys_readahead(). sys_readahead() was a 5-minute hack that actually does generate wonderful IO patterns, but it is also not all that useful (too specialized, and non-portable).

I tried at one point to make us do directory inode read-ahead (ie put the inodes on a read-ahead queue when doing a directory listing), but that failed miserably. All the low-level filesystems are very much designed to have inode reading be synchronous, and it would have implied major surgery to do (and, sadly, my preliminary numbers also seemed to say that it might be a huge time waster, with enough users just wanting the filenames and not the inodes).

The thing is, right now we have very bad IO patterns for things that traverse whole directory trees (like doign a "tar" or a "diff" of a tree that is cold in the cache) because we have way too many serialization points. We do a good job of prefetching within a file, but if you have source trees etc, the median size for files is often smaller than a single page, so the prefetching ends up being a non-issue most of the time, and we do _zero_ prefetching between files ;/

Now, the reason we don't do it is that it seems to be damn hard to do. No question about that. Especially since it's only worth doing (obviously) on the cold-cache case, and that's also when we likely have very little information about what the access patterns might be.. Oh, well.

Even with sys_readahead(), my simple "pre-read a whole tree" often ended up waiting for inode IO (although at least the fact that several inodes fit in one block gets _some_ of that).

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Now it's time for me to give Linus a history lesson on plugging, apparently.

Plugging used to be done by the issuer and with immediate unplugging when you were done issuing the blocks. Both of these actions happened in ll_rw_block() if the caller was submitting more than on buffer_head, and it happened without the caller knowing about plugging. He never had to unplug.

1.2 then expanded that to be able to plug more than one device at the time. It didn't really do much except allow the array of buffers passed in being on separate devices. The plugging was still hidden from the caller.

1.3 and on introduced a more generalised infrastructure for this, moving the plugging to a task queue (tq_disk). This meant that we could finally separate the plugging and unplugging from the direct IO issue. So whenever someone wanted to the a wait_on_buffer/lock_page() equiv for something that might to be issued, it would have to do a run_task_queue(&tq_disk) first which would then unplug all the queues that were plugged.

tq_disk was then removed and moved to a block list during the 2.5 massive io/bio changes. The functionality remained the same, though - you had to kick all queues to force the unplug of the page you wanted. This infrastructure lasted all up to the point where silly people with lots of CPU's started complaining about lock contention for 32-way systems with thousands of disks. This is the point where I reevaluated the benefits of plugging, found it good, and decided to fix it up. Plugging then became a simple state bit in the queue, and you would have to pass in eg the page you wanted when asking to unplug. This would kick just the specific queue you needed. It also got a timer tied to it, so that we could unplug after foo msecs if we wanted. Additionally, it will also self-unplug once a certain plug depth has been reached (like 4 requests).

Anyway, the point I wanted to make is that this was never driven by scheduler activity. So there!

Jens Axboe

Heh. I confused tq_disk and tq_scheduler, methinks.

And yes, "run_task_queue(&tq_disk)" was in lock_page(), not the scheduler.

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I actually think it would be cleaner to introduce a new mapping_lock_page(mapping, page) or something that does the unplug on the _mapping_, and have lock_page be callable with no ref on the inode. Should be a mechanical conversion for most of the safe callers.

But for 2.6.17, how's this?

SUSE Labs, Novell Inc.

lock_page needs the caller to have a reference on the page->mapping inode due to sync_page. So set_page_dirty_lock is obviously buggy according to its comments.

Solve it by introducing a new lock_page_nosync which does not do a sync_page.

Index: linux-2.6/include/linux/pagemap.h
===================================================================
@@ -168,6 +168,7 @@ static inline pgoff_t linear_page_index(
 }
 
 extern void FASTCALL(__lock_page(struct page *page));
+extern void FASTCALL(__lock_page_nosync(struct page *page));
 extern void FASTCALL(unlock_page(struct page *page));
 
 static inline void lock_page(struct page *page)

@@ -176,6 +177,13 @@ static inline void lock_page(struct page
 	if (TestSetPageLocked(page))
 		__lock_page(page);
 }
+
+static inline void lock_page_nosync(struct page *page)
+{
+	might_sleep();
+	if (TestSetPageLocked(page))
+		__lock_page_nosync(page);
+}
 	
 /*
  * This is exported only for wait_on_page_locked/wait_on_page_writeback.

Index: linux-2.6/mm/filemap.c
===================================================================
@@ -544,6 +544,23 @@ void fastcall __lock_page(struct page *p
 }
 EXPORT_SYMBOL(__lock_page);
 
+static int __sleep_on_page_lock(void *word)
+{
+	io_schedule();
+	return 0;
+}
+
+/*
+ * Variant of lock_page that does not require the caller to hold a reference
+ * on the page's mapping.
+ */
+void fastcall __lock_page_nosync(struct page *page)
+{
+	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
+	__wait_on_bit_lock(page_waitqueue(page), &wait, __sleep_on_page_lock,
+							TASK_UNINTERRUPTIBLE);
+}
+
 /*
  * a rather lightweight function, finding and getting a reference to a
  * hashed page atomically.

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -702,7 +702,7 @@ int set_page_dirty_lock(struct page *pag
 {
 	int ret;
 
-	lock_page(page);
+	lock_page_nosync(page);
 	ret = set_page_dirty(page);
 	unlock_page(page);
 	return ret;

It was a great emperor's-clothes-like discovery. But we've survived for so many years without noticing, does it have to be fixed right now for 2.6.17? (I bet I'd be insisting yes if I'd found it.)

The thing I don't like about your lock_page_nosync (reasonable as it is) is that the one case you're using it, set_page_dirty_nolock, would be so much happier not to have to lock the page in the first place - it's only doing _that_ to stabilize page->mapping, and the lock_page forbids it from being called from anywhere that can't sleep, which is often just where we want to call it from. Neil's suggestion, using a spin_lock against the mapping changing, would help there; but seems like more work than I'd want to get into.

So, although I think lock_page_nosync fixes the bug (at least in that one place we've identified there's likely to be such a bug), it seems to be aiming at the wrong target. I'm pacing and thinking, doubt I'll come up with anything better, please don't hold breath.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

It's up to Linus and Andrew I guess. I don't see why not, but I don't much care one way or the other. But thanks having a quick look at it, we may want it for the Suse kernel.

But making PG_lock a spinning lock is completely unrelated to the bug at hand.

It is the correct target. I know all about your set_page_dirty_lock problems, but they aren't what I'm trying to fix.

AFAIKS, you could also make set_page_dirty_lock non sleeping quite easily by making inode slabs RCU freed.

What places want to use set_page_dirty_lock without sleeping? The only place in drivers/ apart from sg/st that SetPageDirty are rd.c and via_dmablit.c, both of which look OK, if a bit crufty.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Neil wasn't suggesting making PG_lock a spinning lock, he was suggesting a further bit used that way. And I thought his suggestion was relevant to the bug at hand. But it's not the way I'd like to go.

Yes, I had noticed yours is a different issue. I'm saying that if we can "fix" set_page_dirty_nolock not to sleep, then your issue is fixed (as least as it affects set_page_dirty_lock, which is all your patch is dealing with, and we hope all it needs to deal with). Because your issue is with the sync_page in the lock_page of set_page_dirty_nolock, and it's that particular lock_page which I'm trying to be rid of.

I now think it can be done: in cases where TestSetPageLocked finds the page already locked, then I believe we can fall back to inode_lock to stabilize. But I do need to consider the possibilities some more.

Which would equally deal with your issue. Yes, but it's always seemed to me too great a risk, to add an RCU delay into such significant slab shrinking - I don't want to handle the fallout.

I've not looked recently, but bio.c sticks in the mind as one which is pushed to the full contortions to allow for sleeping there.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No, I'm wrong, and have been all along in thinking set_page_dirty_lock could be done better avoiding the lock_page. inode_lock gives the hint:
it's not irq safe, nor is mapping->private_lock, and both may be taken by set_page_dirty. The lock_page in set_page_dirty_lock was the obvious reason it couldn't be used at interrupt time, but not the only reason.

So your lock_page_nosync does look the best way forward to me now.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Indeed, there's a whole bunch of functions and punting to workqueues just to work around that :-)

Jens Axboe

On Mon, May 29, 2006 at 07:34:09PM +1000, Nick Piggin wrote:
...

I feel like I must say this: there are some file systems that live outside the kernel (at least for now) that do _NOT_ use the default sync_page. All the stackable file systems that are based on FiST [1], such as Unionfs [2] and eCryptfs (currently in -mm) [3] (respective authors CC'd). As an example, Unionfs must decide which lower file system page to sync (since it may have several to chose from).

Josef "Jeff" Sipek.

[1] http://www.filesystems.org
[2] http://unionfs.filesystems.org
[3] http://ecryptfs.sourceforge.net

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

OK, noted. Thanks. Luckily for them it looks like sync_page might stay for other reasons (eg. raid) ;)

Any good reasons they are not in the tree?

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Ah, no: that's the backing dev's unplug_fn. sync_page is always the same. And it has definite bugs now that splice has been merged.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

eCryptfs got recently into -mm, my guess would be that Mike Halcrow will try to get it into vanilla in the coming months.

Unionfs is into process of getting ready for a review by the fs-devel & linux-kernel crowd.

Josef "Jeff" Sipek.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>