[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [dm-devel] dm-writeboost testing




On Sat, 5 Oct 2013, Akira Hayakawa wrote:

> Mikulas,
> 
> > nvidia binary driver, but it may happen in other parts of the kernel too. 
> > The fact that it works in your setup doesn't mean that it is correct.
> You are right. I am convinced.
> 
> As far as I looked around the kernel code,
> it seems to be choosing kthread when one needs looping in background.
> There are other examples such as loop_thread in drivers/block/loop.c .
> 
> And done. Please git pull.
> commit b6d2d3892e09145cd0b9c5ad69ea2d8c410fdeab is tested in my environment.
> All the looping daemons are running on kthread now.
> 
> The diff between the older version (with wq) and the new version (with kthread)
> is shown below. I defined fancy CREATE_DAEMON macro.
> 
> Another by-product is that
> you are no longer in need to wait for long time in dmsetup remove
> since kthread_stop() forcefully wakes the thread up.

The change seems ok. Please, also move this piece of code in flush_proc
out of the spinlock:
                        if (kthread_should_stop())
                                return 0;

It caused the workqueue warning I reported before and still causes warning 
with kthreads:
note: flush_daemon[5145] exited with preempt_count 1

I will send you next email with more bugs that I found in your code.

Mikulas

> diff --git a/Driver/dm-writeboost-daemon.c b/Driver/dm-writeboost-daemon.c
> index 6090661..65974e2 100644
> --- a/Driver/dm-writeboost-daemon.c
> +++ b/Driver/dm-writeboost-daemon.c
> @@ -10,12 +10,11 @@
>  
>  /*----------------------------------------------------------------*/
>  
> -void flush_proc(struct work_struct *work)
> +int flush_proc(void *data)
>  {
>  	unsigned long flags;
>  
> -	struct wb_cache *cache =
> -		container_of(work, struct wb_cache, flush_work);
> +	struct wb_cache *cache = data;
>  
>  	while (true) {
>  		struct flush_job *job;
> @@ -32,8 +31,12 @@ void flush_proc(struct work_struct *work)
>  				msecs_to_jiffies(100));
>  			spin_lock_irqsave(&cache->flush_queue_lock, flags);
>  
> -			if (cache->on_terminate)
> -				return;
> +			/*
> +			 * flush daemon can exit
> +			 * only if no flush job is queued.
> +			 */
> +			if (kthread_should_stop())
> +				return 0;
>  		}
>  
>  		/*
> @@ -84,6 +87,7 @@ void flush_proc(struct work_struct *work)
>  
>  		kfree(job);
>  	}
> +	return 0;
>  }
>  
>  /*----------------------------------------------------------------*/
> @@ -353,19 +357,15 @@ migrate_write:
>  	}
>  }
>  
> -void migrate_proc(struct work_struct *work)
> +int migrate_proc(void *data)
>  {
> -	struct wb_cache *cache =
> -		container_of(work, struct wb_cache, migrate_work);
> +	struct wb_cache *cache = data;
>  
> -	while (true) {
> +	while (!kthread_should_stop()) {
>  		bool allow_migrate;
>  		size_t i, nr_mig_candidates, nr_mig, nr_max_batch;
>  		struct segment_header *seg, *tmp;
>  
> -		if (cache->on_terminate)
> -			return;
> -
>  		/*
>  		 * If reserving_id > 0
>  		 * Migration should be immediate.
> @@ -430,6 +430,7 @@ void migrate_proc(struct work_struct *work)
>  			list_del(&seg->migrate_list);
>  		}
>  	}
> +	return 0;
>  }
>  
>  /*
> @@ -453,19 +454,16 @@ void wait_for_migration(struct wb_cache *cache, u64 id)
>  
>  /*----------------------------------------------------------------*/
>  
> -void modulator_proc(struct work_struct *work)
> +int modulator_proc(void *data)
>  {
> -	struct wb_cache *cache =
> -		container_of(work, struct wb_cache, modulator_work);
> +	struct wb_cache *cache = data;
>  	struct wb_device *wb = cache->wb;
>  
>  	struct hd_struct *hd = wb->device->bdev->bd_part;
>  	unsigned long old = 0, new, util;
>  	unsigned long intvl = 1000;
>  
> -	while (true) {
> -		if (cache->on_terminate)
> -			return;
> +	while (!kthread_should_stop()) {
>  
>  		new = jiffies_to_msecs(part_stat_read(hd, io_ticks));
>  
> @@ -484,6 +482,7 @@ modulator_update:
>  
>  		schedule_timeout_interruptible(msecs_to_jiffies(intvl));
>  	}
> +	return 0;
>  }
>  
>  /*----------------------------------------------------------------*/
> @@ -517,16 +516,12 @@ static void update_superblock_record(struct wb_cache *cache)
>  	kfree(buf);
>  }
>  
> -void recorder_proc(struct work_struct *work)
> +int recorder_proc(void *data)
>  {
> -	struct wb_cache *cache =
> -		container_of(work, struct wb_cache, recorder_work);
> +	struct wb_cache *cache = data;
>  	unsigned long intvl;
>  
> -	while (true) {
> -		if (cache->on_terminate)
> -			return;
> -
> +	while (!kthread_should_stop()) {
>  		/* sec -> ms */
>  		intvl = cache->update_record_interval * 1000;
>  
> @@ -539,20 +534,17 @@ void recorder_proc(struct work_struct *work)
>  
>  		schedule_timeout_interruptible(msecs_to_jiffies(intvl));
>  	}
> +	return 0;
>  }
>  
>  /*----------------------------------------------------------------*/
>  
> -void sync_proc(struct work_struct *work)
> +int sync_proc(void *data)
>  {
> -	struct wb_cache *cache =
> -		container_of(work, struct wb_cache, sync_work);
> +	struct wb_cache *cache = data;
>  	unsigned long intvl;
>  
> -	while (true) {
> -		if (cache->on_terminate)
> -			return;
> -
> +	while (!kthread_should_stop()) {
>  		/* sec -> ms */
>  		intvl = cache->sync_interval * 1000;
>  
> @@ -566,4 +558,5 @@ void sync_proc(struct work_struct *work)
>  
>  		schedule_timeout_interruptible(msecs_to_jiffies(intvl));
>  	}
> +	return 0;
>  }
> diff --git a/Driver/dm-writeboost-daemon.h b/Driver/dm-writeboost-daemon.h
> index 3bdd862..d21d66c 100644
> --- a/Driver/dm-writeboost-daemon.h
> +++ b/Driver/dm-writeboost-daemon.h
> @@ -9,7 +9,7 @@
>  
>  /*----------------------------------------------------------------*/
>  
> -void flush_proc(struct work_struct *);
> +int flush_proc(void *);
>  
>  /*----------------------------------------------------------------*/
>  
> @@ -19,20 +19,20 @@ void flush_barrier_ios(struct work_struct *);
>  
>  /*----------------------------------------------------------------*/
>  
> -void migrate_proc(struct work_struct *);
> +int migrate_proc(void *);
>  void wait_for_migration(struct wb_cache *, u64 id);
>  
>  /*----------------------------------------------------------------*/
>  
> -void modulator_proc(struct work_struct *);
> +int modulator_proc(void *);
>  
>  /*----------------------------------------------------------------*/
>  
> -void sync_proc(struct work_struct *);
> +int sync_proc(void *);
>  
>  /*----------------------------------------------------------------*/
>  
> -void recorder_proc(struct work_struct *);
> +int recorder_proc(void *);
>  
>  /*----------------------------------------------------------------*/
>  
> diff --git a/Driver/dm-writeboost-metadata.c b/Driver/dm-writeboost-metadata.c
> index 1cd9e0c..4f5fa5e 100644
> --- a/Driver/dm-writeboost-metadata.c
> +++ b/Driver/dm-writeboost-metadata.c
> @@ -994,6 +994,17 @@ void free_migration_buffer(struct wb_cache *cache)
>  
>  /*----------------------------------------------------------------*/
>  
> +#define CREATE_DAEMON(name)\
> +	cache->name##_daemon = kthread_create(name##_proc, cache,\
> +					      #name "_daemon");\
> +	if (IS_ERR(cache->name##_daemon)) {\
> +		r = PTR_ERR(cache->name##_daemon);\
> +		cache->name##_daemon = NULL;\
> +		WBERR("couldn't spawn" #name "daemon");\
> +		goto bad_##name##_daemon;\
> +	}\
> +	wake_up_process(cache->name##_daemon);
> +
>  int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  {
>  	int r = 0;
> @@ -1010,8 +1021,6 @@ int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  
>  	mutex_init(&cache->io_lock);
>  
> -	cache->on_terminate = false;
> -
>  	/*
>  	 * (i) Harmless Initializations
>  	 */
> @@ -1042,12 +1051,6 @@ int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  	 * Recovering the cache metadata
>  	 * prerequires the migration daemon working.
>  	 */
> -	cache->migrate_wq = create_singlethread_workqueue("migratewq");
> -	if (!cache->migrate_wq) {
> -		r = -ENOMEM;
> -		WBERR();
> -		goto bad_migratewq;
> -	}
>  
>  	/* Migration Daemon */
>  	atomic_set(&cache->migrate_fail_count, 0);
> @@ -1070,9 +1073,7 @@ int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  
>  	cache->allow_migrate = true;
>  	cache->reserving_segment_id = 0;
> -	INIT_WORK(&cache->migrate_work, migrate_proc);
> -	queue_work(cache->migrate_wq, &cache->migrate_work);
> -
> +	CREATE_DAEMON(migrate);
>  
>  	r = recover_cache(cache);
>  	if (r) {
> @@ -1086,19 +1087,12 @@ int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  	 * These are only working
>  	 * after the logical device created.
>  	 */
> -	cache->flush_wq = create_singlethread_workqueue("flushwq");
> -	if (!cache->flush_wq) {
> -		r = -ENOMEM;
> -		WBERR();
> -		goto bad_flushwq;
> -	}
>  
>  	/* Flush Daemon */
> -	INIT_WORK(&cache->flush_work, flush_proc);
>  	spin_lock_init(&cache->flush_queue_lock);
>  	INIT_LIST_HEAD(&cache->flush_queue);
>  	init_waitqueue_head(&cache->flush_wait_queue);
> -	queue_work(cache->flush_wq, &cache->flush_work);
> +	CREATE_DAEMON(flush);
>  
>  	/* Deferred ACK for barrier writes */
>  
> @@ -1118,29 +1112,30 @@ int __must_check resume_cache(struct wb_cache *cache, struct dm_dev *dev)
>  
>  	/* Migartion Modulator */
>  	cache->enable_migration_modulator = true;
> -	INIT_WORK(&cache->modulator_work, modulator_proc);
> -	schedule_work(&cache->modulator_work);
> +	CREATE_DAEMON(modulator);
>  
>  	/* Superblock Recorder */
>  	cache->update_record_interval = 60;
> -	INIT_WORK(&cache->recorder_work, recorder_proc);
> -	schedule_work(&cache->recorder_work);
> +	CREATE_DAEMON(recorder);
>  
>  	/* Dirty Synchronizer */
>  	cache->sync_interval = 60;
> -	INIT_WORK(&cache->sync_work, sync_proc);
> -	schedule_work(&cache->sync_work);
> +	CREATE_DAEMON(sync);
>  
>  	return 0;
>  
> -bad_flushwq:
> +bad_sync_daemon:
> +	kthread_stop(cache->recorder_daemon);
> +bad_recorder_daemon:
> +	kthread_stop(cache->modulator_daemon);
> +bad_modulator_daemon:
> +	kthread_stop(cache->flush_daemon);
> +bad_flush_daemon:
>  bad_recover:
> -	cache->on_terminate = true;
> -	cancel_work_sync(&cache->migrate_work);
> +	kthread_stop(cache->migrate_daemon);
> +bad_migrate_daemon:
>  	free_migration_buffer(cache);
>  bad_alloc_migrate_buffer:
> -	destroy_workqueue(cache->migrate_wq);
> -bad_migratewq:
>  	free_ht(cache);
>  bad_alloc_ht:
>  	free_segment_header_array(cache);
> @@ -1153,20 +1148,21 @@ bad_init_rambuf_pool:
>  
>  void free_cache(struct wb_cache *cache)
>  {
> -	cache->on_terminate = true;
> +	/*
> +	 * Must clean up all the volatile data
> +	 * before termination.
> +	 */
> +	flush_current_buffer(cache);
>  
> -	/* Kill in-kernel daemons */
> -	cancel_work_sync(&cache->sync_work);
> -	cancel_work_sync(&cache->recorder_work);
> -	cancel_work_sync(&cache->modulator_work);
> +	kthread_stop(cache->sync_daemon);
> +	kthread_stop(cache->recorder_daemon);
> +	kthread_stop(cache->modulator_daemon);
>  
> -	cancel_work_sync(&cache->flush_work);
> -	destroy_workqueue(cache->flush_wq);
> +	kthread_stop(cache->flush_daemon);
>  
>  	cancel_work_sync(&cache->barrier_deadline_work);
>  
> -	cancel_work_sync(&cache->migrate_work);
> -	destroy_workqueue(cache->migrate_wq);
> +	kthread_stop(cache->migrate_daemon);
>  	free_migration_buffer(cache);
>  
>  	/* Destroy in-core structures */
> diff --git a/Driver/dm-writeboost-target.c b/Driver/dm-writeboost-target.c
> index 6aa4c7c..4b5b7aa 100644
> --- a/Driver/dm-writeboost-target.c
> +++ b/Driver/dm-writeboost-target.c
> @@ -973,12 +973,6 @@ static void writeboost_dtr(struct dm_target *ti)
>  	struct wb_device *wb = ti->private;
>  	struct wb_cache *cache = wb->cache;
>  
> -	/*
> -	 * Synchronize all the dirty writes
> -	 * before termination.
> -	 */
> -	cache->sync_interval = 1;
> -
>  	free_cache(cache);
>  	kfree(cache);
>  
> diff --git a/Driver/dm-writeboost.h b/Driver/dm-writeboost.h
> index 74ff194..092c100 100644
> --- a/Driver/dm-writeboost.h
> +++ b/Driver/dm-writeboost.h
> @@ -16,6 +16,7 @@
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
> +#include <linux/kthread.h>
>  #include <linux/sched.h>
>  #include <linux/timer.h>
>  #include <linux/device-mapper.h>
> @@ -265,8 +266,7 @@ struct wb_cache {
>  	 * and flush daemon asynchronously
>  	 * flush them to the cache device.
>  	 */
> -	struct work_struct flush_work;
> -	struct workqueue_struct *flush_wq;
> +	struct task_struct *flush_daemon;
>  	spinlock_t flush_queue_lock;
>  	struct list_head flush_queue;
>  	wait_queue_head_t flush_wait_queue;
> @@ -288,8 +288,7 @@ struct wb_cache {
>  	 * migrate daemon goes into migration
>  	 * if they are segments to migrate.
>  	 */
> -	struct work_struct migrate_work;
> -	struct workqueue_struct *migrate_wq;
> +	struct task_struct *migrate_daemon;
>  	bool allow_migrate; /* param */
>  
>  	/*
> @@ -314,7 +313,7 @@ struct wb_cache {
>  	 * the migration
>  	 * according to the load of backing store.
>  	 */
> -	struct work_struct modulator_work;
> +	struct task_struct *modulator_daemon;
>  	bool enable_migration_modulator; /* param */
>  
>  	/*
> @@ -323,7 +322,7 @@ struct wb_cache {
>  	 * Update the superblock record
>  	 * periodically.
>  	 */
> -	struct work_struct recorder_work;
> +	struct task_struct *recorder_daemon;
>  	unsigned long update_record_interval; /* param */
>  
>  	/*
> @@ -332,16 +331,9 @@ struct wb_cache {
>  	 * Sync the dirty writes
>  	 * periodically.
>  	 */
> -	struct work_struct sync_work;
> +	struct task_struct *sync_daemon;
>  	unsigned long sync_interval; /* param */
>  
> -	/*
> -	 * on_terminate is true
> -	 * to notify all the background daemons to
> -	 * stop their operations.
> -	 */
> -	bool on_terminate;
> -
>  	atomic64_t stat[STATLEN];
>  };
> 
> Akira
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]