Re: [PATCH v3 2/4] ceph: add __ceph_sync_read helper support

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 10/29/21 6:29 PM, Jeff Layton wrote:
> On Fri, 2021-10-29 at 16:14 +0800, Xiubo Li wrote:
> > On 10/29/21 2:21 AM, Jeff Layton wrote:
> > > On Thu, 2021-10-28 at 17:14 +0800, xiubli@xxxxxxxxxx wrote:
> > > > From: Xiubo Li <xiubli@xxxxxxxxxx>
> > > >
> > > > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > > > ---
> > > >    fs/ceph/file.c  | 31 +++++++++++++++++++++----------
> > > >    fs/ceph/super.h |  2 ++
> > > >    2 files changed, 23 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > > index 6e677b40410e..74db403a4c35 100644
> > > > --- a/fs/ceph/file.c
> > > > +++ b/fs/ceph/file.c
> > > > @@ -901,20 +901,17 @@ static inline void fscrypt_adjust_off_and_len(struct inode *inode, u64 *off, u64
> > > >     * If we get a short result from the OSD, check against i_size; we need to
> > > >     * only return a short read to the caller if we hit EOF.
> > > >     */
> > > > -static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> > > > -			      int *retry_op)
> > > > +ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
> > > > +			 struct iov_iter *to, int *retry_op)
> > > >    {
> > > > -	struct file *file = iocb->ki_filp;
> > > > -	struct inode *inode = file_inode(file);
> > > >    	struct ceph_inode_info *ci = ceph_inode(inode);
> > > >    	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > > >    	struct ceph_osd_client *osdc = &fsc->client->osdc;
> > > >    	ssize_t ret;
> > > > -	u64 off = iocb->ki_pos;
> > > > +	u64 off = *ki_pos;
> > > >    	u64 len = iov_iter_count(to);
> > > >    
> > > > -	dout("sync_read on file %p %llu~%u %s\n", file, off, (unsigned)len,
> > > > -	     (file->f_flags & O_DIRECT) ? "O_DIRECT" : "");
> > > > +	dout("sync_read on inode %p %llu~%u\n", inode, *ki_pos, (unsigned)len);
> > > >    
> > > >    	if (!len)
> > > >    		return 0;
> > > > @@ -1058,18 +1055,32 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> > > >    			break;
> > > >    	}
> > > >    
> > > > -	if (off > iocb->ki_pos) {
> > > > +	if (off > *ki_pos) {
> > > >    		if (ret >= 0 &&
> > > >    		    iov_iter_count(to) > 0 && off >= i_size_read(inode))
> > > >    			*retry_op = CHECK_EOF;
> > > > -		ret = off - iocb->ki_pos;
> > > > -		iocb->ki_pos = off;
> > > > +		ret = off - *ki_pos;
> > > > +		*ki_pos = off;
> > > >    	}
> > > >    out:
> > > >    	dout("sync_read result %zd retry_op %d\n", ret, *retry_op);
> > > > return ret;
> > > >     	 }
> > > >    
> > > > +static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> > > > +			      int *retry_op)
> > > > +{
> > > > +	struct file *file = iocb->ki_filp;
> > > > +	struct inode *inode = file_inode(file);
> > > > +
> > > > +	dout("sync_read on file %p %llu~%u %s\n", file, iocb->ki_pos,
> > > > +	     (unsigned)iov_iter_count(to),
> > > > +	     (file->f_flags & O_DIRECT) ? "O_DIRECT" : "");
> > > > +
> > > > +	return __ceph_sync_read(inode, &iocb->ki_pos, to, retry_op);
> > > > +
> > > > +}
> > > > +
> > > >    struct ceph_aio_request {
> > > >    	struct kiocb *iocb;
> > > >    	size_t total_len;
> > > > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > > > index 027d5f579ba0..57bc952c54e1 100644
> > > > --- a/fs/ceph/super.h
> > > > +++ b/fs/ceph/super.h
> > > > @@ -1235,6 +1235,8 @@ extern int ceph_renew_caps(struct inode *inode, int fmode);
> > > >    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);
> > > >    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);
> > > I went ahead and picked this patch too since #3 had a dependency on it.
> > > If we decide we want #3 for stable though, then we probably ought to
> > > respin these to avoid it.
> > I saw you have merged these two into the testing branch, should I respin
> > for the #3 ?
> Yeah, it's probably worthwhile to mark this for stable. It _is_ a data
> integrity issue after all.
>
> Could you respin it so that it doesn't rely on patch #2?

Sure.