[dm-devel] [PATCH 0/4] dm era: Various minor fixes

[dm-devel] [PATCH 0/4] dm era: Various minor fixes

[Nikos Tsironis]

While working on fixing the bugs that cause lost writes, for which I
have sent separate emails, I bumped into several other minor issues that
I fix in this patch set.

In particular, this series of commits introduces the following fixes:

1. Add explicit check that the data block size hasn't changed
2. Fix bitset memory leaks. The in-core bitmaps were never freed.
3. Fix the writeset tree equality test function to use the right value
   size.
4. Remove unreachable resize operation in pre-resume function.

More information about the fixes can be found in their commit messages.

Nikos Tsironis (4):
  dm era: Verify the data block size hasn't changed
  dm era: Fix bitset memory leaks
  dm era: Use correct value size in equality function of writeset tree
  dm era: Remove unreachable resize operation in pre-resume function

 drivers/md/dm-era-target.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.11.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/4] dm era: Verify the data block size hasn't changed

[Nikos Tsironis]

dm-era doesn't support changing the data block size of existing devices,
so check explicitly that the requested block size for a new target
matches the one stored in the metadata.

Fixes: eec40579d84873 ("dm: add era target")
Cc: stable@vger.kernel.org # v3.15+
Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
---
 drivers/md/dm-era-target.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index b24e3839bb3a..52e3f63335d3 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -564,6 +564,15 @@ static int open_metadata(struct era_metadata *md)
 	}
 
 	disk = dm_block_data(sblock);
+
+	/* Verify the data block size hasn't changed */
+	if (le32_to_cpu(disk->data_block_size) != md->block_size) {
+		DMERR("changing the data block size (from %u to %llu) is not supported",
+		      le32_to_cpu(disk->data_block_size), md->block_size);
+		r = -EINVAL;
+		goto bad;
+	}
+
 	r = dm_tm_open_with_sm(md->bm, SUPERBLOCK_LOCATION,
 			       disk->metadata_space_map_root,
 			       sizeof(disk->metadata_space_map_root),
@@ -575,7 +584,6 @@ static int open_metadata(struct era_metadata *md)
 
 	setup_infos(md);
 
-	md->block_size = le32_to_cpu(disk->data_block_size);
 	md->nr_blocks = le32_to_cpu(disk->nr_blocks);
 	md->current_era = le32_to_cpu(disk->current_era);
 
-- 
2.11.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/4] dm era: Fix bitset memory leaks

[Nikos Tsironis]

Deallocate the memory allocated for the in-core bitsets when destroying
the target and in error paths.

Fixes: eec40579d84873 ("dm: add era target")
Cc: stable@vger.kernel.org # v3.15+
Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
---
 drivers/md/dm-era-target.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index 52e3f63335d3..ffbbd8740253 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -47,6 +47,7 @@ struct writeset {
 static void writeset_free(struct writeset *ws)
 {
 	vfree(ws->bits);
+	ws->bits = NULL;
 }
 
 static int setup_on_disk_bitset(struct dm_disk_bitset *info,
@@ -810,6 +811,8 @@ static struct era_metadata *metadata_open(struct block_device *bdev,
 
 static void metadata_close(struct era_metadata *md)
 {
+	writeset_free(&md->writesets[0]);
+	writeset_free(&md->writesets[1]);
 	destroy_persistent_data_objects(md);
 	kfree(md);
 }
@@ -847,6 +850,7 @@ static int metadata_resize(struct era_metadata *md, void *arg)
 	r = writeset_alloc(&md->writesets[1], *new_size);
 	if (r) {
 		DMERR("%s: writeset_alloc failed for writeset 1", __func__);
+		writeset_free(&md->writesets[0]);
 		return r;
 	}
 
@@ -857,6 +861,8 @@ static int metadata_resize(struct era_metadata *md, void *arg)
 			    &value, &md->era_array_root);
 	if (r) {
 		DMERR("%s: dm_array_resize failed", __func__);
+		writeset_free(&md->writesets[0]);
+		writeset_free(&md->writesets[1]);
 		return r;
 	}
 
-- 
2.11.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 3/4] dm era: Use correct value size in equality function of writeset tree

[Nikos Tsironis]

Fix the writeset tree equality test function to use the right value size
when comparing two btree values.

Fixes: eec40579d84873 ("dm: add era target")
Cc: stable@vger.kernel.org # v3.15+
Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
---
 drivers/md/dm-era-target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index ffbbd8740253..104fb110cd4e 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -389,7 +389,7 @@ static void ws_dec(void *context, const void *value)
 
 static int ws_eq(void *context, const void *value1, const void *value2)
 {
-	return !memcmp(value1, value2, sizeof(struct writeset_metadata));
+	return !memcmp(value1, value2, sizeof(struct writeset_disk));
 }
 
 /*----------------------------------------------------------------*/
-- 
2.11.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 4/4] dm era: Remove unreachable resize operation in pre-resume function

[Nikos Tsironis]

The device metadata are resized in era_ctr(), so the metadata resize
operation in era_preresume() never runs.

Also, note, that if the operation did ever run it would deadlock, since
the worker has not been started at this point.

Fixes: eec40579d84873 ("dm: add era target")
Cc: stable@vger.kernel.org # v3.15+
Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
---
 drivers/md/dm-era-target.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index 104fb110cd4e..c40e132e50cd 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -1567,15 +1567,6 @@ static int era_preresume(struct dm_target *ti)
 {
 	int r;
 	struct era *era = ti->private;
-	dm_block_t new_size = calc_nr_blocks(era);
-
-	if (era->nr_blocks != new_size) {
-		r = in_worker1(era, metadata_resize, &new_size);
-		if (r)
-			return r;
-
-		era->nr_blocks = new_size;
-	}
 
 	start_worker(era);
 
-- 
2.11.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/4] dm era: Various minor fixes

[Ming Hung Tsai]

On Fri, Jan 22, 2021 at 11:30 PM Nikos Tsironis <ntsironis@arrikto.com> wrote:
>
> While working on fixing the bugs that cause lost writes, for which I
> have sent separate emails, I bumped into several other minor issues that
> I fix in this patch set.
>
> In particular, this series of commits introduces the following fixes:
>
> 1. Add explicit check that the data block size hasn't changed
> 2. Fix bitset memory leaks. The in-core bitmaps were never freed.
> 3. Fix the writeset tree equality test function to use the right value
>    size.
> 4. Remove unreachable resize operation in pre-resume function.
>
> More information about the fixes can be found in their commit messages.
>
> Nikos Tsironis (4):
>   dm era: Verify the data block size hasn't changed
>   dm era: Fix bitset memory leaks
>   dm era: Use correct value size in equality function of writeset tree
>   dm era: Remove unreachable resize operation in pre-resume function
>
>  drivers/md/dm-era-target.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)

For the series, except 4/4 where I haven't tried other solutions.

Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 4/4] dm era: Remove unreachable resize operation in pre-resume function

[Mike Snitzer]

On Fri, Jan 22 2021 at 10:25am -0500,
Nikos Tsironis <ntsironis@arrikto.com> wrote:

> The device metadata are resized in era_ctr(), so the metadata resize
> operation in era_preresume() never runs.
> 
> Also, note, that if the operation did ever run it would deadlock, since
> the worker has not been started at this point.
> 
> Fixes: eec40579d84873 ("dm: add era target")
> Cc: stable@vger.kernel.org # v3.15+
> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
> ---
>  drivers/md/dm-era-target.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
> index 104fb110cd4e..c40e132e50cd 100644
> --- a/drivers/md/dm-era-target.c
> +++ b/drivers/md/dm-era-target.c
> @@ -1567,15 +1567,6 @@ static int era_preresume(struct dm_target *ti)
>  {
>  	int r;
>  	struct era *era = ti->private;
> -	dm_block_t new_size = calc_nr_blocks(era);
> -
> -	if (era->nr_blocks != new_size) {
> -		r = in_worker1(era, metadata_resize, &new_size);
> -		if (r)
> -			return r;
> -
> -		era->nr_blocks = new_size;
> -	}
>  
>  	start_worker(era);
>  
> -- 
> 2.11.0
> 

Resize shouldn't actually happen in the ctr.  The ctr loads a temporary
(inactive) table that will only become active upon resume.  That is why
resize should always be done in terms of resume.

I'll look closer but ctr shouldn't do the actual resize, and the
start_worker() should be moved above the resize code you've removed
above.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/4] dm era: Various minor fixes

[Mike Snitzer]

On Wed, Feb 10 2021 at 12:56pm -0500,
Ming Hung Tsai <mtsai@redhat.com> wrote:

> On Fri, Jan 22, 2021 at 11:30 PM Nikos Tsironis <ntsironis@arrikto.com> wrote:
> >
> > While working on fixing the bugs that cause lost writes, for which I
> > have sent separate emails, I bumped into several other minor issues that
> > I fix in this patch set.
> >
> > In particular, this series of commits introduces the following fixes:
> >
> > 1. Add explicit check that the data block size hasn't changed
> > 2. Fix bitset memory leaks. The in-core bitmaps were never freed.
> > 3. Fix the writeset tree equality test function to use the right value
> >    size.
> > 4. Remove unreachable resize operation in pre-resume function.
> >
> > More information about the fixes can be found in their commit messages.
> >
> > Nikos Tsironis (4):
> >   dm era: Verify the data block size hasn't changed
> >   dm era: Fix bitset memory leaks
> >   dm era: Use correct value size in equality function of writeset tree
> >   dm era: Remove unreachable resize operation in pre-resume function
> >
> >  drivers/md/dm-era-target.c | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> For the series, except 4/4 where I haven't tried other solutions.
> 
> Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>

patchwork doesn't parse this, so it falls on me to backfill tags like
this.  In the future, please reply to each patch with your desired tag.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 4/4] dm era: Remove unreachable resize operation in pre-resume function

[Mike Snitzer]

On Wed, Feb 10 2021 at  1:12P -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Jan 22 2021 at 10:25am -0500,
> Nikos Tsironis <ntsironis@arrikto.com> wrote:
> 
> > The device metadata are resized in era_ctr(), so the metadata resize
> > operation in era_preresume() never runs.
> > 
> > Also, note, that if the operation did ever run it would deadlock, since
> > the worker has not been started at this point.

It wouldn't have deadlocked, it'd have queued the work (see wake_worker)

> > 
> > Fixes: eec40579d84873 ("dm: add era target")
> > Cc: stable@vger.kernel.org # v3.15+
> > Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
> > ---
> >  drivers/md/dm-era-target.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> > 
> > diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
> > index 104fb110cd4e..c40e132e50cd 100644
> > --- a/drivers/md/dm-era-target.c
> > +++ b/drivers/md/dm-era-target.c
> > @@ -1567,15 +1567,6 @@ static int era_preresume(struct dm_target *ti)
> >  {
> >  	int r;
> >  	struct era *era = ti->private;
> > -	dm_block_t new_size = calc_nr_blocks(era);
> > -
> > -	if (era->nr_blocks != new_size) {
> > -		r = in_worker1(era, metadata_resize, &new_size);
> > -		if (r)
> > -			return r;
> > -
> > -		era->nr_blocks = new_size;
> > -	}
> >  
> >  	start_worker(era);
> >  
> > -- 
> > 2.11.0
> > 
> 
> Resize shouldn't actually happen in the ctr.  The ctr loads a temporary
> (inactive) table that will only become active upon resume.  That is why
> resize should always be done in terms of resume.
> 
> I'll look closer but ctr shouldn't do the actual resize, and the
> start_worker() should be moved above the resize code you've removed
> above.

Does this work for you?  If so I'll get it staged (like I've just
staged all your other dm-era fixes for 5.12).

 drivers/md/dm-era-target.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index d0e75fd31c1e..ec198e9cdafb 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -1501,15 +1501,6 @@ static int era_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	}
 	era->md = md;
 
-	era->nr_blocks = calc_nr_blocks(era);
-
-	r = metadata_resize(era->md, &era->nr_blocks);
-	if (r) {
-		ti->error = "couldn't resize metadata";
-		era_destroy(era);
-		return -ENOMEM;
-	}
-
 	era->wq = alloc_ordered_workqueue("dm-" DM_MSG_PREFIX, WQ_MEM_RECLAIM);
 	if (!era->wq) {
 		ti->error = "could not create workqueue for metadata object";
@@ -1583,6 +1574,8 @@ static int era_preresume(struct dm_target *ti)
 	struct era *era = ti->private;
 	dm_block_t new_size = calc_nr_blocks(era);
 
+	start_worker(era);
+
 	if (era->nr_blocks != new_size) {
 		r = in_worker1(era, metadata_resize, &new_size);
 		if (r)
@@ -1591,8 +1584,6 @@ static int era_preresume(struct dm_target *ti)
 		era->nr_blocks = new_size;
 	}
 
-	start_worker(era);
-
 	r = in_worker0(era, metadata_era_rollover);
 	if (r) {
 		DMERR("%s: metadata_era_rollover failed", __func__);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 4/4] dm era: Remove unreachable resize operation in pre-resume function

[Nikos Tsironis]

On 2/10/21 8:48 PM, Mike Snitzer wrote:
> On Wed, Feb 10 2021 at  1:12P -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> On Fri, Jan 22 2021 at 10:25am -0500,
>> Nikos Tsironis <ntsironis@arrikto.com> wrote:
>>
>>> The device metadata are resized in era_ctr(), so the metadata resize
>>> operation in era_preresume() never runs.
>>>
>>> Also, note, that if the operation did ever run it would deadlock, since
>>> the worker has not been started at this point.
> 
> It wouldn't have deadlocked, it'd have queued the work (see wake_worker)
> 

Hi Mike,

The resize is performed as an RPC and in_worker1() ends up calling
perform_rpc(). perform_rpc() calls wake_worker() and then waits for the
RPC to complete: wait_for_completion(&rpc->complete). So, start_worker()
is not called until after the RPC has been completed.

But, you are right, it won't deadlock. I was confused by wake_worker:

   static void wake_worker(struct era *era)
   {
           if (!atomic_read(&era->suspended))
                   queue_work(era->wq, &era->worker);
   }

When we suspend the device we set era->suspended to 1, so I mistakenly
thought that the resize operation during preresume would deadlock,
because wake_worker wouldn't queue the work.

But, the resize is only triggered when loading a new table, which
creates a new target by calling era_ctr. There era->suspended is
indirectly initialized to 0, because of kzalloc.

So, wake_worker will indeed queue the work.

>>>
>>> Fixes: eec40579d84873 ("dm: add era target")
>>> Cc: stable@vger.kernel.org # v3.15+
>>> Signed-off-by: Nikos Tsironis <ntsironis@arrikto.com>
>>> ---
>>>   drivers/md/dm-era-target.c | 9 ---------
>>>   1 file changed, 9 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
>>> index 104fb110cd4e..c40e132e50cd 100644
>>> --- a/drivers/md/dm-era-target.c
>>> +++ b/drivers/md/dm-era-target.c
>>> @@ -1567,15 +1567,6 @@ static int era_preresume(struct dm_target *ti)
>>>   {
>>>   	int r;
>>>   	struct era *era = ti->private;
>>> -	dm_block_t new_size = calc_nr_blocks(era);
>>> -
>>> -	if (era->nr_blocks != new_size) {
>>> -		r = in_worker1(era, metadata_resize, &new_size);
>>> -		if (r)
>>> -			return r;
>>> -
>>> -		era->nr_blocks = new_size;
>>> -	}
>>>   
>>>   	start_worker(era);
>>>   
>>> -- 
>>> 2.11.0
>>>
>>
>> Resize shouldn't actually happen in the ctr.  The ctr loads a temporary
>> (inactive) table that will only become active upon resume.  That is why
>> resize should always be done in terms of resume.
>>

I kept the resize in the ctr to maintain the original behavior of
dm-era.

But, I had missed what you are describing here, which indeed makes sense
and it's the correct thing to do.

Thanks a lot for explaining it.

>> I'll look closer but ctr shouldn't do the actual resize, and the
>> start_worker() should be moved above the resize code you've removed
>> above.
> 
> Does this work for you?  If so I'll get it staged (like I've just
> staged all your other dm-era fixes for 5.12).
> 

The patch you attach won't work as is. We can't perform the resize in
the worker, because the worker might run other metadata operations,
e.g., it could start digestion, before resizing the metadata. These
operations will end up using the old size.

This can lead to errors:

1. Create a 1GiB dm-era device

    # dmsetup create eradev --table "0 2097152 era /dev/datavg/erameta /dev/datavg/eradata 8192"

2. Write to a block

    # dd if=/dev/zero of=/dev/mapper/eradev oflag=direct bs=4M count=1 seek=200

3. Suspend the device

    # dmsetup suspend eradev

4. Load a new table reducing the size of the device, so it doesn't
    include the block written at step (2)

    # dmsetup load eradev --table "0 1048576 era /dev/datavg/erameta /dev/datavg/eradata 8192"

5. Resume the device

    # dmsetup resume eradev

In dmesg we see the following:

    device-mapper: era: metadata_digest_transcribe_writeset: dm_array_set_value failed
    device-mapper: era: process_old_eras: digest step failed, stopping digestion

The reason is that the worker started the digestion of the archived
writeset using the old, larger size.

As a result, metadata_digest_transcribe_writeset tried to write beyond
the end of the era array.

Instead, we have to resize the metadata directly in era_preresume, and
not use the worker to do it.

I prepared a new patch doing that, which I will send with a new mail.

Nikos.

>   drivers/md/dm-era-target.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
> index d0e75fd31c1e..ec198e9cdafb 100644
> --- a/drivers/md/dm-era-target.c
> +++ b/drivers/md/dm-era-target.c
> @@ -1501,15 +1501,6 @@ static int era_ctr(struct dm_target *ti, unsigned argc, char **argv)
>   	}
>   	era->md = md;
>   
> -	era->nr_blocks = calc_nr_blocks(era);
> -
> -	r = metadata_resize(era->md, &era->nr_blocks);
> -	if (r) {
> -		ti->error = "couldn't resize metadata";
> -		era_destroy(era);
> -		return -ENOMEM;
> -	}
> -
>   	era->wq = alloc_ordered_workqueue("dm-" DM_MSG_PREFIX, WQ_MEM_RECLAIM);
>   	if (!era->wq) {
>   		ti->error = "could not create workqueue for metadata object";
> @@ -1583,6 +1574,8 @@ static int era_preresume(struct dm_target *ti)
>   	struct era *era = ti->private;
>   	dm_block_t new_size = calc_nr_blocks(era);
>   
> +	start_worker(era);
> +
>   	if (era->nr_blocks != new_size) {
>   		r = in_worker1(era, metadata_resize, &new_size);
>   		if (r)
> @@ -1591,8 +1584,6 @@ static int era_preresume(struct dm_target *ti)
>   		era->nr_blocks = new_size;
>   	}
>   
> -	start_worker(era);
> -
>   	r = in_worker0(era, metadata_era_rollover);
>   	if (r) {
>   		DMERR("%s: metadata_era_rollover failed", __func__);
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel