Messages in this thread | | | Date | Wed, 13 Jan 2021 11:14:58 -0800 (PST) | Subject | Re: [PATCH] dm-snapshot: Flush merged data before committing metadata | From | Palmer Dabbelt <> |
| |
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>
| |