| Message ID | 20191119154453.312400-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Forbid FIEMAP on RT devices | expand |
On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote: > By now, FIEMAP users have no way to identify which device contains the > mapping being reported by the ioctl, so, let's forbid FIEMAP on RT > devices/files until FIEMAP can properly report the device containing the > returned mappings. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > Hi folks, this change has been previously suggested by Christoph while the > fibmap->fiemap work was being discussed on the last version [1] of that set. > And after some thought I do think RT devices shouldn't allow fiemap calls > either, giving the file blocks will actually be on a different device than that > displayed on /proc/mounts which can lead to erroneous assumptions. Fingers are faster than my brain: [1]: https://lore.kernel.org/linux-xfs/20190918132436.GA16210@lst.de/T/ > > fs/xfs/xfs_iops.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index e532db27d0dc..ec7749cbd3ca 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -1138,6 +1138,9 @@ xfs_vn_fiemap( > { > int error; > > + if (XFS_IS_REALTIME_INODE(XFS_I(inode))) > + return -EOPNOTSUPP; > + > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED); > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { > fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR; > -- > 2.23.0 >
On 11/19/19 9:44 AM, Carlos Maiolino wrote: > By now, FIEMAP users have no way to identify which device contains the > mapping being reported by the ioctl, so, let's forbid FIEMAP on RT > devices/files until FIEMAP can properly report the device containing the > returned mappings. I'm not sure I agree with this - we don't return any device information with any mapping, ever. It's always up to the caller to understand what they have asked to have mapped. I'm not sure why RT files should be singled out, as long as the mapping is correct. -Eric > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > Hi folks, this change has been previously suggested by Christoph while the > fibmap->fiemap work was being discussed on the last version [1] of that set. > And after some thought I do think RT devices shouldn't allow fiemap calls > either, giving the file blocks will actually be on a different device than that > displayed on /proc/mounts which can lead to erroneous assumptions. > > fs/xfs/xfs_iops.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index e532db27d0dc..ec7749cbd3ca 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -1138,6 +1138,9 @@ xfs_vn_fiemap( > { > int error; > > + if (XFS_IS_REALTIME_INODE(XFS_I(inode))) > + return -EOPNOTSUPP; > + > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED); > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { > fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR; >
On Tue, Nov 19, 2019 at 10:10:54AM -0600, Eric Sandeen wrote: > On 11/19/19 9:44 AM, Carlos Maiolino wrote: > > By now, FIEMAP users have no way to identify which device contains the > > mapping being reported by the ioctl, so, let's forbid FIEMAP on RT > > devices/files until FIEMAP can properly report the device containing the > > returned mappings. > > I'm not sure I agree with this - we don't return any device information with > any mapping, ever. It's always up to the caller to understand what they have > asked to have mapped. I'm not sure why RT files should be singled out, as long > as the mapping is correct. It's been a long time discussion already on this subject, it all started back with btrfs and their virtual address spaces and how a fiemap should behave on filesystems such as btrfs, which, by instance, the caller can't tell which device the mapping refers to. XFS RT device is the simplest of this kind of case, where the mapping returned does not match with the mounted device, but by now that's the one blocking my FIEMAP work, which I want to fix to move on to the next steps :P > > -Eric > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > > > Hi folks, this change has been previously suggested by Christoph while the > > fibmap->fiemap work was being discussed on the last version [1] of that set. > > And after some thought I do think RT devices shouldn't allow fiemap calls > > either, giving the file blocks will actually be on a different device than that > > displayed on /proc/mounts which can lead to erroneous assumptions. > > > > fs/xfs/xfs_iops.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > index e532db27d0dc..ec7749cbd3ca 100644 > > --- a/fs/xfs/xfs_iops.c > > +++ b/fs/xfs/xfs_iops.c > > @@ -1138,6 +1138,9 @@ xfs_vn_fiemap( > > { > > int error; > > > > + if (XFS_IS_REALTIME_INODE(XFS_I(inode))) > > + return -EOPNOTSUPP; > > + > > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED); > > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { > > fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR; > > >
On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote: > By now, FIEMAP users have no way to identify which device contains the > mapping being reported by the ioctl, This is an incorrect assertion. Users can call FS_IOC_FSGETXATTR and look at the FS_XFLAG_REALTIME flag returned to determine what device the XFS data mapping points at. > so, let's forbid FIEMAP on RT > devices/files until FIEMAP can properly report the device containing the > returned mappings. Also, fiemap on the attribute fork should work regardless of where the data for the file is located. So that's two reasons we shouldn't do this :P Cheers, Dave.
On Wed, Nov 20, 2019 at 07:25:11AM +1100, Dave Chinner wrote: > On Tue, Nov 19, 2019 at 04:44:53PM +0100, Carlos Maiolino wrote: > > By now, FIEMAP users have no way to identify which device contains the > > mapping being reported by the ioctl, > > This is an incorrect assertion. > > Users can call FS_IOC_FSGETXATTR and look at the FS_XFLAG_REALTIME > flag returned to determine what device the XFS data mapping points > at. /me didn't know > > > so, let's forbid FIEMAP on RT > > devices/files until FIEMAP can properly report the device containing the > > returned mappings. > > Also, fiemap on the attribute fork should work regardless of where > the data for the file is located. > True. > So that's two reasons we shouldn't do this :P Fair enough. Thanks, I'll try to find another way to deal with that :P
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index e532db27d0dc..ec7749cbd3ca 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -1138,6 +1138,9 @@ xfs_vn_fiemap( { int error; + if (XFS_IS_REALTIME_INODE(XFS_I(inode))) + return -EOPNOTSUPP; + xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED); if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { fieinfo->fi_flags &= ~FIEMAP_FLAG_XATTR;
By now, FIEMAP users have no way to identify which device contains the mapping being reported by the ioctl, so, let's forbid FIEMAP on RT devices/files until FIEMAP can properly report the device containing the returned mappings. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- Hi folks, this change has been previously suggested by Christoph while the fibmap->fiemap work was being discussed on the last version [1] of that set. And after some thought I do think RT devices shouldn't allow fiemap calls either, giving the file blocks will actually be on a different device than that displayed on /proc/mounts which can lead to erroneous assumptions. fs/xfs/xfs_iops.c | 3 +++ 1 file changed, 3 insertions(+)