* [PATCH] dm-persistent-data: free sm_metadata on failed create
@ 2016-11-30 23:56 Benjamin Marzinski
2016-12-01 10:58 ` Edward Thornber
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2016-11-30 23:56 UTC (permalink / raw)
To: dm-devel; +Cc: Edward Thornber
In dm_sm_metadata_create we temporarily change the dm_space_map
operations from ops, whose destroy function deallocates the
sm_metadata, to bootstrap_ops, whose destroy function doesn't. If we
fail in dm_ll_new_metadata or sm_ll_extend, we exit back to
dm_tm_create_internal, which calls dm_sm_destroy with the intention
of freeing the sm_metadata, but it doesn't.
This patch sets the dm_space_map operations back to ops if
dm_sm_metadata_create fails when it is set to bootstrap_ops.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
drivers/md/persistent-data/dm-space-map-metadata.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/md/persistent-data/dm-space-map-metadata.c b/drivers/md/persistent-data/dm-space-map-metadata.c
index 7e44005..20557e2 100644
--- a/drivers/md/persistent-data/dm-space-map-metadata.c
+++ b/drivers/md/persistent-data/dm-space-map-metadata.c
@@ -775,17 +775,15 @@ int dm_sm_metadata_create(struct dm_space_map *sm,
memcpy(&smm->sm, &bootstrap_ops, sizeof(smm->sm));
r = sm_ll_new_metadata(&smm->ll, tm);
+ if (!r) {
+ if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
+ nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
+ r = sm_ll_extend(&smm->ll, nr_blocks);
+ }
+ memcpy(&smm->sm, &ops, sizeof(smm->sm));
if (r)
return r;
- if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
- nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
- r = sm_ll_extend(&smm->ll, nr_blocks);
- if (r)
- return r;
-
- memcpy(&smm->sm, &ops, sizeof(smm->sm));
-
/*
* Now we need to update the newly created data structures with the
* allocated blocks that they were built from.
--
2.1.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] dm-persistent-data: free sm_metadata on failed create
2016-11-30 23:56 [PATCH] dm-persistent-data: free sm_metadata on failed create Benjamin Marzinski
@ 2016-12-01 10:58 ` Edward Thornber
2016-12-01 16:41 ` Mike Snitzer
0 siblings, 1 reply; 4+ messages in thread
From: Edward Thornber @ 2016-12-01 10:58 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: dm-devel
ack
On Wed, Nov 30, 2016 at 05:56:14PM -0600, Benjamin Marzinski wrote:
> In dm_sm_metadata_create we temporarily change the dm_space_map
> operations from ops, whose destroy function deallocates the
> sm_metadata, to bootstrap_ops, whose destroy function doesn't. If we
> fail in dm_ll_new_metadata or sm_ll_extend, we exit back to
> dm_tm_create_internal, which calls dm_sm_destroy with the intention
> of freeing the sm_metadata, but it doesn't.
>
> This patch sets the dm_space_map operations back to ops if
> dm_sm_metadata_create fails when it is set to bootstrap_ops.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> drivers/md/persistent-data/dm-space-map-metadata.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/persistent-data/dm-space-map-metadata.c b/drivers/md/persistent-data/dm-space-map-metadata.c
> index 7e44005..20557e2 100644
> --- a/drivers/md/persistent-data/dm-space-map-metadata.c
> +++ b/drivers/md/persistent-data/dm-space-map-metadata.c
> @@ -775,17 +775,15 @@ int dm_sm_metadata_create(struct dm_space_map *sm,
> memcpy(&smm->sm, &bootstrap_ops, sizeof(smm->sm));
>
> r = sm_ll_new_metadata(&smm->ll, tm);
> + if (!r) {
> + if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
> + nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
> + r = sm_ll_extend(&smm->ll, nr_blocks);
> + }
> + memcpy(&smm->sm, &ops, sizeof(smm->sm));
> if (r)
> return r;
>
> - if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
> - nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
> - r = sm_ll_extend(&smm->ll, nr_blocks);
> - if (r)
> - return r;
> -
> - memcpy(&smm->sm, &ops, sizeof(smm->sm));
> -
> /*
> * Now we need to update the newly created data structures with the
> * allocated blocks that they were built from.
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: dm-persistent-data: free sm_metadata on failed create
2016-12-01 10:58 ` Edward Thornber
@ 2016-12-01 16:41 ` Mike Snitzer
2016-12-02 11:45 ` Edward Thornber
0 siblings, 1 reply; 4+ messages in thread
From: Mike Snitzer @ 2016-12-01 16:41 UTC (permalink / raw)
To: Edward Thornber; +Cc: dm-devel
Staged for 4.10 inclusion with a revised header, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.10&id=b34af3050734ba53a7afa4f3ab9fe9d6bbfbf802
On Thu, Dec 01 2016 at 5:58am -0500,
Edward Thornber <thornber@redhat.com> wrote:
> ack
>
> On Wed, Nov 30, 2016 at 05:56:14PM -0600, Benjamin Marzinski wrote:
> > In dm_sm_metadata_create we temporarily change the dm_space_map
> > operations from ops, whose destroy function deallocates the
> > sm_metadata, to bootstrap_ops, whose destroy function doesn't. If we
> > fail in dm_ll_new_metadata or sm_ll_extend, we exit back to
> > dm_tm_create_internal, which calls dm_sm_destroy with the intention
> > of freeing the sm_metadata, but it doesn't.
> >
> > This patch sets the dm_space_map operations back to ops if
> > dm_sm_metadata_create fails when it is set to bootstrap_ops.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > drivers/md/persistent-data/dm-space-map-metadata.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/md/persistent-data/dm-space-map-metadata.c b/drivers/md/persistent-data/dm-space-map-metadata.c
> > index 7e44005..20557e2 100644
> > --- a/drivers/md/persistent-data/dm-space-map-metadata.c
> > +++ b/drivers/md/persistent-data/dm-space-map-metadata.c
> > @@ -775,17 +775,15 @@ int dm_sm_metadata_create(struct dm_space_map *sm,
> > memcpy(&smm->sm, &bootstrap_ops, sizeof(smm->sm));
> >
> > r = sm_ll_new_metadata(&smm->ll, tm);
> > + if (!r) {
> > + if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
> > + nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
> > + r = sm_ll_extend(&smm->ll, nr_blocks);
> > + }
> > + memcpy(&smm->sm, &ops, sizeof(smm->sm));
> > if (r)
> > return r;
> >
> > - if (nr_blocks > DM_SM_METADATA_MAX_BLOCKS)
> > - nr_blocks = DM_SM_METADATA_MAX_BLOCKS;
> > - r = sm_ll_extend(&smm->ll, nr_blocks);
> > - if (r)
> > - return r;
> > -
> > - memcpy(&smm->sm, &ops, sizeof(smm->sm));
> > -
> > /*
> > * Now we need to update the newly created data structures with the
> > * allocated blocks that they were built from.
> > --
> > 2.1.0
> >
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: dm-persistent-data: free sm_metadata on failed create
2016-12-01 16:41 ` Mike Snitzer
@ 2016-12-02 11:45 ` Edward Thornber
0 siblings, 0 replies; 4+ messages in thread
From: Edward Thornber @ 2016-12-02 11:45 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel
On Thu, Dec 01, 2016 at 11:41:05AM -0500, Mike Snitzer wrote:
> Staged for 4.10 inclusion with a revised header, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.10&id=b34af3050734ba53a7afa4f3ab9fe9d6bbfbf802
It's not urgent. In general if something goes wrong with a persistent
data transaction we just abort and roll back. So there are a lot of
error paths where we don't bother to repair metadata that's in the
doomed transaction.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-02 11:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-30 23:56 [PATCH] dm-persistent-data: free sm_metadata on failed create Benjamin Marzinski
2016-12-01 10:58 ` Edward Thornber
2016-12-01 16:41 ` Mike Snitzer
2016-12-02 11:45 ` Edward Thornber
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.