Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-43055

ItemPager available flag not reset if run() early exits

    XMLWordPrintable

    Details

    • Triage:
      Triaged
    • Story Points:
      1
    • Is this a Regression?:
      Yes
    • Sprint:
      KV-Engine 2021-Jan

      Description

      There is a bug which, if triggered, will stop a bucket from successfully evicting ever again (until restart/bucket deletion).


      Bug introduced in http://review.couchbase.org/c/kv_engine/+/133197 (6.6.1 pre-GA).

      item_pager.cc,ItemPager::run

      112
          if (((current > upper) || doEvict || wasNotified) &&
      113
              (*available).compare_exchange_strong(inverse, false)) {
      114
              if (kvBucket->getItemEvictionPolicy() == EvictionPolicy::Value) {
      115
                  doEvict = true;
      116
              }
      117
       
      118
              ++stats.pagerRuns;
      119
       
      120
              if (current <= lower) {
      121
                  // early exit - no need to run a paging visitor
      122
                  return true;
      123
              }
      

      The above change introduced the highlighted early exit; in the event that the item pager is notified to run, but the memory usage has dropped below the low watermark, don't evict. However, this is not safe to do -

      item_pager.cc,ItemPager::run

      112
          if (((current > upper) || doEvict || wasNotified) &&
      113
              (*available).compare_exchange_strong(inverse, false)) {
      

      Available is an atomic bool used to track if an item pager is currently running. It has been set to false here, but is only reset once the created PagingVisitor is completed. If the visitor creation is skipped, the flag is never reset, so no further item pager runs will occur.

        Attachments

          Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

            Activity

            james.harrison James Harrison created issue -
            james.harrison James Harrison made changes -
            Field Original Value New Value
            Link This issue is triggering CBSE-9293 [ CBSE-9293 ]
            james.harrison James Harrison made changes -
            Description A bug can potentially stop the {{ItemPager}} from running, halting eviction for a bucket until restart/bucket deletion.

            ----

            Bug introduced in http://review.couchbase.org/c/kv_engine/+/133197

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=122}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
                    if (kvBucket->getItemEvictionPolicy() == EvictionPolicy::Value) {
                        doEvict = true;
                    }

                    ++stats.pagerRuns;

                    if (current <= lower) {
                        // early exit - no need to run a paging visitor
                        return true;
                    }
            {noformat}

            The above change introduced the highlighted early exit; in the event that the item pager is notified to run, but the memory usage has dropped below the low watermark, don't evict. However, this is not safe to do -

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=113}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
            {noformat}

            Available is an atomic bool used to track if an item pager is currently running. It has been set to false _here_, but is only reset once the created {{PagingVisitor}} is completed. If the visitor creation is skipped, the flag is never reset, so no further item pager runs will occur.

            There is a bug which, if triggered, will stop a bucket from successfully evicting ever again (until restart/bucket deletion).

            ----

            Bug introduced in http://review.couchbase.org/c/kv_engine/+/133197

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=122}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
                    if (kvBucket->getItemEvictionPolicy() == EvictionPolicy::Value) {
                        doEvict = true;
                    }

                    ++stats.pagerRuns;

                    if (current <= lower) {
                        // early exit - no need to run a paging visitor
                        return true;
                    }
            {noformat}

            The above change introduced the highlighted early exit; in the event that the item pager is notified to run, but the memory usage has dropped below the low watermark, don't evict. However, this is not safe to do -

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=113}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
            {noformat}

            Available is an atomic bool used to track if an item pager is currently running. It has been set to false _here_, but is only reset once the created {{PagingVisitor}} is completed. If the visitor creation is skipped, the flag is never reset, so no further item pager runs will occur.

            owend Daniel Owen made changes -
            Priority Critical [ 2 ] Blocker [ 1 ]
            owend Daniel Owen made changes -
            Fix Version/s 6.6.1 [ 17002 ]
            owend Daniel Owen made changes -
            Labels approved-for-6.6.1
            owend Daniel Owen made changes -
            Link This issue blocks MB-40528 [ MB-40528 ]
            drigby Dave Rigby made changes -
            Affects Version/s Cheshire-Cat [ 15915 ]
            drigby Dave Rigby made changes -
            Is this a Regression? Unknown [ 10452 ] Yes [ 10450 ]
            drigby Dave Rigby made changes -
            Description There is a bug which, if triggered, will stop a bucket from successfully evicting ever again (until restart/bucket deletion).

            ----

            Bug introduced in http://review.couchbase.org/c/kv_engine/+/133197

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=122}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
                    if (kvBucket->getItemEvictionPolicy() == EvictionPolicy::Value) {
                        doEvict = true;
                    }

                    ++stats.pagerRuns;

                    if (current <= lower) {
                        // early exit - no need to run a paging visitor
                        return true;
                    }
            {noformat}

            The above change introduced the highlighted early exit; in the event that the item pager is notified to run, but the memory usage has dropped below the low watermark, don't evict. However, this is not safe to do -

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=113}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
            {noformat}

            Available is an atomic bool used to track if an item pager is currently running. It has been set to false _here_, but is only reset once the created {{PagingVisitor}} is completed. If the visitor creation is skipped, the flag is never reset, so no further item pager runs will occur.

            There is a bug which, if triggered, will stop a bucket from successfully evicting ever again (until restart/bucket deletion).

            ----

            Bug introduced in http://review.couchbase.org/c/kv_engine/+/133197 (6.6.1 pre-GA).

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=122}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
                    if (kvBucket->getItemEvictionPolicy() == EvictionPolicy::Value) {
                        doEvict = true;
                    }

                    ++stats.pagerRuns;

                    if (current <= lower) {
                        // early exit - no need to run a paging visitor
                        return true;
                    }
            {noformat}

            The above change introduced the highlighted early exit; in the event that the item pager is notified to run, but the memory usage has dropped below the low watermark, don't evict. However, this is not safe to do -

            {noformat:title=item_pager.cc,ItemPager::run|linenumbers|firstline=112|highlight=113}
                if (((current > upper) || doEvict || wasNotified) &&
                    (*available).compare_exchange_strong(inverse, false)) {
            {noformat}

            Available is an atomic bool used to track if an item pager is currently running. It has been set to false _here_, but is only reset once the created {{PagingVisitor}} is completed. If the visitor creation is skipped, the flag is never reset, so no further item pager runs will occur.

            drigby Dave Rigby made changes -
            Resolution Fixed [ 1 ]
            Status Open [ 1 ] Resolved [ 5 ]
            owend Daniel Owen made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            owend Daniel Owen made changes -
            Assignee James Harrison [ james.harrison ] Daniel Owen [ owend ]
            drigby Dave Rigby made changes -
            Link This issue causes CBSE-9345 [ CBSE-9345 ]
            drigby Dave Rigby made changes -
            Affects Version/s 7.0.0-Beta1 [ 17286 ]
            drigby Dave Rigby made changes -
            Fix Version/s Cheshire-Cat [ 15915 ]
            shivani.gupta Shivani Gupta made changes -
            Link This issue blocks CBSE-9345 [ CBSE-9345 ]
            drigby Dave Rigby made changes -
            Affects Version/s 6.6.1 [ 17002 ]
            drigby Dave Rigby made changes -
            Link This issue blocks CBSE-9345 [ CBSE-9345 ]
            james.harrison James Harrison made changes -
            Link This issue is caused by MB-40531 [ MB-40531 ]
            james.harrison James Harrison made changes -
            Assignee Daniel Owen [ owend ] James Harrison [ james.harrison ]
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            owend Daniel Owen made changes -
            Fix Version/s 6.5.2 [ 17223 ]
            Fix Version/s 6.0.5 [ 16729 ]
            james.harrison James Harrison made changes -
            Affects Version/s 6.5.2 [ 17223 ]
            Affects Version/s 6.0.5 [ 16729 ]
            drigby Dave Rigby made changes -
            Affects Version/s 6.5.2 [ 17223 ]
            Affects Version/s 6.0.5 [ 16729 ]
            wayne Wayne Siu made changes -
            Link This issue blocks MB-40511 [ MB-40511 ]
            wayne Wayne Siu made changes -
            Labels approved-for-6.6.1 approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1
            wayne Wayne Siu made changes -
            Link This issue blocks MB-42583 [ MB-42583 ]
            owend Daniel Owen made changes -
            Sprint KV-Engine 2021-Jan [ 1383 ]
            owend Daniel Owen made changes -
            Rank Ranked higher
            drigby Dave Rigby made changes -
            Resolution Fixed [ 1 ]
            Status Reopened [ 4 ] Resolved [ 5 ]
            wayne Wayne Siu made changes -
            Assignee James Harrison [ james.harrison ] Ashwin Govindarajulu [ ashwin.govindarajulu ]
            ashwin.govindarajulu Ashwin Govindarajulu made changes -
            Assignee Ashwin Govindarajulu [ ashwin.govindarajulu ] James Harrison [ james.harrison ]
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            ashwin.govindarajulu Ashwin Govindarajulu made changes -
            Assignee James Harrison [ james.harrison ] Ashwin Govindarajulu [ ashwin.govindarajulu ]
            Resolution Fixed [ 1 ]
            Status Reopened [ 4 ] Closed [ 6 ]
            arunkumar Arunkumar Senthilnathan made changes -
            Labels approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1 approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1 releasenote
            drigby Dave Rigby made changes -
            Labels approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1 releasenote approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1
            arunkumar Arunkumar Senthilnathan made changes -
            Labels approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1 approved-for-6.0.5 approved-for-6.5.2 approved-for-6.6.1 releasenote
            lynn.straus Lynn Straus made changes -
            Fix Version/s 7.0.0 [ 17233 ]
            lynn.straus Lynn Straus made changes -
            Fix Version/s Cheshire-Cat [ 15915 ]

              People

              Assignee:
              ashwin.govindarajulu Ashwin Govindarajulu
              Reporter:
              james.harrison James Harrison
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  PagerDuty