| Message ID | 1365344298-23917-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sun, Apr 07, 2013 at 10:18:18PM +0800, Liu Bo wrote: > We did not allow file data clone within the same file because of > deadlock issues. > > However, we now use nested lock to avoid deadlock between the > parent directory and the child file. > > So it's safe to do file clone at different offset within the > same file. There's a todo comment inside the function saying that cloning within the same file is possible but with an exception of overlapping ranges. How does your patch prevent that? With current code + your patch it's possible to call 2583 /* truncate page cache pages from target inode range */ 2584 truncate_inode_pages_range(&inode->i_data, destoff, 2585 PAGE_CACHE_ALIGN(destoff + len) - 1); where [destoff, destoff+len) intersects with the source [off, off+len) thus destroying the data to be cloned. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 08, 2013 at 02:49:18PM +0200, David Sterba wrote: > On Sun, Apr 07, 2013 at 10:18:18PM +0800, Liu Bo wrote: > > We did not allow file data clone within the same file because of > > deadlock issues. > > > > However, we now use nested lock to avoid deadlock between the > > parent directory and the child file. > > > > So it's safe to do file clone at different offset within the > > same file. > > There's a todo comment inside the function saying that cloning within > the same file is possible but with an exception of overlapping ranges. > How does your patch prevent that? With current code + your patch it's > possible to call > > 2583 /* truncate page cache pages from target inode range */ > 2584 truncate_inode_pages_range(&inode->i_data, destoff, > 2585 PAGE_CACHE_ALIGN(destoff + len) - 1); > > where [destoff, destoff+len) intersects with the source [off, off+len) > thus destroying the data to be cloned. > > david Yeah, that's right, I missed this, what about another sanity check for overlapping ranges, of course in the case of the same file? thanks, liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 08, 2013 at 09:56:44PM +0800, Liu Bo wrote: > Yeah, that's right, I missed this, what about another sanity check for > overlapping ranges, of course in the case of the same file? Yes of course, I can't work right without it. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 898c572..fa66b41 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2473,6 +2473,7 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, int ret; u64 len = olen; u64 bs = root->fs_info->sb->s_blocksize; + int same_inode = 0; /* * TODO: @@ -2509,7 +2510,7 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, ret = -EINVAL; if (src == inode) - goto out_fput; + same_inode = 1; /* the src must be open for reading */ if (!(src_file.file->f_mode & FMODE_READ)) @@ -2540,12 +2541,16 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, } path->reada = 2; - if (inode < src) { - mutex_lock_nested(&inode->i_mutex, I_MUTEX_PARENT); - mutex_lock_nested(&src->i_mutex, I_MUTEX_CHILD); + if (!same_inode) { + if (inode < src) { + mutex_lock_nested(&inode->i_mutex, I_MUTEX_PARENT); + mutex_lock_nested(&src->i_mutex, I_MUTEX_CHILD); + } else { + mutex_lock_nested(&src->i_mutex, I_MUTEX_PARENT); + mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD); + } } else { - mutex_lock_nested(&src->i_mutex, I_MUTEX_PARENT); - mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD); + mutex_lock(&src->i_mutex); } /* determine range to clone */ @@ -2839,7 +2844,8 @@ out: unlock_extent(&BTRFS_I(src)->io_tree, off, off + len - 1); out_unlock: mutex_unlock(&src->i_mutex); - mutex_unlock(&inode->i_mutex); + if (!same_inode) + mutex_unlock(&inode->i_mutex); vfree(buf); btrfs_free_path(path); out_fput:
We did not allow file data clone within the same file because of deadlock issues. However, we now use nested lock to avoid deadlock between the parent directory and the child file. So it's safe to do file clone at different offset within the same file. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ioctl.c | 20 +++++++++++++------- 1 files changed, 13 insertions(+), 7 deletions(-)