From be3448ed5d9d59752cff4df8325ee67eb7d41531 Mon Sep 17 00:00:00 2001
From: Xavi Hernandez <xhernandez@redhat.com>
Date: Mon, 19 Jul 2021 06:56:18 +0200
Subject: [PATCH 592/610] md-cache: fix integer signedness mismatch

md-cache uses a mechanism based on a generation number to detect
modifications made by other clients to the entries and invalidate
the cached data.

This generation number is a 32 bit integer. When it overflows,
special management is done to avoid problems. This overflow condition
is tracked with a single bit.

For many fops, when they are received, the overflow bit and the
current generation number are recorded in a single 64-bit value
which is used later in the cbk.

This is the problematic function:

    uint64_t
    __mdc_get_generation(xlator_t *this, struct md_cache *mdc)
    {
        uint64_t gen = 0, rollover;
        struct mdc_conf *conf = NULL;

        conf = this->private;

        gen = GF_ATOMIC_INC(conf->generation);
        if (gen == 0) {
            gf_log("MDC", GF_LOG_NOTICE, "%p Reset 1", mdc);
            mdc->gen_rollover = !mdc->gen_rollover;
            gen = GF_ATOMIC_INC(conf->generation);
            mdc->ia_time = 0;
            mdc->generation = 0;
            mdc->invalidation_time = gen - 1;
        }

        rollover = mdc->gen_rollover;
        gen |= (rollover << 32);
        return gen;
    }

'conf->generation' is declared as an atomic signed 32-bit integer,
and 'gen' is an unsigned 64-bit value. When 'gen' is assigned from
a signed int, the sign bit is extended to fill the high 32 bits of
'gen'. If the counter has overflown the maximum signed positive
value, it will become negative (sign bit = 1).

In this case, when 'rollover' is later combined with 'gen', all the
high bits remain at '1'.

This value is used later in 'mdc_inode_iatt_set_validate' during
callback processing. The overflow condition and generation numbers
from when the operation was received are recovered this way:

    rollover = incident_time >> 32;
    incident_time = (incident_time & 0xffffffff);

('incident_time' is the saved value from '__mdc_get_generation').

So here rollover will be 0xffffffff, when it's expected to be 0
or 1 only. When this is compared later with the cached overflow
bit, it doesn't match, which prevents updating the cached info.

This is bad in general, but it's even worse when an entry is not
cached and 'rollover' is 0xffffffff the first time. When md-cache
doesn't have cached data it assumes it's everything 0. This causes
a mismatch, which sends an invalidation request to the kernel, but
since the 'rollover' doesn't match, the cached data is not updated.
So the next time the cached data is checked, it will also send an
invalidation to the kernel, indefinitely.

This patch fixes two things:

1. The 'generation' field is made unsigned to avoid sign extension.
2. Invalidation requests are only sent if we already had valid cached
   data. Otherwise it doesn't make sense to send an invalidation.

Upstream patch:
> Upstream-patch-link: https://github.com/gluster/glusterfs/pull/2619
> Fixes: #2617
> Change-Id: Ie40e68288cf143e1bc1a40f46da98f51bb2d6864
> Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

BUG: 1904137
Change-Id: Ie40e68288cf143e1bc1a40f46da98f51bb2d6864
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/c/rhs-glusterfs/+/279188
Tested-by: RHGS Build Bot <nigelb@redhat.com>
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>
---
 xlators/performance/md-cache/src/md-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xlators/performance/md-cache/src/md-cache.c b/xlators/performance/md-cache/src/md-cache.c
index bbbee3b..e0256d6 100644
--- a/xlators/performance/md-cache/src/md-cache.c
+++ b/xlators/performance/md-cache/src/md-cache.c
@@ -79,7 +79,7 @@ struct mdc_conf {
     gf_boolean_t cache_statfs;
     struct mdc_statfs_cache statfs_cache;
     char *mdc_xattr_str;
-    gf_atomic_int32_t generation;
+    gf_atomic_uint32_t generation;
 };
 
 struct mdc_local;
@@ -537,7 +537,7 @@ mdc_inode_iatt_set_validate(xlator_t *this, inode_t *inode, struct iatt *prebuf,
             (iatt->ia_mtime_nsec != mdc->md_mtime_nsec) ||
             (iatt->ia_ctime != mdc->md_ctime) ||
             (iatt->ia_ctime_nsec != mdc->md_ctime_nsec)) {
-            if (conf->global_invalidation &&
+            if (conf->global_invalidation && mdc->valid &&
                 (!prebuf || (prebuf->ia_mtime != mdc->md_mtime) ||
                  (prebuf->ia_mtime_nsec != mdc->md_mtime_nsec) ||
                  (prebuf->ia_ctime != mdc->md_ctime) ||
-- 
1.8.3.1