Re: [Devel] [PATCH rh7 9/9] ploop: fixup barrier handling during relocation
Maxim Patlasov writes:
> Rebase Dima's patch on top of rh7-3.10.0-327.18.2.vz7.14.19,
> but without help of delayed_flush engine:
>
> To ensure consistency on crash/power outage/hard reboot
> events, ploop must implement the following barrier logic
> for RELOC_A|S requests:
>
> 1) After we store data to new place, but before updating
> BAT on disk, we have FLUSH everything (in fact, flushing
> those data would be enough, but it is simplier to flush
> everything).
>
> 2) We should not proceed handling RELOC_A|S until we
> 100% sure new BAT value went to disk platters. So far as
> new BAT is only one page, it's OK to mark corresponding
> bio with FUA flag for io_direct case. For io_kaio, not
> having FUA api, we have to post_fsync BAT update.
>
> PLOOP_REQ_FORCE_FLUSH/PLOOP_REQ_FORCE_FUA introduced
> long time ago probably were intended to ensure the
> logic above, but they actually didn't.
>
> The patch removes PLOOP_REQ_FORCE_FLUSH/PLOOP_REQ_FORCE_FUA,
> and implements barriers in a straightforward and simple way:
> check for RELOC_A|S explicitly and make FLUSH/FUA where
> needed.
>
> Signed-off-by: Maxim Patlasov
> ---
> drivers/block/ploop/dev.c |4 ++--
> drivers/block/ploop/io_direct.c |7 ---
> drivers/block/ploop/io_kaio.c |8 +---
> drivers/block/ploop/map.c | 22 ++
> include/linux/ploop/ploop.h |1 -
> 5 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 2b60dfa..40768b6 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -2610,8 +2610,8 @@ restart:
> top_delta = ploop_top_delta(plo);
> sbl.head = sbl.tail = preq->aux_bio;
>
> - /* Relocated data write required sync before BAT updatee */
> - set_bit(PLOOP_REQ_FORCE_FUA, &preq->state);
> + /* Relocated data write required sync before BAT update
> + * this will happen inside index_update */
>
> if (test_bit(PLOOP_REQ_RELOC_S, &preq->state)) {
> preq->eng_state = PLOOP_E_DATA_WBI;
> diff --git a/drivers/block/ploop/io_direct.c b/drivers/block/ploop/io_direct.c
> index c4d0f63..266f041 100644
> --- a/drivers/block/ploop/io_direct.c
> +++ b/drivers/block/ploop/io_direct.c
> @@ -89,15 +89,11 @@ dio_submit(struct ploop_io *io, struct ploop_request *
> preq,
> sector_t sec, nsec;
> int err;
> struct bio_list_walk bw;
> - int postfua = 0;
> int write = !!(rw & REQ_WRITE);
> int delayed_fua = 0;
>
> trace_submit(preq);
>
> - if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state))
> - postfua = 1;
> -
> if ((rw & REQ_FUA) && ploop_req_delay_fua_possible(preq)) {
> /* Mark req that delayed flush required */
> preq->req_rw |= (REQ_FLUSH | REQ_FUA);
> @@ -233,9 +229,6 @@ flush_bio:
> b->bi_private = preq;
> b->bi_end_io = dio_endio_async;
>
> - if (unlikely(postfua && !bl.head))
> - rw2 |= REQ_FUA;
> -
> ploop_acc_ff_out(preq->plo, rw2 | b->bi_rw);
> submit_bio(rw2, b);
> }
> diff --git a/drivers/block/ploop/io_kaio.c b/drivers/block/ploop/io_kaio.c
> index ed550f4..85863df 100644
> --- a/drivers/block/ploop/io_kaio.c
> +++ b/drivers/block/ploop/io_kaio.c
> @@ -69,6 +69,8 @@ static void kaio_complete_io_state(struct ploop_request *
> preq)
> unsigned long flags;
> int post_fsync = 0;
> int need_fua = !!(preq->req_rw & REQ_FUA);
> + unsigned long state = READ_ONCE(preq->state);
> + int reloc = !!(state & (PLOOP_REQ_RELOC_A_FL|PLOOP_REQ_RELOC_S_FL));
>
> if (preq->error || !(preq->req_rw & REQ_FUA) ||
> preq->eng_state == PLOOP_E_INDEX_READ ||
> @@ -80,9 +82,9 @@ static void kaio_complete_io_state(struct ploop_request *
> preq)
> }
>
> /* Convert requested fua to fsync */
> - if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state) ||
> - test_and_clear_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state) ||
> - (need_fua && !ploop_req_delay_fua_possible(preq))) {
This is the change I dislike the most. io_XXX should not care it is
reloc or not. Caller should rule whenether PREFLUSH/POSTFLUSH should
happen before preq completes. So IMHO this is a crunch, but correct one.
> + if (test_and_clear_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state) ||
> + (need_fua && !ploop_req_delay_fua_possible(preq)) ||
> + (reloc && ploop_req_delay_fua_possible(preq))) {
> post_fsync = 1;
> preq->req_rw &= ~REQ_FUA;
> }
> diff --git a/drivers/block/ploop/map.c b/drivers/block/ploop/map.c
> index 915a216..1883674 100644
> --- a/drivers/block/ploop/map.c
> +++ b/drivers/block/ploop/map.c
> @@ -909,6 +909,7 @@ void ploop_index_update(struct ploop_request * preq)
> struct page * page;
> s
[Devel] [PATCH rh7 9/9] ploop: fixup barrier handling during relocation
Rebase Dima's patch on top of rh7-3.10.0-327.18.2.vz7.14.19,
but without help of delayed_flush engine:
To ensure consistency on crash/power outage/hard reboot
events, ploop must implement the following barrier logic
for RELOC_A|S requests:
1) After we store data to new place, but before updating
BAT on disk, we have FLUSH everything (in fact, flushing
those data would be enough, but it is simplier to flush
everything).
2) We should not proceed handling RELOC_A|S until we
100% sure new BAT value went to disk platters. So far as
new BAT is only one page, it's OK to mark corresponding
bio with FUA flag for io_direct case. For io_kaio, not
having FUA api, we have to post_fsync BAT update.
PLOOP_REQ_FORCE_FLUSH/PLOOP_REQ_FORCE_FUA introduced
long time ago probably were intended to ensure the
logic above, but they actually didn't.
The patch removes PLOOP_REQ_FORCE_FLUSH/PLOOP_REQ_FORCE_FUA,
and implements barriers in a straightforward and simple way:
check for RELOC_A|S explicitly and make FLUSH/FUA where
needed.
Signed-off-by: Maxim Patlasov
---
drivers/block/ploop/dev.c |4 ++--
drivers/block/ploop/io_direct.c |7 ---
drivers/block/ploop/io_kaio.c |8 +---
drivers/block/ploop/map.c | 22 ++
include/linux/ploop/ploop.h |1 -
5 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index 2b60dfa..40768b6 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -2610,8 +2610,8 @@ restart:
top_delta = ploop_top_delta(plo);
sbl.head = sbl.tail = preq->aux_bio;
- /* Relocated data write required sync before BAT updatee */
- set_bit(PLOOP_REQ_FORCE_FUA, &preq->state);
+ /* Relocated data write required sync before BAT update
+* this will happen inside index_update */
if (test_bit(PLOOP_REQ_RELOC_S, &preq->state)) {
preq->eng_state = PLOOP_E_DATA_WBI;
diff --git a/drivers/block/ploop/io_direct.c b/drivers/block/ploop/io_direct.c
index c4d0f63..266f041 100644
--- a/drivers/block/ploop/io_direct.c
+++ b/drivers/block/ploop/io_direct.c
@@ -89,15 +89,11 @@ dio_submit(struct ploop_io *io, struct ploop_request * preq,
sector_t sec, nsec;
int err;
struct bio_list_walk bw;
- int postfua = 0;
int write = !!(rw & REQ_WRITE);
int delayed_fua = 0;
trace_submit(preq);
- if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state))
- postfua = 1;
-
if ((rw & REQ_FUA) && ploop_req_delay_fua_possible(preq)) {
/* Mark req that delayed flush required */
preq->req_rw |= (REQ_FLUSH | REQ_FUA);
@@ -233,9 +229,6 @@ flush_bio:
b->bi_private = preq;
b->bi_end_io = dio_endio_async;
- if (unlikely(postfua && !bl.head))
- rw2 |= REQ_FUA;
-
ploop_acc_ff_out(preq->plo, rw2 | b->bi_rw);
submit_bio(rw2, b);
}
diff --git a/drivers/block/ploop/io_kaio.c b/drivers/block/ploop/io_kaio.c
index ed550f4..85863df 100644
--- a/drivers/block/ploop/io_kaio.c
+++ b/drivers/block/ploop/io_kaio.c
@@ -69,6 +69,8 @@ static void kaio_complete_io_state(struct ploop_request *
preq)
unsigned long flags;
int post_fsync = 0;
int need_fua = !!(preq->req_rw & REQ_FUA);
+ unsigned long state = READ_ONCE(preq->state);
+ int reloc = !!(state & (PLOOP_REQ_RELOC_A_FL|PLOOP_REQ_RELOC_S_FL));
if (preq->error || !(preq->req_rw & REQ_FUA) ||
preq->eng_state == PLOOP_E_INDEX_READ ||
@@ -80,9 +82,9 @@ static void kaio_complete_io_state(struct ploop_request *
preq)
}
/* Convert requested fua to fsync */
- if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state) ||
- test_and_clear_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state) ||
- (need_fua && !ploop_req_delay_fua_possible(preq))) {
+ if (test_and_clear_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state) ||
+ (need_fua && !ploop_req_delay_fua_possible(preq)) ||
+ (reloc && ploop_req_delay_fua_possible(preq))) {
post_fsync = 1;
preq->req_rw &= ~REQ_FUA;
}
diff --git a/drivers/block/ploop/map.c b/drivers/block/ploop/map.c
index 915a216..1883674 100644
--- a/drivers/block/ploop/map.c
+++ b/drivers/block/ploop/map.c
@@ -909,6 +909,7 @@ void ploop_index_update(struct ploop_request * preq)
struct page * page;
sector_t sec;
unsigned long rw;
+ unsigned long state = READ_ONCE(preq->state);
/* No way back, we are going to initiate index write. */
@@ -961,10 +962,6 @@ void ploop_index_update(struct ploop_request * preq)
__TRACE("wbi %p %u %p\n", preq, preq->req_cluster, m);
plo->st.map_single_writes++;
top_delta->ops->map_index(
