Ceph » rbd

This is a code defect still appears in "Master", can we change the affected version, I can't edit on my side

That's correct, cinder-backup can change the configuration setting to read 2GiB+ chunks.
The default setting is less than 2GiB.

So, looks like this is a limitation and ceph have no plan to support it.
is there any documentation noting this limitation ?

dongdong tao wrote:

is there any documentation noting this limitation ?

Negative. I've never heard of such an attempt to issue such large IOs before and I suspect you haven't evaluated the effects on your cluster and client nodes when attempting to perform such large IOs. It's important to remember that when you attempt to manually tweak the software outside the normal configuration and bounds, you are effectively running a unique software configuration and you no longer get the stability assurances from knowing thousands of other deployments are running in the same configuration.

Jason Dillaman wrote:

[..] we have no plans to change the API to support that large of reads.

I am not sure if this would amount to an API change. The function in question, rbd_read2, actually returns ssize_t. The value returned by ictx->io_work_queue->read, which is a ssize_t, is assigned to int. When returning, it's converted back to ssize_t. The following sequence happens:

ssize_t -> int -> ssize_t

(the noted overflow occurs during the 1st conversion if it's greater than INT_MAX).

So this limitation doesn't appear a deliberate decision. Regardless of the merits/demerits of I/O's larger than 2GiB, I think, it could be fixed without API breakage if that's the only concern.

Ponnuvel P wrote:

So this limitation doesn't appear a deliberate decision. Regardless of the merits/demerits of I/O's larger than 2GiB, I think, it could be fixed without API breakage if that's the only concern.

That really isn't the concern. Your single >2GiB IO request will get translated internally as 512 concurrent requests to the OSDs consuming up to 4GiB of RAM. It's really not efficient.

@Jason Borden, I got your concern about the performance/ram issue and that make sense,
but that doesn't explain why we allow a 64 bit "size_t" length to be passed to rbd_read2 at the first place,
I mean if we pass 2GB length to the current rbd_read2, it indeed reads that much data back, but the issue here
is that the return value, which is the one to represent the size of data being read, will be overflowed as we are using int to store the value.

I think another concern is from the code level, as I've tried to make a patch to fix the integer overflow
I found that there are some fundamental code issue here, it's non-trival to fix and the fix will likely make the code ugly.

So, we are using C_SaferCond's rval (an "int") to store the size of data been read, which I don't think it's a good choice , but can understand.
I don't believe we want to change the C_SaferCond's rval type to int64_t, as this context class is used everywhere in ceph.
Another way would be introducing a completely new callback context to just used for these kind 64 bit rval,
but it will require big efforts, and I'm not sure upstream want to do it, cause it might just not able to bring any WIN like your concern.

Jason Dillaman wrote:

Ponnuvel P wrote:

So this limitation doesn't appear a deliberate decision. Regardless of the merits/demerits of I/O's larger than 2GiB, I think, it could be fixed without API breakage if that's the only concern.

That really isn't the concern. Your single >2GiB IO request will get translated internally as 512 concurrent requests to the OSDs consuming up to 4GiB of RAM. It's really not efficient.

Hi Jason,

would really appreciate your thoughts on my last comment at https://tracker.ceph.com/issues/47210#note-7

Thanks!

dongdong tao wrote:

would really appreciate your thoughts on my last comment at https://tracker.ceph.com/issues/47210#note-7

Comment on what, specifically? librbd would need lots of internal refactoring if we wanted to support large IOs and I personally don't see the worth. I think the important thing to think about is why you think it's a better design to be unique and switch your Cinder backup configuration to use 2GiB IOs?

Jason Dillaman wrote:

dongdong tao wrote:

would really appreciate your thoughts on my last comment at https://tracker.ceph.com/issues/47210#note-7

Comment on what, specifically? librbd would need lots of internal refactoring if we wanted to support large IOs and I personally don't see the worth. I think the important thing to think about is why you think it's a better design to be unique and switch your Cinder backup configuration to use 2GiB IOs?

Thank you Jason! you've just commented my concern, I wasn't a hundred percent sure if there is a way to fix this without refactoring lots of the rbd code.
Since you also think it will need lots of code refactoring work in order to surpport the large IOs,
and it won't bring much WIN for us, but likely bring some concern for the Ram and Perf(ceph cluster) issue.

So your first comment " At best we can fix it to throw an immediate error if someone attempts something like that." does make sense to me.