Ceph » CephFS

@Dhairya Parmar This too looks to be similar to https://tracker.ceph.com/issues/64390, yes?

Venky Shankar wrote in #note-2:

@Dhairya Parmar This too looks to be similar to https://tracker.ceph.com/issues/64390, yes?

not really, that issue was about umount getting stuck because the pools being full didn't allow the client to trim cache and if you see here there's a disconnected inode with refs.

I read the buffer code and it is the usage of `unsigned int` that is the limitation. Specifically `unsigned length() const { return _len; }`. Since for `unsigned int` the overflow does not produce undefined behaviour like that it does for signed int but the value is wrapped around using modulo arithmatic i.e N - UNSIGNED_INT_MAX - 1 (where N is the value supplied) so what happens at the client side is that `Client::_write` happily appends all the iov structs(each of 1GiB used in test case) to the `bl` but now since the size of the `bl` is 4294967296(i.e. 4GiB), there is an int overflow and the `bl.length()` is now `0` and hence the error:

unknown file: Failure
C++ exception with description "End of buffer [buffer:2]" thrown in the test body.

E.g.:

with:

  auto out_buf_0 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_0.get(), 'a', BUFSIZE);
  auto out_buf_1 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_1.get(), 'b', BUFSIZE);
  auto out_buf_2 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_2.get(), 'c', BUFSIZE);
  auto out_buf_3 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_3.get(), 'd', BUFSIZE);

  struct iovec iov_out[4] = {
    {out_buf_0.get(), BUFSIZE},
    {out_buf_1.get(), BUFSIZE},
    {out_buf_2.get(), BUFSIZE},
    {out_buf_3.get(), BUFSIZE}
  };

bufferlist:

2024-06-15T04:11:33.079+0530 7f5983879a00 10 client.4291 bl.length: 0
2024-06-15T04:11:33.079+0530 7f5983879a00 10 client.4291 bl: 
buffer::list(len=0,
    buffer::ptr(0~1073745832 0x7f57e7600010 in raw 0x7f57e7600010 len 1073745832 nref 1),
    buffer::ptr(0~1073741736 0x7f57a7400010 in raw 0x7f57a7400010 len 1073741736 nref 1),
    buffer::ptr(0~1073741736 0x7f5767200010 in raw 0x7f5767200010 len 1073741736 nref 1),
    buffer::ptr(0~1073737992 0x7f5727000010 in raw 0x7f5727000010 len 1073741736 nref 1)
)

while with:

  auto out_buf_0 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_0.get(), 'a', BUFSIZE);
  auto out_buf_1 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_1.get(), 'b', BUFSIZE);
  auto out_buf_2 = std::make_unique<char[]>(BUFSIZE);
  memset(out_buf_2.get(), 'c', BUFSIZE);

  struct iovec iov_out[3] = {
    {out_buf_0.get(), BUFSIZE},
    {out_buf_1.get(), BUFSIZE},
    {out_buf_2.get(), BUFSIZE}
  };

bufferlist:

2024-06-15T04:13:13.398+0530 7f44898b2a00 10 client.4300 bl.length: 3221225472
2024-06-15T04:13:13.398+0530 7f44898b2a00 10 client.4300 bl:
buffer::list(len=3221225472,
    buffer::ptr(0~1073745832 0x7f432b800010 in raw 0x7f432b800010 len 1073745832 nref 1),
    buffer::ptr(0~1073741736 0x7f42eb600010 in raw 0x7f42eb600010 len 1073741736 nref 1),
    buffer::ptr(0~1073737904 0x7f42ab400010 in raw 0x7f42ab400010 len 1073741736 nref 1)
)

So either we return from `Client::_write` when we see the bufferlist is >= 4GiB or we enhance the buffer code to make it capable of handling more data

Dhairya Parmar wrote in #note-4:

I read the buffer code and it is the usage of `unsigned int` that is the limitation. Specifically `unsigned length() const { return _len; }`. Since for `unsigned int` the overflow does not produce undefined behaviour like that it does for signed int but the value is wrapped around using modulo arithmatic i.e N - UNSIGNED_INT_MAX - 1 (where N is the value supplied) so what happens at the client side is that `Client::_write` happily appends all the iov structs(each of 1GiB used in test case) to the `bl` but now since the size of the `bl` is 4294967296(i.e. 4GiB), there is an int overflow and the `bl.length()` is now `0` and hence the error:
[...]

E.g.:

with:
[...]

bufferlist:
[...]

while with:
[...]

bufferlist:
[...]

So either we return from `Client::_write` when we see the bufferlist is >= 4GiB or we enhance the buffer code to make it capable of handling more data

FYI this is a generic issue with i/o code paths and not async specific

from my discussion with venky during today's standup, it was decided to move with the solution to return some relevant errno if the buffers go beyond 4GiB.

to my surprise, write calls are able to do max 2GiB. When i initiated an io of 3GiB, the bytes that it returned to me was shockingly 2GiB.

okay so as per https://man7.org/linux/man-pages/man2/write.2.html the max bytes that can be transferred through write() and similar system calls is 2147479552 bytes. Therefore this isn't actually a bug. (this was for https://tracker.ceph.com/issues/66245#note-8)

@Venky Shankar this is still an issue, using buffers > 4GiB crashes the i/o call. This still needs to be taken care of.

Dhairya Parmar wrote in #note-11:

@Venky Shankar this is still an issue, using buffers > 4GiB crashes the i/o call. This still needs to be taken care of.

It's against the standard though, so its probably fine to not worry about it. Or if we really want to, then this is low priority and an intern task :)

Venky Shankar wrote in #note-12:

Dhairya Parmar wrote in #note-11:

@Venky Shankar this is still an issue, using buffers > 4GiB crashes the i/o call. This still needs to be taken care of.

It's against the standard though, so its probably fine to not worry about it. Or if we really want to, then this is low priority and an intern task :)

would linux calls also crash when supplied with such huge buffers?

a snippet from _write():

  if (buf) {
    if (size > 0)
      bl.append(buf, size);
  } else if (iov){
    for (int i = 0; i < iovcnt; i++) {
      if (iov[i].iov_len > 0) {
        bl.append((const char *)iov[i].iov_base, iov[i].iov_len);
      }
    }
  }

the size over here is a clamped (_preadv_pwritev_locked() i.e. its caller does it) version but the same isn't being done while iterating over the iovec structs, meaning this is an incomplete implementation. The for loop should also do what clamp_to_int does:

loff_t totallen = 0;
for (int i = 0; i < iovcnt; i++) {
    totallen += iov[i].iov_len;
}

if (clamp_to_int) {
  totallen = std::min(totallen, (loff_t)INT_MAX);
}

meaning non-vectored I/O would go through since they only depend on size but vectored I/O (async or sync) will suffer.

which translates to having calls like this:

r = objectcacher->file_write(&in->oset, &in->layout,
     in->snaprealm->get_snap_context(),
     offset, size, bl, ceph::real_clock::now(),
     0, iofinish.get(),
     onfinish == nullptr
       ? objectcacher->CFG_block_writes_upfront()
       : false);

so the final code would something be like this:

ssize_t total_appended = 0;
if (buf) {
  if (size > 0)
    bl.append(buf, size);
} else if (iov){
  for (int i = 0; i < iovcnt; i++) {
    if (iov[i].iov_len > 0) {
      if (total_appended >= size) {
        bl.append(nullptr, 0);
      } else { 
        bl.append((const char *)iov[i].iov_base, iov[i].iov_len);
        total_appended += iov[i].iov_len;
      }
    }
  }
}