Re: [PATCH v6 8/9] ceph: add object version support for sync read

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 12/8/21 1:31 AM, Jeff Layton wrote:
> On Thu, 2021-11-04 at 13:52 +0800, xiubli@xxxxxxxxxx wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > The sync read may split the read into several osdc requests, so
> > for each it may in different Rados objects.
> >
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >   fs/ceph/file.c  | 44 ++++++++++++++++++++++++++++++++++++++++++--
> >   fs/ceph/super.h | 18 +++++++++++++++++-
> >   2 files changed, 59 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index 129f6a642f8e..cedd86a6058d 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -871,7 +871,8 @@ enum {
> >    * only return a short read to the caller if we hit EOF.
> >    */
> >   ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> > -			 struct iov_iter *to, int *retry_op)
> > +			 struct iov_iter *to, int *retry_op,
> > +			 struct ceph_object_vers *objvers)
> >   {
> >   	struct ceph_inode_info *ci = ceph_inode(inode);
> >   	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > @@ -880,6 +881,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> >   	u64 off = *ki_pos;
> >   	u64 len = iov_iter_count(to);
> >   	u64 i_size;
> > +	u32 object_count = 8;
> >   
> >   	dout("sync_read on inode %p %llu~%u\n", inode, *ki_pos, (unsigned)len);
> >   
> > @@ -896,6 +898,15 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> >   	if (ret < 0)
> >   		return ret;
> >   
> > +	if (objvers) {
> > +		objvers->count = 0;
> > +		objvers->objvers = kcalloc(object_count,
> > +					   sizeof(struct ceph_object_ver),
> > +					   GFP_KERNEL);
> > +		if (!objvers->objvers)
> > +			return -ENOMEM;
> > +	}
> > +
> >   	ret = 0;
> >   	while ((len = iov_iter_count(to)) > 0) {
> >   		struct ceph_osd_request *req;
> > @@ -938,6 +949,30 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> >   					 req->r_end_latency,
> >   					 len, ret);
> >   
> > +		if (objvers) {
> > +			u32 ind = objvers->count;
> > +
> > +			if (objvers->count >= object_count) {
> > +				int ov_size;
> > +
> > +				object_count *= 2;
> > +				ov_size = sizeof(struct ceph_object_ver);
> > +				objvers->objvers = krealloc_array(objvers,
> > +								  object_count,
> > +								  ov_size,
> > +								  GFP_KERNEL);
> > +				if (!objvers->objvers) {
> > +					objvers->count = 0;
> > +					ret = -ENOMEM;
> > +					break;
> > +				}
> > +			}
> > +
> > +			objvers->objvers[ind].offset = off;
> > +			objvers->objvers[ind].length = len;
> > +			objvers->objvers[ind].objver = req->r_version;
> > +			objvers->count++;
> > +		}
> >   		ceph_osdc_put_request(req);
> >   
> >   		i_size = i_size_read(inode);
> > @@ -995,6 +1030,11 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> >   	}
> >   
> >   	dout("sync_read result %zd retry_op %d\n", ret, *retry_op);
> > +	if (ret < 0 && objvers) {
> > +		objvers->count = 0;
> > +		kfree(objvers->objvers);
> > +		objvers->objvers = NULL;
> > +	}
> >   	return ret;
> >   }
> >   
> > @@ -1008,7 +1048,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> >   	     (unsigned)iov_iter_count(to),
> >   	     (file->f_flags & O_DIRECT) ? "O_DIRECT" : "");
> >   
> > -	return __ceph_sync_read(inode, &iocb->ki_pos, to, retry_op);
> > +	return __ceph_sync_read(inode, &iocb->ki_pos, to, retry_op, NULL);
> >   }
> >   
> >   struct ceph_aio_request {
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 2362d758af97..b347b12e86a9 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -451,6 +451,21 @@ struct ceph_inode_info {
> >   	struct inode vfs_inode; /* at end */
> >   };
> >   
> > +/*
> > + * The version of an object which contains the
> > + * file range of [offset, offset + length).
> > + */
> > +struct ceph_object_ver {
> > +	u64 offset;
> > +	u64 length;
> > +	u64 objver;
> > +};
> > +
> > +struct ceph_object_vers {
> > +	u32 count;
> > +	struct ceph_object_ver *objvers;
> > +};
> > +
> >   static inline struct ceph_inode_info *
> >   ceph_inode(const struct inode *inode)
> >   {
> > @@ -1254,7 +1269,8 @@ extern int ceph_open(struct inode *inode, struct file *file);
> >   extern int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> >   			    struct file *file, unsigned flags, umode_t mode);
> >   extern ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> > -				struct iov_iter *to, int *retry_op);
> > +				struct iov_iter *to, int *retry_op,
> > +				struct ceph_object_vers *objvers);
>
> I think this patch is probably overkill. It's not clear to me what you
> gain from tracking the offset and length in the object version.
>
> Reads across multiple objects aren't expected to be atomic in cephfs. It
> sucks, but RADOS can't reasonably guarantee that sort of consistency. I
> don't think you need to track all of the object versions since you only
> care about the last object in the read.
>
> Instead, could we just allow __ceph_sync_read to take a pointer to a u64
> instead of struct ceph_object_vers * ?

The last version was doing like this.

>   Then just have it fill that out
> with the version of the last object in the read.

I thought since this will be a general helper and might read more than 
one Rados objects and just switched to ceph_object_vers.

I am okay to take a pointer to a u64 to fill the last object's version.

I will send a fixing patch.

BRs

> >   extern int ceph_release(struct inode *inode, struct file *filp);
> >   extern void ceph_fill_inline_data(struct inode *inode, struct page *locked_page,
> >   				  char *data, size_t len);