Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> 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
