On Tue, Aug 21, 2018 at 2:40 AM, Yan, Zheng <ukernel@xxxxxxxxx> wrote: > On Tue, Aug 21, 2018 at 5:21 PM Luis Henriques <lhenriques@xxxxxxxx> wrote: >> >> "Yan, Zheng" <ukernel@xxxxxxxxx> writes: >> ... >> >> + ret = ceph_get_caps(dst_ci, CEPH_CAP_FILE_WR, >> >> + CEPH_CAP_FILE_BUFFER, >> >> + endoff, &got, NULL); >> >> + if (ret < 0) >> >> + goto out; >> >> + >> > >> > Getting FILE_RD caps for src inode? I think adding another >> > ceph_get_caps() call here may cause deadlock (shouldn't hold any cap >> > while waiting). >> >> I thought about that initially, but since I'm using do_splice_direct for >> doing any manual copies I assumed that this would eventually be handled >> somewhere else. Because we don't really FILE_RD if we're doing only >> full (remote) object copies, do we? >> > > We still need to get FILE_RD for full object copies. Because other > client may have dirty data in page cache. we need to until the dirty > data get flushed Plus it's not like the OSD has any idea about the filesystem's caps; the client needs to hold them all on the OSD's behalf while it's doing remote data copies. > >> ... >> >> >> +out_caps: >> >> + ceph_put_cap_refs(dst_ci, got); >> >> +out: >> >> + ceph_free_cap_flush(prealloc_cf); >> >> + >> >> + return ret; >> >> +} >> > >> > There is no i_size check in above function. I think some corner cases >> > may not be handled properly >> >> Do you have any specific case in mind? I'm checking the EOF for the >> source file: > > Sorry I missed this. > >> >> + size = i_size_read(src_inode); >> + /* Don't copy beyond source file EOF */ >> + if (src_off + len > size) >> + len = (size - src_off); >> >> and the final destination file size: > > If other client is writing to the source inode, local i_size can be > stale. Maybe we can use do_splice_direct(... original copy length..) > for the copy-beyond-eof case. See striped_read()'s 'checkeof' for > more information. > > Regards > Yan, Zheng > >> >> + size = i_size_read(dst_inode); >> + endoff = dst_off + len; >> + ret = inode_newsize_ok(dst_inode, endoff); >> + if (ret) >> + goto out; >> >> But maybe I misunderstood your question. >> >> Cheers, >> -- >> Luis
