[rh7,1/2] ploop: Stop fast path reqs living their own lives

Submitted by Kirill Tkhai on March 3, 2020, 3:13 p.m.

Details

Message ID 158324840211.1227740.4379016615567806388.stgit@localhost.localdomain
State New
Series "Series without cover letter"
Headers show

Commit Message

Kirill Tkhai March 3, 2020, 3:13 p.m.
Doing ploop_quiesce() we want any IO becomes stopped.
But fast path requests live their own lives, and this
completelly ignores quiesce state. This may be a reason
of unpredictable behaviour.

This patch fixes the problem.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 drivers/block/ploop/dev.c   |   30 ++++++++++++++++++++++++++++--
 include/linux/ploop/ploop.h |    2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 3f70f674e112..9e01866860bd 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -164,6 +164,22 @@  static void ploop_uncongest(struct ploop_device *plo)
 	}
 }
 
+static int fast_path_reqs_count(struct ploop_device *plo)
+{
+	int ret;
+
+	spin_lock_irq(&plo->lock);
+	ret = plo->fastpath_reqs;
+	spin_unlock_irq(&plo->lock);
+
+	return ret;
+}
+
+static void wait_fast_path_reqs(struct ploop_device *plo)
+{
+	wait_event(plo->fast_path_waitq, fast_path_reqs_count(plo) == 0);
+}
+
 static void ploop_init_request(struct ploop_request *preq)
 {
 	preq->eng_state = PLOOP_E_ENTRY;
@@ -656,6 +672,8 @@  DEFINE_BIO_CB(ploop_fast_end_io)
 	    (test_bit(PLOOP_S_EXITING, &plo->state) ||
 	     !list_empty(&plo->entry_queue)))
 		wake_up_interruptible(&plo->waitq);
+	if (plo->fast_path_disabled_count && !plo->fastpath_reqs)
+		wake_up(&plo->fast_path_waitq);
 	spin_unlock_irqrestore(&plo->lock, flags);
 
 	BIO_ENDIO(plo->queue, orig, err);
@@ -969,7 +987,8 @@  static void ploop_make_request(struct request_queue *q, struct bio *bio)
 
 	/* No fast path, when maintenance is in progress.
 	 * (PLOOP_S_TRACK was checked immediately above) */
-	if (FAST_PATH_DISABLED(plo->maintenance_type))
+	if (FAST_PATH_DISABLED(plo->maintenance_type) ||
+	    plo->fast_path_disabled_count)
 		goto queue;
 
 	/* Attention state, always queue */
@@ -3442,7 +3461,6 @@  static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
 	return err;
 }
 
-
 void ploop_quiesce(struct ploop_device * plo)
 {
 	struct completion qcomp;
@@ -3452,6 +3470,8 @@  void ploop_quiesce(struct ploop_device * plo)
 		return;
 
 	spin_lock_irq(&plo->lock);
+	plo->fast_path_disabled_count++;
+
 	preq = ploop_alloc_request(plo, true);
 	preq->req_rw = 0;
 	preq->state = (1 << PLOOP_REQ_SYNC) | (1 << PLOOP_REQ_BARRIER);
@@ -3469,6 +3489,7 @@  void ploop_quiesce(struct ploop_device * plo)
 		wake_up_interruptible(&plo->waitq);
 	spin_unlock_irq(&plo->lock);
 
+	wait_fast_path_reqs(plo);
 	wait_for_completion(&qcomp);
 	plo->quiesce_comp = NULL;
 }
@@ -3476,6 +3497,10 @@  EXPORT_SYMBOL(ploop_quiesce);
 
 void ploop_relax(struct ploop_device * plo)
 {
+	spin_lock_irq(&plo->lock);
+	plo->fast_path_disabled_count--;
+	spin_unlock_irq(&plo->lock);
+
 	if (!test_bit(PLOOP_S_RUNNING, &plo->state))
 		return;
 
@@ -5456,6 +5481,7 @@  static struct ploop_device *__ploop_dev_alloc(int index)
 	init_waitqueue_head(&plo->req_waitq);
 	init_waitqueue_head(&plo->freeze_waitq);
 	init_waitqueue_head(&plo->event_waitq);
+	init_waitqueue_head(&plo->fast_path_waitq);
 	plo->tune = DEFAULT_PLOOP_TUNE;
 	map_init(plo, &plo->map);
 	track_init(plo);
diff --git a/include/linux/ploop/ploop.h b/include/linux/ploop/ploop.h
index 66591623f79d..8495ca2145fd 100644
--- a/include/linux/ploop/ploop.h
+++ b/include/linux/ploop/ploop.h
@@ -407,6 +407,7 @@  struct ploop_device
 	wait_queue_head_t	req_waitq;
 	wait_queue_head_t	freeze_waitq;
 	wait_queue_head_t	event_waitq;
+	wait_queue_head_t	fast_path_waitq;
 
 	struct ploop_map	map;
 	struct ploop_map	*trans_map;
@@ -473,6 +474,7 @@  struct ploop_device
 	struct block_device *dm_crypt_bdev;
 
 	unsigned long		locking_state; /* plo locked by userspace */
+	unsigned int		fast_path_disabled_count;
 };
 
 enum

Comments

Konstantin Khorenko March 5, 2020, 4:31 p.m.
On 03/03/2020 06:13 PM, Kirill Tkhai wrote:
> Doing ploop_quiesce() we want any IO becomes stopped.
> But fast path requests live their own lives, and this
> completelly ignores quiesce state. This may be a reason
> of unpredictable behaviour.
>
> This patch fixes the problem.
>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  drivers/block/ploop/dev.c   |   30 ++++++++++++++++++++++++++++--
>  include/linux/ploop/ploop.h |    2 ++
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 3f70f674e112..9e01866860bd 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -164,6 +164,22 @@ static void ploop_uncongest(struct ploop_device *plo)
>  	}
>  }
>
> +static int fast_path_reqs_count(struct ploop_device *plo)
> +{
> +	int ret;
> +
> +	spin_lock_irq(&plo->lock);
> +	ret = plo->fastpath_reqs;
> +	spin_unlock_irq(&plo->lock);

Do we really need to take a lock here?

> +
> +	return ret;
> +}
> +
> +static void wait_fast_path_reqs(struct ploop_device *plo)
> +{
> +	wait_event(plo->fast_path_waitq, fast_path_reqs_count(plo) == 0);
> +}
> +
>  static void ploop_init_request(struct ploop_request *preq)
>  {
>  	preq->eng_state = PLOOP_E_ENTRY;
> @@ -656,6 +672,8 @@ DEFINE_BIO_CB(ploop_fast_end_io)
>  	    (test_bit(PLOOP_S_EXITING, &plo->state) ||
>  	     !list_empty(&plo->entry_queue)))
>  		wake_up_interruptible(&plo->waitq);
> +	if (plo->fast_path_disabled_count && !plo->fastpath_reqs)
> +		wake_up(&plo->fast_path_waitq);
>  	spin_unlock_irqrestore(&plo->lock, flags);
>
>  	BIO_ENDIO(plo->queue, orig, err);
> @@ -969,7 +987,8 @@ static void ploop_make_request(struct request_queue *q, struct bio *bio)
>
>  	/* No fast path, when maintenance is in progress.
>  	 * (PLOOP_S_TRACK was checked immediately above) */
> -	if (FAST_PATH_DISABLED(plo->maintenance_type))
> +	if (FAST_PATH_DISABLED(plo->maintenance_type) ||
> +	    plo->fast_path_disabled_count)
>  		goto queue;
>
>  	/* Attention state, always queue */
> @@ -3442,7 +3461,6 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>  	return err;
>  }
>
> -
>  void ploop_quiesce(struct ploop_device * plo)
>  {
>  	struct completion qcomp;
> @@ -3452,6 +3470,8 @@ void ploop_quiesce(struct ploop_device * plo)
>  		return;
>
>  	spin_lock_irq(&plo->lock);
> +	plo->fast_path_disabled_count++;
> +
>  	preq = ploop_alloc_request(plo, true);
>  	preq->req_rw = 0;
>  	preq->state = (1 << PLOOP_REQ_SYNC) | (1 << PLOOP_REQ_BARRIER);
> @@ -3469,6 +3489,7 @@ void ploop_quiesce(struct ploop_device * plo)
>  		wake_up_interruptible(&plo->waitq);
>  	spin_unlock_irq(&plo->lock);
>
> +	wait_fast_path_reqs(plo);
>  	wait_for_completion(&qcomp);
>  	plo->quiesce_comp = NULL;
>  }
> @@ -3476,6 +3497,10 @@ EXPORT_SYMBOL(ploop_quiesce);
>
>  void ploop_relax(struct ploop_device * plo)
>  {
> +	spin_lock_irq(&plo->lock);
> +	plo->fast_path_disabled_count--;
> +	spin_unlock_irq(&plo->lock);
> +
>  	if (!test_bit(PLOOP_S_RUNNING, &plo->state))
>  		return;
>
> @@ -5456,6 +5481,7 @@ static struct ploop_device *__ploop_dev_alloc(int index)
>  	init_waitqueue_head(&plo->req_waitq);
>  	init_waitqueue_head(&plo->freeze_waitq);
>  	init_waitqueue_head(&plo->event_waitq);
> +	init_waitqueue_head(&plo->fast_path_waitq);
>  	plo->tune = DEFAULT_PLOOP_TUNE;
>  	map_init(plo, &plo->map);
>  	track_init(plo);
> diff --git a/include/linux/ploop/ploop.h b/include/linux/ploop/ploop.h
> index 66591623f79d..8495ca2145fd 100644
> --- a/include/linux/ploop/ploop.h
> +++ b/include/linux/ploop/ploop.h
> @@ -407,6 +407,7 @@ struct ploop_device
>  	wait_queue_head_t	req_waitq;
>  	wait_queue_head_t	freeze_waitq;
>  	wait_queue_head_t	event_waitq;
> +	wait_queue_head_t	fast_path_waitq;
>
>  	struct ploop_map	map;
>  	struct ploop_map	*trans_map;
> @@ -473,6 +474,7 @@ struct ploop_device
>  	struct block_device *dm_crypt_bdev;
>
>  	unsigned long		locking_state; /* plo locked by userspace */
> +	unsigned int		fast_path_disabled_count;
>  };
>
>  enum
>
>
> .
>
Kirill Tkhai March 6, 2020, 8:53 a.m.
On 05.03.2020 19:31, Konstantin Khorenko wrote:
> On 03/03/2020 06:13 PM, Kirill Tkhai wrote:
>> Doing ploop_quiesce() we want any IO becomes stopped.
>> But fast path requests live their own lives, and this
>> completelly ignores quiesce state. This may be a reason
>> of unpredictable behaviour.
>>
>> This patch fixes the problem.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  drivers/block/ploop/dev.c   |   30 ++++++++++++++++++++++++++++--
>>  include/linux/ploop/ploop.h |    2 ++
>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
>> index 3f70f674e112..9e01866860bd 100644
>> --- a/drivers/block/ploop/dev.c
>> +++ b/drivers/block/ploop/dev.c
>> @@ -164,6 +164,22 @@ static void ploop_uncongest(struct ploop_device *plo)
>>      }
>>  }
>>
>> +static int fast_path_reqs_count(struct ploop_device *plo)
>> +{
>> +    int ret;
>> +
>> +    spin_lock_irq(&plo->lock);
>> +    ret = plo->fastpath_reqs;
>> +    spin_unlock_irq(&plo->lock);
> 
> Do we really need to take a lock here?

Yes, since fast path reqs may be queued from any processor.

>> +
>> +    return ret;
>> +}
>> +
>> +static void wait_fast_path_reqs(struct ploop_device *plo)
>> +{
>> +    wait_event(plo->fast_path_waitq, fast_path_reqs_count(plo) == 0);
>> +}
>> +
>>  static void ploop_init_request(struct ploop_request *preq)
>>  {
>>      preq->eng_state = PLOOP_E_ENTRY;
>> @@ -656,6 +672,8 @@ DEFINE_BIO_CB(ploop_fast_end_io)
>>          (test_bit(PLOOP_S_EXITING, &plo->state) ||
>>           !list_empty(&plo->entry_queue)))
>>          wake_up_interruptible(&plo->waitq);
>> +    if (plo->fast_path_disabled_count && !plo->fastpath_reqs)
>> +        wake_up(&plo->fast_path_waitq);
>>      spin_unlock_irqrestore(&plo->lock, flags);
>>
>>      BIO_ENDIO(plo->queue, orig, err);
>> @@ -969,7 +987,8 @@ static void ploop_make_request(struct request_queue *q, struct bio *bio)
>>
>>      /* No fast path, when maintenance is in progress.
>>       * (PLOOP_S_TRACK was checked immediately above) */
>> -    if (FAST_PATH_DISABLED(plo->maintenance_type))
>> +    if (FAST_PATH_DISABLED(plo->maintenance_type) ||
>> +        plo->fast_path_disabled_count)
>>          goto queue;
>>
>>      /* Attention state, always queue */
>> @@ -3442,7 +3461,6 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>>      return err;
>>  }
>>
>> -
>>  void ploop_quiesce(struct ploop_device * plo)
>>  {
>>      struct completion qcomp;
>> @@ -3452,6 +3470,8 @@ void ploop_quiesce(struct ploop_device * plo)
>>          return;
>>
>>      spin_lock_irq(&plo->lock);
>> +    plo->fast_path_disabled_count++;
>> +
>>      preq = ploop_alloc_request(plo, true);
>>      preq->req_rw = 0;
>>      preq->state = (1 << PLOOP_REQ_SYNC) | (1 << PLOOP_REQ_BARRIER);
>> @@ -3469,6 +3489,7 @@ void ploop_quiesce(struct ploop_device * plo)
>>          wake_up_interruptible(&plo->waitq);
>>      spin_unlock_irq(&plo->lock);
>>
>> +    wait_fast_path_reqs(plo);
>>      wait_for_completion(&qcomp);
>>      plo->quiesce_comp = NULL;
>>  }
>> @@ -3476,6 +3497,10 @@ EXPORT_SYMBOL(ploop_quiesce);
>>
>>  void ploop_relax(struct ploop_device * plo)
>>  {
>> +    spin_lock_irq(&plo->lock);
>> +    plo->fast_path_disabled_count--;
>> +    spin_unlock_irq(&plo->lock);
>> +
>>      if (!test_bit(PLOOP_S_RUNNING, &plo->state))
>>          return;
>>
>> @@ -5456,6 +5481,7 @@ static struct ploop_device *__ploop_dev_alloc(int index)
>>      init_waitqueue_head(&plo->req_waitq);
>>      init_waitqueue_head(&plo->freeze_waitq);
>>      init_waitqueue_head(&plo->event_waitq);
>> +    init_waitqueue_head(&plo->fast_path_waitq);
>>      plo->tune = DEFAULT_PLOOP_TUNE;
>>      map_init(plo, &plo->map);
>>      track_init(plo);
>> diff --git a/include/linux/ploop/ploop.h b/include/linux/ploop/ploop.h
>> index 66591623f79d..8495ca2145fd 100644
>> --- a/include/linux/ploop/ploop.h
>> +++ b/include/linux/ploop/ploop.h
>> @@ -407,6 +407,7 @@ struct ploop_device
>>      wait_queue_head_t    req_waitq;
>>      wait_queue_head_t    freeze_waitq;
>>      wait_queue_head_t    event_waitq;
>> +    wait_queue_head_t    fast_path_waitq;
>>
>>      struct ploop_map    map;
>>      struct ploop_map    *trans_map;
>> @@ -473,6 +474,7 @@ struct ploop_device
>>      struct block_device *dm_crypt_bdev;
>>
>>      unsigned long        locking_state; /* plo locked by userspace */
>> +    unsigned int        fast_path_disabled_count;
>>  };
>>
>>  enum
>>
>>
>> .
>>