[Devel] [PATCH rh7] ploop: io_direct: delay f_op->fsync() until FLUSH|FUA (v2)

2016-05-26 Thread Maxim Patlasov 
Once we converted extent to initialized it can be part of uncompleted
journal transaction, so we have to force transaction commit at some point.

Instead of forcing transaction commit immediately, the patch delays it
until an incoming bio with FLUSH|FUA arrives. Then, as the very first
step of processing such a bio, we sends corresponding preq to fsync_thread
to perform f_op->fsync().

As a very unlikely case, it is also possible that processing a FLUSH|FUA
bio itself results in converting extents. Then, the patch calls f_op->fsync()
immediately after conversion to preserve FUA semantics.

Changed in v2 (thanks to dmonakhov@):
- simplified bitwise arithmetic in  preq_is_special() helper;
- reject fastmap for REQ_FUA bio-s if PLOOP_IO_FSYNC_DELAYED.

https://jira.sw.ru/browse/PSBM-47026

Signed-off-by: Maxim Patlasov 
---
 drivers/block/ploop/dev.c   |   72 ---
 drivers/block/ploop/io_direct.c |   28 +++
 include/linux/ploop/ploop.h |   12 +++
 3 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 654b60b..861ce86 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -1942,46 +1942,64 @@ err:
 
 /* Main preq state machine */
 
+static inline bool preq_is_special(struct ploop_request * preq)
+{
+   unsigned long state = READ_ONCE(preq->state);
+
+   return state & (PLOOP_REQ_MERGE_FL |
+   PLOOP_REQ_RELOC_A_FL |
+   PLOOP_REQ_RELOC_S_FL |
+   PLOOP_REQ_DISCARD_FL |
+   PLOOP_REQ_ZERO_FL);
+}
+
 static void
 ploop_entry_request(struct ploop_request * preq)
 {
struct ploop_device * plo   = preq->plo;
struct ploop_delta  * top_delta = ploop_top_delta(plo);
+   struct ploop_io * top_io= _delta->io;
struct ploop_delta  * delta;
int level;
int err;
iblock_t iblk;
 
-   /* Control request. */
-   if (unlikely(preq->bl.head == NULL &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_DISCARD, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   complete(plo->quiesce_comp);
-   wait_for_completion(>relax_comp);
-   ploop_complete_request(preq);
-   complete(>relaxed_comp);
-   return;
-   }
+   if (!preq_is_special(preq)) {
+   /* Control request */
+   if (unlikely(preq->bl.head == NULL)) {
+   complete(plo->quiesce_comp);
+   wait_for_completion(>relax_comp);
+   ploop_complete_request(preq);
+   complete(>relaxed_comp);
+   return;
+   }
 
-   /* Empty flush. */
-   if (unlikely(preq->req_size == 0 &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   if (preq->req_rw & REQ_FLUSH) {
-   if (top_delta->io.ops->issue_flush) {
-   top_delta->io.ops->issue_flush(_delta->io, 
preq);
-   return;
-   }
+   /* Need to fsync before start handling FLUSH */
+   if ((preq->req_rw & REQ_FLUSH) &&
+   test_bit(PLOOP_IO_FSYNC_DELAYED, _io->io_state) &&
+   !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
+   spin_lock_irq(>lock);
+   list_add_tail(>list, _io->fsync_queue);
+   if (waitqueue_active(_io->fsync_waitq))
+   wake_up_interruptible(_io->fsync_waitq);
+   spin_unlock_irq(>lock);
+   return;
}
 
-   preq->eng_state = PLOOP_E_COMPLETE;
-   ploop_complete_request(preq);
-   return;
+   /* Empty flush or unknown zero-size request */
+   if (preq->req_size == 0) {
+   if (preq->req_rw & REQ_FLUSH &&
+   !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
+   if (top_io->ops->issue_flush) {
+   top_io->ops->issue_flush(top_io, preq);
+   return;
+   }
+   }
+
+   preq->eng_state = PLOOP_E_COMPLETE;
+   ploop_complete_request(preq);
+   return;
+   }
}
 
if

Re: [Devel] [PATCH rh7] ploop: io_direct: delay f_op->fsync() until FLUSH|FUA

2016-05-26 Thread Maxim Patlasov 

On 05/26/2016 07:58 AM, Dmitry Monakhov wrote:

Maxim Patlasov  writes:


Once we converted extent to initialized it can be part of uncompleted
journal transaction, so we have to force transaction commit at some point.

Instead of forcing transaction commit immediately, the patch delays it
until an incoming bio with FLUSH|FUA arrives. Then, as the very first
step of processing such a bio, we sends corresponding preq to fsync_thread
to perform f_op->fsync().

As a very unlikely case, it is also possible that processing a FLUSH|FUA
bio itself results in converting extents. Then, the patch calls f_op->fsync()
immediately after conversion to preserve FUA semantics.

ACK. With minor comments. See below:


Thank you for thoughtful review, Dima!


https://jira.sw.ru/browse/PSBM-47026

Signed-off-by: Maxim Patlasov 
---
  drivers/block/ploop/dev.c   |   70 ---
  drivers/block/ploop/io_direct.c |   28 +++-
  include/linux/ploop/ploop.h |6 +++
  3 files changed, 76 insertions(+), 28 deletions(-)

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 654b60b..03fc289 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -1942,46 +1942,62 @@ err:
  
  /* Main preq state machine */
  
+static inline bool preq_is_special(struct ploop_request * preq)

+{
+   return test_bit(PLOOP_REQ_MERGE, >state) ||
+   test_bit(PLOOP_REQ_RELOC_A, >state) ||
+   test_bit(PLOOP_REQ_RELOC_S, >state) ||
+   test_bit(PLOOP_REQ_DISCARD, >state) ||
+   test_bit(PLOOP_REQ_ZERO, >state);

Oh. It looks awful. Please use one atomic read here and other places.
#define PLOOP_REQ_RELOC_A_FL (1 << PLOOP_REQ_RELOC_A)
#define PLOOP_REQ_RELOC_S_FL (1 << PLOOP_REQ_RELOC_S)

unsigned long state = READ_ONCE(preq->state);
...
 if (state & (PLOOP_REQ_RELOC_A_FL|PLOOP_REQ_RELOC_B_FL) ...


OK. I'll fix this place in v2 of the patch and other places later in a 
separate "clean-up" patch.



+}
+
  static void
  ploop_entry_request(struct ploop_request * preq)
  {
struct ploop_device * plo   = preq->plo;
struct ploop_delta  * top_delta = ploop_top_delta(plo);
+   struct ploop_io * top_io= _delta->io;
struct ploop_delta  * delta;
int level;
int err;
iblock_t iblk;
  
-	/* Control request. */

-   if (unlikely(preq->bl.head == NULL &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_DISCARD, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   complete(plo->quiesce_comp);
-   wait_for_completion(>relax_comp);
-   ploop_complete_request(preq);
-   complete(>relaxed_comp);
-   return;
-   }
+   if (!preq_is_special(preq)) {
+   /* Control request */
+   if (unlikely(preq->bl.head == NULL)) {
+   complete(plo->quiesce_comp);
+   wait_for_completion(>relax_comp);
+   ploop_complete_request(preq);
+   complete(>relaxed_comp);
+   return;
+   }
  
-	/* Empty flush. */

-   if (unlikely(preq->req_size == 0 &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   if (preq->req_rw & REQ_FLUSH) {
-   if (top_delta->io.ops->issue_flush) {
-   top_delta->io.ops->issue_flush(_delta->io, 
preq);
-   return;
-   }
+   /* Need to fsync before start handling FLUSH */
+   if ((preq->req_rw & REQ_FLUSH) &&
+   test_bit(PLOOP_IO_FSYNC_DELAYED, _io->io_state) &&
+   !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
+   spin_lock_irq(>lock);
+   list_add_tail(>list, _io->fsync_queue);
+   if (waitqueue_active(_io->fsync_waitq))
+   wake_up_interruptible(_io->fsync_waitq);
+   spin_unlock_irq(>lock);
+   return;
}
  
-		preq->eng_state = PLOOP_E_COMPLETE;

-   ploop_complete_request(preq);
-   return;
+   /* Empty flush or unknown zero-size request */

Do you know any zero size requests instead of FLUSH?


No. The comment only tells explicitly what always existed in ploop: 
simply ignore such requests (if they ever come to us).



+   if (preq->req_size == 0) {
+   if

Re: [Devel] [PATCH rh7] ploop: io_direct: delay f_op->fsync() until FLUSH|FUA

2016-05-26 Thread Dmitry Monakhov 
Maxim Patlasov  writes:

> Once we converted extent to initialized it can be part of uncompleted
> journal transaction, so we have to force transaction commit at some point.
>
> Instead of forcing transaction commit immediately, the patch delays it
> until an incoming bio with FLUSH|FUA arrives. Then, as the very first
> step of processing such a bio, we sends corresponding preq to fsync_thread
> to perform f_op->fsync().
>
> As a very unlikely case, it is also possible that processing a FLUSH|FUA
> bio itself results in converting extents. Then, the patch calls f_op->fsync()
> immediately after conversion to preserve FUA semantics.
ACK. With minor comments. See below:
>
> https://jira.sw.ru/browse/PSBM-47026
>
> Signed-off-by: Maxim Patlasov 
> ---
>  drivers/block/ploop/dev.c   |   70 
> ---
>  drivers/block/ploop/io_direct.c |   28 +++-
>  include/linux/ploop/ploop.h |6 +++
>  3 files changed, 76 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 654b60b..03fc289 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -1942,46 +1942,62 @@ err:
>  
>  /* Main preq state machine */
>  
> +static inline bool preq_is_special(struct ploop_request * preq)
> +{
> + return test_bit(PLOOP_REQ_MERGE, >state) ||
> + test_bit(PLOOP_REQ_RELOC_A, >state) ||
> + test_bit(PLOOP_REQ_RELOC_S, >state) ||
> + test_bit(PLOOP_REQ_DISCARD, >state) ||
> + test_bit(PLOOP_REQ_ZERO, >state);
Oh. It looks awful. Please use one atomic read here and other places.
#define PLOOP_REQ_RELOC_A_FL (1 << PLOOP_REQ_RELOC_A)
#define PLOOP_REQ_RELOC_S_FL (1 << PLOOP_REQ_RELOC_S)

unsigned long state = READ_ONCE(preq->state);
...
if (state & (PLOOP_REQ_RELOC_A_FL|PLOOP_REQ_RELOC_B_FL) ...
> +}
> +
>  static void
>  ploop_entry_request(struct ploop_request * preq)
>  {
>   struct ploop_device * plo   = preq->plo;
>   struct ploop_delta  * top_delta = ploop_top_delta(plo);
> + struct ploop_io * top_io= _delta->io;
>   struct ploop_delta  * delta;
>   int level;
>   int err;
>   iblock_t iblk;
>  
> - /* Control request. */
> - if (unlikely(preq->bl.head == NULL &&
> -  !test_bit(PLOOP_REQ_MERGE, >state) &&
> -  !test_bit(PLOOP_REQ_RELOC_A, >state) &&
> -  !test_bit(PLOOP_REQ_RELOC_S, >state) &&
> -  !test_bit(PLOOP_REQ_DISCARD, >state) &&
> -  !test_bit(PLOOP_REQ_ZERO, >state))) {
> - complete(plo->quiesce_comp);
> - wait_for_completion(>relax_comp);
> - ploop_complete_request(preq);
> - complete(>relaxed_comp);
> - return;
> - }
> + if (!preq_is_special(preq)) {
> + /* Control request */
> + if (unlikely(preq->bl.head == NULL)) {
> + complete(plo->quiesce_comp);
> + wait_for_completion(>relax_comp);
> + ploop_complete_request(preq);
> + complete(>relaxed_comp);
> + return;
> + }
>  
> - /* Empty flush. */
> - if (unlikely(preq->req_size == 0 &&
> -  !test_bit(PLOOP_REQ_MERGE, >state) &&
> -  !test_bit(PLOOP_REQ_RELOC_A, >state) &&
> -  !test_bit(PLOOP_REQ_RELOC_S, >state) &&
> -  !test_bit(PLOOP_REQ_ZERO, >state))) {
> - if (preq->req_rw & REQ_FLUSH) {
> - if (top_delta->io.ops->issue_flush) {
> - top_delta->io.ops->issue_flush(_delta->io, 
> preq);
> - return;
> - }
> + /* Need to fsync before start handling FLUSH */
> + if ((preq->req_rw & REQ_FLUSH) &&
> + test_bit(PLOOP_IO_FSYNC_DELAYED, _io->io_state) &&
> + !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
> + spin_lock_irq(>lock);
> + list_add_tail(>list, _io->fsync_queue);
> + if (waitqueue_active(_io->fsync_waitq))
> + wake_up_interruptible(_io->fsync_waitq);
> + spin_unlock_irq(>lock);
> + return;
>   }
>  
> - preq->eng_state = PLOOP_E_COMPLETE;
> - ploop_complete_request(preq);
> - return;
> + /* Empty flush or unknown zero-size request */
Do you know any zero size requests instead of FLUSH?
> + if (preq->req_size == 0) {
> + if (preq->req_rw & REQ_FLUSH &&
> + !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {

> + if (top_io->ops->issue_flush) {
> + top_io->ops->issue_flush(top_io, preq);
>

[Devel] [PATCH rh7] ploop: io_direct: delay f_op->fsync() until FLUSH|FUA

2016-05-25 Thread Maxim Patlasov 
Once we converted extent to initialized it can be part of uncompleted
journal transaction, so we have to force transaction commit at some point.

Instead of forcing transaction commit immediately, the patch delays it
until an incoming bio with FLUSH|FUA arrives. Then, as the very first
step of processing such a bio, we sends corresponding preq to fsync_thread
to perform f_op->fsync().

As a very unlikely case, it is also possible that processing a FLUSH|FUA
bio itself results in converting extents. Then, the patch calls f_op->fsync()
immediately after conversion to preserve FUA semantics.

https://jira.sw.ru/browse/PSBM-47026

Signed-off-by: Maxim Patlasov 
---
 drivers/block/ploop/dev.c   |   70 ---
 drivers/block/ploop/io_direct.c |   28 +++-
 include/linux/ploop/ploop.h |6 +++
 3 files changed, 76 insertions(+), 28 deletions(-)

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 654b60b..03fc289 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -1942,46 +1942,62 @@ err:
 
 /* Main preq state machine */
 
+static inline bool preq_is_special(struct ploop_request * preq)
+{
+   return test_bit(PLOOP_REQ_MERGE, >state) ||
+   test_bit(PLOOP_REQ_RELOC_A, >state) ||
+   test_bit(PLOOP_REQ_RELOC_S, >state) ||
+   test_bit(PLOOP_REQ_DISCARD, >state) ||
+   test_bit(PLOOP_REQ_ZERO, >state);
+}
+
 static void
 ploop_entry_request(struct ploop_request * preq)
 {
struct ploop_device * plo   = preq->plo;
struct ploop_delta  * top_delta = ploop_top_delta(plo);
+   struct ploop_io * top_io= _delta->io;
struct ploop_delta  * delta;
int level;
int err;
iblock_t iblk;
 
-   /* Control request. */
-   if (unlikely(preq->bl.head == NULL &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_DISCARD, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   complete(plo->quiesce_comp);
-   wait_for_completion(>relax_comp);
-   ploop_complete_request(preq);
-   complete(>relaxed_comp);
-   return;
-   }
+   if (!preq_is_special(preq)) {
+   /* Control request */
+   if (unlikely(preq->bl.head == NULL)) {
+   complete(plo->quiesce_comp);
+   wait_for_completion(>relax_comp);
+   ploop_complete_request(preq);
+   complete(>relaxed_comp);
+   return;
+   }
 
-   /* Empty flush. */
-   if (unlikely(preq->req_size == 0 &&
-!test_bit(PLOOP_REQ_MERGE, >state) &&
-!test_bit(PLOOP_REQ_RELOC_A, >state) &&
-!test_bit(PLOOP_REQ_RELOC_S, >state) &&
-!test_bit(PLOOP_REQ_ZERO, >state))) {
-   if (preq->req_rw & REQ_FLUSH) {
-   if (top_delta->io.ops->issue_flush) {
-   top_delta->io.ops->issue_flush(_delta->io, 
preq);
-   return;
-   }
+   /* Need to fsync before start handling FLUSH */
+   if ((preq->req_rw & REQ_FLUSH) &&
+   test_bit(PLOOP_IO_FSYNC_DELAYED, _io->io_state) &&
+   !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
+   spin_lock_irq(>lock);
+   list_add_tail(>list, _io->fsync_queue);
+   if (waitqueue_active(_io->fsync_waitq))
+   wake_up_interruptible(_io->fsync_waitq);
+   spin_unlock_irq(>lock);
+   return;
}
 
-   preq->eng_state = PLOOP_E_COMPLETE;
-   ploop_complete_request(preq);
-   return;
+   /* Empty flush or unknown zero-size request */
+   if (preq->req_size == 0) {
+   if (preq->req_rw & REQ_FLUSH &&
+   !test_bit(PLOOP_REQ_FSYNC_DONE, >state)) {
+   if (top_io->ops->issue_flush) {
+   top_io->ops->issue_flush(top_io, preq);
+   return;
+   }
+   }
+
+   preq->eng_state = PLOOP_E_COMPLETE;
+   ploop_complete_request(preq);
+   return;
+   }
}
 
if (unlikely(test_bit(PLOOP_REQ_SYNC, >state) &&
diff --git a/drivers/block/ploop/io_direct.c b/drivers/block/ploop/io_direct.c
index 8096110..a0b7a4e 100644
--- a/drivers/block/ploop/io_direct.c
+++