lkml.org 
[lkml]   [2021]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] dm-snapshot: Flush merged data before committing metadata
From
On Sun, 27 Dec 2020 23:14:07 PST (-0800), kernel-team@android.com wrote:
> If the origin device has a volatile write-back
> cache and the following events occur:
>
> 1: After finishing merge operation of one set of exceptions,
> merge_callback() is invoked.
> 2: Update the metadata in COW device tracking the merge completion.
> This update to COW device is flushed cleanly.
> 3: System crashes and the origin device's cache where the recent
> merge was completed has not been flushed.
>
> During the next cycle when we read the metadata from the COW device,
> we will skip reading those metadata whose merge was completed in
> step (1). This will lead to data loss/corruption.
>
> To address this, flush the origin device post merge IO before
> updating the metadata.

This is essentially the same as the flushes added in 8b3fd1f53af3 ("dm clone:
Flush destination device before committing metadata") and 694cfe7f31db ("dm
thin: Flush data device before committing metadata"), but for dm-snap. Looks
like this bug has been around since before the git tree, so I'm not sure what
the right thing to do with a Fixes tag is.

> Signed-off-by: Akilesh Kailash <akailash@google.com>
> ---
> drivers/md/dm-snap.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
> index 4668b2cd98f4..11890db71f3f 100644
> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -141,6 +141,11 @@ struct dm_snapshot {
> * for them to be committed.
> */
> struct bio_list bios_queued_during_merge;
> +
> + /*
> + * Flush data after merge.
> + */
> + struct bio flush_bio;
> };
>
> /*
> @@ -1121,6 +1126,17 @@ static void snapshot_merge_next_chunks(struct dm_snapshot *s)
>
> static void error_bios(struct bio *bio);
>
> +static int flush_data(struct dm_snapshot *s)
> +{
> + struct bio *flush_bio = &s->flush_bio;
> +
> + bio_reset(flush_bio);
> + bio_set_dev(flush_bio, s->origin->bdev);
> + flush_bio->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> +
> + return submit_bio_wait(flush_bio);
> +}
> +
> static void merge_callback(int read_err, unsigned long write_err, void *context)
> {
> struct dm_snapshot *s = context;
> @@ -1134,6 +1150,11 @@ static void merge_callback(int read_err, unsigned long write_err, void *context)
> goto shut;
> }
>
> + if (flush_data(s) < 0) {
> + DMERR("Flush after merge failed: shutting down merge");
> + goto shut;
> + }
> +
> if (s->store->type->commit_merge(s->store,
> s->num_merging_chunks) < 0) {
> DMERR("Write error in exception store: shutting down merge");
> @@ -1318,6 +1339,7 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> s->first_merging_chunk = 0;
> s->num_merging_chunks = 0;
> bio_list_init(&s->bios_queued_during_merge);
> + bio_init(&s->flush_bio, NULL, 0);

There's no bio_uninit() in the error handling code here, but there aren't any
in any of the other cleanup blocks either. The bio_uninit() call won't do
anything here, as it does nothing for empty BIOs, so I guess it just doesn't
matter.

> /* Allocate hash table for COW data */
> if (init_hash_tables(s)) {
> @@ -1504,6 +1526,8 @@ static void snapshot_dtr(struct dm_target *ti)
>
> dm_exception_store_destroy(s->store);
>
> + bio_uninit(&s->flush_bio);
> +
> dm_put_device(ti, s->cow);
>
> dm_put_device(ti, s->origin);

Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

\
 
 \ /
  Last update: 2021-01-13 20:16    [W:0.369 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site