Re: [PATCH] dm cache: fix race affecting dirty block count

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

Hello Anssi, Joe, Mike,

I just found your patch in the latest rc and wanted to ask a few
questions. I am not sure how this patch helps at all other than luck in
that dm_cblock_t type is of type int32_t, which should guarantee that it
is atomic on most platforms. Which begs the question, what platform did
you encounter this problem?

The patch purports to solve a race condition by making use of atomic_t.
I am not sure that is enough. If indeed there is a race you need to use
smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
atomic_set(). 

Also I have a concern about why this mail was not CC'ed on LKML. I had
to go to some difficulty in finding this patch. So please CC LKML for
such patches.

Thanks,
-- 
Pranith

-- Begin forwarded Message --


nr_dirty is updated without locking, causing it to drift so that it is
non-zero (either a small positive integer, or a very large one when an
underflow occurs) even when there are no actual dirty blocks.

Fix that by using an atomic type for nr_dirty.

Signed-off-by: Anssi Hannula <anssi hannula iki fi>
Cc: Joe Thornber <ejt redhat com>
Cc: stable vger kernel org

---

So here's a patch for the issue I reported a few days ago.

I'm not sure what the actual effects of having wrong nr_dirty are
beyond the wrong value reported to userspace. Having wrong nr_dirty
causes dm_table_event(cache->ti->table) to be called at incorrect times,
but I'm not sure what the implications of that are.

So someone more knowledged should really amend my commit message
accordingly so that it explains the user-visible issues :)

I did notice that after a reboot (after having incorrect nr_dirty) the
entire cache was considered to be dirty and a full writeback occurred,
but I don't know if that was related at all.


 drivers/md/dm-cache-target.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 0df3ec085ebb..83a7eb5ef113 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -154,7 +154,7 @@ struct cache {
 	/*
 	 * cache_size entries, dirty if set
 	 */
-	dm_cblock_t nr_dirty;
+	atomic_t nr_dirty;
 	unsigned long *dirty_bitset;
 
 	/*
@@ -403,7 +403,7 @@ static bool is_dirty(struct cache *cache, dm_cblock_t b)
 static void set_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cblock)
 {
 	if (!test_and_set_bit(from_cblock(cblock), cache->dirty_bitset)) {
-		cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) + 1);
+		atomic_inc(&cache->nr_dirty);
 		policy_set_dirty(cache->policy, oblock);
 	}
 }
@@ -412,8 +412,7 @@ static void clear_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cbl
 {
 	if (test_and_clear_bit(from_cblock(cblock), cache->dirty_bitset)) {
 		policy_clear_dirty(cache->policy, oblock);
-		cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) - 1);
-		if (!from_cblock(cache->nr_dirty))
+		if (atomic_dec_return(&cache->nr_dirty) == 0)
 			dm_table_event(cache->ti->table);
 	}
 }
@@ -2003,7 +2002,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 	init_waitqueue_head(&cache->migration_wait);
 
 	r = -ENOMEM;
-	cache->nr_dirty = 0;
+	atomic_set(&cache->nr_dirty, 0);
 	cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
 	if (!cache->dirty_bitset) {
 		*error = "could not allocate dirty bitset";
@@ -2513,7 +2512,7 @@ static void cache_status(struct dm_target *ti, status_type_t type,
 		       (unsigned) atomic_read(&cache->stats.demotion),
 		       (unsigned) atomic_read(&cache->stats.promotion),
 		       (unsigned long long) from_cblock(residency),
-		       cache->nr_dirty);
+		       (unsigned) atomic_read(&cache->nr_dirty));
 
 		if (cache->features.write_through)
 			DMEMIT("1 writethrough ");
-- 
1.8.4.5

-- End forwarded Message --

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

Correctly adding Anssi this time.

On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
> Hello Anssi, Joe, Mike,
>
> I just found your patch in the latest rc and wanted to ask a few
> questions. I am not sure how this patch helps at all other than luck in
> that dm_cblock_t type is of type int32_t, which should guarantee that it
> is atomic on most platforms. Which begs the question, what platform did
> you encounter this problem?
>
> The patch purports to solve a race condition by making use of atomic_t.
> I am not sure that is enough. If indeed there is a race you need to use
> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
> atomic_set().
>
> Also I have a concern about why this mail was not CC'ed on LKML. I had
> to go to some difficulty in finding this patch. So please CC LKML for
> such patches.
>
> Thanks,
> --
> Pranith
>
> -- Begin forwarded Message --
>
>
> nr_dirty is updated without locking, causing it to drift so that it is
> non-zero (either a small positive integer, or a very large one when an
> underflow occurs) even when there are no actual dirty blocks.
>
> Fix that by using an atomic type for nr_dirty.
>
> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
> Cc: Joe Thornber <ejt redhat com>
> Cc: stable vger kernel org
>
> ---
>
> So here's a patch for the issue I reported a few days ago.
>
> I'm not sure what the actual effects of having wrong nr_dirty are
> beyond the wrong value reported to userspace. Having wrong nr_dirty
> causes dm_table_event(cache->ti->table) to be called at incorrect times,
> but I'm not sure what the implications of that are.
>
> So someone more knowledged should really amend my commit message
> accordingly so that it explains the user-visible issues :)
>
> I did notice that after a reboot (after having incorrect nr_dirty) the
> entire cache was considered to be dirty and a full writeback occurred,
> but I don't know if that was related at all.
>
>
>  drivers/md/dm-cache-target.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 0df3ec085ebb..83a7eb5ef113 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -154,7 +154,7 @@ struct cache {
>         /*
>          * cache_size entries, dirty if set
>          */
> -       dm_cblock_t nr_dirty;
> +       atomic_t nr_dirty;
>         unsigned long *dirty_bitset;
>
>         /*
> @@ -403,7 +403,7 @@ static bool is_dirty(struct cache *cache, dm_cblock_t b)
>  static void set_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cblock)
>  {
>         if (!test_and_set_bit(from_cblock(cblock), cache->dirty_bitset)) {
> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) + 1);
> +               atomic_inc(&cache->nr_dirty);
>                 policy_set_dirty(cache->policy, oblock);
>         }
>  }
> @@ -412,8 +412,7 @@ static void clear_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cbl
>  {
>         if (test_and_clear_bit(from_cblock(cblock), cache->dirty_bitset)) {
>                 policy_clear_dirty(cache->policy, oblock);
> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) - 1);
> -               if (!from_cblock(cache->nr_dirty))
> +               if (atomic_dec_return(&cache->nr_dirty) == 0)
>                         dm_table_event(cache->ti->table);
>         }
>  }
> @@ -2003,7 +2002,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>         init_waitqueue_head(&cache->migration_wait);
>
>         r = -ENOMEM;
> -       cache->nr_dirty = 0;
> +       atomic_set(&cache->nr_dirty, 0);
>         cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
>         if (!cache->dirty_bitset) {
>                 *error = "could not allocate dirty bitset";
> @@ -2513,7 +2512,7 @@ static void cache_status(struct dm_target *ti, status_type_t type,
>                        (unsigned) atomic_read(&cache->stats.demotion),
>                        (unsigned) atomic_read(&cache->stats.promotion),
>                        (unsigned long long) from_cblock(residency),
> -                      cache->nr_dirty);
> +                      (unsigned) atomic_read(&cache->nr_dirty));
>
>                 if (cache->features.write_through)
>                         DMEMIT("1 writethrough ");
> --
> 1.8.4.5
>
> -- End forwarded Message --
>



-- 
Pranith

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

Corrently adding Anssi this time.

On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
> Hello Anssi, Joe, Mike,
>
> I just found your patch in the latest rc and wanted to ask a few
> questions. I am not sure how this patch helps at all other than luck in
> that dm_cblock_t type is of type int32_t, which should guarantee that it
> is atomic on most platforms. Which begs the question, what platform did
> you encounter this problem?
>
> The patch purports to solve a race condition by making use of atomic_t.
> I am not sure that is enough. If indeed there is a race you need to use
> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
> atomic_set().
>
> Also I have a concern about why this mail was not CC'ed on LKML. I had
> to go to some difficulty in finding this patch. So please CC LKML for
> such patches.
>
> Thanks,
> --
> Pranith
>
> -- Begin forwarded Message --
>
>
> nr_dirty is updated without locking, causing it to drift so that it is
> non-zero (either a small positive integer, or a very large one when an
> underflow occurs) even when there are no actual dirty blocks.
>
> Fix that by using an atomic type for nr_dirty.
>
> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
> Cc: Joe Thornber <ejt redhat com>
> Cc: stable vger kernel org
>
> ---
>
> So here's a patch for the issue I reported a few days ago.
>
> I'm not sure what the actual effects of having wrong nr_dirty are
> beyond the wrong value reported to userspace. Having wrong nr_dirty
> causes dm_table_event(cache->ti->table) to be called at incorrect times,
> but I'm not sure what the implications of that are.
>
> So someone more knowledged should really amend my commit message
> accordingly so that it explains the user-visible issues :)
>
> I did notice that after a reboot (after having incorrect nr_dirty) the
> entire cache was considered to be dirty and a full writeback occurred,
> but I don't know if that was related at all.
>
>
>  drivers/md/dm-cache-target.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 0df3ec085ebb..83a7eb5ef113 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -154,7 +154,7 @@ struct cache {
>         /*
>          * cache_size entries, dirty if set
>          */
> -       dm_cblock_t nr_dirty;
> +       atomic_t nr_dirty;
>         unsigned long *dirty_bitset;
>
>         /*
> @@ -403,7 +403,7 @@ static bool is_dirty(struct cache *cache, dm_cblock_t b)
>  static void set_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cblock)
>  {
>         if (!test_and_set_bit(from_cblock(cblock), cache->dirty_bitset)) {
> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) + 1);
> +               atomic_inc(&cache->nr_dirty);
>                 policy_set_dirty(cache->policy, oblock);
>         }
>  }
> @@ -412,8 +412,7 @@ static void clear_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cbl
>  {
>         if (test_and_clear_bit(from_cblock(cblock), cache->dirty_bitset)) {
>                 policy_clear_dirty(cache->policy, oblock);
> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) - 1);
> -               if (!from_cblock(cache->nr_dirty))
> +               if (atomic_dec_return(&cache->nr_dirty) == 0)
>                         dm_table_event(cache->ti->table);
>         }
>  }
> @@ -2003,7 +2002,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>         init_waitqueue_head(&cache->migration_wait);
>
>         r = -ENOMEM;
> -       cache->nr_dirty = 0;
> +       atomic_set(&cache->nr_dirty, 0);
>         cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
>         if (!cache->dirty_bitset) {
>                 *error = "could not allocate dirty bitset";
> @@ -2513,7 +2512,7 @@ static void cache_status(struct dm_target *ti, status_type_t type,
>                        (unsigned) atomic_read(&cache->stats.demotion),
>                        (unsigned) atomic_read(&cache->stats.promotion),
>                        (unsigned long long) from_cblock(residency),
> -                      cache->nr_dirty);
> +                      (unsigned) atomic_read(&cache->nr_dirty));
>
>                 if (cache->features.write_through)
>                         DMEMIT("1 writethrough ");
> --
> 1.8.4.5
>
> -- End forwarded Message --
>



-- 
Pranith

Re: [PATCH] dm cache: fix race affecting dirty block count

[Anssi Hannula]

03.08.2014, 05:10, Pranith Kumar kirjoitti:
> Corrently adding Anssi this time.
> 
> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>> Hello Anssi, Joe, Mike,

Hi,

>> I just found your patch in the latest rc and wanted to ask a few
>> questions. I am not sure how this patch helps at all other than luck in
>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>> is atomic on most platforms. Which begs the question, what platform did
>> you encounter this problem?

On x86_64. Regular integer increment/decrement/addition/subtraction are
not atomic on x86(_64).

Assembly for increment without patch is
	addl   $0x1,0x108(%rdi)
and for decrement
	subl   $0x1,0x108(%rbx)

while with patch:
	lock incl 0x108(%rdi)
and
	mov    $0xffffffff,%eax
	lock xadd %eax,0x108(%rbx)
.


>> The patch purports to solve a race condition by making use of atomic_t.
>> I am not sure that is enough. If indeed there is a race you need to use
>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>> atomic_set().

The issue was only about concurrent increments/decrements getting lost
completely from time to time, which atomic_inc/dec will prevent without
any explicit barriers.

>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>> to go to some difficulty in finding this patch. So please CC LKML for
>> such patches.

I don't usually CC LKML if there is a subsystem-specific mailing list
unless there is a specific reason to CC LKML as well. I thought this was
standard practice, but I'm happy to spam LKML as well from now on if
that is preferred by others as well.


>> Thanks,
>> --
>> Pranith
>>
>> -- Begin forwarded Message --
>>
>>
>> nr_dirty is updated without locking, causing it to drift so that it is
>> non-zero (either a small positive integer, or a very large one when an
>> underflow occurs) even when there are no actual dirty blocks.
>>
>> Fix that by using an atomic type for nr_dirty.
>>
>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>> Cc: Joe Thornber <ejt redhat com>
>> Cc: stable vger kernel org
>>
>> ---
>>
>> So here's a patch for the issue I reported a few days ago.
>>
>> I'm not sure what the actual effects of having wrong nr_dirty are
>> beyond the wrong value reported to userspace. Having wrong nr_dirty
>> causes dm_table_event(cache->ti->table) to be called at incorrect times,
>> but I'm not sure what the implications of that are.
>>
>> So someone more knowledged should really amend my commit message
>> accordingly so that it explains the user-visible issues :)
>>
>> I did notice that after a reboot (after having incorrect nr_dirty) the
>> entire cache was considered to be dirty and a full writeback occurred,
>> but I don't know if that was related at all.
>>
>>
>>  drivers/md/dm-cache-target.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
>> index 0df3ec085ebb..83a7eb5ef113 100644
>> --- a/drivers/md/dm-cache-target.c
>> +++ b/drivers/md/dm-cache-target.c
>> @@ -154,7 +154,7 @@ struct cache {
>>         /*
>>          * cache_size entries, dirty if set
>>          */
>> -       dm_cblock_t nr_dirty;
>> +       atomic_t nr_dirty;
>>         unsigned long *dirty_bitset;
>>
>>         /*
>> @@ -403,7 +403,7 @@ static bool is_dirty(struct cache *cache, dm_cblock_t b)
>>  static void set_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cblock)
>>  {
>>         if (!test_and_set_bit(from_cblock(cblock), cache->dirty_bitset)) {
>> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) + 1);
>> +               atomic_inc(&cache->nr_dirty);
>>                 policy_set_dirty(cache->policy, oblock);
>>         }
>>  }
>> @@ -412,8 +412,7 @@ static void clear_dirty(struct cache *cache, dm_oblock_t oblock, dm_cblock_t cbl
>>  {
>>         if (test_and_clear_bit(from_cblock(cblock), cache->dirty_bitset)) {
>>                 policy_clear_dirty(cache->policy, oblock);
>> -               cache->nr_dirty = to_cblock(from_cblock(cache->nr_dirty) - 1);
>> -               if (!from_cblock(cache->nr_dirty))
>> +               if (atomic_dec_return(&cache->nr_dirty) == 0)
>>                         dm_table_event(cache->ti->table);
>>         }
>>  }
>> @@ -2003,7 +2002,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>>         init_waitqueue_head(&cache->migration_wait);
>>
>>         r = -ENOMEM;
>> -       cache->nr_dirty = 0;
>> +       atomic_set(&cache->nr_dirty, 0);
>>         cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
>>         if (!cache->dirty_bitset) {
>>                 *error = "could not allocate dirty bitset";
>> @@ -2513,7 +2512,7 @@ static void cache_status(struct dm_target *ti, status_type_t type,
>>                        (unsigned) atomic_read(&cache->stats.demotion),
>>                        (unsigned) atomic_read(&cache->stats.promotion),
>>                        (unsigned long long) from_cblock(residency),
>> -                      cache->nr_dirty);
>> +                      (unsigned) atomic_read(&cache->nr_dirty));
>>
>>                 if (cache->features.write_through)
>>                         DMEMIT("1 writethrough ");
>> --
>> 1.8.4.5
>>
>> -- End forwarded Message --
>>
> 
> 
> 


-- 
Anssi Hannula

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On Sat, Aug 2, 2014 at 11:33 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> 03.08.2014, 05:10, Pranith Kumar kirjoitti:
>> Corrently adding Anssi this time.
>>
>> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>>> Hello Anssi, Joe, Mike,
>
> Hi,
>
>>> I just found your patch in the latest rc and wanted to ask a few
>>> questions. I am not sure how this patch helps at all other than luck in
>>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>>> is atomic on most platforms. Which begs the question, what platform did
>>> you encounter this problem?
>
> On x86_64. Regular integer increment/decrement/addition/subtraction are
> not atomic on x86(_64).
>

You are right. Only loads/stores are atomic for an integer. My point
was more about barriers after atomic_inc/dec.

Also dm_cblock_t is uint32_t, but atomic_t changes that to int. You
should correct that to atomic64_t to preserve original semantics.

>>> The patch purports to solve a race condition by making use of atomic_t.
>>> I am not sure that is enough. If indeed there is a race you need to use
>>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>>> atomic_set().
>
> The issue was only about concurrent increments/decrements getting lost
> completely from time to time, which atomic_inc/dec will prevent without
> any explicit barriers.
>

These increments and decrements will still be lost if you do not use
barriers in presence of concurrent accesses. Please see
Documentation/memory-barriers.txt.

>>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>>> to go to some difficulty in finding this patch. So please CC LKML for
>>> such patches.
>
> I don't usually CC LKML if there is a subsystem-specific mailing list
> unless there is a specific reason to CC LKML as well. I thought this was
> standard practice, but I'm happy to spam LKML as well from now on if
> that is preferred by others as well.
>

I guess it is always a good idea to CC LKML in case of patches in
addition to sub-system mailing lists. For one, it is easy to forward a
message from lkml and then reply to that in case there is something to
discuss.


-- 
Pranith

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On 08/02/2014 10:10 PM, Pranith Kumar wrote:
> Corrently adding Anssi this time.
> 
> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>> Hello Anssi, Joe, Mike,
>>
>> I just found your patch in the latest rc and wanted to ask a few
>> questions. I am not sure how this patch helps at all other than luck in
>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>> is atomic on most platforms. Which begs the question, what platform did
>> you encounter this problem?
>>
>> The patch purports to solve a race condition by making use of atomic_t.
>> I am not sure that is enough. If indeed there is a race you need to use
>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>> atomic_set().
>>
>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>> to go to some difficulty in finding this patch. So please CC LKML for
>> such patches.
>>
>> Thanks,
>> --
>> Pranith
>>
>> -- Begin forwarded Message --
>>
>>
>> nr_dirty is updated without locking, causing it to drift so that it is
>> non-zero (either a small positive integer, or a very large one when an
>> underflow occurs) even when there are no actual dirty blocks.
>>
>> Fix that by using an atomic type for nr_dirty.
>>
>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>> Cc: Joe Thornber <ejt redhat com>
>> Cc: stable vger kernel org

Well, someone got their pointers mixed up. The following should help, but this is not enough if there are indeed concurrent accesses. Are you sure there are concurrent accesses?

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 2c63326..d49ce45 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -1765,11 +1765,11 @@ static void destroy(struct cache *cache)
        if (cache->wq)
                destroy_workqueue(cache->wq);
 
-       if (cache->dirty_bitset)
-               free_bitset(cache->dirty_bitset);
+       if (cache.dirty_bitset)
+               free_bitset(cache.dirty_bitset);
 
-       if (cache->discard_bitset)
-               free_bitset(cache->discard_bitset);
+       if (cache.discard_bitset)
+               free_bitset(cache.discard_bitset);
 
        if (cache->copier)
                dm_kcopyd_client_destroy(cache->copier);
@@ -2269,15 +2269,15 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 
        r = -ENOMEM;
        atomic_set(&cache->nr_dirty, 0);
-       cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
-       if (!cache->dirty_bitset) {
+       cache.dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
+       if (!cache.dirty_bitset) {
                *error = "could not allocate dirty bitset";
                goto bad;
        }
        clear_bitset(cache->dirty_bitset, from_cblock(cache->cache_size));
 
        cache->discard_nr_blocks = cache->origin_blocks;
-       cache->discard_bitset = alloc_bitset(from_oblock(cache->discard_nr_blocks));
+       cache.discard_bitset = alloc_bitset(from_oblock(cache->discard_nr_blocks));
        if (!cache->discard_bitset) {
                *error = "could not allocate discard bitset";
                goto bad;

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On 08/03/2014 12:46 AM, Pranith Kumar wrote:
> On 08/02/2014 10:10 PM, Pranith Kumar wrote:
>> Corrently adding Anssi this time.
>>
>> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>>> Hello Anssi, Joe, Mike,
>>>
>>> I just found your patch in the latest rc and wanted to ask a few
>>> questions. I am not sure how this patch helps at all other than luck in
>>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>>> is atomic on most platforms. Which begs the question, what platform did
>>> you encounter this problem?
>>>
>>> The patch purports to solve a race condition by making use of atomic_t.
>>> I am not sure that is enough. If indeed there is a race you need to use
>>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>>> atomic_set().
>>>
>>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>>> to go to some difficulty in finding this patch. So please CC LKML for
>>> such patches.
>>>
>>> Thanks,
>>> --
>>> Pranith
>>>
>>> -- Begin forwarded Message --
>>>
>>>
>>> nr_dirty is updated without locking, causing it to drift so that it is
>>> non-zero (either a small positive integer, or a very large one when an
>>> underflow occurs) even when there are no actual dirty blocks.
>>>
>>> Fix that by using an atomic type for nr_dirty.
>>>
>>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>>> Cc: Joe Thornber <ejt redhat com>
>>> Cc: stable vger kernel org

I found one more location being incorrectly referenced. Please find a fixed
patch below. Also I think we will need to revert the previous patch since it
clearly does not fix anything if there were races.


From: Pranith Kumar <bobby.prani@gmail.com>
Date: Sun, 3 Aug 2014 00:53:20 -0400
Subject: [PATCH 1/1] dm cache: Fix incorrect assignment to pointer

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 drivers/md/dm-cache-target.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 2c63326..49e47e7 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -1765,11 +1765,11 @@ static void destroy(struct cache *cache)
 	if (cache->wq)
 		destroy_workqueue(cache->wq);
 
-	if (cache->dirty_bitset)
-		free_bitset(cache->dirty_bitset);
+	if (cache.dirty_bitset)
+		free_bitset(cache.dirty_bitset);
 
-	if (cache->discard_bitset)
-		free_bitset(cache->discard_bitset);
+	if (cache.discard_bitset)
+		free_bitset(cache.discard_bitset);
 
 	if (cache->copier)
 		dm_kcopyd_client_destroy(cache->copier);
@@ -2269,16 +2269,16 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 
 	r = -ENOMEM;
 	atomic_set(&cache->nr_dirty, 0);
-	cache->dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
-	if (!cache->dirty_bitset) {
+	cache.dirty_bitset = alloc_bitset(from_cblock(cache->cache_size));
+	if (!cache.dirty_bitset) {
 		*error = "could not allocate dirty bitset";
 		goto bad;
 	}
 	clear_bitset(cache->dirty_bitset, from_cblock(cache->cache_size));
 
 	cache->discard_nr_blocks = cache->origin_blocks;
-	cache->discard_bitset = alloc_bitset(from_oblock(cache->discard_nr_blocks));
-	if (!cache->discard_bitset) {
+	cache.discard_bitset = alloc_bitset(from_oblock(cache->discard_nr_blocks));
+	if (!cache.discard_bitset) {
 		*error = "could not allocate discard bitset";
 		goto bad;
 	}
-- 
1.9.1

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On 08/03/2014 12:57 AM, Pranith Kumar wrote:
> On 08/03/2014 12:46 AM, Pranith Kumar wrote:
>> On 08/02/2014 10:10 PM, Pranith Kumar wrote:
>>> Corrently adding Anssi this time.
>>>
>>> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>>>> Hello Anssi, Joe, Mike,
>>>>
>>>> I just found your patch in the latest rc and wanted to ask a few
>>>> questions. I am not sure how this patch helps at all other than luck in
>>>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>>>> is atomic on most platforms. Which begs the question, what platform did
>>>> you encounter this problem?
>>>>
>>>> The patch purports to solve a race condition by making use of atomic_t.
>>>> I am not sure that is enough. If indeed there is a race you need to use
>>>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>>>> atomic_set().
>>>>
>>>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>>>> to go to some difficulty in finding this patch. So please CC LKML for
>>>> such patches.
>>>>
>>>> Thanks,
>>>> --
>>>> Pranith
>>>>
>>>> -- Begin forwarded Message --
>>>>
>>>>
>>>> nr_dirty is updated without locking, causing it to drift so that it is
>>>> non-zero (either a small positive integer, or a very large one when an
>>>> underflow occurs) even when there are no actual dirty blocks.
>>>>
>>>> Fix that by using an atomic type for nr_dirty.
>>>>
>>>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>>>> Cc: Joe Thornber <ejt redhat com>
>>>> Cc: stable vger kernel org
> 

There are more in the following patch. I think you need to really check what
else I might be missing.

From: Pranith Kumar <bobby.prani@gmail.com>
Date: Sun, 3 Aug 2014 01:15:10 -0400
Subject: [PATCH 1/1] dm cache: Fix more incorrect pointer assignments

Fix incorrect pointer uses and assignments.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 drivers/md/dm-cache-target.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 49e47e7..1627035 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -1777,13 +1777,13 @@ static void destroy(struct cache *cache)
 	if (cache->cmd)
 		dm_cache_metadata_close(cache->cmd);
 
-	if (cache->metadata_dev)
+	if (cache.metadata_dev)
 		dm_put_device(cache->ti, cache->metadata_dev);
 
-	if (cache->origin_dev)
+	if (cache.origin_dev)
 		dm_put_device(cache->ti, cache->origin_dev);
 
-	if (cache->cache_dev)
+	if (cache.cache_dev)
 		dm_put_device(cache->ti, cache->cache_dev);
 
 	if (cache->policy)
@@ -1861,13 +1861,13 @@ struct cache_args {
 
 static void destroy_cache_args(struct cache_args *ca)
 {
-	if (ca->metadata_dev)
+	if (ca.metadata_dev)
 		dm_put_device(ca->ti, ca->metadata_dev);
 
-	if (ca->cache_dev)
+	if (ca.cache_dev)
 		dm_put_device(ca->ti, ca->cache_dev);
 
-	if (ca->origin_dev)
+	if (ca.origin_dev)
 		dm_put_device(ca->ti, ca->origin_dev);
 
 	kfree(ca);
@@ -2190,7 +2190,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 	cache->origin_dev = ca->origin_dev;
 	cache->cache_dev = ca->cache_dev;
 
-	ca->metadata_dev = ca->origin_dev = ca->cache_dev = NULL;
+	ca.metadata_dev = ca.origin_dev = ca.cache_dev = NULL;
 
 	/* FIXME: factor out this whole section */
 	origin_blocks = cache->origin_sectors = ca->origin_sectors;
-- 
1.9.1

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On Sun, Aug 3, 2014 at 1:17 AM, Pranith Kumar <bobby.prani@gmail.com> wrote:
> On 08/03/2014 12:57 AM, Pranith Kumar wrote:
>> On 08/03/2014 12:46 AM, Pranith Kumar wrote:
>>> On 08/02/2014 10:10 PM, Pranith Kumar wrote:
>>>> Corrently adding Anssi this time.
>>>>
>>>> On Sat, Aug 2, 2014 at 10:00 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>>>>> Hello Anssi, Joe, Mike,
>>>>>
>>>>> I just found your patch in the latest rc and wanted to ask a few
>>>>> questions. I am not sure how this patch helps at all other than luck in
>>>>> that dm_cblock_t type is of type int32_t, which should guarantee that it
>>>>> is atomic on most platforms. Which begs the question, what platform did
>>>>> you encounter this problem?
>>>>>
>>>>> The patch purports to solve a race condition by making use of atomic_t.
>>>>> I am not sure that is enough. If indeed there is a race you need to use
>>>>> smp_mb__{before/after}_atomic() for both your uses of atomic_inc() and
>>>>> atomic_set().
>>>>>
>>>>> Also I have a concern about why this mail was not CC'ed on LKML. I had
>>>>> to go to some difficulty in finding this patch. So please CC LKML for
>>>>> such patches.
>>>>>
>>>>> Thanks,
>>>>> --
>>>>> Pranith
>>>>>
>>>>> -- Begin forwarded Message --
>>>>>
>>>>>
>>>>> nr_dirty is updated without locking, causing it to drift so that it is
>>>>> non-zero (either a small positive integer, or a very large one when an
>>>>> underflow occurs) even when there are no actual dirty blocks.
>>>>>
>>>>> Fix that by using an atomic type for nr_dirty.
>>>>>
>>>>> Signed-off-by: Anssi Hannula <anssi hannula iki fi>
>>>>> Cc: Joe Thornber <ejt redhat com>
>>>>> Cc: stable vger kernel org
>>
>

Bah, please ignore all the previous patches about pointers. It is me
who is drunk and got my pointers mixed up. :(

-- 
Pranith

Re: [PATCH] dm cache: fix race affecting dirty block count

[Joe Thornber]

On Sun, Aug 03, 2014 at 12:01:17AM -0400, Pranith Kumar wrote:
> Also dm_cblock_t is uint32_t, but atomic_t changes that to int. You
> should correct that to atomic64_t to preserve original semantics.

atomic_t used to have only 24 bits of range due to the Sparc
implementation holding a lock in one of the bytes.  I understand this
limitation was removed during 2.6 and the full 32 bits are now
available.

eg,

    https://github.com/jthornber/linux-2.6/commit/37682177af68478fa83429b735fa16913c2fbb2b

> These increments and decrements will still be lost if you do not use
> barriers in presence of concurrent accesses. Please see
> Documentation/memory-barriers.txt.

You do not need to use barriers for plain atomic_inc/dec().

    https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L187

You _do_ need to use a memory barrier for the ops that return a value
(such as atomic_dec_and_test()), But only if there's some other state
that needs synchronising.  See the nice example in atomic_ops.txt:

    https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L321

We just trigger a stateless event when the counter hits zero, so the
patch is fine.

- Joe

Re: [PATCH] dm cache: fix race affecting dirty block count

[Pranith Kumar]

On Mon, Aug 4, 2014 at 6:48 AM, Joe Thornber <thornber@redhat.com> wrote:
> On Sun, Aug 03, 2014 at 12:01:17AM -0400, Pranith Kumar wrote:
>> Also dm_cblock_t is uint32_t, but atomic_t changes that to int. You
>> should correct that to atomic64_t to preserve original semantics.
>
> atomic_t used to have only 24 bits of range due to the Sparc
> implementation holding a lock in one of the bytes.  I understand this
> limitation was removed during 2.6 and the full 32 bits are now
> available.
>

I meant to point out that atomic_t is a signed integer (int) type
using the full 32 bits with signed operations. dm_cblock_t is unsgined
int.

>
>> These increments and decrements will still be lost if you do not use
>> barriers in presence of concurrent accesses. Please see
>> Documentation/memory-barriers.txt.
>
> You do not need to use barriers for plain atomic_inc/dec().
>
>     https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L187

That talks about implementation of atomic_inc/dec() for arch porters.
Users of atomic_inc/dec() should use memory barriers.

>
> You _do_ need to use a memory barrier for the ops that return a value
> (such as atomic_dec_and_test()), But only if there's some other state
> that needs synchronising.  See the nice example in atomic_ops.txt:
>
>     https://github.com/jthornber/linux-2.6/blob/thin-dev/Documentation/atomic_ops.txt#L321

Again when it says it needs explicit memory barriers, it is for the
arch porters. So atomic_dec_and_test(), atomic_dec_return() etc., have
implicit memory barriers.

>
> We just trigger a stateless event when the counter hits zero, so the
> patch is fine.
>

Your use of atomic_dec_return() is what is fixing the race issue here
I guess , as it has implicit memory barriers. But I suggest checking
out if you need barriers for atomic_inc() and atomic_set() too.

-- 
Pranith