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

[David Howells]

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