From: Susan LeGendre-McGhee <slegendr@xxxxxxxxxx>
Signed-off-by: Susan LeGendre-McGhee <slegendr@xxxxxxxxxx>
Signed-off-by: Matthew Sakai <msakai@xxxxxxxxxx>
---
drivers/md/dm-vdo/block-map.c | 8 ++++----
drivers/md/dm-vdo/data-vio.c | 9 +++++----
drivers/md/dm-vdo/dm-vdo-target.c | 12 +++++-------
drivers/md/dm-vdo/packer.c | 16 +++++++---------
drivers/md/dm-vdo/packer.h | 2 +-
drivers/md/dm-vdo/repair.c | 4 ++--
drivers/md/dm-vdo/slab-depot.c | 16 +++++++++++-----
drivers/md/dm-vdo/vdo.c | 2 +-
drivers/md/dm-vdo/vio.c | 1 -
9 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/drivers/md/dm-vdo/block-map.c b/drivers/md/dm-vdo/block-map.c
index e3fadb5f2c2d..5be400743c03 100644
--- a/drivers/md/dm-vdo/block-map.c
+++ b/drivers/md/dm-vdo/block-map.c
@@ -542,7 +542,7 @@ static unsigned int distribute_page_over_waitq(struct page_info *info,
/*
* Increment the busy count once for each pending completion so that this page does not
- * stop being busy until all completions have been processed (VDO-83).
+ * stop being busy until all completions have been processed.
*/
info->busy += num_pages;
@@ -1097,9 +1097,9 @@ static void write_pages(struct vdo_completion *flush_completion)
struct vdo_page_cache *cache = ((struct page_info *) flush_completion->parent)->cache;
/*
- * We need to cache these two values on the stack since in the error case below, it is
- * possible for the last page info to cause the page cache to get freed. Hence once we
- * launch the last page, it may be unsafe to dereference the cache [VDO-4724].
+ * We need to cache these two values on the stack since it is possible for the last
+ * page info to cause the page cache to get freed. Hence once we launch the last page,
+ * it may be unsafe to dereference the cache.
*/
bool has_unflushed_pages = (cache->pages_to_flush > 0);
page_count_t pages_in_flush = cache->pages_in_flush;
diff --git a/drivers/md/dm-vdo/data-vio.c b/drivers/md/dm-vdo/data-vio.c
index d77adeb5006e..f6c32dc9a822 100644
--- a/drivers/md/dm-vdo/data-vio.c
+++ b/drivers/md/dm-vdo/data-vio.c
@@ -453,10 +453,11 @@ static void attempt_logical_block_lock(struct vdo_completion *completion)
/*
* If the new request is a pure read request (not read-modify-write) and the lock_holder is
- * writing and has received an allocation (VDO-2683), service the read request immediately
- * by copying data from the lock_holder to avoid having to flush the write out of the
- * packer just to prevent the read from waiting indefinitely. If the lock_holder does not
- * yet have an allocation, prevent it from blocking in the packer and wait on it.
+ * writing and has received an allocation, service the read request immediately by copying
+ * data from the lock_holder to avoid having to flush the write out of the packer just to
+ * prevent the read from waiting indefinitely. If the lock_holder does not yet have an
+ * allocation, prevent it from blocking in the packer and wait on it. This is necessary in
+ * order to prevent returning data that may not have actually been written.
*/
if (!data_vio->write && READ_ONCE(lock_holder->allocation_succeeded)) {
copy_to_bio(data_vio->user_bio, lock_holder->vio.data + data_vio->offset);
diff --git a/drivers/md/dm-vdo/dm-vdo-target.c b/drivers/md/dm-vdo/dm-vdo-target.c
index 7afd1dfec649..0114fa4d48a2 100644
--- a/drivers/md/dm-vdo/dm-vdo-target.c
+++ b/drivers/md/dm-vdo/dm-vdo-target.c
@@ -945,13 +945,11 @@ static void vdo_io_hints(struct dm_target *ti, struct queue_limits *limits)
* Sets the maximum discard size that will be passed into VDO. This value comes from a
* table line value passed in during dmsetup create.
*
- * The value 1024 is the largest usable value on HD systems. A 2048 sector discard on a
- * busy HD system takes 31 seconds. We should use a value no higher than 1024, which takes
- * 15 to 16 seconds on a busy HD system.
- *
- * But using large values results in 120 second blocked task warnings in /var/log/kern.log.
- * In order to avoid these warnings, we choose to use the smallest reasonable value. See
- * VDO-3062 and VDO-3087.
+ * The value 1024 is the largest usable value on HD systems. A 2048 sector discard on a
+ * busy HD system takes 31 seconds. We should use a value no higher than 1024, which takes
+ * 15 to 16 seconds on a busy HD system. However, using large values results in 120 second
+ * blocked task warnings in kernel logs. In order to avoid these warnings, we choose to
+ * use the smallest reasonable value.
*
* The value is displayed in sysfs, and also used by dm-thin to determine whether to pass
* down discards. The block layer splits large discards on this boundary when this is set.
diff --git a/drivers/md/dm-vdo/packer.c b/drivers/md/dm-vdo/packer.c
index e391cac6c92d..b0ffb21ec436 100644
--- a/drivers/md/dm-vdo/packer.c
+++ b/drivers/md/dm-vdo/packer.c
@@ -595,15 +595,13 @@ void vdo_attempt_packing(struct data_vio *data_vio)
}
/*
- * The check of may_vio_block_in_packer() here will set the data_vio's compression state to
- * VIO_PACKING if the data_vio is allowed to be compressed (if it has already been
- * canceled, we'll fall out here). Once the data_vio is in the VIO_PACKING state, it must
- * be guaranteed to be put in a bin before any more requests can be processed by the packer
- * thread. Otherwise, a canceling data_vio could attempt to remove the canceled data_vio
- * from the packer and fail to rendezvous with it (VDO-2809). We must also make sure that
- * we will actually bin the data_vio and not give up on it as being larger than the space
- * used in the fullest bin. Hence we must call select_bin() before calling
- * may_vio_block_in_packer() (VDO-2826).
+ * The advance_data_vio_compression_stage() check here verifies that the data_vio is
+ * allowed to be compressed (if it has already been canceled, we'll fall out here). Once
+ * the data_vio is in the DATA_VIO_PACKING state, it must be guaranteed to be put in a bin
+ * before any more requests can be processed by the packer thread. Otherwise, a canceling
+ * data_vio could attempt to remove the canceled data_vio from the packer and fail to
+ * rendezvous with it. Thus, we must call select_bin() first to ensure that we will
+ * actually add the data_vio to a bin before advancing to the DATA_VIO_PACKING stage.
*/
bin = select_bin(packer, data_vio);
if ((bin == NULL) ||
diff --git a/drivers/md/dm-vdo/packer.h b/drivers/md/dm-vdo/packer.h
index 2dcc40bd4417..0f3be44710b5 100644
--- a/drivers/md/dm-vdo/packer.h
+++ b/drivers/md/dm-vdo/packer.h
@@ -58,7 +58,7 @@ struct compressed_block {
*
* There is one special bin which is used to hold data_vios which have been canceled and removed
* from their bin by the packer. These data_vios need to wait for the canceller to rendezvous with
- * them (VDO-2809) and so they sit in this special bin.
+ * them and so they sit in this special bin.
*/
struct packer_bin {
/* List links for packer.packer_bins */
diff --git a/drivers/md/dm-vdo/repair.c b/drivers/md/dm-vdo/repair.c
index a75278eb8aa4..847aca9fbe47 100644
--- a/drivers/md/dm-vdo/repair.c
+++ b/drivers/md/dm-vdo/repair.c
@@ -1504,8 +1504,8 @@ static int extract_new_mappings(struct repair_completion *repair)
static noinline int compute_usages(struct repair_completion *repair)
{
/*
- * VDO-5182: function is declared noinline to avoid what is likely a spurious valgrind
- * error about this structure being uninitialized.
+ * This function is declared noinline to avoid a spurious valgrind error regarding the
+ * following structure being uninitialized.
*/
struct recovery_point recovery_point = {
.sequence_number = repair->tail,
diff --git a/drivers/md/dm-vdo/slab-depot.c b/drivers/md/dm-vdo/slab-depot.c
index 42126bd60242..5fa7e0838b32 100644
--- a/drivers/md/dm-vdo/slab-depot.c
+++ b/drivers/md/dm-vdo/slab-depot.c
@@ -334,7 +334,11 @@ static void launch_write(struct slab_summary_block *block)
/*
* Flush before writing to ensure that the slab journal tail blocks and reference updates
- * covered by this summary update are stable (VDO-2332).
+ * covered by this summary update are stable. Otherwise, a subsequent recovery could
+ * encounter a slab summary update that refers to a slab journal tail block that has not
+ * actually been written. In such cases, the slab journal referenced will be treated as
+ * empty, causing any data within the slab which predates the existing recovery journal
+ * entries to be lost.
*/
pbn = (depot->summary_origin +
(VDO_SLAB_SUMMARY_BLOCKS_PER_ZONE * allocator->zone_number) +
@@ -499,7 +503,7 @@ static void reap_slab_journal(struct slab_journal *journal)
* journal block writes can be issued while previous slab summary updates have not yet been
* made. Even though those slab journal block writes will be ignored if the slab summary
* update is not persisted, they may still overwrite the to-be-reaped slab journal block
- * resulting in a loss of reference count updates (VDO-2912).
+ * resulting in a loss of reference count updates.
*/
journal->flush_waiter.callback = flush_for_reaping;
acquire_vio_from_pool(journal->slab->allocator->vio_pool,
@@ -770,7 +774,8 @@ static void write_slab_journal_block(struct vdo_waiter *waiter, void *context)
/*
* This block won't be read in recovery until the slab summary is updated to refer to it.
- * The slab summary update does a flush which is sufficient to protect us from VDO-2331.
+ * The slab summary update does a flush which is sufficient to protect us from corruption
+ * due to out of order slab journal, reference block, or block map writes.
*/
vdo_submit_metadata_vio(uds_forget(vio), block_number, write_slab_journal_endio,
complete_write, REQ_OP_WRITE);
@@ -1201,7 +1206,8 @@ static void write_reference_block(struct vdo_waiter *waiter, void *context)
/*
* Flush before writing to ensure that the recovery journal and slab journal entries which
- * cover this reference update are stable (VDO-2331).
+ * cover this reference update are stable. This prevents data corruption that can be caused
+ * by out of order writes.
*/
WRITE_ONCE(block->slab->allocator->ref_counts_statistics.blocks_written,
block->slab->allocator->ref_counts_statistics.blocks_written + 1);
@@ -1775,7 +1781,7 @@ static void add_entries(struct slab_journal *journal)
(journal->slab->status == VDO_SLAB_REBUILDING)) {
/*
* Don't add entries while rebuilding or while a partial write is
- * outstanding (VDO-2399).
+ * outstanding, as it could result in reference count corruption.
*/
break;
}
diff --git a/drivers/md/dm-vdo/vdo.c b/drivers/md/dm-vdo/vdo.c
index e0eddd4007b8..a40f059d39b3 100644
--- a/drivers/md/dm-vdo/vdo.c
+++ b/drivers/md/dm-vdo/vdo.c
@@ -544,7 +544,7 @@ int vdo_make(unsigned int instance, struct device_config *config, char **reason,
int result;
struct vdo *vdo;
- /* VDO-3769 - Set a generic reason so we don't ever return garbage. */
+ /* Initialize with a generic failure reason to prevent returning garbage. */
*reason = "Unspecified error";
result = uds_allocate(1, struct vdo, __func__, &vdo);
diff --git a/drivers/md/dm-vdo/vio.c b/drivers/md/dm-vdo/vio.c
index eb6838ddabbb..4832ea46551f 100644
--- a/drivers/md/dm-vdo/vio.c
+++ b/drivers/md/dm-vdo/vio.c
@@ -123,7 +123,6 @@ int create_multi_block_metadata_vio(struct vdo *vdo, enum vio_type vio_type,
struct vio *vio;
int result;
- /* If struct vio grows past 256 bytes, we'll lose benefits of VDOSTORY-176. */
BUILD_BUG_ON(sizeof(struct vio) > 256);
/*
--
2.42.0
