On Wed, Oct 10, 2018 at 09:47:00AM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Change the remap_file_range functions to take a number of bytes to
> > operate upon and return the number of bytes they operated on. This is a
> > requirement for allowing fs implementations to return short clone/dedupe
> > results to the user, which will enable us to obey resource limits in a
> > graceful manner.
> >
> > A subsequent patch will enable copy_file_range to signal to the
> > ->clone_file_range implementation that it can handle a short length,
> > which will be returned in the function's return value. Neither clone
> > ioctl can take advantage of this, alas.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> [...]
> > @@ -141,8 +142,8 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> > }
> >
> > /* Try to use clone_file_range to clone up within the same fs */
> > - error = do_clone_file_range(old_file, 0, new_file, 0, len);
> > - if (!error)
> > + cloned = do_clone_file_range(old_file, 0, new_file, 0, len);
> > + if (cloned == len)
> > goto out;
> > /* Couldn't clone, so now we try to copy the data */
> > error = 0;
>
> This error = 0 not needed anymore, but not a big deal...
Fixed.
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 693bd0620a81..c8c890c22898 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -434,14 +434,14 @@ enum ovl_copyop {
> > OVL_DEDUPE,
> > };
> >
> > -static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> > +static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> > struct file *file_out, loff_t pos_out,
> > - u64 len, unsigned int flags, enum ovl_copyop op)
> > + loff_t len, unsigned int flags, enum ovl_copyop op)
> > {
> > struct inode *inode_out = file_inode(file_out);
> > struct fd real_in, real_out;
> > const struct cred *old_cred;
> > - ssize_t ret;
> > + loff_t ret;
> >
> > ret = ovl_real_fdget(file_out, &real_out);
> > if (ret)
> > @@ -489,9 +489,9 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
> > OVL_COPY);
> > }
> >
> > -static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> > - struct file *file_out, loff_t pos_out,
> > - u64 len, unsigned int flags)
> > +static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> > + struct file *file_out, loff_t pos_out,
> > + loff_t len, unsigned int flags)
> > {
> > enum ovl_copyop op;
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 917934770b08..f43b0620afd4 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1589,10 +1589,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> > * more efficient if both clone and copy are supported (e.g. NFS).
> > */
> > if (file_in->f_op->remap_file_range) {
> > - ret = file_in->f_op->remap_file_range(file_in, pos_in,
> > - file_out, pos_out, len, 0);
> > - if (ret == 0) {
> > - ret = len;
> > + s64 cloned;
>
> loff_t?
Fixed.
> > +
> > + cloned = file_in->f_op->remap_file_range(file_in, pos_in,
> > + file_out, pos_out,
> > + min_t(loff_t, MAX_RW_COUNT, len), 0);
> > + if (cloned >= 0) {
> > + ret = cloned;
> > goto done;
> > }
> > }
>
> Commit message wasn't clear enough on the behavior of copy_file_range()
> before and after the patch IMO. Maybe it would be better to pospone this
> semantic change to the RFR_SHORTEN patch and keep if (cloned == len)
> in this patch?
There shouldn't be any behavior change here -- all implementations
return a negative error code or the length that was passed in. I'll
clarify that in the commit message.
--D
> Thanks,
> Amir.
