[dm-devel] dm thin: superblock may write succeed before other metadata blocks because of wirting metadata in async mode.
Mike Snitzer
snitzer at redhat.com
Tue Jun 19 15:00:32 UTC 2018
On Tue, Jun 19 2018 at 10:43am -0400,
Joe Thornber <thornber at redhat.com> wrote:
> On Tue, Jun 19, 2018 at 09:11:06AM -0400, Mike Snitzer wrote:
> > On Mon, May 21 2018 at 8:53pm -0400,
> > Monty Pavel <monty_pavel at sina.com> wrote:
> >
> > >
> > > If dm_bufio_write_dirty_buffers func is called by __commit_transaction
> > > func and power loss happens during executing it, coincidencely
> > > superblock wrote correctly but some metadata blocks didn't. The reason
> > > is we write all metadata in async mode. We can guarantee that we send
> > > superblock after other blocks but we cannot guarantee that superblock
> > > write completely early than other blocks.
> > > So, We need to commit other metadata blocks before change superblock.
> > >
> > > Signed-off-by: Monty Pavel <monty_pavel at sina.com>
> > > ---
> > > drivers/md/dm-thin-metadata.c | 8 ++++++++
> > > 1 files changed, 8 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
> > > index 36ef284..897d7d6 100644
> > > --- a/drivers/md/dm-thin-metadata.c
> > > +++ b/drivers/md/dm-thin-metadata.c
> > > @@ -813,6 +813,14 @@ static int __commit_transaction(struct dm_pool_metadata *pmd)
> > > if (r)
> > > return r;
> > >
> > > + r = dm_tm_commit(pmd->tm, sblock);
> > > + if (r)
> > > + return r;
> > > +
> > > + r = superblock_lock(pmd, &sblock);
> > > + if (r)
> > > + return r;
> > > +
> > > disk_super = dm_block_data(sblock);
> > > disk_super->time = cpu_to_le32(pmd->time);
> > > disk_super->data_mapping_root = cpu_to_le64(pmd->root);
>
> I don't believe you've tested this; sblock is passed to dm_tm_commit()
> uninitialised, and you didn't even bother to remove the later (and correct)
> call to dm_tm_commit().
I pointed out to Joe that the patch, in isolation, is decieving. It
_looks_ like sblock may be uninitialized, etc. But once the patch is
applied and you look at the entirety of __commit_transaction() it is
clear that you're reusing the existing superblock_lock() to safely
accomplish your additional call to dm_tm_commit().
> What is the issue that started you looking in this area?
Right, as my previous reply asked: please clarify if you _know_ your
patch fixes an actual problem you've experienced. The more details the
better.
Thanks,
Mike
More information about the dm-devel
mailing list