On Fri, 2019-06-14 at 09:52 +0100, Luis Henriques wrote:
> So, do you think the patch below would be enough? It's totally
> untested, but I wanted to know if that would be acceptable before
> running some tests on it.
>
> Cheers,
> --
> Luis
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index c5517ffeb11c..f6b0683dd8dc 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -1949,6 +1949,21 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> goto out;
> }
>
> + ret = ceph_do_getattr(dst_inode, CEPH_CAP_AUTH_SHARED, false);
> + if (ret < 0) {
> + dout("failed to get auth caps on dst file (%zd)\n", ret);
> + goto out;
> + }
> +
I think this is still racy. You could lose As caps before file_modified
is called. IMO, this code should hold a reference to As caps until the
c_f_r operation is complete.
That may get tricky however if you do need to issue a setattr to change
the mode, as the MDS may try to recall As caps at that point. You won't
be able to release them until you drop the reference, so will that
deadlock? I'm not sure.
> + /* Should dst_inode lock be held throughout the copy operation? */
> + inode_lock(dst_inode);
> + ret = file_modified(dst_file);
> + inode_unlock(dst_inode);
> + if (ret < 0) {
> + dout("failed to modify dst file before copy (%zd)\n", ret);
> + goto out;
> + }
> +
> /*
> * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other
> * clients may have dirty data in their caches. And OSDs know nothing
--
Jeff Layton <jlayton@xxxxxxxxxx>
