Ceph » CephFS

Venky Shankar wrote:

Let's get an analysis done how get_cap_ref() is called for a cap that the client does not have.

This was more of a - if the MDS hasn't issued caps and the client is requesting for that cap ref then we must disallow this

in Client::get_caps, we check for the caps issued to the inode

int have = in->caps_issued(&implemented);

then

if ((have & need) == need) {
    int revoking = implemented & ~have;
    ldout(cct, 10) << "get_caps " << *in << " have " << ccap_string(have)
                << " need " << ccap_string(need) << " want " << ccap_string(want)
                << " revoking " << ccap_string(revoking)
                << dendl;
    if ((revoking & want) == 0) {
        *phave = need | (have & want);
        in->get_cap_ref(need);
        cap_hit();
        return 0;
    }
}

i.e. we only call get_cap_ref if the `need` set bits align with the caps issued, now if we check the Client::get_cap_ref or Inode::get_cap_ref code:

void Client::get_cap_ref(Inode *in, int cap)
{
  if ((cap & CEPH_CAP_FILE_BUFFER) &&
      in->cap_refs[CEPH_CAP_FILE_BUFFER] == 0) {
    ldout(cct, 5) << __func__ << " got first FILE_BUFFER ref on " << *in << dendl;
    in->iget();
  }
  if ((cap & CEPH_CAP_FILE_CACHE) &&
      in->cap_refs[CEPH_CAP_FILE_CACHE] == 0) {
    ldout(cct, 5) << __func__ << " got first FILE_CACHE ref on " << *in << dendl;
    in->iget();
  }
  in->get_cap_ref(cap);
}

void Inode::get_cap_ref(int cap)
{
  int n = 0;
  while (cap) {
    if (cap & 1) {
      int c = 1 << n;
      cap_refs[c]++;
      //cout << "inode " << *this << " get " << cap_string(c) << " " << (cap_refs[c]-1) << " -> " << cap_refs[c] << std::endl;
    }
    cap >>= 1;
    n++;
  }
}

We never check for the issued caps.

However nowhere in our code have we ever incremented ref count of a cap that was not issued/implemented but as a general practice we must.

Dhairya, is this really an issue after all? If the cap refs are accounted as expected, then is there a need to change that behaviour?

Venky Shankar wrote:

Dhairya, is this really an issue after all? If the cap refs are accounted as expected, then is there a need to change that behaviour?

its fine, as i said im seeing this more of a general practice to first verify and then increment.