Re: [RFC PATCH 07/35] libceph: Change ceph_osdc_call()'s reply to a ceph_databuf

David Howells
2025-03-17T22:12:25Z

Viacheslav Dubeyko <Slava.Dubeyko@ibm.com> wrote:

> > + struct ceph_databuf *reply;
> > + void *p, *q, *end;
>
> If I understood correctly the logic, q represents a pointer on current
> position. So, maybe, it makes sense to rename p into something like
> "begin"? In this case, we will have begin pointer, end pointer and p could
> be used as the name of pointer on current position.

"hdr" might be a better choice.

> > + iov_iter_advance(&reply->iter, q - p);
> >
> > - if (offset_in_page(p) + object_map_bytes > reply_len) {
> > + if (object_map_bytes > ceph_databuf_len(reply)) {
>
> Does it mean that we had bug before here? Because it was offset_in_page(p) +
> object_map_bytes before.

No. The iov_iter_advance() call advances the iterator over the header which
renders the subtraction unnecessary.

> > rbd_dev->object_map_size = object_map_size;
>
> Why do we have object_map_size and object_map_bytes at the same time? It could
> be confusing for my taste. Maybe, we need to rename the object_map_size to
> object_map_num_objects?

Those names preexist.

> > + reply = ceph_databuf_reply_alloc(1, inbound_size, GFP_KERNEL);
>
> Interesting... We allocated memory page before. Now we allocate the memory
> of inbound size. Potentially, it could be any size of starting from zero
> bytes and including several memory pages. Could we have an issue here?

Shouldn't do. ceph_databuf_reply_alloc() will expand databuf's bvec[] as
necessary to accommodate sufficient pages for the requested amount of
bufferage.

David