A discard unmapping a cluster locks its L2 index, punches the cluster, clears the index and, after the L2 writeback, drops the cluster refcount in process_indexes_end(), which makes the cluster reusable. A data write submitted to the cluster before the index was locked may still be in flight at that moment. If it lands after the cluster is allocated again, it corrupts the data or the metadata of the new owner.
Make such a discard wait in process_indexes_end() until the writes counted in its L2 page are completed, right before the refcount is dropped. The discard switches the generation of the page, so writes submitted later are counted separately, and waits for the previous generation to drain. Writes submitted after the index is locked can't refer the cluster, so there is no need to wait for them. Only one discard at a time switches the generation of a page, others wait for it to finish first. Otherwise a second switch would return new writes to the generation the first discard waits for, and the last of them would complete without waking it. The hole punch in issue_discard() is not delayed: a write landing into the cluster between the punch and the reuse is harmless, since allocate_cluster() punches a reused cluster again. Clusters shared with snapshots and compressed clusters are never written directly, so discards of them do not wait. https://virtuozzo.atlassian.net/browse/VSTOR-154659 Fixes: 6e1c3e5a91fd ("drivers/md/dm-qcow2: update metadata on whole cluster discard") Feature: dm-qcow2: block device over QCOW2 files driver Signed-off-by: Andrey Zhadchenko <[email protected]> --- drivers/md/dm-qcow2-map.c | 83 +++++++++++++++++++++++++++++++++++- drivers/md/dm-qcow2-target.c | 3 ++ drivers/md/dm-qcow2.h | 6 ++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c index 2c0e7bdb251dc..b07747f80030f 100644 --- a/drivers/md/dm-qcow2-map.c +++ b/drivers/md/dm-qcow2-map.c @@ -622,12 +622,87 @@ static void inc_writers(struct md_page *md, struct qio *qio) atomic_inc(&md->active_writers[qio->writer_gen]); } +static void discard_waiters_to_list(struct qcow2 *qcow2, struct md_page *md, + struct list_head *list) +{ + struct qio *discard, *tmp; + + lockdep_assert_held(&qcow2->md_pages_lock); + list_for_each_entry_safe(discard, tmp, &qcow2->discard_wait_list, link) { + if (discard->ext->lx_md == md) + list_move_tail(&discard->link, list); + } +} + static void dec_writers(struct qio *qio) { struct md_page *md = qio->writer_md; + struct qcow2 *qcow2 = qio->qcow2; + u8 gen = qio->writer_gen; + LIST_HEAD(wait_list); + unsigned long flags; qio->writer_md = NULL; - atomic_dec(&md->active_writers[qio->writer_gen]); + /* + * Fully ordered RMW pairs with smp_mb() in delay_discard_if_writers(): + * either we see switched generation, or the discard sees our put. + * The generation can't be switched back to ours while a discard + * waits for it, so the same generation means nobody is to wake. + */ + if (atomic_dec_return(&md->active_writers[gen]) || + READ_ONCE(md->writer_gen) == gen) + return; + + spin_lock_irqsave(&qcow2->md_pages_lock, flags); + discard_waiters_to_list(qcow2, md, &wait_list); + spin_unlock_irqrestore(&qcow2->md_pages_lock, flags); + qcow2_dispatch_qios(qcow2, NULL, &wait_list); +} + +static bool delay_discard_if_writers(struct qcow2 *qcow2, struct md_page *md, + struct qio *qio) +{ + LIST_HEAD(wake_list); + bool ret = false; + u8 gen; + + spin_lock_irq(&qcow2->md_pages_lock); + if (qio->flags & QIO_DISCARD_WAIT_FL) { + if (atomic_read(&md->active_writers[qio->writer_gen])) + goto delay; + qio->flags &= ~QIO_DISCARD_WAIT_FL; + md->gen_switched = false; + discard_waiters_to_list(qcow2, md, &wake_list); + goto unlock; + } + + /* another discard already waits: we can't switch without breaking it */ + if (md->gen_switched) + goto delay; + + /* + * md->active_writers checks are valid only if increases are + * from main kwork (as we are) or under md_pages_lock. + */ + gen = md->writer_gen; + if (!atomic_read(&md->active_writers[gen])) + goto unlock; + + WRITE_ONCE(md->writer_gen, !gen); + smp_mb(); /* Pairs with atomic_dec_return() in dec_writers() */ + if (!atomic_read(&md->active_writers[gen])) + goto unlock; + + md->gen_switched = true; + qio->writer_gen = gen; + qio->flags |= QIO_DISCARD_WAIT_FL; +delay: + list_add_tail(&qio->link, &qcow2->discard_wait_list); + ret = true; +unlock: + spin_unlock_irq(&qcow2->md_pages_lock); + qcow2_dispatch_qios(qcow2, NULL, &wake_list); + return ret; } static u32 calc_cow_mask(struct qcow2 *qcow2, u64 ext_l2, @@ -3547,6 +3622,9 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio, ext->only_set_ext_l2 = true; else if (zeroes && !qcow2->ext_l2) ext->set_all_zeroes = true; + /* wait only if the cluster can be reused later */ + if (unmap && !map->clu_is_cow) + ext->wait_writers = true; (*qio)->flags |= QIO_IS_DISCARD_FL; return 1; @@ -4294,6 +4372,9 @@ next: qio = qio_list_pop(qio_list); */ if (delay_if_has_wpc_readers(qcow2, ext->lx_md, &qio)) goto next; + if (ext->wait_writers && + delay_discard_if_writers(qcow2, ext->lx_md, qio)) + goto next; pos = ext->unuse_clu_pos; for (; pos < ext->unuse_clu_end; pos += clu_size) { diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c index 07ca928002e0a..24a42c5a66759 100644 --- a/drivers/md/dm-qcow2-target.c +++ b/drivers/md/dm-qcow2-target.c @@ -228,6 +228,7 @@ void qcow2_destroy(struct qcow2 *qcow2) "qcow2: list %d is not empty", i); WARN_ON(!list_empty(&qcow2->paused_qios) || + !list_empty(&qcow2->discard_wait_list) || !list_empty(&qcow2->wb_batch_list) || !list_empty(&qcow2->slow_wb_batch_list) || timer_pending(&qcow2->slow_wb_timer)); @@ -400,6 +401,7 @@ int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index, struct md_pag atomic_set(&(*md)->wpc_readers, 0); (*md)->wpc_noread_count = 0; (*md)->writer_gen = 0; + (*md)->gen_switched = false; atomic_set(&(*md)->active_writers[0], 0); atomic_set(&(*md)->active_writers[1], 0); INIT_LIST_HEAD(&(*md)->wait_list); @@ -666,6 +668,7 @@ static struct qcow2 *qcow2_alloc_delta(struct qcow2_target *tgt, struct qcow2 *u INIT_LIST_HEAD(&qcow2->qios[i]); INIT_LIST_HEAD(&qcow2->resubmit_qios); INIT_LIST_HEAD(&qcow2->paused_qios); + INIT_LIST_HEAD(&qcow2->discard_wait_list); INIT_LIST_HEAD(&qcow2->wb_batch_list); INIT_LIST_HEAD(&qcow2->slow_wb_batch_list); spin_lock_init(&qcow2->deferred_lock); diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h index 5755788e0e5ae..8eb0c49f1e4eb 100644 --- a/drivers/md/dm-qcow2.h +++ b/drivers/md/dm-qcow2.h @@ -136,6 +136,7 @@ struct md_page { #define MD_WRITEBACK_ERROR (1U << 3) /* Last writeback failed with error */ unsigned int status; u8 writer_gen; + bool gen_switched; struct page *page; struct list_head wait_list; /* To link in qcow2::{,slow}wb_batch_list and qcow2::QLIST_COMPLETED_WB */ @@ -280,6 +281,7 @@ struct qcow2 { struct list_head qios[QLIST_COUNT]; struct list_head resubmit_qios; struct list_head paused_qios; /* For pause_submitting_qios */ + struct list_head discard_wait_list; /* For batching md update: */ struct list_head wb_batch_list; @@ -321,6 +323,7 @@ struct qio_ext { u32 cow_mask; bool only_set_ext_l2:1; bool set_all_zeroes:1; + bool wait_writers:1; u8 lx_level; @@ -351,6 +354,7 @@ struct qio { #define QIO_IS_MERGE_FL (1 << 3) /* This is service merge qio */ #define QIO_IS_DISCARD_FL (1 << 4) /* This zeroes index (discard or backward merge) */ #define QIO_IS_L1COW_FL (1 << 5) /* This qio only wants COW at L1 */ +#define QIO_DISCARD_WAIT_FL (1 << 6) /* Discard waits writer_gen drain */ #define QIO_SPLIT_INHERITED_FLAGS (QIO_IS_DISCARD_FL) u8 flags; #define REF_INDEX_INVALID 2 @@ -363,7 +367,7 @@ struct qio { * bio processing should resume. */ u8 queue_list_id:4; - u8 writer_gen:1; + u8 writer_gen:1; /* for discard this is the generation it waits */ atomic_t remaining; -- 2.43.5 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
