<div dir="ltr">Hi Chris,<div><br></div><div>Based on my understanding, the patch in your email is different:</div><div><br></div><div>1) rtems_rfs_bitmap_map_set:</div><div>By changing negating the if condition, the updated logic only modifies the element map[index] when the original value of map[index] is RTEMS_RFS_BITMAP_ELEMENT_SET.</div><div><br></div><div>This is not quite right, as we are explicitly updating the 'offset' bit in map[index]. That bit should be updated regardless.</div><div><br></div><div>2) rtems_rfs_bitmap_map_clear:</div><div>Same problem, whether we modify map[index] should not depend on if map[index] == RTEMS_RFS_BITMAP_ELEMENT_SET.</div><div><br></div><div>I believe my patch corrects the logic flaw and is the right fix. Please let me know if you have any further questions, and I will be happy to explain in detail.</div><div><br></div><div>Thanks,</div><div>Fan</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 12, 2017 at 9:48 AM, Chris Johns <span dir="ltr"><<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Many thanks for post the patches. The other patches look fine. I am wondering is this is equivalent to what you have?<br>
<br>
diff --git a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
index 15a9050133..d14082691a 100644<br>
--- a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
+++ b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
@@ -196,9 +196,9 @@ rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
<span class="">   search_map = control->search_bits;<br>
   index      = rtems_rfs_bitmap_map_index (bit);<br>
   offset     = rtems_rfs_bitmap_map_offset (bit);<br>
-  map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
</span>-  if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
+  if (!rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
   {<br>
+    map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
<span class="">     bit = index;<br>
     index  = rtems_rfs_bitmap_map_index (bit);<br>
     offset = rtems_rfs_bitmap_map_offset (bit);<br>
</span>@@ -226,6 +226,8 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
<span class="">   search_map        = control->search_bits;<br>
   index             = rtems_rfs_bitmap_map_index (bit);<br>
   offset            = rtems_rfs_bitmap_map_offset (bit);<br>
</span>+  if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
+  {<br>
     map[index]        = rtems_rfs_bitmap_clear (map[index], 1 << offset);<br>
<span class="">     bit               = index;<br>
     index             = rtems_rfs_bitmap_map_index (bit);<br>
</span>@@ -233,6 +235,7 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
     search_map[index] = rtems_rfs_bitmap_clear (search_map[index], 1 << offset);<br>
     rtems_rfs_buffer_mark_dirty (control->buffer);<br>
     control->free++;<br>
+  }<br>
   return 0;<br>
 }<br>
<span class="HOEnZb"><font color="#888888"><br>
Chris<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On 10/10/17 4:28 pm, Fan Deng wrote:<br>
> The bitmap allocation accounting logic in rtems-rfs-bitmaps.c is flawed<br>
> around control->free. Specifically:<br>
><br>
> In rtems_rfs_bitmap_map_set():<br>
> control->free is only decremented when its corresponding search bit is<br>
> toggled. This is wrong and will miss on average 31/32 set updates.<br>
><br>
> In rtems_rfs_bitmap_map_clear():<br>
> control->free is incremented unconditionally.<br>
><br>
> The correct behavior is:<br>
> When updating the map, check if the bit is already set/clear. Only update<br>
> control->free when the bit is toggled.<br>
><br>
> This change enforced the correct behavior.<br>
><br>
> Tested by inspecting the internal data structure.<br>
> ---<br>
>  cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c | 40 +++++++++++++++++++++---------<wbr>--<br>
>  1 file changed, 26 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> index 15a9050133..c0c6921db1 100644<br>
> --- a/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> +++ b/cpukit/libfs/src/rfs/rtems-<wbr>rfs-bitmaps.c<br>
> @@ -183,11 +183,12 @@ int<br>
>  rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
>                            rtems_rfs_bitmap_bit      bit)<br>
>  {<br>
> -  rtems_rfs_bitmap_map map;<br>
> -  rtems_rfs_bitmap_map search_map;<br>
> -  int                  index;<br>
> -  int                  offset;<br>
> -  int                 rc;<br>
> +  rtems_rfs_bitmap_map     map;<br>
> +  rtems_rfs_bitmap_map     search_map;<br>
> +  int                      index;<br>
> +  int                      offset;<br>
> +  int                      rc;<br>
> +  rtems_rfs_bitmap_element element;<br>
>    rc = rtems_rfs_bitmap_load_map (control, &map);<br>
>    if (rc > 0)<br>
>      return rc;<br>
> @@ -196,15 +197,20 @@ rtems_rfs_bitmap_map_set (rtems_rfs_bitmap_control* control,<br>
>    search_map = control->search_bits;<br>
>    index      = rtems_rfs_bitmap_map_index (bit);<br>
>    offset     = rtems_rfs_bitmap_map_offset (bit);<br>
> -  map[index] = rtems_rfs_bitmap_set (map[index], 1 << offset);<br>
> +  element    = map[index];<br>
> +  map[index] = rtems_rfs_bitmap_set (element, 1 << offset);<br>
> +  // If the element does not change, the bit was already set. There will be no<br>
> +  // further action to take.<br>
> +  if (rtems_rfs_bitmap_match(<wbr>element, map[index]))<br>
> +      return 0;<br>
> +  control->free--;<br>
> +  rtems_rfs_buffer_mark_dirty (control->buffer);<br>
>    if (rtems_rfs_bitmap_match(map[<wbr>index], RTEMS_RFS_BITMAP_ELEMENT_SET))<br>
>    {<br>
>      bit = index;<br>
>      index  = rtems_rfs_bitmap_map_index (bit);<br>
>      offset = rtems_rfs_bitmap_map_offset (bit);<br>
>      search_map[index] = rtems_rfs_bitmap_set (search_map[index], 1 << offset);<br>
> -    control->free--;<br>
> -    rtems_rfs_buffer_mark_dirty (control->buffer);<br>
>    }<br>
>    return 0;<br>
>  }<br>
> @@ -213,11 +219,12 @@ int<br>
>  rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
>                              rtems_rfs_bitmap_bit      bit)<br>
>  {<br>
> -  rtems_rfs_bitmap_map map;<br>
> -  rtems_rfs_bitmap_map search_map;<br>
> -  int                  index;<br>
> -  int                  offset;<br>
> -  int                  rc;<br>
> +  rtems_rfs_bitmap_map      map;<br>
> +  rtems_rfs_bitmap_map     search_map;<br>
> +  int                      index;<br>
> +  int                      offset;<br>
> +  int                      rc;<br>
> +  rtems_rfs_bitmap_element element;<br>
>    rc = rtems_rfs_bitmap_load_map (control, &map);<br>
>    if (rc > 0)<br>
>      return rc;<br>
> @@ -226,7 +233,12 @@ rtems_rfs_bitmap_map_clear (rtems_rfs_bitmap_control* control,<br>
>    search_map        = control->search_bits;<br>
>    index             = rtems_rfs_bitmap_map_index (bit);<br>
>    offset            = rtems_rfs_bitmap_map_offset (bit);<br>
> -  map[index]        = rtems_rfs_bitmap_clear (map[index], 1 << offset);<br>
> +  element           = map[index];<br>
> +  map[index]        = rtems_rfs_bitmap_clear (element, 1 << offset);<br>
> +  // If the element does not change, the bit was already clear. There will be no<br>
> +  // further action to take.<br>
> +  if (rtems_rfs_bitmap_match(<wbr>element, map[index]))<br>
> +      return 0;<br>
>    bit               = index;<br>
>    index             = rtems_rfs_bitmap_map_index (bit);<br>
>    offset            = rtems_rfs_bitmap_map_offset(<wbr>bit);<br>
><br>
</div></div></blockquote></div><br></div></div>