backing dev unplugging
Subsystems (13)
PATCH 1/1 backing dev unplugging
Hi,
Here's a first cut at killing global plugging of block devices to reduce the nasty contention blk_plug_lock caused. This introduceds per-queue plugging, controlled by the backing_dev_info. Observations:
- Most uses of blk_run_queues() without a specific context was bogus. Usually the act of kicking the targets in question should be (and already are) performed by other activities, such as making the vm flushing run to free memory.
- Some use of blk_run_queues() really just want to kick the final queue where the bio goes to. I've added bio_sync() (BIO_RW_SYNC) to manage those, if the queue needs unplugging we'll do it when holding the lock for the queue already.
- The dm bit needs careful examination and checking of Joe. It could be more clever and maintain plug state of each target, I just added a dm_table unplug all functionality.
Patch is against 2.6.4-rc2-mm1.
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +35 -62
@@ -42,12 +42,6 @@*/ static kmem_cache_t *request_cachep; -/* -* plug management -*/ -static LIST_HEAD (blk_plug_list); -static spinlock_t blk_plug_lock __cacheline_aligned_in_smp = SPIN_LOCK_UNLOCKED; -static wait_queue_head_t congestion_wqh[2 ];/*
@@ -247,8 +241,6 @@*/ blk_queue_bounce_limit (q,BLK_BOUNCE_HIGH ); -INIT_LIST_HEAD (&q->plug_list); -blk_queue_activity_fn (q,NULL ,NULL ); }
@@ -1100,13 +1092,11 @@* don't plug a stopped queue, it must be paired with blk_start_queue() * which will restart the queueing */ -if (!blk_queue_plugged (q) - && !test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) { -spin_lock (&blk_plug_lock); -list_add_tail (&q->plug_list, &blk_plug_list); +if (test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) +return ; + +if (!test_and_set_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags))mod_timer (&q->unplug_timer, jiffies + q->unplug_delay); -spin_unlock (&blk_plug_lock); - } }EXPORT_SYMBOL (blk_plug_device);
@@ -1118,15 +1108,12 @@int blk_remove_plug (request_queue_t *q) {WARN_ON (!irqs_disabled ()); -if (blk_queue_plugged (q)) { -spin_lock (&blk_plug_lock); -list_del_init (&q->plug_list); -del_timer (&q->unplug_timer); -spin_unlock (&blk_plug_lock); -return 1 ; - } -return 0 ; +if (!test_and_clear_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags)) +return 0 ; + +del_timer (&q->unplug_timer); +return 1 ; }EXPORT_SYMBOL (blk_remove_plug);
@@ -1157,14 +1144,11 @@* Linux uses plugging to build bigger requests queues before letting * the device have at them. If a queue is plugged, the I/O scheduler * is still adding and merging requests on the queue. Once the queue -* gets unplugged (either by manually calling this function, or by -* calling blk_run_queues()), the request_fn defined for the -* queue is invoked and transfers started. +* gets unplugged, the request_fn defined for the queue is invoked and +* transfers started. **/ -void generic_unplug_device (void *data) +void generic_unplug_device (request_queue_t *q) { -request_queue_t *q = data; -spin_lock_irq (q->queue_lock);__generic_unplug_device (q);spin_unlock_irq (q->queue_lock);
@@ -1172,9 +1156,23 @@EXPORT_SYMBOL (generic_unplug_device); +void blk_backing_dev_unplug (struct backing_dev_info *bdi) +{ +request_queue_t *q = bdi->unplug_io_data; + +/* +* devices don't necessarily have an ->unplug_fn defined +*/ +if (q->unplug_fn) + q->unplug_fn (q); +} + +EXPORT_SYMBOL (blk_backing_dev_unplug); +static void blk_unplug_work (void *data) {request_queue_t *q = data; + q->unplug_fn (q); }
@@ -1241,41 +1239,13 @@EXPORT_SYMBOL (blk_run_queue); -/** -* blk_run_queues - fire all plugged queues -* -* Description: -* Start I/O on all plugged queues known to the block layer. Queues that -* are currently stopped are ignored. This is equivalent to the older -* tq_disk task queue run. -**/ -#define blk_plug_entry (entry) list_entry((entry), request_queue_t, plug_list) -void blk_run_queues (void ) +void blk_run_backing_dev (struct backing_dev_info *bdi) { -LIST_HEAD (local_plug_list); - -spin_lock_irq (&blk_plug_lock); - -/* -* this will happen fairly often -*/ -if (list_empty (&blk_plug_list)) - goto out; - -list_splice_init (&blk_plug_list, &local_plug_list); - -while (!list_empty (&local_plug_list)) { -request_queue_t *q =blk_plug_entry (local_plug_list.next); - -spin_unlock_irq (&blk_plug_lock); - q->unplug_fn (q); -spin_lock_irq (&blk_plug_lock); - } -out: -spin_unlock_irq (&blk_plug_lock); +if (bdi) + bdi->unplug_io_fn (bdi); } -EXPORT_SYMBOL (blk_run_queues); +EXPORT_SYMBOL (blk_run_backing_dev);/** * blk_cleanup_queue: - release a &request_queue_t when it is no longer needed
@@ -1382,6 +1352,10 @@memset (q,0 ,sizeof (*q));init_timer (&q->unplug_timer);atomic_set (&q->refcnt,1 ); + + q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug; + q->backing_dev_info.unplug_io_data = q; +return q; }
@@ -2036,7 +2010,6 @@DEFINE_WAIT (wait);wait_queue_head_t *wqh = &congestion_wqh[rw]; -blk_run_queues ();prepare_to_wait (wqh, &wait,TASK_UNINTERRUPTIBLE ); ret =io_schedule_timeout (timeout);finish_wait (wqh, &wait);
@@ -2294,7 +2267,7 @@if (blk_queue_plugged (q)) {int nr_queued = q->rq.count[READ ] + q->rq.count[WRITE ]; -if (nr_queued == q->unplug_thresh) +if (nr_queued == q->unplug_thresh ||bio_sync (bio))__generic_unplug_device (q); }spin_unlock_irq (q->queue_lock);
drivers/md/dm.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm.c +10 -2
@@ -560,6 +560,13 @@return 0 ; } +static void dm_unplug_all (request_queue_t *q) +{ +struct mapped_device *md = q->queuedata; + +dm_table_unplug_all (md->map); +} +static int dm_any_congested (void *congested_data,int bdi_bits) {int r;
@@ -657,6 +664,7 @@ md->queue->backing_dev_info.congested_fn = dm_any_congested; md->queue->backing_dev_info.congested_data = md;blk_queue_make_request (md->queue, dm_request); + md->queue->unplug_fn = dm_unplug_all; md->io_pool =mempool_create (MIN_IOS , mempool_alloc_slab, mempool_free_slab, _io_cache);
@@ -880,7 +888,7 @@* Then we wait for the already mapped ios to * complete. */ -blk_run_queues (); +dm_table_unplug_all (md->map);while (1 ) {set_current_state (TASK_INTERRUPTIBLE );
@@ -921,7 +929,7 @@__flush_deferred_io (md, def);up_write (&md->lock); -blk_run_queues (); +dm_table_unplug_all (md->map);return 0 ; }
drivers/md/dm-crypt.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-crypt.c +1 -1
@@ -668,7 +668,7 @@/* out of memory -> run queues */ if (remaining) -blk_run_queues (); +blk_congestion_wait (bio_data_dir (clone),HZ /100 ); }/* drop reference, clones could have returned before we reach this */
drivers/md/dm.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm.h +1 -0
@@ -116,6 +116,7 @@void dm_table_suspend_targets (struct dm_table *t);void dm_table_resume_targets (struct dm_table *t);int dm_table_any_congested (struct dm_table *t,int bdi_bits); +void dm_table_unplug_all (struct dm_table *t);/*----------------------------------------------------------------- * A registry of target types.
drivers/md/dm-table.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-table.c +16 -0
@@ -882,6 +882,22 @@return r; } +void dm_table_unplug_all (struct dm_table *t) +{ +struct list_head *d, *devices; + + devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *q =bdev_get_queue (dd->bdev); + +if (q->unplug_fn) { +set_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags); + q->unplug_fn (q); + } + } +} +EXPORT_SYMBOL (dm_vcalloc);EXPORT_SYMBOL (dm_get_device);EXPORT_SYMBOL (dm_put_device);
drivers/md/md.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/md.c +4 -3
@@ -335,6 +335,8 @@struct bio_vec vec;struct completion event; + rw |= (1 <<BIO_RW_SYNC ); +bio_init (&bio); bio.bi_io_vec = &vec; vec.bv_page = page;
@@ -349,7 +351,6 @@ bio.bi_private = &event; bio.bi_end_io = bi_complete;submit_bio (rw, &bio); -blk_run_queues ();wait_for_completion (&event);return test_bit (BIO_UPTODATE , &bio.bi_flags);
@@ -2718,7 +2719,7 @@ run = thread->run;if (run) {run (thread->mddev); -blk_run_queues (); +blk_run_queue (thread->mddev->queue); }if (signal_pending (current))flush_signals (current);
@@ -3286,7 +3287,7 @@ test_bit(MD_RECOVERY_ERR, &mddev->recovery)) break; -blk_run_queues (); +blk_run_queue (mddev->queue); repeat:if (jiffies >= mark[last_mark] +SYNC_MARK_STEP ) {
drivers/md/raid1.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/raid1.c +3 -0
@@ -451,6 +451,7 @@static void device_barrier (conf_t *conf,sector_t sect) { +blk_run_queue (conf->mddev->queue);spin_lock_irq (&conf->resync_lock);wait_event_lock_irq (conf->wait_idle, !waitqueue_active (&conf->wait_resume), conf->resync_lock);
@@ -478,6 +479,7 @@* thread has put up a bar for new requests. * Continue immediately if no resync is active currently. */ +blk_run_queue (conf->mddev->queue);spin_lock_irq (&conf->resync_lock);wait_event_lock_irq (conf->wait_resume, !conf->barrier, conf->resync_lock); conf->nr_pending++;
@@ -644,6 +646,7 @@static void close_sync (conf_t *conf) { +blk_run_queue (conf->mddev->queue);spin_lock_irq (&conf->resync_lock);wait_event_lock_irq (conf->wait_resume, !conf->barrier, conf->resync_lock);spin_unlock_irq (&conf->resync_lock);
drivers/md/raid5.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/raid5.c +2 -2
@@ -249,6 +249,7 @@break ;if (!sh) { conf->inactive_blocked =1 ; +blk_run_queue (conf->mddev->queue); wait_event_lock_irq(conf->wait_for_stripe, !list_empty(&conf->inactive_list) && (atomic_read(&conf->active_stripes) < (NR_STRIPES *3/4)
@@ -1292,9 +1293,8 @@ } } } -static void raid5_unplug_device (void *data) +static void raid5_unplug_device (request_queue_t *q) { -request_queue_t *q = data;mddev_t *mddev = q->queuedata;raid5_conf_t *conf =mddev_to_conf (mddev);unsigned long flags;
drivers/md/raid6main.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/raid6main.c +1 -2
@@ -1454,9 +1454,8 @@ } } } -static void raid6_unplug_device (void *data) +static void raid6_unplug_device (request_queue_t *q) { -request_queue_t *q = data;mddev_t *mddev = q->queuedata;raid6_conf_t *conf =mddev_to_conf (mddev);unsigned long flags;
drivers/mtd/devices/blkmtd.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/mtd/devices/blkmtd.c +2 -4
@@ -147,8 +147,7 @@ bio->bi_private = &event; bio->bi_end_io = bi_read_complete;if (bio_add_page (bio, page,PAGE_SIZE ,0 ) ==PAGE_SIZE ) { -submit_bio (READ , bio); -blk_run_queues (); +submit_bio (READ_SYNC , bio);wait_for_completion (&event); err =test_bit (BIO_UPTODATE , &bio->bi_flags) ?0 : -EIO ;bio_put (bio);
@@ -179,8 +178,7 @@init_completion (&event); bio->bi_private = &event; bio->bi_end_io = bi_write_complete; -submit_bio (WRITE , bio); -blk_run_queues (); +submit_bio (WRITE_SYNC , bio);wait_for_completion (&event);DEBUG (3 ,"submit_bio completed, bi_vcnt = %d\n" , bio->bi_vcnt); err =test_bit (BIO_UPTODATE , &bio->bi_flags) ?0 : -EIO ;
fs/buffer.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/buffer.c +2 -3
@@ -132,7 +132,7 @@do {prepare_to_wait (wqh, &wait,TASK_UNINTERRUPTIBLE );if (buffer_locked (bh)) { -blk_run_queues (); +blk_run_address_space (bh->b_bdev->bd_inode->i_mapping);io_schedule (); } }while (buffer_locked (bh));
@@ -491,7 +491,6 @@pg_data_t *pgdat;wakeup_bdflush (1024 ); -blk_run_queues ();yield ();for_each_pgdat (pgdat) {
@@ -2928,7 +2927,7 @@int block_sync_page (struct page *page) { -blk_run_queues (); +blk_run_address_space (page->mapping);return 0 ; }
fs/direct-io.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/direct-io.c +2 -2
@@ -364,7 +364,7 @@if (dio->bio_list ==NULL ) { dio->waiter = current;spin_unlock_irqrestore (&dio->bio_lock, flags); -blk_run_queues (); +blk_run_address_space (dio->inode->i_mapping);io_schedule ();spin_lock_irqsave (&dio->bio_lock, flags); dio->waiter =NULL ;
@@ -1035,7 +1035,7 @@if (ret ==0 ) ret = dio->result;finished_one_bio (dio);/* This can free the dio */ -blk_run_queues (); +blk_run_address_space (inode->i_mapping);if (should_wait) {unsigned long flags;/*
fs/jfs/jfs_logmgr.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/jfs/jfs_logmgr.c +2 -4
@@ -1971,8 +1971,7 @@ bio->bi_end_io = lbmIODone; bio->bi_private = bp; -submit_bio (READ , bio); -blk_run_queues (); +submit_bio (READ_SYNC , bio);wait_event (bp->l_ioevent, (bp->l_flag != lbmREAD));
@@ -2116,9 +2115,8 @@/* check if journaling to disk has been disabled */ if (!log->no_integrity) { -submit_bio (WRITE , bio); +submit_bio (WRITE_SYNC , bio);INCREMENT (lmStat.submitted); -blk_run_queues (); }else { bio->bi_size =0 ;
fs/ntfs/compress.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/ntfs/compress.c +1 -1
@@ -668,7 +668,7 @@"uptodate! Unplugging the disk queue " "and rescheduling.");get_bh (tbh); -blk_run_queues (); +blk_run_address_space (mapping);schedule ();put_bh (tbh);if (unlikely (!buffer_uptodate (tbh)))
fs/ufs/truncate.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/ufs/truncate.c +1 -1
@@ -456,7 +456,7 @@break ;if (IS_SYNC (inode) && (inode->i_state &I_DIRTY ))ufs_sync_inode (inode); -blk_run_queues (); +blk_run_address_space (inode->i_mapping);yield (); } offset = inode->i_size & uspi->s_fshift;
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +6 -12
@@ -1013,7 +1013,7 @@ {PB_TRACE (pb,"lock" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_sema);PB_SET_OWNER (pb);PB_TRACE (pb,"locked" ,0 );
@@ -1109,7 +1109,7 @@if (atomic_read (&pb->pb_pin_count) ==0 )break ;if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);schedule (); }remove_wait_queue (&pb->pb_waiters, &wait);
@@ -1407,7 +1407,7 @@if (pb->pb_flags &PBF_RUN_QUEUES ) { pb->pb_flags &= ~PBF_RUN_QUEUES ;if (atomic_read (&pb->pb_io_remaining) >1 ) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping); } }
@@ -1471,7 +1471,7 @@ {PB_TRACE (pb,"iowait" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_iodonesema);PB_TRACE (pb,"iowaited" , (long )pb->pb_error);return pb->pb_error;
@@ -1617,7 +1617,6 @@pagebuf_daemon ( void *data) { -int count;page_buf_t *pb;struct list_head *curr, *next, tmp;
@@ -1640,7 +1639,6 @@spin_lock (&pbd_delwrite_lock); - count =0 ;list_for_each_safe (curr, next, &pbd_delwrite_queue) { pb =list_entry (curr, page_buf_t, pb_list);
@@ -1657,7 +1655,7 @@ pb->pb_flags &= ~PBF_DELWRI ; pb->pb_flags |=PBF_WRITE ;list_move (&pb->pb_list, &tmp); - count++; +blk_run_address_space (pb->pb_target->pbr_mapping); } }
@@ -1671,8 +1669,6 @@if (as_list_len >0 )purge_addresses (); -if (count) -blk_run_queues (); force_flush =0 ; }while (pagebuf_daemon_active);
@@ -1734,13 +1730,11 @@pagebuf_lock (pb);pagebuf_iostrategy (pb);if (++flush_cnt >32 ) { -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping); flush_cnt =0 ; } } -blk_run_queues (); -while (!list_empty (&tmp)) { pb =list_entry (tmp.next, page_buf_t, pb_list);
include/linux/backing-dev.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/include/linux/backing-dev.h +2 -0
@@ -28,6 +28,8 @@int memory_backed;/* Cannot clean pages with writepage */ congested_fn *congested_fn;/* Function pointer if device is md/dm */ void *congested_data;/* Pointer to aux data for congested func */ +void (*unplug_io_fn)(struct backing_dev_info *); +void *unplug_io_data; };extern struct backing_dev_info default_backing_dev_info;
include/linux/bio.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/include/linux/bio.h +2 -0
@@ -124,6 +124,7 @@#define BIO_RW_AHEAD 1#define BIO_RW_BARRIER 2#define BIO_RW_FAILFAST 3 +#define BIO_RW_SYNC 4/* * various member access, note that bio_data should of course not be used
@@ -138,6 +139,7 @@#define bio_cur_sectors (bio) (bio_iovec(bio)->bv_len >> 9)#define bio_data (bio) (page_address(bio_page((bio))) + bio_offset((bio)))#define bio_barrier (bio) ((bio)->bi_rw & (1 << BIO_RW_BARRIER)) +#define bio_sync (bio) ((bio)->bi_rw & (1 << BIO_RW_SYNC))/* * will die
include/linux/blkdev.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/include/linux/blkdev.h +12 -6
@@ -243,7 +243,7 @@typedef void (request_fn_proc ) (request_queue_t *q);typedef int (make_request_fn ) (request_queue_t *q,struct bio *bio);typedef int (prep_rq_fn ) (request_queue_t *,struct request *); -typedef void (unplug_fn ) (void *q); +typedef void (unplug_fn ) (request_queue_t *);struct bio_vec ;typedef int (merge_bvec_fn ) (request_queue_t *,struct bio *,struct bio_vec *);
@@ -315,8 +315,6 @@unsigned long bounce_pfn;int bounce_gfp; -struct list_head plug_list; -/* * various queue flags, see QUEUE_* below */
@@ -369,8 +367,9 @@#define QUEUE_FLAG_READFULL 3/* write queue has been filled */ #define QUEUE_FLAG_WRITEFULL 4/* read queue has been filled */ #define QUEUE_FLAG_DEAD 5/* queue being torn down */ +#define QUEUE_FLAG_PLUGGED 7/* queue is plugged */ -#define blk_queue_plugged (q) !list_empty(&(q)->plug_list) +#define blk_queue_plugged (q) test_bit(QUEUE_FLAG_PLUGGED, &(q)->queue_flags)#define blk_queue_tagged (q) test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)#define blk_queue_stopped (q) test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
@@ -514,7 +513,8 @@extern void blk_start_queue (request_queue_t *q);extern void blk_stop_queue (request_queue_t *q);extern void __blk_stop_queue (request_queue_t *q); -extern void blk_run_queue (request_queue_t *q); +extern void blk_run_queue (request_queue_t *); +extern void blk_run_backing_dev (struct backing_dev_info *);extern void blk_queue_activity_fn (request_queue_t *,activity_fn *,void *);extern struct request *blk_rq_map_user (request_queue_t *,int ,void __user *,unsigned int );extern int blk_rq_unmap_user (struct request *,void __user *,unsigned int ,int );
@@ -525,6 +525,12 @@return bdev->bd_disk->queue; } +static inline void blk_run_address_space (struct address_space *mapping) +{ +if (mapping) +blk_run_backing_dev (mapping->backing_dev_info); +} +/* * end_request() and friends. Must be called with the request queue spinlock * acquired. All functions called within end_request() _must_be_ atomic.
@@ -571,7 +577,7 @@extern int blk_rq_map_sg (request_queue_t *,struct request *,struct scatterlist *);extern void blk_dump_rq_flags (struct request *,char *); -extern void generic_unplug_device (void *); +extern void generic_unplug_device (request_queue_t *);extern long nr_blockdev_pages (void );int blk_get_queue (request_queue_t *);
include/linux/fs.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/include/linux/fs.h +2 -0
@@ -82,6 +82,8 @@#define WRITE 1#define READA 2/* read-ahead - don't block if no resources */ #define SPECIAL 4/* For non-blockdevice requests in request queue */ +#define READ_SYNC (READ | BIO_RW_SYNC) +#define WRITE_SYNC (WRITE | BIO_RW_SYNC)#define SEL_IN 1#define SEL_OUT 2
include/linux/raid/md_k.h renamed from opt/kernel/linux-2.6.4-rc2-mm1/include/linux/raid/md_k.h +0 -26
@@ -326,7 +326,6 @@if (condition) \break ; \spin_unlock_irq (&lock); \ -blk_run_queues (); \schedule (); \spin_lock_irq (&lock); \ } \
@@ -341,30 +340,5 @@__wait_event_lock_irq (wq, condition, lock); \ } while (0) - -#define __wait_disk_event (wq, condition) \ -do { \ - wait_queue_t __wait; \ - init_waitqueue_entry(&__wait, current); \ - \ - add_wait_queue(&wq, &__wait); \ - for (;;) { \ - set_current_state(TASK_UNINTERRUPTIBLE); \ - if (condition) \ - break; \ - blk_run_queues(); \ - schedule(); \ - } \ - current->state = TASK_RUNNING; \ - remove_wait_queue(&wq, &__wait); \ -} while (0) - -#define wait_disk_event (wq, condition) \ -do { \ - if (condition) \ - break; \ - __wait_disk_event(wq, condition); \ -} while (0) -#endif
kernel/power/disk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/kernel/power/disk.c +0 -1
@@ -84,7 +84,6 @@while (shrink_all_memory (10000 ))printk ("." );printk ("|\n" ); -blk_run_queues (); }
kernel/power/pmdisk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/kernel/power/pmdisk.c +1 -1
@@ -859,7 +859,6 @@static void wait_io (void ) { -blk_run_queues ();while (atomic_read (&io_done))io_schedule (); }
@@ -895,6 +894,7 @@ goto Done; } + rw |=BIO_RW_SYNC ;if (rw ==WRITE )bio_set_pages_dirty (bio);start_io ();
kernel/power/swsusp.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/kernel/power/swsusp.c +0 -5
@@ -707,11 +707,6 @@free_some_memory (); -/* No need to invalidate any vfsmnt list -- -* they will be valid after resume, anyway. -*/ -blk_run_queues (); -/* Save state of all device drivers, and stop them. */ if ((res =device_suspend (4 ))==0 )/* If stopping device drivers worked, we proceed basically into
mm/mempool.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/mm/mempool.c +0 -2
@@ -233,8 +233,6 @@if (!(gfp_mask & __GFP_WAIT))return NULL ; -blk_run_queues (); -prepare_to_wait (&pool->wait, &wait,TASK_UNINTERRUPTIBLE );if (!pool->curr_nr)io_schedule ();
mm/readahead.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/mm/readahead.c +5 -0
@@ -15,9 +15,14 @@#include <linux/backing-dev.h> #include <linux/pagevec.h> +static void default_unplug_io_fn (struct backing_dev_info *bdi) +{ +} +struct backing_dev_info default_backing_dev_info = { .ra_pages = (VM_MAX_READAHEAD *1024 ) /PAGE_CACHE_SIZE , .state =0 , + .unplug_io_fn = default_unplug_io_fn, };EXPORT_SYMBOL_GPL (default_backing_dev_info);
Signature · 1 line
Jens Axboe
Quoted 4 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info.
This is such an improvement over what we have now it isn't funny.
Ken, the next -mm is starting to look like linux-3.1.0 so I think it would be best if you could benchmark Jens's patch against 2.6.4-rc2-mm1.
Andrew Morton wrote on Wednesday, March 10, 2004 11:56 AM
Quoted 10 lines
Quoted 4 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info.This is such an improvement over what we have now it isn't funny.
Ken, the next -mm is starting to look like linux-3.1.0 so I think it would be best if you could benchmark Jens's patch against 2.6.4-rc2-mm1.
I'm planning on couple experiments: one is just Jens's change on top of what we have so we can validate the backing dev unplug. Then we will run 2.6.4-rc2-mm1 + Jens's patch.
- Ken
Quoted 7 lines · Kenneth Chen
Andrew Morton wrote on Wednesday, March 10, 2004 11:56 AM
Quoted 6 lines
Quoted 4 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info.
Quoted 8 lines
Quoted 4 lines
This is such an improvement over what we have now it isn't funny.
Ken, the next -mm is starting to look like linux-3.1.0 so I think it would be best if you could benchmark Jens's patch against 2.6.4-rc2-mm1.
I'm planning on couple experiments: one is just Jens's change on top of what we have so we can validate the backing dev unplug. Then we will run 2.6.4-rc2-mm1 + Jens's patch.
It'll be hard to apply the patch against anything but -mm, since it builds (or at least will conflict with) other changes in there. I deliberately made a -mm version this time though I usually make -linus and adapt if necessary, for Andrew to get some testing on this.
Signature · 1 line
Jens Axboe
Quoted 4 lines · Jens Axboe
It'll be hard to apply the patch against anything but -mm, since it builds (or at least will conflict with) other changes in there. I deliberately made a -mm version this time though I usually make -linus and adapt if necessary, for Andrew to get some testing on this.
I tried applying to a BK tree (that was fresh as of a couple of hours ago), and ignored the dm related changes, and it seemed to work ok after I fixed up rejects in direct-io.c and blkdev.h (which didn't seem hard, unless I'm missing something):
10 qla2200 fc controllers, 64 cpus, 112 disks
stock BK tree: ~43945 I/Os per second
w/Jens' patch: ~47149 I/Os per second
Jesse
Quoted 11 lines · Jesse Barnes
Quoted 4 lines · Jens Axboe
It'll be hard to apply the patch against anything but -mm, since it builds (or at least will conflict with) other changes in there. I deliberately made a -mm version this time though I usually make -linus and adapt if necessary, for Andrew to get some testing on this.
I tried applying to a BK tree (that was fresh as of a couple of hours ago), and ignored the dm related changes, and it seemed to work ok after I fixed up rejects in
direct-io.candblkdev.h(which didn't seem hard, unless I'm missing something):
Quoted 4 lines
10 qla2200 fc controllers, 64 cpus, 112 disks
stock BK tree: ~43945 I/Os per second
w/Jens' patch: ~47149 I/Os per second
Do you have a profile for both runs (what was the work load, btw)?
Signature · 1 line
Jens Axboe
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines · Jens Axboe
And what fs, if any? The XFS changes probably need someone knowledgable about XFS internals to verify them.
Yeah, sorry. I forgot to post CPU time info too (I'm putting that together now). The benchmark is doing small, direct I/O requests directly to the block devices (e.g. /dev/sdd1).
Jesse
Quoted 7 lines · Jesse Barnes
Quoted 2 lines · Jens Axboe
And what fs, if any? The XFS changes probably need someone knowledgable about XFS internals to verify them.
Yeah, sorry. I forgot to post CPU time info too (I'm putting that together now). The benchmark is doing small, direct I/O requests directly to the block devices (e.g.
/dev/sdd1).
Neat thanks, looking forward to seeing them.
Signature · 1 line
Jens Axboe
Quoted 10 lines · Jens Axboe
Quoted 7 lines · Jesse Barnes
Quoted 2 lines · Jens Axboe
And what fs, if any? The XFS changes probably need someone knowledgable about XFS internals to verify them.
Yeah, sorry. I forgot to post CPU time info too (I'm putting that together now). The benchmark is doing small, direct I/O requests directly to the block devices (e.g.
/dev/sdd1).Neat thanks, looking forward to seeing them.
Here's a bit more info. I can probably do some more benchmarking later if you need it (though sometimes getting time on that machine can be hard).
The benchmark creates a thread for each block device. Each one opens the device with O_DIRECT and starts doing I/O.
10 qla2200 fc controllers, 64 cpus, 112 disks
-------------------------------------
stock BK tree: ~43945 I/Os per second
[root@revenue sio]# readprofile -m /root/System.map | sort -nr +2 | head -20
204038 default_idle 6376.1875
700889 snidle 1825.2318
234641 cpu_idle 488.8354
25954 blk_run_queues 45.0590
4475 dio_bio_end_io 11.6536
6137 scsi_end_request 11.2812
18002 scsi_request_fn 9.5350
447 ia64_spinlock_contention 6.9844
19326 __make_request 5.3446
3020 sn_dma_flush 4.4940
856 generic_unplug_device 2.4318
961 scsi_device_unbusy 2.3101
360 current_kernel_time 1.8750
547 put_io_context 1.7094
932 dio_await_one 1.6181
409 blk_queue_bounce 1.5977
3484 qla2x00_start_scsi 1.4140
147 pid_alive 1.1484
436 as_set_request 1.1354
283 kmem_cache_alloc 1.1055CPU time from vmstat:
usr sys id wa
0 15 20 65 0 19 0 81 0 17 0 83 0 19 0 81 0 16 0 83 0 18 0 82 0 18 0 82 0 13 0 87 0 16 0 84 0 12 26 62
-------------------------------------
w/Jens' patch: ~47149 I/Os per second
[root@revenue sio]# readprofile -m /root/System.map | sort -nr +2 | head -20
181993 default_idle 5687.2812 624772 snidle 1627.0104 209129 cpu_idle 435.6854
4755 dio_bio_end_io 12.3828
6593 scsi_end_request 12.1195
435 ia64_spinlock_contention 6.7969
2959 sn_dma_flush 4.4033
7459 scsi_request_fn 3.9507
449 blk_run_backing_dev 3.5078
1452 scsi_device_unbusy 3.4904
250 kobject_put 2.6042
590 put_io_context 1.8438
995 dio_await_one 1.6365
313 current_kernel_time 1.6302
361 kmem_cache_alloc 1.4102
3435 qla2x00_start_scsi 1.3941
5032 __make_request 1.3674
480 as_set_request 1.2500
603 scsi_put_command 1.1085
6019 schedule 1.1064
CPU time from vmstat:
usr sys id wa
0 5 60 35 0 8 28 64 0 8 20 72 0 8 19 73 0 8 17 75 0 8 15 77 0 8 15 77 0 8 12 80 0 8 11 81 0 8 11 81
Quoted 3 lines · [email protected] (Jesse Barnes) wrote:
-------------------------------------
w/Jens' patch: ~47149 I/Os per second
Happier.
Quoted 8 lines
[root@revenue sio]# readprofile -m
/root/System.map| sort -nr +2 | head -20181993 default_idle 5687.2812 624772 snidle 1627.0104 209129 cpu_idle 435.68544755
dio_bio_end_io12.3828
6593scsi_end_request12.1195
435ia64_spinlock_contention6.7969
2959sn_dma_flush4.4033
Do you know where that spinlock contention is coming from?
(We have a little patch for x86 which places the spinning code inline in the caller of spin_lock() so it appears nicely in profiles.)
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Andrew Morton
Quoted 3 lines · Andrew> [email protected] (Jesse Barnes) wrote:
Quoted 3 lines
-------------------------------------
w/Jens' patch: ~47149 I/Os per second
Andrew> Happier.
Quoted 5 lines
Quoted 5 lines
[root@revenue sio]# readprofile -m
/root/System.map| sort -nr +2 | head -20 181993default_idle5687.2812 624772 snidle 1627.0104 209129cpu_idle435.6854 4755dio_bio_end_io12.3828 6593scsi_end_request12.1195 435ia64_spinlock_contention6.7969 2959sn_dma_flush4.4033
Andrew> Do you know where that spinlock contention is coming from?
Andrew> (We have a little patch for x86 which places the spinning Andrew> code inline in the caller of spin_lock() so it appears Andrew> nicely in profiles.)
And real men use profiling tools that provide a call-graph, so this hack isn't necessary... ;-)
Jesse, if you want to try q-tools and need some help in getting the per-CPU results merged, let me know (it's something that's planned for a future release, but there is only so many hours in a day so this hasn't been done yet).
--david
Quoted 4 lines · David Mosberger
Jesse, if you want to try q-tools and need some help in getting the per-CPU results merged, let me know (it's something that's planned for a future release, but there is only so many hours in a day so this hasn't been done yet).
Yeah, I've been really wanting to try it out (esp. when you mentioned that you'd be able to profile interrupt off code :). I'll try to get some more time on the machine and take a look.
Also, wli just informed me that I screwed up another aspect of the profile too, the readprofile command i grabbed from the manpage (as my time on the machine ran out) didn't output things in a very useful order.
Thanks,
Jesse
Quoted 7 lines · Andrew Morton
Quoted 4 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info.This is such an improvement over what we have now it isn't funny.
It is pretty scary, once I dove into it...
Signature · 1 line
Jens Axboe
Quoted 10 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Andrew Morton on Wed March 10, 2004 11:56 AM
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info.This is such an improvement over what we have now it isn't funny.
Ken, the next -mm is starting to look like linux-3.1.0 so I think it would be best if you could benchmark Jens's patch against 2.6.4-rc2-mm1.
Our latest measurement on the 32P, 1000 disks setup indicates this patch is working as expected performance wise. We saw 200% improvement on the throughput compare to global blk_plug_list. (compare to the per-cpu blk_plug_list, performance is the same).
- Ken
Quoted 16 lines · Jens Axboe
Hi,
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info. Observations:- Most uses of
blk_run_queues()without a specific context was bogus. Usually the act of kicking the targets in question should be (and already are) performed by other activities, such as making the vm flushing run to free memory.- Some use of
blk_run_queues()really just want to kick the final queue where the bio goes to. I've addedbio_sync()(BIO_RW_SYNC) to manage those, if the queue needs unplugging we'll do it when holding the lock for the queue already.
Quoted 5 lines
- The dm bit needs careful examination and checking of Joe. It could be more clever and maintain plug state of each target, I just added a
dm_tableunplug all functionality.Patch is against 2.6.4-rc2-mm1.
I like it a lot.
Any chance some of these newly-shortened functions can become static inline as well?
Quoted 29 lines
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +4
-13@@ -1100,13 +1092,11 @@* don't plug a stopped queue, it must be paired with blk_start_queue() * which will restart the queueing */ -if (!blk_queue_plugged (q) - && !test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) { -spin_lock (&blk_plug_lock); -list_add_tail (&q->plug_list, &blk_plug_list); +if (test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) +return ; + +if (!test_and_set_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags))mod_timer (&q->unplug_timer, jiffies + q->unplug_delay); -spin_unlock (&blk_plug_lock); - } }EXPORT_SYMBOL (blk_plug_device);@@ -1118,15 +1108,12 @@int blk_remove_plug (request_queue_t *q) {WARN_ON (!irqs_disabled ()); -if (blk_queue_plugged (q)) { -spin_lock (&blk_plug_lock); -list_del_init (&q->plug_list); -del_timer (&q->unplug_timer); -spin_unlock (&blk_plug_lock); -return 1 ; - }
drivers/block/ll_rw_blk.c line 1119
Quoted 10 lines
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +5
-1-return 0 ; +if (!test_and_clear_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags)) +return 0 ; + +del_timer (&q->unplug_timer); +return 1 ; }EXPORT_SYMBOL (blk_remove_plug);
I tend to like
if (test_and_clear_bit()) call_out_of_line_function()
style for small functions like this, and inline those.
Jeff
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Jens Axboe
Hi,
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused. This introduceds per-queue plugging, controlled by thebacking_dev_info. Observations:- Most uses of
blk_run_queues()without a specific context was bogus. Usually the act of kicking the targets in question should be (and already are) performed by other activities, such as making the vm flushing run to free memory.- Some use of
blk_run_queues()really just want to kick the final queue where the bio goes to. I've addedbio_sync()(BIO_RW_SYNC) to manage those, if the queue needs unplugging we'll do it when holding the lock for the queue already.
Quoted 7 lines
Quoted 5 lines
- The dm bit needs careful examination and checking of Joe. It could be more clever and maintain plug state of each target, I just added a
dm_tableunplug all functionality.Patch is against 2.6.4-rc2-mm1.
I like it a lot.
Thanks
Quoted 2 lines
Any chance some of these newly-shortened functions can become static inline as well?
Yeah most likely. Remember this is just a first version, there's room for a little cleanup here and there for sure.
Quoted 39 lines
Quoted 39 lines
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +4
-6@@ -1100,13 +1092,11 @@* don't plug a stopped queue, it must be paired with blk_start_queue() * which will restart the queueing */ -if (!blk_queue_plugged (q) - && !test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) { -spin_lock (&blk_plug_lock); -list_add_tail (&q->plug_list, &blk_plug_list); +if (test_bit (QUEUE_FLAG_STOPPED , &q->queue_flags)) +return ; + +if (!test_and_set_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags))mod_timer (&q->unplug_timer, jiffies + q->unplug_delay); -spin_unlock (&blk_plug_lock); - } }
EXPORT_SYMBOL(blk_plug_device);drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +5
-8@@ -1118,15 +1108,12 @@int blk_remove_plug (request_queue_t *q) {WARN_ON (!irqs_disabled ()); -if (blk_queue_plugged (q)) { -spin_lock (&blk_plug_lock); -list_del_init (&q->plug_list); -del_timer (&q->unplug_timer); -spin_unlock (&blk_plug_lock); -return 1 ; - } -return 0 ; +if (!test_and_clear_bit (QUEUE_FLAG_PLUGGED , &q->queue_flags)) +return 0 ; + +del_timer (&q->unplug_timer); +return 1 ; }
Quoted 8 lines
drivers/block/ll_rw_blk.c line 1119
Quoted 1 line
drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/block/ll_rw_blk.c +0
-0EXPORT_SYMBOL (blk_remove_plug);I tend to like
if (test_and_clear_bit()) call_out_of_line_function()style for small functions like this, and inline those.
I like to do
if (expected condition)
do_stuff
slow_path
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused.
Shouldn't we take read_lock(&md->map_lock) in dm_table_unplug_all()?
If so, what are the lock ranking issues here? The queue lock is not held yet, so it seems pretty simple?
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused.Shouldn't we take
read_lock(&md->map_lock)indm_table_unplug_all()?
Ugh yes, we certainly should.
Quoted 2 lines
If so, what are the lock ranking issues here? The queue lock is not held yet, so it seems pretty simple?
As far as I can tell, it's pretty straight forward. The unplug_fn() will grab the queue lock for 'ordinary' devices, for dm on dm you'd nest the maplock inside each other (which should be quite alright, as far as I can tell, without pulling any nasty tricks).
Signature · 1 line
Jens Axboe
In article <[email protected]>,
Quoted 9 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused.Shouldn't we take
read_lock(&md->map_lock)indm_table_unplug_all()?Ugh yes, we certainly should.
With the latest patches from Joe it would be more like
map = dm_get_table(md);
if (map) {
dm_table_unplug_all(map);
dm_table_put(map);
}No lock ranking issues, you just get a refcounted map (table, really).
Mike.
(Please use reply-to-all)
Quoted 15 lines · Miquel van Smoorenburg
In article <[email protected]>,
Quoted 9 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused.Shouldn't we take
read_lock(&md->map_lock)indm_table_unplug_all()?Ugh yes, we certainly should.
With the latest patches from Joe it would be more like
Quoted 7 lines
map = dm_get_table(md); if (map) { dm_table_unplug_all(map); dm_table_put(map); }No lock ranking issues, you just get a refcounted map (table, really).
Ah, OK. Jens, you'll be needing this (on rc2-mm1):
dm.c: protect md->map with a rw spin lock rather than the md->lock semaphore. Also ensure that everyone accesses md->map through dm_get_table(), rather than directly.
---
25-akpm/drivers/md/dm-table.c | 3 + 25-akpm/drivers/md/dm.c | 88 +++++++++++++++++++++++++----------------- 2 files changed, 57 insertions(+), 34 deletions(-)
drivers/md/dm.c renamed from drivers/md/dm.c~dm-map-rwlock-ng +54 -34
@@ -49,7 +49,7 @@ struct target_io {struct mapped_device {struct rw_semaphore lock; -rwlock_t maplock; +rwlock_t map_lock;atomic_t holders;unsigned long flags;
@@ -238,6 +238,24 @@ static int queue_io(struct mapped_devicereturn 0 ;/* deferred successfully */ } +/* +* Everyone (including functions in this file), should use this +* function to access the md->map field, and make sure they call +* dm_table_put() when finished. +*/ +struct dm_table *dm_get_table (struct mapped_device *md) +{ +struct dm_table *t; + +read_lock (&md->map_lock); + t = md->map; +if (t) +dm_table_get (t); +read_unlock (&md->map_lock); + +return t; +} +/*----------------------------------------------------------------- * CRUD START: * A more elegant soln is in the works that uses the queue
@@ -346,6 +364,7 @@ static void __map_bio(struct dm_target *struct clone_info {struct mapped_device *md; +struct dm_table *map;struct bio *bio;struct dm_io *io;sector_t sector;
@@ -399,7 +418,7 @@ static struct bio *clone_bio(struct biostatic void __clone_and_map (struct clone_info *ci) {struct bio *clone, *bio = ci->bio; -struct dm_target *ti =dm_table_find_target (ci->md->map, ci->sector); +struct dm_target *ti =dm_table_find_target (ci->map, ci->sector);sector_t len =0 , max =max_io_len (ci->md, ci->sector, ti);struct target_io *tio;
@@ -460,7 +479,7 @@ static void __clone_and_map(struct clone ci->sector += max; ci->sector_count -= max; - ti =dm_table_find_target (ci->md->map, ci->sector); + ti =dm_table_find_target (ci->map, ci->sector); len =to_sector (bv->bv_len) - max; clone = split_bvec(bio, ci->sector, ci->idx,
@@ -485,6 +504,7 @@ static void __split_bio(struct mapped_destruct clone_info ci; ci.md = md; + ci.map =dm_get_table (md); ci.bio = bio; ci.io =alloc_io (md); ci.io->error =0 ;
@@ -501,6 +521,7 @@ static void __split_bio(struct mapped_de/* drop the extra reference count */ dec_pending (ci.io,0 ); +dm_table_put (ci.map); }/*----------------------------------------------------------------- * CRUD END
@@ -563,15 +584,16 @@ static int dm_request(request_queue_t *qstatic int dm_any_congested (void *congested_data,int bdi_bits) {int r; -struct mapped_device *md = congested_data; +struct mapped_device *md = (struct mapped_device *) congested_data; +struct dm_table *map =dm_get_table (md); -read_lock (&md->maplock); -if (md->map ==NULL ||test_bit (DMF_BLOCK_IO , &md->flags)) +if (!map ||test_bit (DMF_BLOCK_IO , &md->flags)) +/* FIXME: shouldn't suspended count a congested ? */ r = bdi_bits;else - r =dm_table_any_congested (md->map, bdi_bits); -read_unlock (&md->maplock); + r =dm_table_any_congested (map, bdi_bits); +dm_table_put (map);return r; }
@@ -646,7 +668,7 @@ static struct mapped_device *alloc_dev(umemset (md,0 ,sizeof (*md));init_rwsem (&md->lock); -rwlock_init (&md->maplock); +rwlock_init (&md->map_lock);atomic_set (&md->holders,1 ); md->queue =blk_alloc_queue (GFP_KERNEL );
@@ -746,12 +768,12 @@ static int __bind(struct mapped_device *if (size ==0 )return 0 ; -write_lock (&md->maplock); +write_lock (&md->map_lock); md->map = t; -write_unlock (&md->maplock); -dm_table_event_callback (md->map, event_callback, md); +write_unlock (&md->map_lock);dm_table_get (t); +dm_table_event_callback (md->map, event_callback, md);dm_table_set_restrictions (t, q);return 0 ; }
@@ -764,9 +786,9 @@ static void __unbind(struct mapped_devicreturn ;dm_table_event_callback (map,NULL ,NULL ); -write_lock (&md->maplock); +write_lock (&md->map_lock); md->map =NULL ; -write_unlock (&md->maplock); +write_unlock (&md->map_lock);dm_table_put (map); }
@@ -803,12 +825,16 @@ void dm_get(struct mapped_device *md)void dm_put (struct mapped_device *md) { +struct dm_table *map =dm_get_table (md); +if (atomic_dec_and_test (&md->holders)) { -if (!test_bit (DMF_SUSPENDED , &md->flags) && md->map) -dm_table_suspend_targets (md->map); +if (!test_bit (DMF_SUSPENDED , &md->flags) && map) +dm_table_suspend_targets (map);__unbind (md);free_dev (md); } + +dm_table_put (map); }/*
@@ -859,6 +885,7 @@ int dm_swap_table(struct mapped_device **/ int dm_suspend (struct mapped_device *md) { +struct dm_table *map;DECLARE_WAITQUEUE (wait, current);down_write (&md->lock);
@@ -894,8 +921,11 @@ int dm_suspend(struct mapped_device *md)down_write (&md->lock);remove_wait_queue (&md->wait, &wait);set_bit (DMF_SUSPENDED , &md->flags); -if (md->map) -dm_table_suspend_targets (md->map); + + map =dm_get_table (md); +if (map) +dm_table_suspend_targets (map); +dm_table_put (map);up_write (&md->lock);return 0 ;
@@ -904,22 +934,25 @@ int dm_suspend(struct mapped_device *md)int dm_resume (struct mapped_device *md) {struct bio *def; +struct dm_table *map =dm_get_table (md);down_write (&md->lock); -if (!md->map || +if (!map || !test_bit (DMF_SUSPENDED , &md->flags) || - !dm_table_get_size (md->map)) { + !dm_table_get_size (map)) {up_write (&md->lock); +dm_table_put (map);return -EINVAL ; } -dm_table_resume_targets (md->map); +dm_table_resume_targets (map);clear_bit (DMF_SUSPENDED , &md->flags);clear_bit (DMF_BLOCK_IO , &md->flags); def =bio_list_get (&md->deferred);__flush_deferred_io (md, def);up_write (&md->lock); +dm_table_put (map);blk_run_queues ();
@@ -971,19 +1004,6 @@ struct gendisk *dm_disk(struct mapped_dereturn md->disk; } -struct dm_table *dm_get_table (struct mapped_device *md) -{ -struct dm_table *t; - -down_read (&md->lock); - t = md->map; -if (t) -dm_table_get (t); -up_read (&md->lock); - -return t; -} -int dm_suspended (struct mapped_device *md) {return test_bit (DMF_SUSPENDED , &md->flags);
drivers/md/dm-table.c renamed from drivers/md/dm-table.c~dm-map-rwlock-ng +3 -0
@@ -279,6 +279,9 @@ void dm_table_get(struct dm_table *t)void dm_table_put (struct dm_table *t) { +if (!t) +return ; +if (atomic_dec_and_test (&t->holders))table_destroy (t); }
_
Quoted 13 lines · Andrew Morton
Quoted 11 lines · Miquel van Smoorenburg
With the latest patches from Joe it would be more like
map = dm_get_table(md); if (map) { dm_table_unplug_all(map); dm_table_put(map); }No lock ranking issues, you just get a refcounted map (table, really).
Quoted 8 lines
Ah, OK. Jens, you'll be needing this (on rc2-mm1):
dm.c: protectmd->mapwith a rw spin lock rather than themd->locksemaphore. Also ensure that everyone accessesmd->mapthroughdm_get_table(), rather than directly.25-akpm/drivers/md/dm-table.c | 3 + 25-akpm/drivers/md/dm.c | 88 +++++++++++++++++++++++++-----------------
.. and this final one on top of it, presumably.
See https://www.redhat.com/archives/dm-devel/2004-March/msg00036.html
dm.c: remove __dm_request (merge with previous patch).
drivers/md/dm.c +8 -15
@@ -506,8 +506,13 @@ static void __split_bio(struct mapped_de {struct clone_info ci; - ci.md = md; ci.map =dm_get_table (md); +if (!ci.map) { +bio_io_error (bio, bio->bi_size); +return ; + } + + ci.md = md; ci.bio = bio; ci.io =alloc_io (md); ci.io->error =0 ;
@@ -530,17 +535,6 @@ static void __split_bio(struct mapped_de* CRUD END *---------------------------------------------------------------*/ - -static inline void __dm_request (struct mapped_device *md,struct bio *bio) -{ -if (!md->map) { -bio_io_error (bio, bio->bi_size); -return ; - } - -__split_bio (md, bio); -} -/* * The request function that just remaps the bio built up by * dm_merge_bvec.
@@ -579,7 +573,7 @@ static int dm_request(request_queue_t *qdown_read (&md->lock); } -__dm_request (md, bio); +__split_bio (md, bio);up_read (&md->lock);return 0 ; }
@@ -591,7 +585,6 @@ static int dm_any_congested(void *congesstruct dm_table *map =dm_get_table (md);if (!map ||test_bit (DMF_BLOCK_IO , &md->flags)) -/* FIXME: shouldn't suspended count a congested ? */ r = bdi_bits;else r =dm_table_any_congested (map, bdi_bits);
@@ -850,7 +843,7 @@ static void __flush_deferred_io(struct mwhile (c) { n = c->bi_next; c->bi_next =NULL ; -__dm_request (md, c); +__split_bio (md, c); c = n; } }
Mike.
Quoted 11 lines · Miquel van Smoorenburg
dm.c: protectmd->mapwith a rw spin lock rather than themd->lockQuoted 5 lines
semaphore. Also ensure that everyone accesses
md->mapthroughdm_get_table(), rather than directly.25-akpm/drivers/md/dm-table.c | 3 + 25-akpm/drivers/md/dm.c | 88 +++++++++++++++++++++++++-----------------.. and this final one on top of it, presumably.
See https://www.redhat.com/archives/dm-devel/2004-March/msg00036.html
Yup, thanks. Lots happening. I'll take a trot through the kernel maternity ward, see if I can drag out another -mm later today.
Quoted 25 lines · Andrew Morton
(Please use reply-to-all)
Quoted 21 lines · Miquel van Smoorenburg
In article <[email protected]>,
Quoted 9 lines · Jens Axboe
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
Here's a first cut at killing global plugging of block devices to reduce the nasty contention
blk_plug_lockcaused.Shouldn't we take
read_lock(&md->map_lock)indm_table_unplug_all()?Ugh yes, we certainly should.
With the latest patches from Joe it would be more like
map = dm_get_table(md); if (map) { dm_table_unplug_all(map); dm_table_put(map); }
Quoted 7 lines
Quoted 1 line
No lock ranking issues, you just get a refcounted map (table, really).
Ah, OK. Jens, you'll be needing this (on rc2-mm1):
dm.c: protectmd->mapwith a rw spin lock rather than themd->locksemaphore. Also ensure that everyone accessesmd->mapthroughdm_get_table(), rather than directly.
Neato, much better. I'll build on top of that.
Signature · 1 line
Jens Axboe
Hi Jens,
Quoted 57 lines · Jens Axboe
...[snip]...
diff -ur -X/home/axboe/cdrom/exclude/opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.clinux-2.6.4-rc2-mm1-plug/fs/xfs/linux/xfs_buf.cfs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +4
-6@@ -1013,7 +1013,7 @@ {PB_TRACE (pb,"lock" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_sema);PB_SET_OWNER (pb);PB_TRACE (pb,"locked" ,0 );@@ -1109,7 +1109,7 @@if (atomic_read (&pb->pb_pin_count) ==0 )break ;if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);schedule (); }remove_wait_queue (&pb->pb_waiters, &wait);@@ -1407,7 +1407,7 @@if (pb->pb_flags &PBF_RUN_QUEUES ) { pb->pb_flags &= ~PBF_RUN_QUEUES ;if (atomic_read (&pb->pb_io_remaining) >1 ) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping); } }@@ -1471,7 +1471,7 @@ {PB_TRACE (pb,"iowait" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_iodonesema);PB_TRACE (pb,"iowaited" , (long )pb->pb_error);return pb->pb_error;@@ -1617,7 +1617,6 @@pagebuf_daemon ( void *data) { -int count;page_buf_t *pb;struct list_head *curr, *next, tmp;@@ -1640,7 +1639,6 @@spin_lock (&pbd_delwrite_lock); - count =0 ;list_for_each_safe (curr, next, &pbd_delwrite_queue) { pb =list_entry (curr, page_buf_t, pb_list);
fs/xfs/linux/xfs_buf.c line 1659
Quoted 6 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +1
-1@@ -1657,7 +1655,7 @@ pb->pb_flags &= ~PBF_DELWRI ; pb->pb_flags |=PBF_WRITE ;list_move (&pb->pb_list, &tmp); - count++; +blk_run_address_space (pb->pb_target->pbr_mapping); }
This moves the blk_run_address_space to before we submit the I/O (this bit of code is moving buffers off the delwri queue onto a temporary queue, buffers on the temporary queue are then submitted a little further down) - I suspect we need to move this new blk_run_address_space call down into the temp list processing, just after pagebuf_iostrategy.
Quoted 16 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +0
-3}@@ -1671,8 +1669,6 @@if (as_list_len >0 )purge_addresses (); -if (count) -blk_run_queues (); force_flush =0 ; }while (pagebuf_daemon_active);@@ -1734,13 +1730,11 @@pagebuf_lock (pb);pagebuf_iostrategy (pb);if (++flush_cnt >32 ) { -blk_run_queues ();
fs/xfs/linux/xfs_buf.c line 1740
Quoted 10 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +1
-2+blk_run_address_space (pb->pb_target->pbr_mapping); flush_cnt =0 ; } } -blk_run_queues (); -while (!list_empty (&tmp)) { pb =list_entry (tmp.next, page_buf_t, pb_list);
For this second one, we probably just want to ditch the flush_cnt there (this change is doing blk_run_address_space on every 32nd buffer target, and not the intervening ones). We will be doing a bunch more blk_run_address_space calls than we probably need to, not sure if thats going to become an issue or not, let me prod some of the other XFS folks for more insight there...
thanks.
Signature · 1 line
Nathan
Quoted 10 lines · Nathan Scott
For this second one, we probably just want to ditch the
flush_cntthere (this change is doingblk_run_address_spaceon every 32nd buffer target, and not the intervening ones). We will be doing a bunch moreblk_run_address_spacecalls than we probably need to, not sure if thats going to become an issue or not, let me prod some of the other XFS folks for more insight there...thanks.
The concept there was that we were just pushing things down into the elevator in a batch, then unplugging it afterwards. The do it every 32 I/O's was added to avoid some request starvation issues - which are probably historical now.
I was lazy and did not look at the context on the code, but there are two paths in here. One is unmount flushing all the entries for a specific filesystem, the other is background flushing. I think the background flush activity can live without the blk_run_address_space call being there at all. If we need to grab the lock on the metadata later we will make the call then. The unmount case needs to call it, but since that specifies a specific target, it can just make one call.
Steve
Quoted 59 lines · Nathan Scott
Hi Jens,
Quoted 56 lines · Jens Axboe
...[snip]...
diff -ur -X/home/axboe/cdrom/exclude/opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.clinux-2.6.4-rc2-mm1-plug/fs/xfs/linux/xfs_buf.cfs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +4
-6@@ -1013,7 +1013,7 @@ {PB_TRACE (pb,"lock" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_sema);PB_SET_OWNER (pb);PB_TRACE (pb,"locked" ,0 );@@ -1109,7 +1109,7 @@if (atomic_read (&pb->pb_pin_count) ==0 )break ;if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);schedule (); }remove_wait_queue (&pb->pb_waiters, &wait);@@ -1407,7 +1407,7 @@if (pb->pb_flags &PBF_RUN_QUEUES ) { pb->pb_flags &= ~PBF_RUN_QUEUES ;if (atomic_read (&pb->pb_io_remaining) >1 ) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping); } }@@ -1471,7 +1471,7 @@ {PB_TRACE (pb,"iowait" ,0 );if (atomic_read (&pb->pb_io_remaining)) -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping);down (&pb->pb_iodonesema);PB_TRACE (pb,"iowaited" , (long )pb->pb_error);return pb->pb_error;@@ -1617,7 +1617,6 @@pagebuf_daemon ( void *data) { -int count;page_buf_t *pb;struct list_head *curr, *next, tmp;@@ -1640,7 +1639,6 @@spin_lock (&pbd_delwrite_lock); - count =0 ;list_for_each_safe (curr, next, &pbd_delwrite_queue) { pb =list_entry (curr, page_buf_t, pb_list);
Quoted 14 lines
fs/xfs/linux/xfs_buf.c line 1659
Quoted 7 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +1
-1@@ -1657,7 +1655,7 @@ pb->pb_flags &= ~PBF_DELWRI ; pb->pb_flags |=PBF_WRITE ;list_move (&pb->pb_list, &tmp); - count++; +blk_run_address_space (pb->pb_target->pbr_mapping); }This moves the
blk_run_address_spaceto before we submit the I/O (this bit of code is moving buffers off the delwri queue onto a temporary queue, buffers on the temporary queue are then submitted a little further down) - I suspect we need to move this newblk_run_address_spacecall down into the temp list processing, just afterpagebuf_iostrategy.
I'm not surprised, the XFS 'conversion' was done quickly and is expected to be half-assed :)
Thanks a lot for taking a look at this.
Quoted 21 lines
Quoted 21 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +1
-3}@@ -1671,8 +1669,6 @@if (as_list_len >0 )purge_addresses (); -if (count) -blk_run_queues (); force_flush =0 ; }while (pagebuf_daemon_active);@@ -1734,13 +1730,11 @@pagebuf_lock (pb);pagebuf_iostrategy (pb);if (++flush_cnt >32 ) { -blk_run_queues (); +blk_run_address_space (pb->pb_target->pbr_mapping); flush_cnt =0 ; } }
Quoted 12 lines
fs/xfs/linux/xfs_buf.c line 1740
Quoted 5 lines
fs/xfs/linux/xfs_buf.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/fs/xfs/linux/xfs_buf.c +0
-2-blk_run_queues (); -while (!list_empty (&tmp)) { pb =list_entry (tmp.next, page_buf_t, pb_list);For this second one, we probably just want to ditch the
flush_cntthere (this change is doingblk_run_address_spaceon every 32nd buffer target, and not the intervening ones). We will be doing a bunch moreblk_run_address_spacecalls than we probably need to, not sure if thats going to become an issue or not, let me prod some of the other XFS folks for more insight there...
If any of you could send me a replacement xfs_buf bit, I'd much appreciate it.
Signature · 1 line
Jens Axboe
Jens,
Small locking changes to the dm bits. I've just seen that you've posted an updated version of your patch to lkml, so I'll post another version of this patch to that thread too.
- Joe
Fix md->map protection in the global unplug removal patch.
drivers/md/dm.c +13 -4
@@ -578,8 +578,12 @@ static int dm_request(request_queue_t *qstatic void dm_unplug_all (request_queue_t *q) {struct mapped_device *md = q->queuedata; +struct dm_table *map =dm_get_table (md); -dm_table_unplug_all (md->map); +if (map) { +dm_table_unplug_all (map); +dm_table_put (map); + } }static int dm_any_congested (void *congested_data,int bdi_bits)
@@ -904,11 +908,17 @@ int dm_suspend(struct mapped_device *md)add_wait_queue (&md->wait, &wait);up_write (&md->lock); +/* unplug */ + map =dm_get_table (md); +if (map) { +dm_table_unplug_all (map); +dm_table_put (map); + } +/* * Then we wait for the already mapped ios to * complete. */ -dm_table_unplug_all (md->map);while (1 ) {set_current_state (TASK_INTERRUPTIBLE );
@@ -953,10 +963,9 @@ int dm_resume(struct mapped_device *md) def =bio_list_get (&md->deferred);__flush_deferred_io (md, def);up_write (&md->lock); +dm_table_unplug_all (map);dm_table_put (map); -dm_table_unplug_all (md->map); -return 0 ; }
Quoted 5 lines · Joe Thornber
Jens,
Small locking changes to the dm bits. I've just seen that you've posted an updated version of your patch to lkml, so I'll post another version of this patch to that thread too.
Thanks Joe, you don't have to generate a new diff, I'll just adapt this version and post an update.
Signature · 1 line
Jens Axboe
Am Mi, den 10.03.2004 schrieb Jens Axboe um 13:45:
drivers/md/dm-crypt.c line 671
Quoted 9 lines
diff -ur -X
/home/axboe/cdrom/exclude/opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-crypt.clinux-2.6.4-rc2-mm1-plug/drivers/md/dm-crypt.cdrivers/md/dm-crypt.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-crypt.c +1
-1@@ -668,7 +668,7 @@/* out of memory -> run queues */ if (remaining) -blk_run_queues (); +blk_congestion_wait (bio_data_dir (clone),HZ /100 );
Why did you change this? It was the way I wanted it.
If we were out of memory the buffers were allocated from a mempool and I want to get it out as soon as possible. If we are OOM the write will most likely be the VM trying to free some memory and it would be counterproductive to wait. It is not unlikely that we are the only writer to that disk so there's a chance that the queue is not congested.
Quoted 14 lines · Christophe Saout
Am Mi, den 10.03.2004 schrieb Jens Axboe um 13:45:
drivers/md/dm-crypt.c line 671
Quoted 9 lines
diff -ur -X
/home/axboe/cdrom/exclude/opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-crypt.clinux-2.6.4-rc2-mm1-plug/drivers/md/dm-crypt.cdrivers/md/dm-crypt.c renamed from opt/kernel/linux-2.6.4-rc2-mm1/drivers/md/dm-crypt.c +1
-1@@ -668,7 +668,7 @@/* out of memory -> run queues */ if (remaining) -blk_run_queues (); +blk_congestion_wait (bio_data_dir (clone),HZ /100 );Why did you change this? It was the way I wanted it.
Quoted 5 lines
If we were out of memory the buffers were allocated from a mempool and I want to get it out as soon as possible. If we are OOM the write will most likely be the VM trying to free some memory and it would be counterproductive to wait. It is not unlikely that we are the only writer to that disk so there's a chance that the queue is not congested.
Because it wasn't right, like most of those blk_run_queues(). The vm should get things going, you basically just want to take a little nap and retry. The idea with a small wait is to allow io to make some progress, you gain nothing retrying right away (except wasting CPU spinning). It might want to be 1 instead of HZ/100, though. I doubt it would make much difference, since it's an OOM condition.
Signature · 1 line
Jens Axboe
Am Do, den 11.03.2004 schrieb Jens Axboe um 13:22:
Quoted 6 lines
Quoted 5 lines
If we were out of memory the buffers were allocated from a mempool and I want to get it out as soon as possible. If we are OOM the write will most likely be the VM trying to free some memory and it would be counterproductive to wait. It is not unlikely that we are the only writer to that disk so there's a chance that the queue is not congested.
Quoted 6 lines
Because it wasn't right, like most of those
blk_run_queues(). The vm should get things going, you basically just want to take a little nap and retry. The idea with a small wait is to allow io to make some progress, you gain nothing retrying right away (except wasting CPU spinning). It might want to be 1 instead of HZ/100, though. I doubt it would make much difference, since it's an OOM condition.
Well, okay. You're right then. I think the wait is unnecessary because the next thing that the code tries to do is to allocate a new page, waiting allowed and the VM will run the queue and wait itself. So if you think it's safe you can even remove it.
Thanks.