| Message ID | 1477510441-24861-1-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: > With overlayfs, it is wrong to compare file_inode(inode)->i_sb > of regular files with those of non-regular files, because the > former reference the real (upper/lower) sb and the latter reference > the overlayfs sb. > > Move the test for same super block after the sanity tests for > clone range of directory and non-regular file. Better: compare ->f_path.dentry->d_sb instead of file_inode()->i_sb. We don't want to be mixing files that come from overlayfs and ones that come from the underlying layers. BTW, would it be worthwhile adding clone_file_range() support to overlayfs? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> With overlayfs, it is wrong to compare file_inode(inode)->i_sb >> of regular files with those of non-regular files, because the >> former reference the real (upper/lower) sb and the latter reference >> the overlayfs sb. >> >> Move the test for same super block after the sanity tests for >> clone range of directory and non-regular file. > > Better: compare ->f_path.dentry->d_sb instead of file_inode()->i_sb. > We don't want to be mixing files that come from overlayfs and ones > that come from the underlying layers. That's not a good option. When source file is in lower and dest file is in upper, then clone range should go forward iff both lower and upper inodes are on the same sb. The test as it is checks for this properly. > > BTW, would it be worthwhile adding clone_file_range() support to overlayfs? > There is nothing to add. It works quite well because clone_file_range works on inode level. In fact, the entire 'clone' group of xfstests passes with overlayfs of lower and upper on the same XFS filesystem (with reflink support). Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 6:25 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> With overlayfs, it is wrong to compare file_inode(inode)->i_sb >>> of regular files with those of non-regular files, because the >>> former reference the real (upper/lower) sb and the latter reference >>> the overlayfs sb. >>> >>> Move the test for same super block after the sanity tests for >>> clone range of directory and non-regular file. >> >> Better: compare ->f_path.dentry->d_sb instead of file_inode()->i_sb. >> We don't want to be mixing files that come from overlayfs and ones >> that come from the underlying layers. > > That's not a good option. > When source file is in lower and dest file is in upper, then clone range > should go forward iff both lower and upper inodes are on the same sb. > The test as it is checks for this properly. > Ping. Do you have any more question about this change? Do you mind queuing this up for next along with the rest of the clone_file_range() changes? It'd be nice to have all xfstests pass for overlayfs on 4.10... >> >> BTW, would it be worthwhile adding clone_file_range() support to overlayfs? >> > > There is nothing to add. It works quite well because clone_file_range works > on inode level. In fact, the entire 'clone' group of xfstests passes > with overlayfs > of lower and upper on the same XFS filesystem (with reflink support). > > Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 3, 2016 at 8:02 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Oct 28, 2016 at 6:25 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Fri, Oct 28, 2016 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Wed, Oct 26, 2016 at 9:34 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> With overlayfs, it is wrong to compare file_inode(inode)->i_sb >>>> of regular files with those of non-regular files, because the >>>> former reference the real (upper/lower) sb and the latter reference >>>> the overlayfs sb. >>>> >>>> Move the test for same super block after the sanity tests for >>>> clone range of directory and non-regular file. >>> >>> Better: compare ->f_path.dentry->d_sb instead of file_inode()->i_sb. >>> We don't want to be mixing files that come from overlayfs and ones >>> that come from the underlying layers. >> >> That's not a good option. >> When source file is in lower and dest file is in upper, then clone range >> should go forward iff both lower and upper inodes are on the same sb. >> The test as it is checks for this properly. >> > > Ping. > > Do you have any more question about this change? > Do you mind queuing this up for next along with the rest of the > clone_file_range() changes? Applied and pushed to #overlayfs-next. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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/read_write.c b/fs/read_write.c index 1244bd5..d547301 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1657,6 +1657,11 @@ int vfs_clone_file_range(struct file *file_in, loff_t pos_in, struct inode *inode_out = file_inode(file_out); int ret; + if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode)) + return -EISDIR; + if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode)) + return -EINVAL; + /* * FICLONE/FICLONERANGE ioctls enforce that src and dest files are on * the same mount. Practically, they only need to be on the same file @@ -1665,11 +1670,6 @@ int vfs_clone_file_range(struct file *file_in, loff_t pos_in, if (inode_in->i_sb != inode_out->i_sb) return -EXDEV; - if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode)) - return -EISDIR; - if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode)) - return -EINVAL; - if (!(file_in->f_mode & FMODE_READ) || !(file_out->f_mode & FMODE_WRITE) || (file_out->f_flags & O_APPEND))
With overlayfs, it is wrong to compare file_inode(inode)->i_sb of regular files with those of non-regular files, because the former reference the real (upper/lower) sb and the latter reference the overlayfs sb. Move the test for same super block after the sanity tests for clone range of directory and non-regular file. This change fixes xfstest generic/157, which returned EXDEV instead of EISDIR/EINVAL in the following test cases over overlayfs: echo "Try to reflink a dir" _reflink_range $testdir1/dir1 0 $testdir1/file2 0 $blksz echo "Try to reflink a device" _reflink_range $testdir1/dev1 0 $testdir1/file2 0 $blksz Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/read_write.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)