Re: [Devel] [PATCH 3/3] ploop: fixup FORCE_{FLUSH,FUA} handling v2
Dima, I agree with general approach of this patch, but there are some (easy-to-fix) issues. See, please, inline comments below... On 06/20/2016 11:58 AM, Dmitry Monakhov wrote: barrier code is broken in many ways: Currently only ->dio_submit() handles PLOOP_REQ_FORCE_{FLUSH,FUA} correctly. But request also can goes though ->dio_submit_alloc()->dio_submit_pad and write_page (for indexes) So in case of grow_dev we have following sequance: E_RELOC_DATA_READ: ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); ->delta->allocate ->io->submit_allloc: dio_submit_alloc ->dio_submit_pad E_DATA_WBI : data written, time to update index ->delta->allocate_complete:ploop_index_update ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); ->write_page ->ploop_map_wb_complete ->ploop_wb_complete_post_process ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); E_RELOC_NULLIFY: ->submit() BUG#2: currecntly kaio write_page silently ignores REQ_FUA Sorry, I can't agree, it actually does not ignore: static void kaio_write_page(struct ploop_io * io, struct ploop_request * preq, struct page * page, sector_t sec, int fua) { /* No FUA in kaio, convert it to fsync */ if (fua) set_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state); BUG#3: io_direct:dio_submit if fua_delay is not possible we MUST tag all bios via REQ_FUA not just latest one. No need to tag *all*. See inline comments below. This patch unify barrier handling like follows: - Get rid of FORCE_{FLUSH,FUA} - Introduce DELAYED_FLUSH, currecntly it supported only by io_direct - fix up fua handling for dio_submit This makes reloc sequence optimal: io_direct RELOC_S: R1, W2, WBI:FLUSH|FUA RELOC_A: R1, W2, WBI:FLUSH|FUA, W1:NULLIFY|FUA io_kaio RELOC_S: R1, W2:FUA, WBI:FUA RELOC_A: R1, W2:FUA, WBI:FUA, W1:NULLIFY|FUA https://jira.sw.ru/browse/PSBM-47107 Signed-off-by: Dmitry Monakhov --- drivers/block/ploop/dev.c | 8 +--- drivers/block/ploop/io_direct.c | 29 +- drivers/block/ploop/io_kaio.c | 17 ++-- drivers/block/ploop/map.c | 45 ++--- include/linux/ploop/ploop.h | 8 5 files changed, 54 insertions(+), 53 deletions(-) diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c index 96f7850..fbc5f2f 100644 --- a/drivers/block/ploop/dev.c +++ b/drivers/block/ploop/dev.c @@ -1224,6 +1224,9 @@ static void ploop_complete_request(struct ploop_request * preq) __TRACE("Z %p %u\n", preq, preq->req_cluster); + if (!preq->error) { + WARN_ON(test_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state)); + } while (preq->bl.head) { struct bio * bio = preq->bl.head; preq->bl.head = bio->bi_next; @@ -2530,9 +2533,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 updatee +* this will happen inside index_update */ if (test_bit(PLOOP_REQ_RELOC_S, &preq->state)) { preq->eng_state = PLOOP_E_DATA_WBI; plo->st.bio_out++; diff --git a/drivers/block/ploop/io_direct.c b/drivers/block/ploop/io_direct.c index a6d83fe..d7ecd4a 100644 --- a/drivers/block/ploop/io_direct.c +++ b/drivers/block/ploop/io_direct.c @@ -90,21 +90,12 @@ dio_submit(struct ploop_io *io, struct ploop_request * preq, trace_submit(preq); preflush = !!(rw & REQ_FLUSH); - - if (test_and_clear_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state)) - preflush = 1; - - if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state)) - postfua = 1; - - if (!postfua && ploop_req_delay_fua_possible(rw, preq)) { - + postfua = !!(rw & REQ_FUA); + if (ploop_req_delay_fua_possible(rw, preq)) { /* Mark req that delayed flush required */ - set_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state); - } else if (rw & REQ_FUA) { - postfua = 1; + set_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state); + postfua = 0; } "postfua" is a horrible name, let us see if we can get rid of it completely. Also, the way how ploop_req_delay_fua_possible implemented is prone to errors (see below an issue in kaio_complete_io_state). Let's rework it like this: static inline bool ploop_req_delay_fua_possible(struct ploop_request *preq) { return preq->eng_state == PLOOP_E_DATA_WBI; } Then, that chunk in the dio_submit above might look as: /* If we can delay, mark req that delayed flush required */ if ((r
Re: [Devel] [PATCH 3/3] ploop: fixup FORCE_{FLUSH,FUA} handling v2
Maxim Patlasov writes: > Dima, > > I agree with general approach of this patch, but there are some > (easy-to-fix) issues. See, please, inline comments below... > > On 06/20/2016 11:58 AM, Dmitry Monakhov wrote: >> barrier code is broken in many ways: >> Currently only ->dio_submit() handles PLOOP_REQ_FORCE_{FLUSH,FUA} correctly. >> But request also can goes though ->dio_submit_alloc()->dio_submit_pad and >> write_page (for indexes) >> So in case of grow_dev we have following sequance: >> >> E_RELOC_DATA_READ: >> ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); >>->delta->allocate >> ->io->submit_allloc: dio_submit_alloc >> ->dio_submit_pad >> E_DATA_WBI : data written, time to update index >>->delta->allocate_complete:ploop_index_update >> ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); >> ->write_page >> ->ploop_map_wb_complete >>->ploop_wb_complete_post_process >> ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); >> E_RELOC_NULLIFY: >> >> ->submit() >> >> BUG#2: currecntly kaio write_page silently ignores REQ_FUA > > Sorry, I can't agree, it actually does not ignore: I've misstyped. I ment to say REQ_FLUSH. > >> static void >> kaio_write_page(struct ploop_io * io, struct ploop_request * preq, >> struct page * page, sector_t sec, int fua) >> { >> /* No FUA in kaio, convert it to fsync */ >> if (fua) >> set_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state); > > >> BUG#3: io_direct:dio_submit if fua_delay is not possible we MUST tag all >> bios via REQ_FUA >> not just latest one. > > No need to tag *all*. See inline comments below. > >> This patch unify barrier handling like follows: >> - Get rid of FORCE_{FLUSH,FUA} >> - Introduce DELAYED_FLUSH, currecntly it supported only by io_direct >> - fix up fua handling for dio_submit >> >> This makes reloc sequence optimal: >> io_direct >> RELOC_S: R1, W2, WBI:FLUSH|FUA >> RELOC_A: R1, W2, WBI:FLUSH|FUA, W1:NULLIFY|FUA >> io_kaio >> RELOC_S: R1, W2:FUA, WBI:FUA >> RELOC_A: R1, W2:FUA, WBI:FUA, W1:NULLIFY|FUA >> >> https://jira.sw.ru/browse/PSBM-47107 >> Signed-off-by: Dmitry Monakhov >> --- >> drivers/block/ploop/dev.c | 8 +--- >> drivers/block/ploop/io_direct.c | 29 +- >> drivers/block/ploop/io_kaio.c | 17 ++-- >> drivers/block/ploop/map.c | 45 >> ++--- >> include/linux/ploop/ploop.h | 8 >> 5 files changed, 54 insertions(+), 53 deletions(-) >> >> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c >> index 96f7850..fbc5f2f 100644 >> --- a/drivers/block/ploop/dev.c >> +++ b/drivers/block/ploop/dev.c >> @@ -1224,6 +1224,9 @@ static void ploop_complete_request(struct >> ploop_request * preq) >> >> __TRACE("Z %p %u\n", preq, preq->req_cluster); >> >> +if (!preq->error) { >> +WARN_ON(test_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state)); >> +} >> while (preq->bl.head) { >> struct bio * bio = preq->bl.head; >> preq->bl.head = bio->bi_next; >> @@ -2530,9 +2533,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 updatee >> + * this will happen inside index_update */ >> if (test_bit(PLOOP_REQ_RELOC_S, &preq->state)) { >> preq->eng_state = PLOOP_E_DATA_WBI; >> plo->st.bio_out++; >> diff --git a/drivers/block/ploop/io_direct.c >> b/drivers/block/ploop/io_direct.c >> index a6d83fe..d7ecd4a 100644 >> --- a/drivers/block/ploop/io_direct.c >> +++ b/drivers/block/ploop/io_direct.c >> @@ -90,21 +90,12 @@ dio_submit(struct ploop_io *io, struct ploop_request * >> preq, >> trace_submit(preq); >> >> preflush = !!(rw & REQ_FLUSH); >> - >> -if (test_and_clear_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state)) >> -preflush = 1; >> - >> -if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state)) >> -postfua = 1; >> - >> -if (!postfua && ploop_req_delay_fua_possible(rw, preq)) { >> - >> +postfua = !!(rw & REQ_FUA); >> +if (ploop_req_delay_fua_possible(rw, preq)) { >> /* Mark req that delayed flush required */ >> -set_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state); >> -} else if (rw & REQ_FUA) { >> -postfua = 1; >> +set_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state); >> +postfua = 0; >> } > > "postfua" is a horrible name, let us see if we can get rid of it > completely. Also, the way how ploop_req_delay_fua_possible implemented > is prone to err
Re: [Devel] [PATCH 3/3] ploop: fixup FORCE_{FLUSH,FUA} handling v2
On 06/21/2016 12:25 AM, Dmitry Monakhov wrote: Maxim Patlasov writes: Dima, I agree with general approach of this patch, but there are some (easy-to-fix) issues. See, please, inline comments below... On 06/20/2016 11:58 AM, Dmitry Monakhov wrote: barrier code is broken in many ways: Currently only ->dio_submit() handles PLOOP_REQ_FORCE_{FLUSH,FUA} correctly. But request also can goes though ->dio_submit_alloc()->dio_submit_pad and write_page (for indexes) So in case of grow_dev we have following sequance: E_RELOC_DATA_READ: ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); ->delta->allocate ->io->submit_allloc: dio_submit_alloc ->dio_submit_pad E_DATA_WBI : data written, time to update index ->delta->allocate_complete:ploop_index_update ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); ->write_page ->ploop_map_wb_complete ->ploop_wb_complete_post_process ->set_bit(PLOOP_REQ_FORCE_FUA, &preq->state); E_RELOC_NULLIFY: ->submit() BUG#2: currecntly kaio write_page silently ignores REQ_FUA Sorry, I can't agree, it actually does not ignore: I've misstyped. I ment to say REQ_FLUSH. static void kaio_write_page(struct ploop_io * io, struct ploop_request * preq, struct page * page, sector_t sec, int fua) { /* No FUA in kaio, convert it to fsync */ if (fua) set_bit(PLOOP_REQ_KAIO_FSYNC, &preq->state); BUG#3: io_direct:dio_submit if fua_delay is not possible we MUST tag all bios via REQ_FUA not just latest one. No need to tag *all*. See inline comments below. This patch unify barrier handling like follows: - Get rid of FORCE_{FLUSH,FUA} - Introduce DELAYED_FLUSH, currecntly it supported only by io_direct - fix up fua handling for dio_submit This makes reloc sequence optimal: io_direct RELOC_S: R1, W2, WBI:FLUSH|FUA RELOC_A: R1, W2, WBI:FLUSH|FUA, W1:NULLIFY|FUA io_kaio RELOC_S: R1, W2:FUA, WBI:FUA RELOC_A: R1, W2:FUA, WBI:FUA, W1:NULLIFY|FUA https://jira.sw.ru/browse/PSBM-47107 Signed-off-by: Dmitry Monakhov --- drivers/block/ploop/dev.c | 8 +--- drivers/block/ploop/io_direct.c | 29 +- drivers/block/ploop/io_kaio.c | 17 ++-- drivers/block/ploop/map.c | 45 ++--- include/linux/ploop/ploop.h | 8 5 files changed, 54 insertions(+), 53 deletions(-) diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c index 96f7850..fbc5f2f 100644 --- a/drivers/block/ploop/dev.c +++ b/drivers/block/ploop/dev.c @@ -1224,6 +1224,9 @@ static void ploop_complete_request(struct ploop_request * preq) __TRACE("Z %p %u\n", preq, preq->req_cluster); + if (!preq->error) { + WARN_ON(test_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state)); + } while (preq->bl.head) { struct bio * bio = preq->bl.head; preq->bl.head = bio->bi_next; @@ -2530,9 +2533,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 updatee +* this will happen inside index_update */ if (test_bit(PLOOP_REQ_RELOC_S, &preq->state)) { preq->eng_state = PLOOP_E_DATA_WBI; plo->st.bio_out++; diff --git a/drivers/block/ploop/io_direct.c b/drivers/block/ploop/io_direct.c index a6d83fe..d7ecd4a 100644 --- a/drivers/block/ploop/io_direct.c +++ b/drivers/block/ploop/io_direct.c @@ -90,21 +90,12 @@ dio_submit(struct ploop_io *io, struct ploop_request * preq, trace_submit(preq); preflush = !!(rw & REQ_FLUSH); - - if (test_and_clear_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state)) - preflush = 1; - - if (test_and_clear_bit(PLOOP_REQ_FORCE_FUA, &preq->state)) - postfua = 1; - - if (!postfua && ploop_req_delay_fua_possible(rw, preq)) { - + postfua = !!(rw & REQ_FUA); + if (ploop_req_delay_fua_possible(rw, preq)) { /* Mark req that delayed flush required */ - set_bit(PLOOP_REQ_FORCE_FLUSH, &preq->state); - } else if (rw & REQ_FUA) { - postfua = 1; + set_bit(PLOOP_REQ_DELAYED_FLUSH, &preq->state); + postfua = 0; } "postfua" is a horrible name, let us see if we can get rid of it completely. Also, the way how ploop_req_delay_fua_possible implemented is prone to errors (see below an issue in kaio_complete_io_state). Let's rework it like this: Let it be postflush. static inline bool ploop_req_delay_fua_possible(struct ploop_request *preq) { return preq->eng_state ==