Re: [dm-devel] A review of dm-writeboost

[Akira Hayakawa <ruby.wktk@gmail.com>]

Mikulas,

Let me ask you about this comment
of choosing the best API.
For the rest, I will reply later.

> BTW. You should use wait_event_interruptible_lock_irq instead of 
> wait_event_interruptible and wait_event_interruptible_lock_irq_timeout 
> instead of wait_event_interruptible_timeout. The functions with "lock_irq" 
> automatically drop and re-acquire the spinlock, so that the condition is 
> tested while the lock is held - so that there are no asynchronous accesses 
> to !list_empty(&cache->flush_queue).

wait_event_interruptible_lock_irq_timeout is
added to the kernel since 3.12 by this patch.
https://lkml.org/lkml/2013/8/21/362
However, it is only used by zfcp itself.

I am afraid I want to refrain from this new API
and keep the code as it is now.
Later if this API is widely accepted
it is time to use it in writeboost
is my opinion.
The fact this API is not added for a long time
makes me feel it should not be used at least at this moment of time.
I want to use only those truly stable.
writeboost as a storage kernel module should be stable.
I believe depending only on the stable APIs is a
good way of making a software stable.

All said, I tried to fix this.
Is this change what you meant?

@@ -24,10 +24,10 @@ int flush_proc(void *data)

                spin_lock_irqsave(&cache->flush_queue_lock, flags);
                while (list_empty(&cache->flush_queue)) {
-                       spin_unlock_irqrestore(&cache->flush_queue_lock, flags);
-                       wait_event_interruptible_timeout(
+                       wait_event_interruptible_lock_irq_timeout(
                                cache->flush_wait_queue,
-                               (!list_empty(&cache->flush_queue)),
+                               !list_empty(&cache->flush_queue),
+                               cache->flush_queue_lock,
                                msecs_to_jiffies(100));

                        /*
@@ -36,8 +36,6 @@ int flush_proc(void *data)
                         */
                        if (kthread_should_stop())
                                return 0;
-                       else
-                               spin_lock_irqsave(&cache->flush_queue_lock, flags);
                }

Akira